diff --git a/.github/unit-tests-suites.yml b/.github/unit-tests-suites.yml index 5c52b60061..59b1901bd6 100644 --- a/.github/unit-tests-suites.yml +++ b/.github/unit-tests-suites.yml @@ -99,3 +99,11 @@ suites: script: make:test paths: - 'environments/make/**' + + openverse: + type: wordpress + name: Openverse Theme + script: openverse:test + paths: + - 'environments/openverse/**' + - 'wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/**' diff --git a/environments/openverse/.wp-env.test.json b/environments/openverse/.wp-env.test.json new file mode 100644 index 0000000000..4ed3d81ea3 --- /dev/null +++ b/environments/openverse/.wp-env.test.json @@ -0,0 +1,12 @@ +{ + "core": "WordPress/WordPress#master", + "phpVersion": "8.4", + "testsEnvironment": false, + "plugins": [], + "themes": [ + "../wordpress.org/public_html/wp-content/themes/pub/wporg-openverse" + ], + "lifecycleScripts": { + "afterStart": "bash openverse/bin/after-start-test.sh" + } +} diff --git a/environments/openverse/bin/after-start-test.sh b/environments/openverse/bin/after-start-test.sh new file mode 100755 index 0000000000..2815193f22 --- /dev/null +++ b/environments/openverse/bin/after-start-test.sh @@ -0,0 +1,14 @@ +#!/bin/bash +# +# Runs after wp-env start for the test environment. +# Installs PHPUnit 11 and Yoast polyfills in the test container. +# + +set -euo pipefail + +CONFIG="--config openverse/.wp-env.test.json" +RUN="npx wp-env $CONFIG run cli" + +echo "Installing PHPUnit 11 and polyfills..." +$RUN composer global require -W phpunit/phpunit:^11.0 2>&1 +$RUN composer require --dev yoast/phpunit-polyfills:^4.0 --working-dir=/wordpress-phpunit 2>&1 diff --git a/environments/package.json b/environments/package.json index 9dd9208342..3bab252c3f 100644 --- a/environments/package.json +++ b/environments/package.json @@ -25,6 +25,8 @@ "make:test": "npm run make:test:env -- start && npm run make:test:posting-access && npm run make:test:trac-watcher && npm run make:test:cli", "jobs:env": "wp-env --config jobs/.wp-env.json", "browsehappy:env": "wp-env --config browsehappy/.wp-env.json", + "openverse:test:env": "wp-env --config openverse/.wp-env.test.json", + "openverse:test": "npm run openverse:test:env -- start && npm run openverse:test:env -- run cli --env-cwd=wp-content/themes/wporg-openverse phpunit", "translate:env": "wp-env --config translate/.wp-env.json", "translate:import": "npm run translate:env -- run cli -- wp eval-file wp-content/env-bin/import-from-wporg.php", "translate:refresh": "npm run translate:env -- run cli -- wp option delete wporg_translate_env_seeded" diff --git a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/functions.php b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/functions.php index a861d77e38..06b562afe9 100644 --- a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/functions.php +++ b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/functions.php @@ -255,9 +255,14 @@ function wporg_ov_customizer( $wp_customize ) { * Examples: * - https://ru.wordpress.org/openverse → {ov_redirect_url}/ru/ * - https://wordpress.org/openverse/search/?q=dog → {ov_redirect_url}/search/?q=dog + * + * The returned URL always carries at least a trailing path separator, so a bare + * `/openverse` resolves to `{ov_redirect_url}/` rather than to the origin alone. + * + * @return string */ function get_target_url() { - $target_url = get_theme_mod( 'ov_redirect_url', OPENVERSE_STANDALONE_URL ); + $target_url = get_standalone_origin(); $curr_locale = get_locale(); $locale = get_locale_slug( $curr_locale ); @@ -265,15 +270,82 @@ function get_target_url() { $target_url .= '/' . $locale; } - $path = $_SERVER['REQUEST_URI']; - if ( $path ) { - $count = 1; // Only replace the leading Openverse subpath. - $target_url .= str_replace( OPENVERSE_SUBPATH, '', $path, $count ); + // Sanitising is required by WordPress.Security.ValidatedSanitizedInput. + // Not `sanitize_text_field()`, which strips percent-encoded octets and + // would turn `?q=cat%20dog` into `?q=catdog`. + $path = isset( $_SERVER['REQUEST_URI'] ) ? esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ) : ''; + + // Only a leading, whole-segment subpath is removed. `str_replace()` could + // express neither constraint: it replaced every occurrence, including one + // in a later segment such as `/image/openverse-logo/`, one in the query + // string, and the `/openverse` inside a longer segment like + // `/openverse-search`. + if ( OPENVERSE_SUBPATH === $path + || str_starts_with( $path, OPENVERSE_SUBPATH . '/' ) + || str_starts_with( $path, OPENVERSE_SUBPATH . '?' ) + ) { + $path = substr( $path, strlen( OPENVERSE_SUBPATH ) ); } + // The origin has no trailing slash, so the path must supply the separator. + // Prepending it is also what keeps the remainder in the path rather than + // the authority; `ltrim()` only collapses a doubled slash. + $target_url .= '/' . ltrim( $path, '/' ); + return $target_url; } +/** + * The origin the standalone Openverse site is served from. + * + * The Customizer's `sanitize_callback` drops a trailing slash and + * `wp theme mod set` does not, so it is dropped here too. + * + * @return string + */ +function get_standalone_origin() { + return untrailingslashit( get_theme_mod( 'ov_redirect_url', OPENVERSE_STANDALONE_URL ) ); +} + +/** + * Whether the redirect to the standalone site is switched on. + * + * `wp theme mod set` stores the string it is handed, so the setting can hold + * `'false'`, which PHP reads as true. `wp_validate_boolean()` reads it the way + * whoever typed it meant it. + * + * @return bool + */ +function is_redirect_enabled() { + return wp_validate_boolean( get_theme_mod( 'ov_is_redirect_enabled', false ) ); +} + +/** + * Whether a redirect target is usable. + * + * The path is appended straight after the origin, so a target's authority is + * always the one an administrator configured. Comparing the two makes that an + * enforced property rather than an assumption, and it is why this does not need + * `wp_safe_redirect()`: no request can reach a host the origin did not supply, + * and the allow-list that would require widens redirects for the whole site. + * + * @param string $target_url URL the theme intends to redirect to. + * @return bool + */ +function is_valid_target_url( $target_url ) { + $parts = wp_parse_url( $target_url ); + + if ( empty( $parts['host'] ) || empty( $parts['scheme'] ) ) { + return false; + } + + if ( ! in_array( strtolower( $parts['scheme'] ), array( 'http', 'https' ), true ) ) { + return false; + } + + return wp_parse_url( get_standalone_origin(), PHP_URL_HOST ) === $parts['host']; +} + /** * Provide configuration for the theme to redirect to the given standalone * Openverse site. The destination URL can be configured and the behaviour can diff --git a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/index.php b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/index.php index 0a7e323f96..a178465cfb 100644 --- a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/index.php +++ b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/index.php @@ -18,16 +18,19 @@ If the theme mod `ov_is_redirect_enabled` is set to `true`, redirect to the standalone site and exit immediately. If not, print what would have been the redirect URL to the HTML as a comment. + + The target is checked before redirecting. A misconfigured `ov_redirect_url` + would otherwise send a permanent redirect that sits in visitors' caches long + after the setting was corrected. Rendering the page is the safer failure. */ -$is_redirect_enabled = get_theme_mod( 'ov_is_redirect_enabled' ); $target_url = get_target_url(); -if ( $is_redirect_enabled ) { +if ( is_redirect_enabled() && is_valid_target_url( $target_url ) ) { wp_redirect( $target_url, 301 ); exit; } else { - echo ""; + echo ''; } get_header(); diff --git a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/phpunit.xml.dist b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/phpunit.xml.dist new file mode 100644 index 0000000000..cb45860339 --- /dev/null +++ b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/phpunit.xml.dist @@ -0,0 +1,13 @@ + + + + tests/ + tests/bootstrap.php + tests/locales-stub.php + + + diff --git a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/Target_Url_Test.php b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/Target_Url_Test.php new file mode 100644 index 0000000000..bfabeefbbd --- /dev/null +++ b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/Target_Url_Test.php @@ -0,0 +1,327 @@ +request_uri = $_SERVER['REQUEST_URI'] ?? null; + + add_filter( 'theme_mod_ov_is_redirect_enabled', '__return_true' ); + add_filter( 'theme_mod_ov_redirect_url', array( $this, 'origin' ) ); + } + + /** + * Restores the globals and filters the test changed. + */ + protected function tearDown(): void { + remove_filter( 'theme_mod_ov_is_redirect_enabled', '__return_true' ); + remove_filter( 'theme_mod_ov_redirect_url', array( $this, 'origin' ) ); + remove_filter( 'locale', array( $this, 'polish' ) ); + + if ( null === $this->request_uri ) { + unset( $_SERVER['REQUEST_URI'] ); + } else { + $_SERVER['REQUEST_URI'] = $this->request_uri; + } + + parent::tearDown(); + } + + /** + * Filter callback supplying the configured origin. + */ + public function origin(): string { + return self::ORIGIN; + } + + /** + * Filter callback switching the site to Polish. + */ + public function polish(): string { + return 'pl_PL'; + } + + /** + * Requests and the URL each one should be forwarded to. + * + * @return array + */ + public static function requests(): array { + return array( + 'site root' => array( '/openverse/', self::ORIGIN . '/' ), + 'no trailing slash' => array( '/openverse', self::ORIGIN . '/' ), + 'query straight after' => array( '/openverse?q=dog', self::ORIGIN . '/?q=dog' ), + 'search' => array( '/openverse/search/?q=dog', self::ORIGIN . '/search/?q=dog' ), + 'encoded space' => array( '/openverse/search/?q=cat%20dog', self::ORIGIN . '/search/?q=cat%20dog' ), + 'non-ascii' => array( '/openverse/search/?q=caf%C3%A9', self::ORIGIN . '/search/?q=caf%C3%A9' ), + 'several parameters' => array( '/openverse/search/?q=dog&license=cc0', self::ORIGIN . '/search/?q=dog&license=cc0' ), + 'page' => array( '/openverse/about/', self::ORIGIN . '/about/' ), + 'subpath inside a later segment' => array( '/openverse/image/openverse-logo/', self::ORIGIN . '/image/openverse-logo/' ), + 'subpath as a later segment' => array( '/openverse/tag/openverse/', self::ORIGIN . '/tag/openverse/' ), + 'subpath in the query' => array( '/openverse/search/?q=/openverse', self::ORIGIN . '/search/?q=/openverse' ), + 'longer first segment' => array( '/openverse-search', self::ORIGIN . '/openverse-search' ), + ); + } + + /** + * Only a leading, whole-segment subpath is removed, and the rest is + * forwarded untouched. + * + * @param string $request_uri The incoming request. + * @param string $expected The URL it should map to. + */ + #[DataProvider( 'requests' )] + public function test_forwards_the_request_path( string $request_uri, string $expected ): void { + $_SERVER['REQUEST_URI'] = $request_uri; + + $this->assertSame( $expected, get_target_url() ); + } + + /** + * Requests whose remainder could be read as part of a host name. + * + * @return array + */ + public static function awkward_requests(): array { + return array( + 'at sign after the subpath' => array( '/openverse/openverse@example.com/' ), + 'dot after the subpath' => array( '/openverse/openverse.example.com/' ), + 'doubled slash' => array( '/openverse//example.com' ), + 'at sign only' => array( '/openverse/@example.com' ), + 'backslash' => array( '/openverse/\\\\example.com' ), + 'encoded slashes' => array( '/openverse/%2F%2Fexample.com' ), + 'traversal' => array( '/openverse/..//example.com' ), + 'subpath twice over' => array( '/openverse/openverse/openverse@example.com/' ), + ); + } + + /** + * The forwarded URL always stays on the configured host. + * + * The remainder is appended to an origin that carries no trailing slash, so + * anything that does not start the path cleanly would land in the authority + * instead. + * + * @param string $request_uri The incoming request. + */ + #[DataProvider( 'awkward_requests' )] + public function test_keeps_the_configured_host( string $request_uri ): void { + $_SERVER['REQUEST_URI'] = $request_uri; + + $target = get_target_url(); + + $this->assertStringStartsWith( + self::ORIGIN . '/', + $target, + 'Forwarded somewhere other than a path under the origin: ' . $target + ); + } + + /** + * A locale slug is inserted between the origin and the path. + */ + public function test_inserts_the_locale_before_the_path(): void { + add_filter( 'locale', array( $this, 'polish' ) ); + $_SERVER['REQUEST_URI'] = '/openverse/search/?q=dog'; + + $this->assertSame( self::ORIGIN . '/pl/search/?q=dog', get_target_url() ); + } + + /** + * Configured origins, and whether the theme will redirect to them. + * + * @return array + */ + public static function targets(): array { + return array( + 'https' => array( 'https://openverse.org/search/', true ), + 'http' => array( 'http://openverse.org/search/', true ), + 'explicit port' => array( 'https://openverse.org:8443/search/', true ), + 'no scheme' => array( 'openverse.org/search/', false ), + 'scheme relative' => array( '//openverse.org/search/', false ), + 'other scheme' => array( 'ftp://openverse.org/search/', false ), + 'javascript' => array( 'javascript:alert(1)', false ), + 'path only' => array( '/search/', false ), + 'empty' => array( '', false ), + 'another host' => array( 'https://example.com/search/', false ), + ); + } + + /** + * Only an absolute http or https URL on an allowed host is redirected to. + * + * The authority comes from the configured origin, never from the request, + * so the guard checks the target's host against that origin directly. + * + * @param string $target The URL the theme would redirect to. + * @param bool $expected Whether it should be redirected to. + */ + #[DataProvider( 'targets' )] + public function test_redirects_only_to_an_absolute_http_url( string $target, bool $expected ): void { + $this->assertSame( $expected, is_valid_target_url( $target ) ); + } + + /** + * Every request the theme forwards is accepted and sent as it was built. + * + * Asserting the guard on its own only restates it. This walks the whole + * path, from request through the guard to the header `wp_redirect()` would + * send, so a target the guard accepts but core would mangle shows up here. + * + * @param string $request_uri The incoming request. + */ + #[DataProvider( 'every_request' )] + public function test_a_forwarded_request_is_sent_unchanged( string $request_uri ): void { + $_SERVER['REQUEST_URI'] = $request_uri; + $target = get_target_url(); + + $this->assertTrue( is_valid_target_url( $target ), "Refused to redirect to {$target}" ); + $this->assertSame( + $target, + wp_sanitize_redirect( $target ), + "wp_redirect() would not have sent {$target} unchanged" + ); + } + + /** + * Every request used anywhere in this file. + * + * @return array + */ + public static function every_request(): array { + $requests = array(); + + foreach ( self::requests() as $name => $case ) { + $requests[ $name ] = array( $case[0] ); + } + + return array_merge( $requests, self::awkward_requests() ); + } + + /** + * Values the switch can hold, and whether each one means "on". + * + * @return array + */ + public static function switch_values(): array { + return array( + 'boolean true' => array( true, true ), + 'boolean false' => array( false, false ), + 'one' => array( '1', true ), + 'zero' => array( '0', false ), + 'empty string' => array( '', false ), + 'the word true' => array( 'true', true ), + 'the word false' => array( 'false', false ), + 'upper case' => array( 'FALSE', false ), + ); + } + + /** + * The switch reads the string forms `wp theme mod set` stores. + * + * The command stores its argument verbatim, so the setting can hold + * `'false'`, and a plain truthiness test would turn the redirect on for it. + * + * @param mixed $stored What the theme mod holds. + * @param bool $expected Whether the redirect should run. + */ + #[DataProvider( 'switch_values' )] + public function test_reads_the_switch_as_it_was_written( $stored, bool $expected ): void { + remove_filter( 'theme_mod_ov_is_redirect_enabled', '__return_true' ); + add_filter( 'theme_mod_ov_is_redirect_enabled', static fn() => $stored ); + + $this->assertSame( $expected, is_redirect_enabled() ); + + remove_all_filters( 'theme_mod_ov_is_redirect_enabled' ); + add_filter( 'theme_mod_ov_is_redirect_enabled', '__return_true' ); + } + + /** + * Origins the setting can hold, and the URL each one forwards a search to. + * + * @return array + */ + public static function origins(): array { + return array( + 'plain' => array( 'https://openverse.org', 'https://openverse.org/search/' ), + 'trailing slash' => array( 'https://openverse.org/', 'https://openverse.org/search/' ), + 'http' => array( 'http://openverse.org', 'http://openverse.org/search/' ), + 'explicit port' => array( 'https://openverse.org:8443', 'https://openverse.org:8443/search/' ), + 'upper case scheme' => array( 'HTTPS://openverse.org', 'HTTPS://openverse.org/search/' ), + 'no scheme' => array( 'openverse.org', null ), + 'scheme relative' => array( '//openverse.org', null ), + 'other scheme' => array( 'ftp://openverse.org', null ), + ); + } + + /** + * A configured origin either forwards to itself or is refused outright. + * + * The trailing-slash row matters because the Customizer strips one and + * `wp theme mod set` does not. + * + * @param string $origin The configured origin. + * @param string|null $expected The URL a search forwards to, or null when + * the theme should render the page instead. + */ + #[DataProvider( 'origins' )] + public function test_forwards_only_to_a_usable_origin( string $origin, ?string $expected ): void { + remove_filter( 'theme_mod_ov_redirect_url', array( $this, 'origin' ) ); + add_filter( 'theme_mod_ov_redirect_url', static fn() => $origin ); + $_SERVER['REQUEST_URI'] = '/openverse/search/'; + + $target = get_target_url(); + $sent = is_valid_target_url( $target ) ? $target : null; + + $this->assertSame( $expected, $sent ); + + remove_all_filters( 'theme_mod_ov_redirect_url' ); + add_filter( 'theme_mod_ov_redirect_url', array( $this, 'origin' ) ); + } +} diff --git a/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/bootstrap.php b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/bootstrap.php new file mode 100644 index 0000000000..274f265c69 --- /dev/null +++ b/wordpress.org/public_html/wp-content/themes/pub/wporg-openverse/tests/bootstrap.php @@ -0,0 +1,60 @@ + (object) array( 'slug' => 'pl' ), + ); + } +}