diff --git a/cspell.json b/cspell.json index 5c7e7da..1777d7e 100644 --- a/cspell.json +++ b/cspell.json @@ -29,6 +29,7 @@ "nexcess", "packagist", "pagenow", + "PCRE", "phpdotenv", "phpstan", "phpunit", diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php index 4569885..1832a28 100644 --- a/src/Conflict/Redirector.php +++ b/src/Conflict/Redirector.php @@ -84,7 +84,10 @@ public function after_deactivation( $request_uri ): string { private function screen_from_path( string $path ): string { $screen = basename( $path ); - if ( (bool) preg_match( '/^[A-Za-z0-9_-]+\.php$/', $screen ) ) { + // Anchored with \z rather than $, which in PCRE also matches immediately before a trailing + // newline -- so "edit.php\n" would satisfy $ and a line break would leave here inside the + // one value this class promises is validated. + if ( (bool) preg_match( '/^[A-Za-z0-9_-]+\.php\z/', $screen ) ) { return $screen; } @@ -107,7 +110,7 @@ private function screen_from_path( string $path ): string { } /** - * The current request's query, sanitised, ready to append to a screen name. + * The current request's query, re-encoded, ready to append to a screen name. * * The query carries which list, which page and which filter the user was looking at, so * dropping it would re-render the screen showing something else. It is taken apart and rebuilt @@ -135,17 +138,53 @@ private function query_string( string $request_uri ): string { $args = []; wp_parse_str( $query, $args ); - $sanitized = map_deep( $args, 'sanitize_text_field' ); + $args = $this->without_line_breaks( $args ); - if ( ! is_array( $sanitized ) || $sanitized === [] ) { + if ( $args === [] ) { return ''; } - $rebuilt = http_build_query( $sanitized, '', '&', PHP_QUERY_RFC3986 ); + $rebuilt = http_build_query( $args, '', '&', PHP_QUERY_RFC3986 ); return $rebuilt === '' ? '' : '?' . $rebuilt; } + /** + * The parsed query with CR, LF and NUL taken out of every string in it, and nothing else. + * + * The property being protected is that the destination cannot end a header: it is handed to + * wp_safe_redirect(), which puts it in a Location. That is all that is being protected, because + * it is all that is left to protect -- http_build_query() re-encodes both halves of every pair + * with PHP_QUERY_RFC3986, so no value can add a parameter, open a fragment or arrive as markup, + * whatever it holds. + * + * Deliberately not sanitize_text_field(), which is what stood here and must not be restored. + * wp_parse_str() has already url-decoded these values, and _sanitize_text_fields() deletes every + * '%xx' sequence it can find and entity-encodes a bare '<' -- so a search for '100%ab' would be + * re-run as '100', and one for 'a $args Query arguments as wp_parse_str() produced them. + * + * @return array + */ + private function without_line_breaks( array $args ): array { + foreach ( $args as $key => $value ) { + if ( is_array( $value ) ) { + $args[ $key ] = $this->without_line_breaks( $value ); + continue; + } + + if ( is_string( $value ) ) { + $args[ $key ] = str_replace( [ "\r", "\n", "\0" ], '', $value ); + } + } + + return $args; + } + /** * An absolute admin URL for a screen, on whichever of the three admins this request belongs to. * diff --git a/src/Conflict/Rewriter.php b/src/Conflict/Rewriter.php index 5953826..cff4030 100644 --- a/src/Conflict/Rewriter.php +++ b/src/Conflict/Rewriter.php @@ -84,11 +84,20 @@ public function rewrite( string $markup ): string { // phpcs:disable WordPress.Security.NonceVerification.Recommended -- verified below, once // the plugin named turns out to be one this library owns. Nothing is acted on until then. - $basename = isset( $_GET['plugin'] ) - ? sanitize_text_field( wp_unslash( $_GET['plugin'] ) ) - : ''; - - if ( $basename === '' ) { + $basename = isset( $_GET['plugin'] ) ? wp_unslash( $_GET['plugin'] ) : ''; + + // Unslashed and no further. Core mints the activation-error nonce from + // wp_unslash( $_REQUEST['plugin'] ) verbatim (wp-admin/plugins.php), so sanitizing here + // would verify an action core never signed: a plugin whose folder name holds a '%xx' + // sequence, a '<' or a leading space comes back changed from sanitize_text_field(), and + // both the nonce check and the registry lookup below would then miss -- silently, and on + // the one screen this class exists to improve. Nothing sanitizing would remove is needed + // here either: the value is compared against a basename the host configured and hashed into + // a nonce action, and never reaches the page. What does reach it is the sub-plugin's message. + // + // is_string() because sanitize_text_field() was doing that job: '?plugin[]=x' arrives as an + // array, and an array reaching wp_verify_nonce() is a string conversion, not a refusal. + if ( ! is_string( $basename ) || $basename === '' ) { return $markup; } diff --git a/tests/unit/Conflict/RedirectorTest.php b/tests/unit/Conflict/RedirectorTest.php index 5b1676a..68b6f65 100644 --- a/tests/unit/Conflict/RedirectorTest.php +++ b/tests/unit/Conflict/RedirectorTest.php @@ -66,9 +66,23 @@ public static function request_uris(): Generator { yield 'an absolute url' => [ 'https://example.test/wp-admin/edit.php?post_type=page', admin_url( 'edit.php?post_type=page' ) ]; // Rebuilt rather than carried over, so a value that would otherwise start a parameter or a - // fragment of its own comes back encoded, and markup does not come back at all. + // fragment of its own comes back encoded. Encoding is the whole defence, which is why the + // value itself is left alone: what the user searched for is what gets searched again. yield 'a query value that could break out' => [ '/wp-admin/edit.php?s=foo%26post_type%3Dpage', admin_url( 'edit.php?s=foo%26post_type%3Dpage' ) ]; - yield 'a query value carrying markup' => [ '/wp-admin/edit.php?s=%3Cb%3Ehi%3C%2Fb%3E', admin_url( 'edit.php?s=hi' ) ]; + yield 'a query value carrying markup' => [ '/wp-admin/edit.php?s=%3Cb%3Ehi%3C%2Fb%3E', admin_url( 'edit.php?s=%3Cb%3Ehi%3C%2Fb%3E' ) ]; + + // The two shapes sanitize_text_field() destroys, and the reason it is not used here: it runs + // after wp_parse_str() has url-decoded the value, so it deletes every '%xx' sequence and + // entity-encodes a bare '<'. A search for '100%ab' would come back as '100' and one for + // 'a [ '/wp-admin/edit.php?s=100%25ab', admin_url( 'edit.php?s=100%25ab' ) ]; + yield 'a query value holding a less-than' => [ '/wp-admin/edit.php?s=a%3Cb', admin_url( 'edit.php?s=a%3Cb' ) ]; + + // CR, LF and the NUL byte are the exception, because the destination is handed to + // wp_safe_redirect() and ends up in a Location header. + yield 'a query value carrying a line break' => [ '/wp-admin/edit.php?s=a%0D%0Ab', admin_url( 'edit.php?s=ab' ) ]; + yield 'a query value carrying a null byte' => [ '/wp-admin/edit.php?s=a%00b', admin_url( 'edit.php?s=ab' ) ]; // An admin root names the dashboard by leaving it out, exactly as core's own /wp-admin/ link // does. Sending an admin who asked for the dashboard to the plugins list instead is the @@ -100,6 +114,7 @@ public static function request_uris(): Generator { */ public function test_it_falls_back_when_the_request_uri_is_not_a_string(): void { $this->setFunctionReturn( 'is_network_admin', false ); + $this->setFunctionReturn( 'is_user_admin', false ); /** @phpstan-ignore-next-line argument.type (the point of the test is the value the type forbids). */ $destination = ( new Redirector() )->after_deactivation( [ '/wp-admin/edit.php' ] ); diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index 32d6cf2..d2e2a38 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -130,12 +130,54 @@ public function test_it_uses_the_sub_plugin_whose_standalone_the_request_names() $this->assertStringNotContainsString( 'The wrong one.', $filtered ); } + /** + * Core mints the activation-error nonce from `wp_unslash( $_REQUEST['plugin'] )` and nothing + * else (wp-admin/plugins.php), so the value this class verifies against has to be the same one. + * Sanitizing it first signs one action and checks another, and the rewrite then declines on + * exactly the screen it exists to improve — silently, because declining looks identical to + * "this plugin is none of ours". + * + * @dataProvider standalones_sanitizing_would_alter + * + * @param string $standalone Basename of a standalone whose folder name sanitizing changes. + */ + public function test_it_rewrites_for_a_standalone_whose_basename_sanitizing_would_alter( string $standalone ): void { + $rewriter = $this->make_rewriter( + $this->make_sub_plugin( + [ + 'standalone_plugin_basename' => $standalone, + 'conflict_notice_message' => static fn() => 'Ours.', + ] + ) + ); + + $_GET['plugin'] = $standalone; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . $standalone ); + + $this->assertStringContainsString( 'Ours.', $rewriter->rewrite( self::MARKUP ) ); + } + + /** + * Every one of these is a directory a plugin can really be unzipped into, and every one of them + * comes back changed from `sanitize_text_field()`: '%xx' sequences are deleted outright, a bare + * '<' is entity-encoded, and leading whitespace is trimmed. + * + * @return Generator + */ + public static function standalones_sanitizing_would_alter(): Generator { + yield 'a folder name holding a percent sequence' => [ 'give%20recurring/give-recurring.php' ]; + yield 'a folder name holding a less-than' => [ 'give [ ' give-recurring/give-recurring.php' ]; + } + /** * Nothing draws an admin notice on the front end, and `get_current_screen()` does not exist * there — the guard is what keeps this filter from fataling if another plugin ever applies * `wp_admin_notice_markup` outside wp-admin. */ public function test_it_leaves_the_markup_alone_outside_the_admin(): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); set_current_screen( 'front' ); @@ -149,6 +191,8 @@ public function test_it_leaves_the_markup_alone_outside_the_admin(): void { * quoting it — is somebody else's. */ public function test_it_leaves_the_markup_alone_off_the_plugins_screen(): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); set_current_screen( 'dashboard' ); @@ -176,6 +220,8 @@ public function test_it_rewrites_the_markup_on_the_network_plugins_screen(): voi * for that plugin is the accurate one. */ public function test_it_leaves_the_markup_alone_for_a_plugin_no_sub_plugin_claims(): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); $_GET['plugin'] = 'akismet/akismet.php'; @@ -190,6 +236,8 @@ public function test_it_leaves_the_markup_alone_for_a_plugin_no_sub_plugin_claim * @param callable $arrange Turns the request in setUp() into the one this case is about. */ public function test_it_leaves_the_markup_alone( callable $arrange ): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); $arrange(); @@ -258,6 +306,8 @@ static function (): void { * core's sentence in place is the better of the two bad outcomes. */ public function test_a_message_that_sanitises_away_leaves_the_markup_alone(): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => '' ] ) ); @@ -269,6 +319,8 @@ public function test_a_message_that_sanitises_away_leaves_the_markup_alone(): vo * A message that is only whitespace is the same failure with a friendlier shape. */ public function test_a_whitespace_only_message_leaves_the_markup_alone(): void { + $this->assert_the_arrangement_rewrites(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => " \n\t" ] ) ); $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); @@ -301,6 +353,23 @@ public function test_it_strips_unsafe_markup_from_the_replacement_but_keeps_a_li $this->assertStringContainsString( 'alert(2)', $filtered ); } + /** + * The request setUp() leaves behind really does earn a rewrite. + * + * Every "leaves the markup alone" test asserts that the markup came back unchanged, and unchanged + * markup is also what a broken arrangement produces: a renamed screen id, a nonce action core + * reworded, a fixture that stopped claiming the standalone. Without a control saying the + * arrangement was rewriting a moment ago, all of those pass instead of failing. It builds its own + * rewriter with a message of its own, so the tests about the *message* are controlled too. + */ + private function assert_the_arrangement_rewrites(): void { + $this->assertStringContainsString( + 'Ours.', + $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ) + ->rewrite( self::MARKUP ) + ); + } + /** * A sub-plugin claiming the standalone this suite's request names. *