Preserve the canonical admin URL in the default login redirect - #507
Open
Obiwanbanan wants to merge 1 commit into
Open
Obiwanbanan wants to merge 1 commit into
Obiwanbanan wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
When the login redirect type is `default`, the target is re-wrapped as a `url_redirect` and pushed through the redirect pipeline. That routes it into Misc::sanitize_url(), which returns the normalised `relative` form, and Misc::parse_url(), which strips the trailing slash. admin_url() therefore degrades from https://example.com/wp-admin/ to /wp-admin. The normalisation itself is correct for what parse_url() is for -- building a comparison key -- but the same value is then returned as an HTTP Location. Behind a reverse proxy the missing trailing slash is load-bearing: the web server answers the slash-less path with its own canonical 301 built from the internal listen port, e.g. http://example.com:8080/wp-admin/, which is not reachable from outside. Administrators can log in but never arrive at the admin area. admin_url() is already canonical and already safe, so return it verbatim instead of normalising it. Fixes aamplugin#506
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
1 participant
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.
Fixes #506.
The problem
When the login redirect type is default, get_user_redirect_url() re-wraps admin_url() as a url_redirect and pushes it through the redirect pipeline. That routes it into Misc::sanitize_url(), which returns the normalised relative form (scheme and host dropped), and Misc::parse_url(), which strips the trailing slash.
So https://example.com/wp-admin/ comes back out as /wp-admin.
The normalisation is not wrong in itself — parse_url() exists to build a comparison key, and lowercasing, sorting query params and trimming the slash are all reasonable for that. The problem is that the same value is then returned as an HTTP Location.
Why it matters
Behind a reverse proxy, the missing trailing slash is load-bearing. nginx answers the slash-less path with its own canonical 301, built from the port it listens on rather than the public one:
That port is internal to the cluster, so the browser times out. Administrators can sign in but never arrive at the admin area — while /wp-admin/ opened directly works fine, which makes the fault look account-specific. Since a 301 is cached permanently, it also survives clearing cookies, clearing the cache and switching browsers.
The change
Return admin_url() verbatim for the default type. It is already canonical and already safe, so there is nothing for the normaliser to add.
Verification
Measured with our own login_redirect filter removed, so AAM is the only thing under test:
Environment: WordPress 7.0.4, PHP 8.4, Bedrock layout, nginx listen 8080 behind a TLS-terminating ingress.
Scope
Deliberately narrow — it only touches the default branch, so configured redirects keep going through the existing pipeline unchanged.
A custom Redirect to a URL value entered as https://example.com/wp-admin/ is still stored and returned as /wp-admin, so custom targets lose the trailing slash too. Fixing that properly means separating "normalise for comparison" from "sanitise for redirect" inside Misc, which is a wider change and riskier — parse_url() output is also used for URI access rule matching. Glad to extend this PR in that direction if you would rather have both handled together. Issue #506 also notes two smaller things spotted in Misc::parse_url() while reading it.