fix(web): harden return_url handling in checkout and auth forms - #6
Open
nwilkens wants to merge 12 commits into
Open
fix(web): harden return_url handling in checkout and auth forms#6nwilkens wants to merge 12 commits into
nwilkens wants to merge 12 commits into
Conversation
The portal payment success pages are unauthenticated and assigned the `return_url` query param straight to window.location.href, making the billing domain an open redirect usable for phishing. Target origins are now parsed and matched exactly against VITE_PORTAL_RETURN_URL_ALLOWLIST, which defaults to empty (deny-all). Anything absent, unparseable, non-http(s), or off the allowlist renders the terminal state instead of redirecting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Userinfo is not part of the origin, so it passed the allowlist check and survived into the returned URL. On the allowed host it still forces an Authorization header, raises the browser's login prompt on a trusted host, and makes the link preview read as another domain. Also report dropped allowlist entries instead of failing closed in silence, and reject wildcard hosts rather than keeping a literal that matches nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
allowImportingTsExtensions was set on the config `pnpm build` runs, and test files were inside that program, so a broken test failed the production build. Split them into tsconfig.test.json, run it from the test script, and declare @types/node, which until now resolved only through a hoist no package.json asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers blob: inheriting the allowed origin, protocol-relative values that pass only because new URL() gets no base, backslash and userinfo forms, vbscript/file/filesystem/view-source, subdomain and prefix lookalikes, trailing-dot and unicode hosts, scheme downgrade, and that the return value is the canonical serialization. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The old assertion built its own `new URL('//evil.example')` inside the test
body and never called resolveReturnUrl, so it asserted a fact about the
platform parser. Adding a base to `new URL(value)` failed zero tests.
Assert the allowed-host forms instead: they are the only values that
distinguish the two implementations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wildcard clause reached `*` by name, so a semicolon-separated list still yielded the hostname `a.example;https` with no warning: an allowlist that looks populated and matches nothing. `;` is not a forbidden host code point, same as `*`. Check the hostname shape instead -- DNS labels, or localhost / an IP literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requiring a dot dropped http://intranet, http://billing:8080 and https://my_host.example, all of which are working internal origins. Wildcards and separator garbage are still dropped, and the warning now names the cause that actually applies instead of guessing at both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The auth forms guard their redirect target with `startsWith('/')`.
`/.\/evil.example` passes that test and resolves, on our own origin, to
the pathname `//evil.example`; the browser re-reads it as
protocol-relative, pushState throws SecurityError, and the history
package's catch falls through to location.assign. Comparing origins does
not catch it, so validate the resolved pathname instead and return the
parsed path rather than the caller's string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the startsWith('/') test in the login, registration and
email-validation forms. The registration form validates before writing
to session storage and the validation form validates again after reading
it back, so a value planted by an older build or through devtools cannot
be replayed after the email round-trip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed a bare authority was refused there. It is not: //evil.example starts with a slash and passes. The origin comparison below catches it. An overstated comment of this kind is what let the original bug survive review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
modules/web/package.json had a `test` job in turbo.json but no npm script to invoke it, so `pnpm --prefix modules/web test` (the natural mirror of the existing lint job) failed with "Missing script". Added the script and the CI job. Verified locally against the pinned pnpm 11.6.0 pattern: 49/49 passing, exit 0. Without this, PR #6's new tests never ran except by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nwilkens
changed the title
travispaul
reviewed
Aug 26, 2026
Origin comparison alone already protects resolveReturnUrl; the hostname regex only ever ran on the operator's own config, and a mistyped entry there fails closed (matches nothing) either way.
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.
Two redirect targets were validated by pattern-matching the raw string. Both are now resolved with the URL parser and checked against what it actually produces.
Checkout success page
return_urlwas assigned towindow.location.hrefand rendered as an anchor href, with no validation and no guard on the route. It is now checked against a deployment-configured origin allowlist that defaults to empty.Both sinks share the check — guarding only the automatic redirect would have left the anchor, which is the half that can execute script.
Scheme restriction is load-bearing rather than defensive: a
blob:URL inherits its inner URL's origin, so an origin comparison alone accepts one.Auth forms
Login, registration and email validation guarded
returnUrlwithstartsWith('/'). That test accepts values which resolve to a foreign origin, and one family of them navigated off-site.Replaced with a validator that resolves the candidate and rejects a resolved pathname beginning with
//. An origin comparison does not close this — for these values the resolved origin is our own, and only the pathname reveals the problem.Validation runs on both the sessionStorage write and the read, so a value written by an older build is not trusted on replay.
Notes for review
web-apphad no test infrastructure. This uses Node's built-in runner and adds no framework; the test-only compiler flag lives in a separate tsconfig so a broken test cannot fail the production build.Not addressed here
The success page still renders a payment confirmation to any visitor, on both of its routes, with no proof a checkout occurred. The redirect is now safe; the unguarded claim is not.