diff --git a/includes/Core/Authentication/Authentication.php b/includes/Core/Authentication/Authentication.php index cc088be0f6b..3e27c9e47e7 100644 --- a/includes/Core/Authentication/Authentication.php +++ b/includes/Core/Authentication/Authentication.php @@ -284,7 +284,9 @@ public function register() { add_filter( 'googlesitekit_setup_data', $this->get_method_proxy( 'inline_js_setup_data' ) ); add_action( 'admin_init', $this->get_method_proxy( 'handle_oauth' ) ); - add_action( 'admin_init', $this->get_method_proxy( 'check_connected_proxy_url' ) ); + // Run after the connected proxy URL migration (default priority) so it + // encodes the stored value before this check compares it to the site URL. + add_action( 'admin_init', $this->get_method_proxy( 'check_connected_proxy_url' ), 20 ); add_action( 'admin_action_' . self::ACTION_CONNECT, $this->get_method_proxy( 'handle_connect' ) ); add_action( 'admin_action_' . self::ACTION_DISCONNECT, $this->get_method_proxy( 'handle_disconnect' ) ); diff --git a/includes/Core/Authentication/Connected_Proxy_URL.php b/includes/Core/Authentication/Connected_Proxy_URL.php index b811ece498b..fbcc5924391 100644 --- a/includes/Core/Authentication/Connected_Proxy_URL.php +++ b/includes/Core/Authentication/Connected_Proxy_URL.php @@ -30,24 +30,45 @@ class Connected_Proxy_URL extends Setting { * Matches provided URL with the current proxy URL in the settings. * * @since 1.17.0 + * @since n.e.x.t Compares the given URL against the decoded stored URL. * * @param string $url URL to match against the current one in the settings. * @return bool TRUE if URL matches the current one, otherwise FALSE. */ public function matches_url( $url ) { - $sanitize = $this->get_sanitize_callback(); - $normalized = $sanitize( $url ); - return $normalized === $this->get(); + return trailingslashit( $url ) === $this->get(); } /** - * Gets the callback for sanitizing the setting's value before saving. + * Gets the connected proxy URL, decoded from the form the option holds. * - * @since 1.17.0 + * A stored value that fails to decode reads as no value at all, so a + * corrupted option sends the site back through the connection flow rather + * than through a comparison against an unreadable URL. + * + * @since n.e.x.t + * + * @return string|bool Connected proxy URL, or FALSE when the option holds + * no value or one that fails to decode. + */ + public function get() { + $stored_url = parent::get(); + + return is_string( $stored_url ) ? base64_decode( $stored_url, true ) : $stored_url; + } + + /** + * Sets the connected proxy URL. + * + * We encode the URL to prevent database-wide search-and-replace tasks + * from changing the URL used to connect to the Site Kit Proxy service. + * + * @since n.e.x.t * - * @return callable A sanitizing function. + * @param string $value Connected proxy URL. + * @return bool TRUE on success, FALSE on failure. */ - protected function get_sanitize_callback() { - return 'trailingslashit'; + public function set( $value ) { + return parent::set( base64_encode( trailingslashit( $value ) ) ); } } diff --git a/includes/Core/Util/Migration_N_E_X_T.php b/includes/Core/Util/Migration_N_E_X_T.php new file mode 100644 index 00000000000..13f861b1784 --- /dev/null +++ b/includes/Core/Util/Migration_N_E_X_T.php @@ -0,0 +1,109 @@ +options = $options ?: new Options( $context ); + $this->connected_proxy_url = new Connected_Proxy_URL( $this->options ); + } + + /** + * Registers hooks. + * + * @since n.e.x.t + */ + public function register() { + add_action( 'admin_init', array( $this, 'migrate' ) ); + } + + /** + * Migrates the DB. + * + * @since n.e.x.t + */ + public function migrate() { + $db_version = $this->options->get( self::DB_VERSION_OPTION ); + + if ( ! $db_version || version_compare( $db_version, self::DB_VERSION, '<' ) ) { + $this->migrate_connected_proxy_url(); + + $this->options->set( self::DB_VERSION_OPTION, self::DB_VERSION ); + } + } + + /** + * Migrates a plain text connected proxy URL to the encoded format. + * + * Earlier plugin versions stored the URL in plain text. A stored value that + * still starts with its scheme goes back through Connected_Proxy_URL::set(), + * which stores it encoded. + * + * @since n.e.x.t + */ + protected function migrate_connected_proxy_url() { + $stored_url = $this->options->get( Connected_Proxy_URL::OPTION ); + + if ( ! is_string( $stored_url ) || 0 !== strpos( $stored_url, 'http' ) ) { + return; + } + + $this->connected_proxy_url->set( $stored_url ); + } +} diff --git a/includes/Plugin.php b/includes/Plugin.php index 4ca0114edb9..3750f5ab81a 100644 --- a/includes/Plugin.php +++ b/includes/Plugin.php @@ -241,6 +241,7 @@ function () use ( $options, $activation_flag ) { ( new Core\Util\Migration_1_150_0( $this->context, $options ) )->register(); ( new Core\Util\Migration_1_163_0( $this->context, $options ) )->register(); ( new Core\Util\Migration_1_177_0( $this->context, $options ) )->register(); + ( new Core\Util\Migration_N_E_X_T( $this->context, $options ) )->register(); ( new Core\Dashboard_Sharing\Dashboard_Sharing( $this->context ) )->register(); ( new Core\Key_Metrics\Key_Metrics( $this->context, $user_options, $options ) )->register(); ( new Core\Prompts\Prompts( $this->context, $user_options ) )->register(); diff --git a/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php b/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php index 0b18d44ade5..7159e863f6f 100644 --- a/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php +++ b/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php @@ -692,7 +692,7 @@ public function test_set_connected_proxy_url() { do_action( 'googlesitekit_authorize_user', array(), array(), array() ); remove_filter( 'home_url', $home_url_hook ); - $this->assertEquals( 'https://example.com/subsite/', $options->get( Connected_Proxy_URL::OPTION ), 'Connected proxy URL should be set to filtered home_url.' ); // PHPCS: line 692. + $this->assertEquals( base64_encode( 'https://example.com/subsite/' ), $options->get( Connected_Proxy_URL::OPTION ), 'The `set()` method should store the connected proxy URL base64-encoded from the filtered home_url.' ); // PHPCS: line 692. } public function test_check_connected_proxy_url() { @@ -735,6 +735,51 @@ function ( $caps ) { ); } + public function test_check_connected_proxy_url__after_the_home_url_changes() { + remove_all_actions( 'admin_init' ); + + $user_id = $this->factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + + $context = new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ); + $options = new Options( $context ); + $user_options = new User_Options( $context ); + + $authentication = new Authentication( $context, $options, $user_options ); + $authentication->register(); + + // Connect the site with the home URL it runs on. + $authentication->get_connected_proxy_url_instance()->set( $context->get_canonical_home_url() ); + + // Emulate credentials. + $this->fake_proxy_site_connection(); + + // Emulate OAuth access token. + $authentication->get_oauth_client()->set_token( array( 'access_token' => 'valid-auth-token' ) ); + + // Grant the administrator the Permissions::SETUP capability regardless + // of authentication. + add_filter( + 'user_has_cap', + function ( $caps ) { + $caps[ Permissions::SETUP ] = true; + return $caps; + } + ); + + // Move the site to another domain, as a search and replace over the + // database does. + update_option( 'home', 'https://new-domain.example.com' ); + + do_action( 'admin_init' ); + + $this->assertEquals( + Disconnected_Reason::REASON_CONNECTED_URL_MISMATCH, + $user_options->get( Disconnected_Reason::OPTION ), + 'Site Kit should flag the URL change once the home URL moves to another domain.' + ); + } + /** * Test handle_proxy_permissions(). */ diff --git a/tests/phpunit/integration/Core/Authentication/Connected_Proxy_URLTest.php b/tests/phpunit/integration/Core/Authentication/Connected_Proxy_URLTest.php index d8258df6e1b..cbd61434f4e 100644 --- a/tests/phpunit/integration/Core/Authentication/Connected_Proxy_URLTest.php +++ b/tests/phpunit/integration/Core/Authentication/Connected_Proxy_URLTest.php @@ -2,11 +2,11 @@ /** * Class Google\Site_Kit\Core\Authentication\Connected_Proxy_URLTest * - * @package Google\Site_Kit + * @package Google\Site_Kit\Tests\Core\Authentication * @copyright 2021 Google LLC * @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0 * @link https://sitekit.withgoogle.com - * */ + */ namespace Google\Site_Kit\Tests\Core\Authentication; @@ -48,6 +48,58 @@ public function test_matches_url() { $this->assertTrue( $connected_proxy_url->matches_url( 'https://example.com/' ), 'URL should match when both have trailing slashes.' ); } + public function test_matches_url__after_a_database_search_and_replace() { + $connected_proxy_url = new Connected_Proxy_URL( $this->options ); + $connected_proxy_url->register(); + + $connected_proxy_url->set( 'https://old-site.example.com' ); + + // Simulate a database search and replace of the site domain. + $stored_url = $this->options->get( Connected_Proxy_URL::OPTION ); + $rewritten_url = str_replace( 'old-site.example.com', 'new-site.example.com', $stored_url ); + + $this->assertSame( $stored_url, $rewritten_url, 'A search and replace should find no domain to rewrite inside the stored value.' ); + $this->assertTrue( $connected_proxy_url->matches_url( 'https://old-site.example.com' ), 'The stored URL should still match the connected URL after a search and replace.' ); + } + + public function test_set__stores_an_encoded_url() { + $connected_proxy_url = new Connected_Proxy_URL( $this->options ); + $connected_proxy_url->register(); + + $connected_proxy_url->set( 'https://example.com' ); + + $this->assertEquals( + base64_encode( 'https://example.com/' ), + $this->options->get( Connected_Proxy_URL::OPTION ), + 'The `set()` method should store the URL as a base64-encoded string, with a trailing slash.' + ); + } + + public function test_get__decodes_the_stored_url() { + $connected_proxy_url = new Connected_Proxy_URL( $this->options ); + $connected_proxy_url->register(); + + $connected_proxy_url->set( 'https://example.com' ); + + $this->assertEquals( 'https://example.com/', $connected_proxy_url->get(), 'The `get()` method should return the URL in plain text.' ); + } + + public function test_get__returns_false_when_not_set() { + $connected_proxy_url = new Connected_Proxy_URL( $this->options ); + $connected_proxy_url->register(); + + $this->assertFalse( $connected_proxy_url->get(), 'The `get()` method should return FALSE when no value exists.' ); + } + + public function test_get__returns_false_for_a_value_that_fails_to_decode() { + $connected_proxy_url = new Connected_Proxy_URL( $this->options ); + $connected_proxy_url->register(); + + $this->options->set( Connected_Proxy_URL::OPTION, 'not*a*valid*value' ); + + $this->assertFalse( $connected_proxy_url->get(), 'The `get()` method should return FALSE for a stored value that fails to decode.' ); + } + /** * @inheritDoc */ diff --git a/tests/phpunit/integration/Core/Util/Migration_N_E_X_TTest.php b/tests/phpunit/integration/Core/Util/Migration_N_E_X_TTest.php new file mode 100644 index 00000000000..32920280d14 --- /dev/null +++ b/tests/phpunit/integration/Core/Util/Migration_N_E_X_TTest.php @@ -0,0 +1,186 @@ +context = new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ); + $this->options = new Options( $this->context ); + $this->connected_proxy_url = new Connected_Proxy_URL( $this->options ); + + // Drop the option, so each test starts from the value it stores itself. + $this->options->delete( Connected_Proxy_URL::OPTION ); + + $this->delete_db_version(); + } + + public function get_new_migration_instance() { + return new Migration_N_E_X_T( + $this->context, + $this->options + ); + } + + public function test_register() { + $migration = $this->get_new_migration_instance(); + remove_all_actions( 'admin_init' ); + + $migration->register(); + + $this->assertNotFalse( + has_action( 'admin_init', array( $migration, 'migrate' ) ), + 'Migration should register migrate on admin_init.' + ); + } + + public function test_migrate__encodes_a_legacy_plain_text_url() { + $migration = $this->get_new_migration_instance(); + + // Store the option in plain text, the way earlier plugin versions saved it. + $this->options->set( Connected_Proxy_URL::OPTION, 'https://example.com/' ); + + $migration->migrate(); + + $this->assertEquals( + base64_encode( 'https://example.com/' ), + $this->options->get( Connected_Proxy_URL::OPTION ), + 'Migration should store the legacy plain text URL encoded.' + ); + $this->assertEquals( + 'https://example.com/', + $this->connected_proxy_url->get(), + 'The `get()` method should still return the plain text URL after the migration.' + ); + } + + public function test_migrate__keeps_an_already_encoded_url() { + $migration = $this->get_new_migration_instance(); + + $this->connected_proxy_url->set( 'https://example.com' ); + $stored_connected_url = $this->options->get( Connected_Proxy_URL::OPTION ); + + $migration->migrate(); + + $this->assertSame( + $stored_connected_url, + $this->options->get( Connected_Proxy_URL::OPTION ), + 'Migration should keep an already encoded value unchanged.' + ); + } + + public function test_migrate__runs_before_the_connected_proxy_url_check() { + remove_all_actions( 'admin_init' ); + + $user_id = $this->factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + + $user_options = new User_Options( $this->context ); + + // Register in the order the plugin does. The connected proxy URL check + // runs at a later priority, so the migration encodes the stored value + // before the check reads it. + $authentication = new Authentication( $this->context, $this->options, $user_options ); + $authentication->register(); + $this->get_new_migration_instance()->register(); + + // Store the home URL in plain text, the way earlier plugin versions saved it. + $this->options->set( Connected_Proxy_URL::OPTION, $this->context->get_canonical_home_url() ); + + // Emulate credentials. + $this->fake_proxy_site_connection(); + + // Emulate OAuth access token. + $authentication->get_oauth_client()->set_token( array( 'access_token' => 'valid-auth-token' ) ); + + // Grant the administrator the Permissions::SETUP capability regardless + // of authentication. + add_filter( + 'user_has_cap', + function ( $caps ) { + $caps[ Permissions::SETUP ] = true; + return $caps; + } + ); + + do_action( 'admin_init' ); + + $this->assertEquals( + base64_encode( trailingslashit( $this->context->get_canonical_home_url() ) ), + $this->options->get( Connected_Proxy_URL::OPTION ), + 'Migration should encode the plain text URL on admin_init.' + ); + $this->assertFalse( + $user_options->get( Disconnected_Reason::OPTION ), + 'Site Kit should stay connected when the migration encodes the URL the site still runs on.' + ); + } + + public function test_migrate__skips_when_the_option_is_missing() { + $migration = $this->get_new_migration_instance(); + + $migration->migrate(); + + $this->assertOptionNotExists( Connected_Proxy_URL::OPTION ); + } + + public function test_migrate__sets_db_version() { + $migration = $this->get_new_migration_instance(); + + $migration->migrate(); + + $this->assertEquals( 'n.e.x.t', $this->get_db_version(), "Database version should update to the migration's target version after the migration runs." ); + } + + public function test_migrate__skips_when_the_db_version_is_current() { + $migration = $this->get_new_migration_instance(); + + $this->options->set( Migration_N_E_X_T::DB_VERSION_OPTION, Migration_N_E_X_T::DB_VERSION ); + + // Store the option in plain text, the way earlier plugin versions saved it. + $this->options->set( Connected_Proxy_URL::OPTION, 'https://example.com/' ); + + $migration->migrate(); + + $this->assertEquals( + 'https://example.com/', + $this->options->get( Connected_Proxy_URL::OPTION ), + 'Migration should leave the stored value alone when the database version already matches its target.' + ); + } + + protected function get_db_version() { + return $this->options->get( Migration_N_E_X_T::DB_VERSION_OPTION ); + } + + protected function delete_db_version() { + $this->options->delete( Migration_N_E_X_T::DB_VERSION_OPTION ); + } +}