From be9c005b7eee2efe6ced86cf3086c63388235765 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:13:49 +0200 Subject: [PATCH] Read the request raw, where core has not slashed it yet `wp_magic_quotes()` is what adds the slashes `wp_unslash()` takes off, and wp-settings.php calls it *after* `do_action( 'plugins_loaded' )`. Both conflict gates and the redirect run at priority 5, so every value they read is exactly as the SAPI left it and `wp_unslash()` there is a plain `stripslashes_deep()` over user input. It costs a destination and a gate. An admin at /wp-admin/edit.php?s=C:\projects who trips a conflict is sent back to a search for "C:projects" -- re-rendering the screen the user asked for being the entire point of the redirect -- and in the action gate `?action=\` strips to '' while `?action2=-\1` strips to '-1', which are the two values that gate reads as "nothing is being asked for". Both are then admitted, resolved and exited out from under work core still goes on to dispatch, by the class whose job is refusing exactly those requests. `Conflict\Rewriter` keeps both of its uses: it runs from `wp_admin_notice_markup` at admin render time, long after the slashing, where the idiom is the correct one. Where these can still run after the slashing is the inline fallback a too-late boot reports, and the direction is safe there: a slashed action arg only ever reads as more of an action than it is, and a slashed URI costs a stray backslash in a re-encoded query arg rather than a deleted one. --- cspell.json | 2 ++ src/Conflict/Gatekeeper.php | 21 ++++++++++++------ src/Conflict/Resolver.php | 13 ++++++++--- tests/unit/Conflict/GatekeeperTest.php | 9 ++++++++ tests/unit/Conflict/ResolverTest.php | 30 ++++++++++++++++++++------ 5 files changed, 60 insertions(+), 15 deletions(-) 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.