Two places in Munchable read a destination out of a query string. The OAuth callback:
// GET /auth/callback?code=...&next=/...
const next = safeReturnPath(searchParams.get('next'), '/');
if (code) {
const { error } = await supabase.auth.exchangeCodeForSession(code);
if (!error) return NextResponse.redirect(new URL(next, origin));
}
And the sign-in page, where a page that needs a session sends you with ?next=/that/page and expects you back:
const DEFAULT_NEXT = '/continue';
setNext(safeReturnPath(params.get('next'), DEFAULT_NEXT));
That parameter is attacker-controlled by construction. It lives in a link, and the entire point of the feature is that somebody else writes the link. Our own site writes most of them. Anybody can write one.
The whole validator
export function safeReturnPath(raw: string | null | undefined, fallback: string): string {
if (!raw) return fallback;
if (!raw.startsWith('/')) return fallback;
if (raw.startsWith('//') || raw.startsWith('/\\')) return fallback;
return raw;
}
Three refusals, each one a prefix test on a raw string. No parsing. That is the design, and the third line is the one worth the post.
The approach I did not take
The instinct, especially if you have been told not to do security with string matching, is to parse:
// Do not do this
const url = new URL(raw, origin);
if (url.origin !== origin) return fallback;
return url.pathname + url.search;
It reads as the careful version. It resolves the value the way the browser will, compares the resolved origin, and only then accepts. It is also more code, more imports, and dependent on getting origin right in every environment, and that is before the interesting part.
Here is the interesting part, run in Chrome on our own site:
new URL("/admin", location.href).href // https://munchable.app/admin
new URL("//evil.example", location.href).href // https://evil.example/
new URL("/\\evil.example", location.href).href // https://evil.example/
new URL("https://evil.example", location.href).href // https://evil.example/
Node's URL parser agrees, because both implement the WHATWG URL Standard, which lists this exact shape as a validation error that still parses:
invalid-reverse-solidus: The URL has a special scheme and it uses U+005C (
\) instead of U+002F (/).
A validation error, not a parse failure. For a special scheme such as https, the parser treats a backslash the way it treats a forward slash, so /\evil.example is //evil.example is a protocol-relative URL to somebody else's host. It even survives the DOM:
const a = document.createElement("a");
a.setAttribute("href", "/\\evil.example");
a.href; // "https://evil.example/"
The parse-and-compare version above handles this correctly, as it happens, because it resolves first. The problem is the family of checks that live next to it. if (!raw.startsWith('/')) reject on its own passes /\evil.example. So does if (raw.includes('://')) reject. So does a regex for ^/[^/]. Each of those is written by somebody who has heard of protocol-relative URLs and is thinking in forward slashes, which is every one of us, because a backslash is not a path separator on the web and your brain files it under "Windows".
Why the dumb version is the right version
The three lines do not try to work out where the string would go. They refuse every string whose destination is not obvious:
-
!rawcatches null, undefined and empty string, which are the common cases, not the attacks. -
!startsWith('/')rejectshttps://evil.example,javascript:..., and also plainadmin, which is a relative path that would resolve against whatever page you happened to be on. - The third line removes the two forms that start with a slash and still leave the origin.
What survives is a string starting with exactly one / followed by something that is not a slash or a backslash. There is no resolution step that could surprise you, because nothing is resolved. The value is handed to new URL(next, origin) afterwards, and by then it cannot mean anything other than a path on this origin.
The test is the same shape, which is to say it is mostly a list:
it('falls back for anything that could leave the origin', () => {
for (const bad of ['//evil.example', '/\\evil.example', 'https://evil.example', 'admin', '', null, undefined]) {
assert.equal(safeReturnPath(bad, '/continue'), '/continue', `input: ${String(bad)}`);
}
});
it('accepts same-origin relative paths, query string included', () => {
assert.equal(safeReturnPath('/admin', '/'), '/admin');
assert.equal(safeReturnPath('/admin/tickets?ticket=abc', '/'), '/admin/tickets?ticket=abc');
});
Seven hostile inputs and two good ones. A table test is the correct form for a function that is a list of refusals, and the input: ${String(bad)} message exists because expected '/continue' to equal '/continue' is a useless failure when the loop is on its fifth iteration.
The fallback is a parameter, and that is not decoration
safeReturnPath(searchParams.get('next'), '/') // the OAuth callback
safeReturnPath(params.get('next'), DEFAULT_NEXT) // the sign-in page
The callback sends a rejected value home. The sign-in page sends it to /continue, the post-signup chooser, because somebody arriving there with a mangled parameter is still mid-signup and dumping them on the marketing homepage loses the account they were creating.
A hardcoded '/' inside the validator would have made both call sites do the same wrong thing in one of the two cases. Validation decides what is unacceptable; the caller decides what happens next. Those are different jobs and they belong to different functions.
One downstream effect worth noticing: the sign-in page reads the validated value to pick its own heading.
const returning = next !== DEFAULT_NEXT;
Arriving with a destination means an existing account is coming back, so the page says "Sign in to continue" rather than "Create your account". Because the value was normalised first, a junk parameter cannot flip that copy. Validate early enough and the rest of the component gets to be naive.
What it does not do
It does not check that the path exists. /not-a-page is accepted and lands on our 404, which is correct: this function's job is the origin, and a wrong path on our own site is a bad link, not a security event.
It also does not help with any open redirect further inside the app, and the only reason that is fine is that there are none. One function, one concern, and a four-line audit of every caller.
See it working
-
munchable.app/get-started is the sign-in page. Add
?next=/supportand the heading changes to "Sign in to continue", because the parameter survived. Add?next=//example.comand it does not, because it did not. - munchable.app/support is the most common real destination, and it is deliberately usable without signing in at all.
- munchable.app/privacy is what the account you are signing in to is governed by, which is the thing a redirect to someone else's login form would be trying to borrow.
If you have a ?next=, a ?returnUrl= or a ?redirect= anywhere, try it with /\ right now. It takes ten seconds and the parser is on the attacker's side.
Top comments (0)