8 [2/2]. Send the user back only to a screen the admin can serve - #80
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe redirector now requires string request URIs, validates PHP screen names against existing admin files, and handles site, network, and user admin contexts. Tests cover invalid inputs, unsupported paths, trailing newlines, and valid admin destinations. ChangesRedirect validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant Redirector
participant AdminScreenCheck
participant Filesystem
Request->>Redirector: provide request URI
Redirector->>AdminScreenCheck: extract and validate screen
AdminScreenCheck->>Filesystem: check applicable admin file
Filesystem-->>AdminScreenCheck: file exists or does not exist
AdminScreenCheck-->>Redirector: valid screen or fallback
Redirector-->>Request: selected redirect destination
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
nikolaystrikhar
force-pushed
the
61-redirector-screen-and-signature
branch
from
August 25, 2026 08:44
6e04ab7 to
f2cc4c6
Compare
nikolaystrikhar
changed the title
nikolaystrikhar
changed the title
nikolaystrikhar
force-pushed
the
61-redirector-screen-and-signature
branch
from
August 25, 2026 10:23
f2cc4c6 to
e607bcc
Compare
nikolaystrikhar
changed the title
nikolaystrikhar
changed the title
d4mation
approved these changes
Aug 25, 2026
Base automatically changed from
58-assertions-that-cannot-pass-wrongly
to
main
August 25, 2026 12:57
…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.
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.
nikolaystrikhar
force-pushed
the
61-redirector-screen-and-signature
branch
from
August 25, 2026 12:57
e607bcc to
55d9835
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What:
Conflict\Redirector::after_deactivation()documents thestringits one caller passes, and returns only a screen the current admin has a file for.Usage: where an admin lands after a conflict is resolved.
/wp-admin/admin.php?page=give-settings/wp-login.phpadmin_url( 'wp-login.php' )— a 404plugins.php/wp-content/plugins/give/give.phpadmin_url( 'give.php' )— a 404plugins.php/wp-admin/edits.php(typo)admin_url( 'edits.php' )— a 404plugins.php/wp-admin/network/options-general.phpnetwork_admin_url( … )— a 404network_admin_url( 'plugins.php' )Why this way:
The type is a promise to callers, not a declaration. Resolver makes a string of
$_SERVER['REQUEST_URI']before it calls, sostringis something analysis can hold callers to; declaring it would answer a host's filtered$_SERVERwith aTypeErrorfrom insideplugins_loaded.A well-formed basename is not a screen. The pattern admitted any
*.php, so a URI naming one was rebuilt as an admin URL for a file that is not in wp-admin — a 404 on the request whose job is putting the admin back where they were.Asked of the filesystem, not of a list of core's screens. A list would be wrong the first time a plugin registered a page: host screens are
admin.phporedit.phpwith apageargument, sois_file()says yes to screens nobody has written yet. Asked in the admin the destination will be built in, since the network and user admins hold only core's files.The pattern still runs first, and still with
\z. It is what keeps a crafted name inside the directory being asked about; the anchor test stubsis_file()true so the anchor stays the only thing refusing a trailing newline.Summary by CodeRabbit