diff --git a/cspell.json b/cspell.json index b4c52d8..b139d5f 100644 --- a/cspell.json +++ b/cspell.json @@ -66,6 +66,7 @@ "unsets", "unslash", "unslashed", + "unslashing", "unstubbed", "unwire", "unwires", @@ -83,6 +84,7 @@ "ignoreWords": [ "ance", "Brien", + "Cprojects", "defered", "Dpage", "giverecurring", diff --git a/src/Conflict/Gatekeeper.php b/src/Conflict/Gatekeeper.php index 9701acb..a57ca7b 100644 --- a/src/Conflict/Gatekeeper.php +++ b/src/Conflict/Gatekeeper.php @@ -206,9 +206,17 @@ private function is_action_request(): bool { // admin_action_{$action} on the raw value, so an action named outside a-z0-9_- -- a // non-Latin script, a bare '+' -- is work a plugin can be hooked to, and sanitizing // first would empty it and admit the very request this gate exists to refuse. - $action = wp_unslash( $raw ); - - if ( is_string( $action ) && $action !== '' && $action !== self::NO_ACTION ) { + // + // Not through wp_unslash() either, for the reason Conflict\Resolver reads the request URI + // raw: core adds its slashes in wp_magic_quotes(), which wp-settings.php calls *after* + // do_action( 'plugins_loaded' ), so there are none on this value yet and unslashing is a + // plain stripslashes() over the query string. '\' would strip to '' and '-\1' to '-1' -- + // both of them values this gate reads as asking for nothing -- and the request would be + // admitted and resolved out from under work core is still going to dispatch. Should this + // ever be reached after the slashing, from the inline fallback a too-late boot reports, + // a slashed value only ever reads as more of an action than it is, which is the + // direction a gate may safely be wrong in. + if ( $raw !== '' && $raw !== self::NO_ACTION ) { return true; } } @@ -245,9 +253,10 @@ private function current_script(): string { continue; } - $path = wp_unslash( $candidate ); - - return strtolower( basename( is_string( $path ) ? $path : '' ) ); + // Read as the SAPI left it. wp_magic_quotes() slashes $_SERVER, and wp-settings.php + // calls it after do_action( 'plugins_loaded' ), so nothing here has been slashed yet and + // wp_unslash() would only delete backslashes that arrived in the path itself. + return strtolower( basename( $candidate ) ); } return ''; diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 8f54b3d..eadd8f5 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -259,10 +259,17 @@ protected function redirect(): void { return; } - $request_uri = isset( $_SERVER['REQUEST_URI'] ) ? wp_unslash( $_SERVER['REQUEST_URI'] ) : ''; + // Read raw, not through wp_unslash(). Core adds the slashes in wp_magic_quotes(), which + // wp-settings.php calls *after* do_action( 'plugins_loaded' ) -- so at priority 5 there are + // none on this value and unslashing is a plain stripslashes() over the URL bar. An admin + // searching for a Windows path who trips a conflict would be sent back to a search for + // "C:projects", which is the one thing the redirect exists to get right. Reached from the + // inline fallback a too-late boot reports, this can run after the slashing instead, where the + // cost is a stray backslash in a re-encoded query arg rather than a deleted one. + $request_uri = $_SERVER['REQUEST_URI'] ?? ''; - // $_SERVER carries whatever the SAPI put there and wp_unslash() hands back the shape it was - // given, so the string the redirector is promised is made one here rather than assumed. + // $_SERVER carries whatever the SAPI put there, so the string the redirector is promised is + // made one here rather than assumed. if ( ! is_string( $request_uri ) ) { $request_uri = ''; } diff --git a/tests/unit/Conflict/GatekeeperTest.php b/tests/unit/Conflict/GatekeeperTest.php index e547f83..7205984 100644 --- a/tests/unit/Conflict/GatekeeperTest.php +++ b/tests/unit/Conflict/GatekeeperTest.php @@ -299,6 +299,15 @@ public static function gets_that_carry_an_action(): Generator { // most confident it refuses. yield 'an action named outside the Latin alphabet' => [ 'action', 'экспорт' ]; yield 'an action named in punctuation' => [ 'action', '+' ]; + + // Core adds its slashes in wp_magic_quotes(), which wp-settings.php calls after + // do_action( 'plugins_loaded' ) — so at priority 5 these arrive exactly as the URL bar sent + // them and there is nothing to unslash. Doing it anyway is a stripslashes() over user input, + // and these are the two values it empties into the answers the gate reads as "no action + // asked for": a request the gate then admits, and core goes on to dispatch + // admin_action_\ for. + yield 'an action a stripslashes would empty' => [ 'action', '\\' ]; + yield 'an action a stripslashes would turn into no-op' => [ 'action2', '-\\1' ]; } /** diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index b365fee..7795f9b 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -345,10 +345,11 @@ static function () use ( $detector ): Detector { } /** - * The redirector is handed the current request, unslashed — not the referrer. What the user is - * looking at is what has to be re-rendered without the standalone's code in memory, and core slashes - * $_SERVER on the way in, so a query string with an apostrophe in it would otherwise gain a - * backslash every time a conflict was resolved. + * The redirector is handed the current request — not the referrer — exactly as the SAPI left it. + * What the user is looking at is what has to be re-rendered without the standalone's code in + * memory, and core adds its slashes in `wp_magic_quotes()`, which wp-settings.php calls after + * `do_action( 'plugins_loaded' )`: at priority 5 there are none to take off, so unslashing here + * would delete a backslash the user typed rather than one core added. */ public function test_it_asks_the_redirector_where_to_send_the_current_request(): void { $redirector = new class() extends Redirector { @@ -385,14 +386,31 @@ static function () use ( $redirector ): Redirector { $this->standalone_is( true ); $this->register(); - $_SERVER['REQUEST_URI'] = '/wp-admin/edit.php?s=O\\\'Brien'; + $_SERVER['REQUEST_URI'] = '/wp-admin/edit.php?s=C:\\projects'; $location = $this->capture_resolution(); - $this->assertSame( [ '/wp-admin/edit.php?s=O\'Brien' ], $redirector->asked ); + $this->assertSame( [ '/wp-admin/edit.php?s=C:\\projects' ], $redirector->asked ); $this->assertSame( admin_url( 'tools.php' ), $location ); } + /** + * The same claim through the real redirector, because handing the URI over intact is only half of + * it: an admin searching for a Windows path who trips a conflict has to be sent back to the search + * they ran, not to a search for `C:projects`. The backslash leaves re-encoded, which is + * `Conflict\Redirector`'s doing and its own tests' subject; that it is still there to encode is + * this one's. + */ + public function test_a_backslash_in_the_query_survives_into_the_destination(): void { + $this->standalone_is( true ); + $this->register(); + $_SERVER['REQUEST_URI'] = '/wp-admin/edit.php?s=C:\\projects'; + + $location = $this->capture_resolution(); + + $this->assertStringContainsString( 's=C%3A%5Cprojects', $location ); + } + /** * A request with no REQUEST_URI is not hypothetical: the too-late fallback runs this sequence * inline, and a host may have booted from something that never set one.