From 1d8d68117576b69b31c840d4a2f71dce4876bf1c Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:39:02 +0200 Subject: [PATCH 1/2] Promise the request URI is a string, and keep the guard that assumes nothing The docblock said `string|null`, the parameter was declared as nothing, the guard refused anything that was not a non-empty string, and Resolver -- the one caller -- had already made a string of $_SERVER['REQUEST_URI'] before it called. Four answers to one question. Narrowed the documented type to what the caller really passes rather than widening it to what the guard tolerates. `string` is a promise callers can be held to statically: Resolver's own narrowing is now checkable instead of redundant, and a host reaching for this class is told what to hand over. Widening the docblock to `mixed` would have thrown that away to describe a runtime backstop, and declaring the type would have been worse still -- the value comes from the SAPI and any plugin may have filtered $_SERVER, so under strict_types a broken one would fatal from inside plugins_loaded, on the request whose job is to get the admin back to a working screen. So the type is documented and not declared, and the guard stays exactly as wide as it was. The test that pinned the guard now runs over null, an array, an integer and a boolean, since every one of those has to land on the plugins list rather than raise. `null` moves out of the request-URI provider to join them. --- src/Conflict/Redirector.php | 12 ++++++- tests/unit/Conflict/RedirectorTest.php | 43 ++++++++++++++++++++------ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php index 822730b..efe56ea 100644 --- a/src/Conflict/Redirector.php +++ b/src/Conflict/Redirector.php @@ -36,13 +36,23 @@ class Redirector { * * The update screens are the exception, because reloading either of them re-runs an update. * + * The parameter is documented `string` and declared as nothing, and the two say different things + * on purpose. `string` is the contract: Resolver reads $_SERVER['REQUEST_URI'] and makes a string + * of whatever it finds there, so a caller that hands over anything else has a bug static analysis + * should name. Declaring it would turn that same bug into a TypeError raised under strict_types + * from inside plugins_loaded -- on the one path whose job is to get an admin back to a working + * screen -- for a value the SAPI supplies and any plugin may have filtered on the way. So the + * type is a promise to callers, and the guard below is what happens when the promise is broken. + * * @since 1.0.0 * - * @param string|null $request_uri The current request URI, i.e. $_SERVER['REQUEST_URI']. + * @param string $request_uri The current request URI, i.e. $_SERVER['REQUEST_URI']. * * @return string Absolute admin URL to send the user to. */ public function after_deactivation( $request_uri ): string { + // Both halves earn their place: the type check is the only refusal a non-string ever meets, + // and an empty string names no screen to go back to. if ( ! is_string( $request_uri ) || $request_uri === '' ) { return $this->admin_url_for( 'plugins.php' ); } diff --git a/tests/unit/Conflict/RedirectorTest.php b/tests/unit/Conflict/RedirectorTest.php index 3456dea..c259f0e 100644 --- a/tests/unit/Conflict/RedirectorTest.php +++ b/tests/unit/Conflict/RedirectorTest.php @@ -29,10 +29,10 @@ class RedirectorTest extends WPTestCase { /** * @dataProvider request_uris * - * @param string|null $request_uri Current request URI, as $_SERVER would carry it. - * @param string $expected Destination that request URI must produce. + * @param string $request_uri Current request URI, as $_SERVER would carry it. + * @param string $expected Destination that request URI must produce. */ - public function test_it_decides_where_to_send_the_user( $request_uri, string $expected ): void { + public function test_it_decides_where_to_send_the_user( string $request_uri, string $expected ): void { $this->setFunctionReturn( 'is_network_admin', false ); $this->setFunctionReturn( 'is_user_admin', false ); @@ -44,11 +44,12 @@ public function test_it_decides_where_to_send_the_user( $request_uri, string $ex * really arrives in: a bare path, a path under a subdirectory install, and -- from a proxy that * rewrites it -- an absolute URL. * - * @return Generator + * @return Generator */ public static function request_uris(): Generator { - yield 'no request uri at all' => [ null, admin_url( 'plugins.php' ) ]; - yield 'an empty request uri' => [ '', admin_url( 'plugins.php' ) ]; + // What Resolver passes when $_SERVER carries no REQUEST_URI, or carries one it will not + // vouch for as a string. Everything the documented type forbids is below, in its own test. + yield 'an empty request uri' => [ '', admin_url( 'plugins.php' ) ]; // Reloading either re-runs the update it is in the middle of. yield 'a plugin update screen' => [ '/wp-admin/update.php?action=upgrade-plugin&plugin=give', admin_url( 'plugins.php' ) ]; @@ -161,19 +162,41 @@ static function ( $url, $component = -1 ) { } /** - * $_SERVER carries whatever the SAPI put there, and a host may filter it besides, so the guard - * is a runtime one rather than a promise the signature can keep. + * The parameter is documented `string` and declared as nothing, so every one of these is a + * static error at the call site and none of them is a fatal at runtime. That is the whole of the + * arrangement: $_SERVER carries whatever the SAPI put there and any plugin may have filtered it + * on the way, so a declared type would answer a host's broken $_SERVER with a TypeError raised + * from inside plugins_loaded -- on the request that was supposed to hand the admin a working + * screen back. The type tells callers what to pass; the guard survives them not doing it. + * + * @dataProvider request_uris_the_type_forbids + * + * @param mixed $request_uri A value the documented type does not admit. */ - public function test_it_falls_back_when_the_request_uri_is_not_a_string(): void { + public function test_it_falls_back_when_the_request_uri_is_not_a_string( $request_uri ): 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' ] ); + $destination = ( new Redirector() )->after_deactivation( $request_uri ); $this->assertSame( admin_url( 'plugins.php' ), $destination ); } + /** + * `null` leads, because it is the shape a reader expects to be allowed and is not: Resolver + * turns a missing REQUEST_URI into an empty string before it calls, so nothing in this library + * ever passes null, and the documented type says exactly that. + * + * @return Generator + */ + public static function request_uris_the_type_forbids(): Generator { + yield 'null' => [ null ]; + yield 'an array' => [ [ '/wp-admin/edit.php' ] ]; + yield 'an integer' => [ 0 ]; + yield 'a boolean' => [ false ]; + } + public function test_it_keeps_a_network_admin_request_in_the_network_admin(): void { $this->setFunctionReturn( 'is_network_admin', true ); From 6e04ab71e36a1424f52dff80b9b983b52f4bd990 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:42:26 +0200 Subject: [PATCH 2/2] Send the user back only to a screen the admin can serve The pattern said a basename was well formed, and the class read that as naming an admin screen. `/wp-content/plugins/give/give.php`, `/wp-login.php` and `/wp-admin/edits.php` all satisfy it, so each was rebuilt as an admin URL for a file that is not in wp-admin -- an unstyled 404 from the web server, on the one request whose job is to hand the admin back the screen they were on. The documented answer for a URI naming no admin screen is the plugins list, and that is what all three take now. Asked of the filesystem, not of a list of core's screens. A list would be wrong the first time a plugin registered a top-level page, and the class has to keep working for a host's own screens: those are `admin.php`, `edit.php` or `tools.php` with a `page` argument on them, so the file is core's however many screens hang off it, and `is_file()` says yes to every one of them without knowing any of their names. What it cannot tell apart is a screen from one of the admin's own includes -- a browser is only ever on the first kind, and both are inside wp-admin either way. Asked of the admin the destination will be built in, matching admin_url_for()'s three branches. The network and user admins hold only the files core gives them, so a network request naming `options-general.php` was never on that screen, and network_admin_url() would name the file that is missing. The name still meets the pattern first, which is what keeps a crafted URI from climbing out of the directory being asked about, and the `\z` anchor is still the only thing refusing a trailing newline: the test for it stubs `is_file()` true, since there is no "edit.php\n" on disk either and the anchor could otherwise be taken out with nothing noticing. --- src/Conflict/Redirector.php | 68 ++++++++++++++---- tests/unit/Conflict/RedirectorTest.php | 99 +++++++++++++++++++++++--- 2 files changed, 146 insertions(+), 21 deletions(-) diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php index efe56ea..1e98165 100644 --- a/src/Conflict/Redirector.php +++ b/src/Conflict/Redirector.php @@ -62,8 +62,8 @@ public function after_deactivation( $request_uri ): string { $screen = $this->screen_from_path( is_string( $path ) ? $path : '' ); // Nothing that names an admin screen, so there is nothing to re-render: a front-end - // permalink, a directory that is not an admin root, a traversal attempt. Those take the same - // route as no request URI at all. + // permalink, a directory that is not an admin root, a php file this admin does not serve, a + // traversal attempt. Those take the same route as no request URI at all. if ( $screen === '' ) { return $this->admin_url_for( 'plugins.php' ); } @@ -78,12 +78,13 @@ public function after_deactivation( $request_uri ): string { /** * The admin screen a request path names, or an empty string if it names none. * - * Read from the path's basename rather than from the URI, and returned only once it looks like - * an admin screen. The request URI is a path on a site that may live in a subdirectory, may be - * behind a TLS-terminating proxy whose scheme disagrees with admin_url(), and on multisite may - * sit under the network or user admin -- so nothing built from admin_url() would recognise it. - * Taking the basename is also what keeps a crafted URI out of the destination: only a validated - * screen name leaves here, and admin_url_for() supplies everything in front of it. + * Read from the path's basename rather than from the URI, and returned only once it is a screen: + * the name has to be well formed, and this admin has to have a file of that name to serve. The + * request URI is a path on a site that may live in a subdirectory, may be behind a + * TLS-terminating proxy whose scheme disagrees with admin_url(), and on multisite may sit under + * the network or user admin -- so nothing built from admin_url() would recognise it. Taking the + * basename is also what keeps a crafted URI out of the destination: only a validated screen name + * leaves here, and admin_url_for() supplies everything in front of it. * * @since 1.0.0 * @@ -94,10 +95,11 @@ public function after_deactivation( $request_uri ): string { private function screen_from_path( string $path ): string { $screen = basename( $path ); - // 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 ) ) { + // Two questions, and both have to be yes. 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. The pattern + // runs first because it is also what makes the name safe to put after a directory below. + if ( (bool) preg_match( '/^[A-Za-z0-9_-]+\.php\z/', $screen ) && $this->is_admin_screen( $screen ) ) { return $screen; } @@ -119,6 +121,48 @@ private function screen_from_path( string $path ): string { return ''; } + /** + * Whether the admin this request belongs to has a screen of that name to be sent back to. + * + * Well formed is not the same as naming a screen. `wp-login.php`, `wp-cron.php` and a plugin's + * own bootstrap file all satisfy the pattern above, and each of them would be rebuilt as an admin + * URL for a file that is not there -- the web server's own 404, in place of the plugins list this + * class documents for a request that names no admin screen. + * + * Asked of the filesystem rather than of a list of core's screens, because a list would be wrong + * the first time a plugin registered a top-level page: a host's screens are `admin.php`, + * `edit.php`, `options-general.php` or `tools.php` with a `page` argument on them, so the file is + * core's however many screens are hung off it and every one of them still comes back intact. What + * `is_file()` cannot tell apart is a screen from one of the admin's own includes -- but a browser + * is only ever on the first kind, and both are inside wp-admin either way. + * + * The three branches are admin_url_for()'s, in the same order and for the same reason: the answer + * has to be about the directory the destination will be built in. The network and user admins + * serve only the files core gives them -- there is no `wp-admin/network/options-general.php` -- + * and a request under one of them cannot have been on a screen it does not hold. + * + * The name reaching here has already matched the pattern above, so it is [A-Za-z0-9_-] and a + * '.php' and nothing else: there is no way for it to climb out of the directory being asked + * about, and no readability question either, since the web server is what serves the file, not us. + * + * @since 1.0.0 + * + * @param string $screen A screen name that has passed the pattern in screen_from_path(). + * + * @return bool + */ + private function is_admin_screen( string $screen ): bool { + if ( is_network_admin() ) { + return is_file( ABSPATH . 'wp-admin/network/' . $screen ); + } + + if ( is_user_admin() ) { + return is_file( ABSPATH . 'wp-admin/user/' . $screen ); + } + + return is_file( ABSPATH . 'wp-admin/' . $screen ); + } + /** * The current request's query, re-encoded, ready to append to a screen name. * diff --git a/tests/unit/Conflict/RedirectorTest.php b/tests/unit/Conflict/RedirectorTest.php index c259f0e..fa943a3 100644 --- a/tests/unit/Conflict/RedirectorTest.php +++ b/tests/unit/Conflict/RedirectorTest.php @@ -104,6 +104,12 @@ public static function request_uris(): Generator { yield 'the network admin root' => [ '/wp-admin/network/', admin_url( 'index.php' ) ]; yield 'the user admin root' => [ '/wp-admin/user/', admin_url( 'index.php' ) ]; + // A host's own screens are core's files with a `page` argument on them, which is why the + // check below is for the file and not for a list of core's screens: a plugin page is a + // screen the admin serves, and it comes back whole. + yield 'a plugin page under admin.php' => [ '/wp-admin/admin.php?page=give-settings', admin_url( 'admin.php?page=give-settings' ) ]; + yield 'a plugin page under a core list' => [ '/wp-admin/edit.php?post_type=give_forms&page=give-reports', admin_url( 'edit.php?post_type=give_forms&page=give-reports' ) ]; + // Nothing that fails to name an admin screen is worth reloading, so it takes the same route // as no request uri at all. yield 'a front-end permalink' => [ '/2026/08/hello-world/', admin_url( 'plugins.php' ) ]; @@ -112,6 +118,19 @@ public static function request_uris(): Generator { yield 'a traversal attempt' => [ '/wp-admin/../../etc/passwd', admin_url( 'plugins.php' ) ]; yield 'garbage' => [ 'not a url at all', admin_url( 'plugins.php' ) ]; + // A well-formed name is not a screen. Every one of these would otherwise be rebuilt as an + // admin URL for a file that is not in wp-admin, and land the user on the web server's own 404 + // -- on the request that was supposed to put them back where they were. + yield 'a php file outside the admin' => [ '/wp-content/plugins/give/give.php', admin_url( 'plugins.php' ) ]; + yield 'the login screen' => [ '/wp-login.php', admin_url( 'plugins.php' ) ]; + yield 'the cron endpoint' => [ '/wp-cron.php', admin_url( 'plugins.php' ) ]; + yield 'an admin path naming no screen' => [ '/wp-admin/not-a-screen.php', admin_url( 'plugins.php' ) ]; + yield 'an admin screen name with a typo' => [ '/wp-admin/edits.php?post_type=page', admin_url( 'plugins.php' ) ]; + + // Asked of the admin this request belongs to, not of the three at once: sites.php is the + // network admin's screen and there is no wp-admin/sites.php for a single site to go back to. + yield 'a network screen from the site admin' => [ '/wp-admin/sites.php', admin_url( 'plugins.php' ) ]; + // The host is discarded with everything else in front of the screen name: the destination is // assembled from admin_url() and a basename, so a uri naming somewhere else cannot leave the // admin it was resolved on. @@ -131,7 +150,12 @@ public static function request_uris(): Generator { * entitled to lean on, and the anchor is what holds if it ever changes — so the parse is stubbed * to hand the path over verbatim, which is the only way to put the question to the pattern. * - * The clean path is asserted first, under the same stub. Without it the fallback below would be + * `is_file()` is stubbed for the same reason the parse is. There is no wp-admin file named + * "edit.php\n" either, so the screen check would refuse this one on its own and the anchor could + * be taken out without a test noticing. Answering every file question yes leaves the pattern as + * the only thing that can refuse, which is what this test is asking about. + * + * The clean path is asserted first, under the same stubs. Without it the fallback below would be * satisfied just as well by a stub that broke every destination, which is the wrong reason to * pass. */ @@ -147,18 +171,30 @@ static function ( $url, $component = -1 ) { true ); - $redirector = new Redirector(); + $redirector = new Redirector(); + $expected_screen = admin_url( 'edit.php' ); + $expected_fallback = admin_url( 'plugins.php' ); - $this->assertSame( - admin_url( 'edit.php' ), - $redirector->after_deactivation( '/wp-admin/edit.php' ), - 'A path naming a screen still resolves to it, so the parse is not what refuses below.' - ); + // Everything else this test needs is built before is_file() stops telling the truth, and the + // stub is undone the moment the calls under test return: it is process-global -- the + // autoloader asks it too -- so the window it is wrong in has to be those two calls and + // nothing else. The trait's `@after` is the backstop. + $restore = $this->setFunctionReturn( 'is_file', true ); + + try { + $screen = $redirector->after_deactivation( '/wp-admin/edit.php' ); + $fallback = $redirector->after_deactivation( "/wp-admin/edit.php\n" ); + } finally { + $restore(); + } $this->assertSame( - admin_url( 'plugins.php' ), - $redirector->after_deactivation( "/wp-admin/edit.php\n" ) + $expected_screen, + $screen, + 'A path naming a screen still resolves to it, so neither stub is what refuses below.' ); + + $this->assertSame( $expected_fallback, $fallback ); } /** @@ -224,6 +260,51 @@ public function test_it_falls_back_to_the_network_plugins_list(): void { ); } + /** + * The network admin serves only the files core gives it: there is no + * wp-admin/network/options-general.php, so no super admin was ever on that screen and there is + * nothing there to send one back to. The site admin's copy of the same name is not the answer + * either -- the destination is built with network_admin_url(), which would name the file that is + * missing. Asking the current admin's directory is what makes those two the same question. + */ + public function test_it_refuses_a_screen_the_network_admin_does_not_serve(): void { + $this->setFunctionReturn( 'is_network_admin', true ); + + $redirector = new Redirector(); + + $this->assertSame( + network_admin_url( 'sites.php' ), + $redirector->after_deactivation( '/wp-admin/network/sites.php' ), + 'A screen the network admin does have still resolves to itself, so the refusal below is not blanket.' + ); + + $this->assertSame( + network_admin_url( 'plugins.php' ), + $redirector->after_deactivation( '/wp-admin/network/options-general.php' ) + ); + } + + /** + * The third admin holds fewer screens still, and `edit.php` is one it does not have. + */ + public function test_it_refuses_a_screen_the_user_admin_does_not_serve(): void { + $this->setFunctionReturn( 'is_network_admin', false ); + $this->setFunctionReturn( 'is_user_admin', true ); + + $redirector = new Redirector(); + + $this->assertSame( + user_admin_url( 'profile.php' ), + $redirector->after_deactivation( '/wp-admin/user/profile.php' ), + 'A screen the user admin does have still resolves to itself, so the refusal below is not blanket.' + ); + + $this->assertSame( + user_admin_url( 'plugins.php' ), + $redirector->after_deactivation( '/wp-admin/user/edit.php' ) + ); + } + /** * A super admin who resolves a conflict from the network admin has to come back to it. Thrown * onto the current blog's screens instead, they are looking at a list where a network-activated