Skip to content

16. Send the user back only to a screen the admin can serve #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikolaystrikhar wants to merge 2 commits into
base: 58-assertions-that-cannot-pass-wrongly
Choose a base branch
from
+190 −32
Open

16. Send the user back only to a screen the admin can serve #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d8d681
Promise the request URI is a string, and keep the guard that assumes …
nikolaystrikhar Aug 24, 2026
6e04ab7
Send the user back only to a screen the admin can serve
nikolaystrikhar Aug 24, 2026
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
80 changes: 67 additions & 13 deletions src/Conflict/Redirector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
}
Expand All @@ -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' );
}
Expand All @@ -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
*
Expand All @@ -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;
}

Expand All @@ -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.
*
Expand Down
142 changes: 123 additions & 19 deletions tests/unit/Conflict/RedirectorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );

Expand All @@ -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<string,array{0:string|null,1:string}>
* @return Generator<string,array{0:string,1:string}>
*/
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' ) ];
Expand Down Expand Up @@ -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' ) ];
Expand All @@ -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.
Expand All @@ -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.
*/
Expand All @@ -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<string,array{0:mixed}>
*/
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 );

Expand Down Expand Up @@ -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
Expand Down
Loading