From bd1b5ac3ad5d11dc506b02d121c9ef8a11d30717 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Thu, 13 Aug 2026 15:23:13 +0200 Subject: [PATCH] Stop sanitising two values that were already safe, and were not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sanitize_text_field() ran over query values wp_parse_str() had already url-decoded, and it deletes every %[a-f0-9]{2} match and strips markup. So a conflict resolved on edit.php?s=100%25ab re-rendered the screen searching for 100. The point of the redirect is to re-request what the user asked for, and http_build_query( …, PHP_QUERY_RFC3986 ) is what actually prevents breakout; all the sanitize call added was the mangling. Line breaks are stripped on their own now, which is the one property the Location header needs. The rewriter verified its nonce against a sanitised basename, but core mints that nonce from the raw wp_unslash( $_REQUEST['plugin'] ). Any basename sanitizing alters -- a folder name with a percent sequence, a leading space -- failed verification, and the activation-error screen kept core's wording on exactly the conflict this feature exists to explain. An explicit is_string() does the refusing that sanitize_text_field() was incidentally doing. Also anchors the screen-name regex with \z: PCRE $ matches before a trailing newline too. --- cspell.json | 1 + src/Conflict/Redirector.php | 49 ++++++++++++++++-- src/Conflict/Rewriter.php | 19 +++++-- tests/unit/Conflict/RedirectorTest.php | 19 ++++++- tests/unit/Conflict/RewriterTest.php | 69 ++++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 12 deletions(-) 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. *