Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
"unsets",
"unslash",
"unslashed",
"unslashing",
"unstubbed",
"unwire",
"unwires",
Expand All @@ -83,6 +84,7 @@
"ignoreWords": [
"ance",
"Brien",
"Cprojects",
"defered",
"Dpage",
"giverecurring",
Expand Down
21 changes: 15 additions & 6 deletions src/Conflict/Gatekeeper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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 '';
Expand Down
13 changes: 10 additions & 3 deletions src/Conflict/Resolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
}
Expand Down
9 changes: 9 additions & 0 deletions tests/unit/Conflict/GatekeeperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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' ];
}

/**
Expand Down
30 changes: 24 additions & 6 deletions tests/unit/Conflict/ResolverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down
Loading