24: Read the two request values the way core wrote them

cspell.json

@@ -29,6 +29,7 @@
         "nexcess",
         "packagist",
         "pagenow",
+        "PCRE",
         "phpdotenv",
         "phpstan",
         "phpunit",

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<b' as 'a&lt;b'. Re-rendering the screen the user asked for is
+	 * the entire point of the redirect, and that quietly re-renders a different one.
+	 *
+	 * @since 1.0.0
+	 *
+	 * @param array<array-key,mixed> $args Query arguments as wp_parse_str() produced them.
+	 *
+	 * @return array<array-key,mixed>
+	 */
+	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.
 	 *

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;
 		}
 

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<b' as 'a&lt;b' -- the redirect exists to re-render what the user asked for, and that
+		// re-renders something else.
+		yield 'a query value holding a percent sequence' => [ '/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' ] );

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<string,array{0:string}>
+	 */
+	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<recurring/give-recurring.php' ];
+		yield 'a folder name holding a leading space'    => [ ' 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() => '<script></script>' ] )
 		);
@@ -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.
 	 *