diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php index 822730b..1e98165 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' ); } @@ -52,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' ); } @@ -68,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 * @@ -84,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; } @@ -109,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 3456dea..fa943a3 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' ) ]; @@ -103,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' ) ]; @@ -111,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. @@ -130,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. */ @@ -146,34 +171,68 @@ 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 ); } /** - * $_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 ); @@ -201,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