fix(login): close ?next= control-char open-redirect + cover register
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
49
frontend/src/lib/safeNext.test.ts
Normal file
49
frontend/src/lib/safeNext.test.ts
Normal file
@@ -0,0 +1,49 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { safeNext } from './safeNext';
|
||||
|
||||
describe('safeNext', () => {
|
||||
it('allows same-origin absolute paths', () => {
|
||||
expect(safeNext('/library')).toBe('/library');
|
||||
expect(safeNext('/search?q=berserk')).toBe('/search?q=berserk');
|
||||
expect(safeNext('/manga/123#page-2')).toBe('/manga/123#page-2');
|
||||
});
|
||||
|
||||
it('falls back to / for missing or empty input', () => {
|
||||
expect(safeNext(null)).toBe('/');
|
||||
expect(safeNext(undefined)).toBe('/');
|
||||
expect(safeNext('')).toBe('/');
|
||||
});
|
||||
|
||||
it('rejects protocol-relative URLs (open redirect)', () => {
|
||||
expect(safeNext('//evil.com')).toBe('/');
|
||||
expect(safeNext('//evil.com/path')).toBe('/');
|
||||
});
|
||||
|
||||
it('rejects backslash tricks browsers normalise to //', () => {
|
||||
expect(safeNext('/\\evil.com')).toBe('/');
|
||||
expect(safeNext('/\\/evil.com')).toBe('/');
|
||||
});
|
||||
|
||||
it('rejects control/whitespace chars browsers strip mid-parse', () => {
|
||||
// These start with a single "/" so the prefix checks alone would pass
|
||||
// them, but a browser strips the tab/newline/CR and the value
|
||||
// collapses to a protocol-relative //evil.com — the %09/%0A/%0D
|
||||
// open-redirect bypass.
|
||||
expect(safeNext('/\t/evil.com')).toBe('/'); // %09 tab
|
||||
expect(safeNext('/\n/evil.com')).toBe('/'); // %0A newline
|
||||
expect(safeNext('/\r/evil.com')).toBe('/'); // %0D carriage return
|
||||
expect(safeNext('/ /evil.com')).toBe('/'); // space
|
||||
expect(safeNext(`/${String.fromCharCode(0)}/evil.com`)).toBe('/'); // NUL
|
||||
expect(safeNext(`/${String.fromCharCode(0x2028)}/evil.com`)).toBe('/'); // line sep
|
||||
});
|
||||
|
||||
it('rejects absolute and scheme URLs', () => {
|
||||
expect(safeNext('https://evil.com')).toBe('/');
|
||||
expect(safeNext('http://evil.com')).toBe('/');
|
||||
expect(safeNext('javascript:alert(1)')).toBe('/');
|
||||
});
|
||||
|
||||
it('rejects bare paths without a leading slash', () => {
|
||||
expect(safeNext('library')).toBe('/');
|
||||
});
|
||||
});
|
||||
34
frontend/src/lib/safeNext.ts
Normal file
34
frontend/src/lib/safeNext.ts
Normal file
@@ -0,0 +1,34 @@
|
||||
/**
|
||||
* Sanitise a `?next=` post-login/registration redirect target.
|
||||
*
|
||||
* The login/register flow round-trips the page the user was bounced from via
|
||||
* `?next=`. Navigating to an attacker-supplied value would be an open
|
||||
* redirect, so only same-origin absolute paths are allowed: the value must
|
||||
* start with a single `/` and must not be protocol-relative (`//evil.com`)
|
||||
* or a backslash trick (`/\evil.com`, which some parsers normalise to `//`).
|
||||
*
|
||||
* Crucially, the structural prefix checks are not enough on their own:
|
||||
* browsers **strip ASCII tab/newline/CR mid-parse**, so `/\t/evil.com`
|
||||
* (which starts with a single `/`) collapses to the protocol-relative
|
||||
* `//evil.com` once navigated — an open-redirect bypass via `%09`/`%0A`/
|
||||
* `%0D`. So we reject any control or whitespace character (and backslashes)
|
||||
* *before* the prefix checks. Anything else — absolute URLs, `javascript:`
|
||||
* URIs, empty/missing — falls back to the site root.
|
||||
*/
|
||||
function hasUnsafeNextChar(s: string): boolean {
|
||||
for (let i = 0; i < s.length; i++) {
|
||||
const c = s.charCodeAt(i);
|
||||
// C0 controls + space (<= 0x20), DEL (0x7f), or backslash (0x5c) —
|
||||
// characters a URL parser may strip or treat as a path separator.
|
||||
if (c <= 0x20 || c === 0x7f || c === 0x5c) return true;
|
||||
}
|
||||
// Any remaining Unicode whitespace (NBSP, line/paragraph separators, …).
|
||||
return /\s/.test(s);
|
||||
}
|
||||
|
||||
export function safeNext(next: string | null | undefined): string {
|
||||
if (typeof next !== 'string' || next.length === 0) return '/';
|
||||
if (hasUnsafeNextChar(next)) return '/';
|
||||
if (!next.startsWith('/') || next.startsWith('//')) return '/';
|
||||
return next;
|
||||
}
|
||||
@@ -2,6 +2,7 @@
|
||||
import { goto } from '$app/navigation';
|
||||
import { login } from '$lib/api/auth';
|
||||
import { session } from '$lib/session.svelte';
|
||||
import { safeNext } from '$lib/safeNext';
|
||||
|
||||
let username = $state('');
|
||||
let password = $state('');
|
||||
@@ -15,7 +16,10 @@
|
||||
try {
|
||||
const user = await login({ username, password });
|
||||
session.setUser(user);
|
||||
await goto('/');
|
||||
// Return the user to the page they were bounced from (?next=),
|
||||
// guarded against open redirects; default to the site root.
|
||||
const next = new URLSearchParams(window.location.search).get('next');
|
||||
await goto(safeNext(next));
|
||||
} catch (e) {
|
||||
error = (e as Error).message;
|
||||
} finally {
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
import { register } from '$lib/api/auth';
|
||||
import { authConfig } from '$lib/auth-config.svelte';
|
||||
import { session } from '$lib/session.svelte';
|
||||
import { safeNext } from '$lib/safeNext';
|
||||
|
||||
let username = $state('');
|
||||
let password = $state('');
|
||||
@@ -24,7 +25,10 @@
|
||||
try {
|
||||
const user = await register({ username, password });
|
||||
session.setUser(user);
|
||||
await goto('/');
|
||||
// Honour ?next= the same way login does (registration logs the
|
||||
// user in), guarded against open redirects.
|
||||
const next = new URLSearchParams(window.location.search).get('next');
|
||||
await goto(safeNext(next));
|
||||
} catch (e) {
|
||||
error = (e as Error).message;
|
||||
} finally {
|
||||
|
||||
Reference in New Issue
Block a user