From 96eae6c690d67d006a97e0b2b44a65460080f214 Mon Sep 17 00:00:00 2001 From: Pedro Silva Date: Fri, 7 Jul 2023 18:44:53 +0100 Subject: [PATCH 1/3] Add security to the onboarding return URL. --- .../src/Repository/PartnerReferralsData.php | 14 + .../src/Exception/RuntimeException.php | 18 + .../src/Helper/OnboardingUrl.php | 318 ++++++++++++++++++ .../ppcp-onboarding/src/Helper/UrlHelper.php | 42 +++ .../src/Render/OnboardingRenderer.php | 17 +- .../src/Settings/SettingsListener.php | 34 +- psalm-baseline.xml | 3 +- .../Onboarding/Helper/OnboardingUrlTest.php | 193 +++++++++++ 8 files changed, 630 insertions(+), 9 deletions(-) create mode 100644 modules/ppcp-onboarding/src/Exception/RuntimeException.php create mode 100644 modules/ppcp-onboarding/src/Helper/OnboardingUrl.php create mode 100644 modules/ppcp-onboarding/src/Helper/UrlHelper.php create mode 100644 tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php diff --git a/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php b/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php index a955dcf23..fd93e57be 100644 --- a/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php +++ b/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php @@ -127,4 +127,18 @@ class PartnerReferralsData { ) ); } + + /** + * Append the validation token to the return_url + * + * @param array $data The referral data. + * @param string $token The token to be appended. + * @return array + */ + public function append_onboarding_token( array $data, string $token ): array { + $separator = strpos( $data['partner_config_override']['return_url'], '?' ) === false ? '?' : '&'; + + $data['partner_config_override']['return_url'] .= $separator . 'ppcpToken=' . $token; + return $data; + } } diff --git a/modules/ppcp-onboarding/src/Exception/RuntimeException.php b/modules/ppcp-onboarding/src/Exception/RuntimeException.php new file mode 100644 index 000000000..fc14fc94f --- /dev/null +++ b/modules/ppcp-onboarding/src/Exception/RuntimeException.php @@ -0,0 +1,18 @@ +cache = $cache; + $this->cache_key_prefix = $cache_key_prefix; + $this->user_id = $user_id; + } + + /** + * Validates the token, if it's valid then delete it. + * If it's invalid don't delete it, to prevent malicious requests from invalidating the token. + * + * @param Cache $cache The cache object where the URL is stored. + * @param string $onboarding_token The token to validate. + * @param int $user_id User ID to associate the link with. + * @return bool + */ + public static function validate_token_and_delete( Cache $cache, string $onboarding_token, int $user_id ): bool { + if ( ! $onboarding_token ) { + return false; + } + + $token_data = json_decode( UrlHelper::url_safe_base64_decode( $onboarding_token ) ?: '', true ); + + if ( ! $token_data ) { + return false; + } + + if ( ! isset( $token_data['u'] ) || ! isset( $token_data['k'] ) ) { + return false; + } + + if ( $token_data['u'] !== $user_id ) { + return false; + } + + $onboarding_url = new self( $cache, $token_data['k'], $token_data['u'] ); + + if ( ! $onboarding_url->load() ) { + return false; + } + + if ( ( $onboarding_url->token() ?: '' ) !== $onboarding_token ) { + return false; + } + + $onboarding_url->delete(); + return true; + } + + /** + * Load cached data if is valid and initialize object. + * + * @return bool + */ + public function load(): bool { + if ( ! $this->cache->has( $this->cache_key() ) ) { + return false; + } + + $cached_data = $this->cache->get( $this->cache_key() ); + + if ( ! $this->validate_cache_data( $cached_data ) ) { + return false; + } + + $this->secret = $cached_data['secret']; + $this->time = $cached_data['time']; + $this->url = $cached_data['url']; + + return true; + } + + /** + * Initializes the object + * + * @return void + */ + public function init(): void { + try { + $this->secret = bin2hex( random_bytes( 16 ) ); + } catch ( \Exception $e ) { + $this->secret = wp_generate_password( 16 ); + } + + $this->time = time(); + $this->url = null; + } + + /** + * Validates data from cache + * + * @param array $cache_data The data retrieved from the cache. + * @return bool + */ + private function validate_cache_data( $cache_data ): bool { + if ( ! is_array( $cache_data ) ) { + return false; + } + + if ( + ! ( $cache_data['user_id'] ?? false ) + || ! ( $cache_data['hash_check'] ?? false ) + || ! ( $cache_data['secret'] ?? false ) + || ! ( $cache_data['time'] ?? false ) + || ! ( $cache_data['url'] ?? false ) + ) { + return false; + } + + if ( $cache_data['user_id'] !== $this->user_id ) { + return false; + } + + // Detect if salt has changed. + if ( $cache_data['hash_check'] !== wp_hash( '' ) ) { + return false; + } + + // If we want we can also validate time for expiration eventually. + + return true; + } + + /** + * Returns the URL + * + * @return string + * @throws RuntimeException Throws in case the URL isn't initialized. + */ + public function get(): string { + if ( null === $this->url ) { + throw new RuntimeException( 'Object not initialized.' ); + } + return $this->url; + } + + /** + * Returns the Token + * + * @return string + * @throws RuntimeException Throws in case the object isn't initialized. + */ + public function token(): string { + if ( + null === $this->secret + || null === $this->time + || null === $this->user_id + ) { + throw new RuntimeException( 'Object not initialized.' ); + } + + // Trim the hash to make sure the token isn't too long. + $hash = substr( + wp_hash( + implode( + '|', + array( + $this->cache_key_prefix, + $this->user_id, + $this->secret, + $this->time, + ) + ) + ), + 0, + 32 + ); + + $token = wp_json_encode( + array( + 'k' => $this->cache_key_prefix, + 'u' => $this->user_id, + 'h' => $hash, + ) + ); + + if ( ! $token ) { + throw new RuntimeException( 'Unable to generate token.' ); + } + + return UrlHelper::url_safe_base64_encode( $token ); + } + + /** + * Sets the URL + * + * @param string $url The URL to store in the cache. + * @return void + */ + public function set( string $url ): void { + $this->url = $url; + } + + /** + * Persists the URL and related data in cache + * + * @return void + */ + public function persist(): void { + if ( + null === $this->secret + || null === $this->time + || null === $this->user_id + || null === $this->url + ) { + return; + } + + $this->cache->set( + $this->cache_key(), + array( + 'hash_check' => wp_hash( '' ), // To detect if salt has changed. + 'secret' => $this->secret, + 'time' => $this->time, + 'user_id' => $this->user_id, + 'url' => $this->url, + ), + $this->cache_ttl + ); + } + + /** + * Deletes the token from cache + * + * @return void + */ + public function delete(): void { + $this->cache->delete( $this->cache_key() ); + } + + /** + * Returns the compiled cache key + * + * @return string + */ + private function cache_key(): string { + return implode( '_', array( $this->cache_key_prefix, $this->user_id ) ); + } + +} diff --git a/modules/ppcp-onboarding/src/Helper/UrlHelper.php b/modules/ppcp-onboarding/src/Helper/UrlHelper.php new file mode 100644 index 000000000..b50402ead --- /dev/null +++ b/modules/ppcp-onboarding/src/Helper/UrlHelper.php @@ -0,0 +1,42 @@ +cache->has( $environment . '-' . $product ) ) { - return $this->cache->get( $environment . '-' . $product ); + $cache_key = $environment . '-' . $product; + + $onboarding_url = new OnboardingUrl( $this->cache, $cache_key, get_current_user_id() ); + + if ( $onboarding_url->load() ) { + return $onboarding_url->get() ?: ''; } + $onboarding_url->init(); + + $data = $this->partner_referrals_data + ->append_onboarding_token( $data, $onboarding_url->token() ?: '' ); + $url = $is_production ? $this->production_partner_referrals->signup_link( $data ) : $this->sandbox_partner_referrals->signup_link( $data ); $url = add_query_arg( $args, $url ); - $this->cache->set( $environment . '-' . $product, $url, 3 * MONTH_IN_SECONDS ); + $onboarding_url->set( $url ); + $onboarding_url->persist(); return $url; } diff --git a/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php b/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php index 0756e73f9..8e00e40c4 100644 --- a/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php +++ b/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php @@ -9,12 +9,15 @@ declare(strict_types=1); namespace WooCommerce\PayPalCommerce\WcGateway\Settings; +use WooCommerce\PayPalCommerce\AdminNotices\Entity\Message; +use WooCommerce\PayPalCommerce\AdminNotices\Repository\Repository; use WooCommerce\PayPalCommerce\ApiClient\Authentication\Bearer; use WooCommerce\PayPalCommerce\ApiClient\Authentication\PayPalBearer; use WooCommerce\PayPalCommerce\ApiClient\Exception\RuntimeException; use WooCommerce\PayPalCommerce\ApiClient\Helper\Cache; use WooCommerce\PayPalCommerce\Http\RedirectorInterface; use WooCommerce\PayPalCommerce\Onboarding\State; +use WooCommerce\PayPalCommerce\Onboarding\Helper\OnboardingUrl; use WooCommerce\PayPalCommerce\WcGateway\Gateway\PayPalGateway; use WooCommerce\PayPalCommerce\WcGateway\Helper\DCCProductStatus; use WooCommerce\PayPalCommerce\WcGateway\Helper\PayUponInvoiceProductStatus; @@ -168,7 +171,7 @@ class SettingsListener { * Listens if the merchant ID should be updated. */ public function listen_for_merchant_id() { - if ( ! $this->is_valid_site_request() || $this->state->current_state() === State::STATE_ONBOARDED ) { + if ( ! $this->is_valid_site_request() ) { return; } @@ -177,17 +180,23 @@ class SettingsListener { * phpcs:disable WordPress.Security.NonceVerification.Missing * phpcs:disable WordPress.Security.NonceVerification.Recommended */ - if ( ! isset( $_GET['merchantIdInPayPal'] ) || ! isset( $_GET['merchantId'] ) ) { + if ( ! isset( $_GET['merchantIdInPayPal'] ) || ! isset( $_GET['merchantId'] ) || ! isset( $_GET['ppcpToken'] ) ) { return; } - $merchant_id = sanitize_text_field( wp_unslash( $_GET['merchantIdInPayPal'] ) ); - $merchant_email = sanitize_text_field( wp_unslash( $_GET['merchantId'] ) ); + + $merchant_id = sanitize_text_field( wp_unslash( $_GET['merchantIdInPayPal'] ) ); + $merchant_email = sanitize_text_field( wp_unslash( $_GET['merchantId'] ) ); + $onboarding_token = sanitize_text_field( wp_unslash( $_GET['ppcpToken'] ) ); // phpcs:enable WordPress.Security.NonceVerification.Missing // phpcs:enable WordPress.Security.NonceVerification.Recommended $this->settings->set( 'merchant_id', $merchant_id ); $this->settings->set( 'merchant_email', $merchant_email ); + if ( ! OnboardingUrl::validate_token_and_delete( $this->signup_link_cache, $onboarding_token, get_current_user_id() ) ) { + $this->onboarding_redirect( false ); + } + $is_sandbox = $this->settings->has( 'sandbox_on' ) && $this->settings->get( 'sandbox_on' ); if ( $is_sandbox ) { $this->settings->set( 'merchant_id_sandbox', $merchant_id ); @@ -203,8 +212,23 @@ class SettingsListener { */ do_action( 'woocommerce_paypal_payments_onboarding_before_redirect' ); - $redirect_url = $this->get_onboarding_redirect_url(); if ( ! $this->settings->has( 'client_id' ) || ! $this->settings->get( 'client_id' ) ) { + $this->onboarding_redirect( false ); + } + + $this->onboarding_redirect(); + } + + /** + * Redirect to the onboarding URL. + * + * @param bool $success Should redirect to the success or error URL. + * @return void + */ + private function onboarding_redirect( bool $success = true ): void { + $redirect_url = $this->get_onboarding_redirect_url(); + + if ( ! $success ) { $redirect_url = add_query_arg( 'ppcp-onboarding-error', '1', $redirect_url ); } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index bddf0058e..42ce967ca 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -685,9 +685,10 @@ listen_for_merchant_id listen_for_vaulting_enabled - + wp_unslash( $_GET['merchantId'] ) wp_unslash( $_GET['merchantIdInPayPal'] ) + wp_unslash( $_GET['ppcpToken'] ) wp_unslash( $_POST['ppcp-nonce'] ) diff --git a/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php b/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php new file mode 100644 index 000000000..63b495792 --- /dev/null +++ b/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php @@ -0,0 +1,193 @@ +alias(function($string) { + return hash('md5', $string); + }); + + $this->cache = \Mockery::mock(Cache::class); + $this->onboardingUrl = new OnboardingUrl($this->cache, $this->cache_key_prefix, $this->user_id); + } + + public function test_validate_token_and_delete_valid() + { + // Prepare the data + $cacheData = [ + 'hash_check' => wp_hash(''), + 'secret' => 'test_secret', + 'time' => time(), + 'user_id' => $this->user_id, + 'url' => 'https://example.com' + ]; + + $token = [ + 'k' => $this->cache_key_prefix, + 'u' => $this->user_id, + 'h' => substr(wp_hash(implode( '|', array( + $this->cache_key_prefix, + $cacheData['user_id'], + $cacheData['secret'], + $cacheData['time'], + ))), 0, 32) + ]; + + $onboarding_token = UrlHelper::url_safe_base64_encode(json_encode($token)); + + // Expectations + $this->cache->shouldReceive('has')->once()->andReturn(true); + $this->cache->shouldReceive('get')->once()->andReturn($cacheData); + $this->cache->shouldReceive('delete')->once(); + + $this->assertTrue( + OnboardingUrl::validate_token_and_delete($this->cache, $onboarding_token, $this->user_id) + ); + } + + public function test_load_valid() + { + // Expectations + $this->cache->shouldReceive('has')->once()->andReturn(true); + $this->cache->shouldReceive('get')->once()->andReturn([ + 'hash_check' => wp_hash(''), + 'secret' => 'test_secret', + 'time' => time(), + 'user_id' => $this->user_id, + 'url' => 'https://example.com' + ]); + + $this->assertTrue($this->onboardingUrl->load()); + } + + public function test_load_invalid() + { + // Expectations + $this->cache->shouldReceive('has')->once()->andReturn(false); + + $this->assertFalse($this->onboardingUrl->load()); + } + + public function test_get_not_initialized() + { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Object not initialized.'); + + $this->onboardingUrl->get(); + } + + public function test_token_not_initialized() + { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Object not initialized.'); + + $this->onboardingUrl->token(); + } + + public function test_persist_not_initialized() + { + // Expectations + $this->cache->shouldReceive('set')->never(); + + $this->onboardingUrl->persist(); + + $this->assertTrue(true); + } + + public function test_delete() + { + // Expectations + $this->cache->shouldReceive('delete')->once(); + + $this->onboardingUrl->delete(); + + $this->assertTrue(true); + } + + public function test_init() + { + $this->onboardingUrl->init(); + + $token = $this->onboardingUrl->token(); + $this->assertNotEmpty($token); + } + + public function test_set_and_get() + { + $this->onboardingUrl->init(); + $this->onboardingUrl->set('https://example.com'); + + $url = $this->onboardingUrl->get(); + $this->assertEquals('https://example.com', $url); + } + + public function test_persist() + { + $this->onboardingUrl->init(); + $this->onboardingUrl->set('https://example.com'); + + // Expectations + $this->cache->shouldReceive('set')->once(); + + $this->onboardingUrl->persist(); + + $this->assertTrue(true); + } + + public function test_token() + { + $this->onboardingUrl->init(); + $this->onboardingUrl->set('https://example.com'); + + $token = $this->onboardingUrl->token(); + $this->assertNotEmpty($token); + } + + public function test_validate_token_and_delete_invalid() + { + // Prepare the data + $token = [ + 'k' => $this->cache_key_prefix, + 'u' => $this->user_id, + 'h' => 'invalid_hash' + ]; + + $onboarding_token = UrlHelper::url_safe_base64_encode(json_encode($token)); + + // Expectations + $this->cache->shouldReceive('has')->once()->andReturn(true); + $this->cache->shouldReceive('get')->once()->andReturn([ + 'hash_check' => wp_hash(''), + 'secret' => 'test_secret', + 'time' => time(), + 'user_id' => $this->user_id, + 'url' => 'https://example.com' + ]); + $this->cache->shouldReceive('delete')->never(); + + $this->assertFalse( + OnboardingUrl::validate_token_and_delete($this->cache, $onboarding_token, $this->user_id) + ); + } + +} From 35fbe42733cf4b71789326676903ea171940a838 Mon Sep 17 00:00:00 2001 From: Pedro Silva Date: Wed, 12 Jul 2023 14:57:57 +0100 Subject: [PATCH 2/3] Refactor WooCommerce\PayPalCommerce\Onboarding\Exception\RuntimeException to \RuntimeException Refactor to use add_query_arg in append_onboarding_token --- .../src/Repository/PartnerReferralsData.php | 5 ++--- .../src/Exception/RuntimeException.php | 18 ------------------ .../src/Helper/OnboardingUrl.php | 2 +- .../src/Settings/SettingsListener.php | 2 +- .../Onboarding/Helper/OnboardingUrlTest.php | 2 +- 5 files changed, 5 insertions(+), 24 deletions(-) delete mode 100644 modules/ppcp-onboarding/src/Exception/RuntimeException.php diff --git a/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php b/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php index fd93e57be..af8bae174 100644 --- a/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php +++ b/modules/ppcp-api-client/src/Repository/PartnerReferralsData.php @@ -136,9 +136,8 @@ class PartnerReferralsData { * @return array */ public function append_onboarding_token( array $data, string $token ): array { - $separator = strpos( $data['partner_config_override']['return_url'], '?' ) === false ? '?' : '&'; - - $data['partner_config_override']['return_url'] .= $separator . 'ppcpToken=' . $token; + $data['partner_config_override']['return_url'] = + add_query_arg( 'ppcpToken', $token, $data['partner_config_override']['return_url'] ); return $data; } } diff --git a/modules/ppcp-onboarding/src/Exception/RuntimeException.php b/modules/ppcp-onboarding/src/Exception/RuntimeException.php deleted file mode 100644 index fc14fc94f..000000000 --- a/modules/ppcp-onboarding/src/Exception/RuntimeException.php +++ /dev/null @@ -1,18 +0,0 @@ -is_valid_site_request() ) { return; } diff --git a/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php b/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php index 63b495792..5dadd2c79 100644 --- a/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php +++ b/tests/PHPUnit/Onboarding/Helper/OnboardingUrlTest.php @@ -5,7 +5,7 @@ namespace WooCommerce\PayPalCommerce\Onboarding\Helper; use PHPUnit\Framework\TestCase; use WooCommerce\PayPalCommerce\ApiClient\Helper\Cache; -use WooCommerce\PayPalCommerce\Onboarding\Exception\RuntimeException; +use RuntimeException; use function Brain\Monkey\Functions\when; class OnboardingUrlTest extends TestCase From 14e4f0743f28fcd9e7761d787e4c99e30975be0a Mon Sep 17 00:00:00 2001 From: Emili Castells Guasch Date: Fri, 14 Jul 2023 11:09:25 +0200 Subject: [PATCH 3/3] Update changelog --- changelog.txt | 1 + readme.txt | 1 + 2 files changed, 2 insertions(+) diff --git a/changelog.txt b/changelog.txt index 37e6ce54e..86c294501 100644 --- a/changelog.txt +++ b/changelog.txt @@ -10,6 +10,7 @@ * Fix - Advanced Card Processing gateway becomes invisible post-plugin update unless admin pages are accessed once #1432 * Fix - Incompatibility with WooCommerce One Page Checkout (or similar use cases) in Version 2.1.0 #1473 * Fix - Prevent Repetitive Token Migration and Database Overload After 2.1.0 Update #1461 +* Fix - Onboarding from connection page with CSRF parameter manipulates email and merchant id fields #1502 * Enhancement - Remove feature flag requirement for express cart/checkout block integration #1483 * Enhancement - Add notice when shop currency is unsupported #1433 * Enhancement - Improve ACDC error message when empty fields #1360 diff --git a/readme.txt b/readme.txt index 7c9d182c4..609a55f1d 100644 --- a/readme.txt +++ b/readme.txt @@ -91,6 +91,7 @@ Follow the steps below to connect the plugin to your PayPal account: * Fix - Advanced Card Processing gateway becomes invisible post-plugin update unless admin pages are accessed once #1432 * Fix - Incompatibility with WooCommerce One Page Checkout (or similar use cases) in Version 2.1.0 #1473 * Fix - Prevent Repetitive Token Migration and Database Overload After 2.1.0 Update #1461 +* Fix - Onboarding from connection page with CSRF parameter manipulates email and merchant id fields #1502 * Enhancement - Remove feature flag requirement for express cart/checkout block integration #1483 * Enhancement - Add notice when shop currency is unsupported #1433 * Enhancement - Improve ACDC error message when empty fields #1360