feat(redirects): make allowed redirect hosts configurable - #54
Merged
Conversation
The localhost-only allowlist exists so a reachable emulator cannot be turned into an open redirect, but it also blocks test environments that fake production-like hostnames to stay close to production — a reasonable setup that had no way to opt in. Configured hosts add to the localhost set rather than replacing it, so the guard stays on by default and existing callbacks keep working. Entries are normalized at startup, meaning a malformed host fails loudly instead of silently never matching a request.
Greptile SummaryThe PR makes redirect-host allowlisting configurable while retaining the localhost defaults and completes validation for malformed host patterns.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported malformed host-pattern acceptance is addressed by validation before and after canonicalization. Important Files Changed
|
The emptiness-and-whitespace check was too weak to honor what the function documents. `*example.test` (a wildcard missing its dot), `*.` and `app.example.test/path` all normalized cleanly, started the emulator, and then matched nothing — leaving a 400 on the redirect the entry was added to allow, which is the precise failure startup validation is meant to turn into a loud one. Validating the shape also closes the bogus-IPv6 path: `host:notaport` reached the bracketing branch only because its port was non-numeric, and came out as `[host:notaport]`.
Three follow-ups from review, all cases where the guard's shape did not match what it claims to enforce. A bare internationalized host could not be spelled in a way that matched. `URL.hostname` punycodes, so a request for møller.test arrives as xn--mller-vua.test; the origin form worked only because URL converted it on the way through, leaving the documented bare form with no correct spelling and a startup error that said nothing about why. The scheme was never looked at. `javascript://localhost/%0aalert(1)` parses with a hostname of localhost, so a script URI passed the localhost check on the strength of an authority it never navigates to — the precise thing the check exists to refuse. Refused now regardless of configuration, since `*` widens which host may be redirected to, not what a redirect may execute. Custom app schemes stay allowed: a native client's myapp://callback is a real redirect target and carries no script. IPv6 patterns were validated by character class, so `[:::]` and `[....]` were accepted and then matched nothing — the same silent no-match that startup validation was added to turn into a loud failure. Also covers the two entry points nothing exercised: the flag's repeated, comma-separated and inline forms, the environment variable a compose file sets, and the flag winning over it.
Five review follow-ups, most of them one shape: a check that validated one spelling of a host while requests arrive in another. `isIPv6` accepts every legal way to write an address, but `URL.hostname` compresses and strips leading zeros, so `[FD00::0001]` passed validation, started the emulator and then matched nothing — the silent no-match this validation exists to turn into a loud failure. Both sides now reduce to the single form a request carries, optional trailing dot included. The strip is guarded so `*.` cannot collapse into `*` and widen to every host. Control characters are refused before parsing rather than after, because URL parsing removes them instead of failing on them: `http://local<TAB>host/` validated as localhost and then reached the Location header raw. A `--redirect-hosts` occurrence contributing no entries now exits instead of leaving an empty array behind, which is not nullish and so discarded WORKOS_EMULATE_REDIRECT_HOSTS on the way past. The unconfigured-host error named only the CLI flag, which a caller of createEmulator has no way to pass.
Four review follow-ups, all the shape of the last round: a check whose stated invariant did not hold at the edges. `*` widens which host may be redirected to, never what a redirect may execute — but the scheme check is a denylist, and a denylist is always one scheme short. `view-source:javascript:alert(1)`, `jar:` and `about:` all parse with an empty hostname, so the host check had nothing to say about them and `*` waved them into a Location header on an authority they never had. Refused by shape now, so the guard does not depend on having enumerated every script-bearing scheme. That also refuses RFC 8252's path-only `com.example.app:/cb`, which never worked here either; the `myapp://callback` form is unaffected. An underscored host failed at startup on a config that works. `URL` passes an underscore through untouched, so `my_host.example.test` is a host a request really does arrive with — the shape a compose service name takes, which is what the environment variable exists to serve. Refusing it was the mirror of the bug the last round fixed: a loud failure where nothing was wrong. `*:3000` and `https://*` reduced to the fully-open `*`. Both read as a narrowing and are not one, since ports and schemes are never part of the host check. It is the same accident `stripTrailingDot` already guards for `*.`, and the one way this feature could hand back a weaker default than the operator asked for. Space is out of the forbidden-character bound. It cannot split a header, so it bought nothing there, and `searchParams.get` decodes `+` to a space — which turned an unencoded `+` in the inner query, the shape base64 state and a `+` in an email both take, into a 400 on a redirect that had worked.
Three review follow-ups, the first of them the same shape as the last round: a check whose stated invariant did not hold at the edges. `URL` rewrites every IPv4 shorthand it accepts, so `10.1`, `192.168.001.1` and `2130706433` validated by shape, started the emulator and then matched nothing — the silent no-match this validation exists to turn into a loud failure, surviving in the one address family people actually type. Canonicalizing runs after the shape check rather than before it, because `URL` drops a path instead of failing on one and would otherwise hand back a clean host for `app.example.test/path`. The second shape check catches what canonicalizing destroys: `999.999.999.999` is IPv4-shaped and not an address, so it now fails at startup rather than never matching either. Logout echoed `return_to` into the `Location` header verbatim while the three authorize endpoints re-serialized theirs. A space is legal in the URI a request carries — `searchParams.get` decodes a raw `+` into one — and is not legal in the header it produces. Interactive mode renders the redirect_uri into a hidden field and left every check to the POST, so a host the emulator will not redirect to served a working sign-in form and refused only after someone had filled it in.
`store.reset()` drops every data entry, so an option written into the store at startup stops taking effect after the first reset and nothing says so. Redirect hosts were restored; `interactiveAuth`, `webhookRetryConfig` and `webhookDebugMode` were not — so reset() quietly returned an interactive emulator to serving redirects instead of login pages. Kept in one closure so the next option written here cannot become half of a pair again.
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
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.
The authorize endpoints only ever accepted
localhost,127.0.0.1and[::1]as aredirect_urihost, which keeps a reachable emulator from being used as an open redirect. Test environments that fake production-like hostnames — to stay as close to production as possible — had no way to opt in.--redirect-hosts(repeatable, comma-separated),WORKOS_EMULATE_REDIRECT_HOSTS, andcreateEmulator({ allowedRedirectHosts }). The flag wins over the environment.app.example.test), a subdomain wildcard (*.example.test, which deliberately does not match the apex), or*to turn the check off. A whole origin (https://app.example.test:8443) is accepted and reduced to its hostname, since an origin is usually what people have on hand.https://, anything with whitespace) fails loudly instead of silently rejecting every request. Survivesreset().[FD00::0001]), IPv4 in any shorthandURLaccepts (10.1,192.168.001.1,2130706433), an internationalized name either bare or punycoded (møller.test), and a trailing dot optional. Without that, an ordinary way of writing an address validated, started the emulator and then matched nothing.redirect_urion/user_management/authorize,/sso/authorizeand/data-integrations/:slug/authorize, plusreturn_toon/user_management/sessions/logout.GET, before the login page renders theredirect_uriinto a hidden field, rather than leaving every check to the form'sPOST. A host the emulator will not redirect to no longer serves a working sign-in form and then refuses once someone has filled it in.return_tointo theLocationheader the way the three authorize endpoints already did, instead of echoing it verbatim. A space is legal in the URI a request carries —searchParams.getdecodes a raw+into one — and is not legal in the header it produces.assertLocalRedirectUriis renamed toassertAllowedRedirectUriand now takes the store. It was never part of the public export surface, so this is not a breaking change for consumers.Separately, every option-derived store entry is now applied from one closure, at startup and again from
reset().store.reset()drops all data, and only the redirect hosts were being restored — soreset()quietly returned an interactive emulator to serving redirects, and droppedwebhookRetryConfigandwebhookDebugModewith it.Closes #49