diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 5dc3953..5890a44 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.90.5" +version = "0.90.6" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 10ddecd..0c9e4e4 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.90.5" +version = "0.90.6" edition = "2021" default-run = "mangalord" diff --git a/frontend/e2e/auth-flow.spec.ts b/frontend/e2e/auth-flow.spec.ts index 942061a..a7f4ce0 100644 --- a/frontend/e2e/auth-flow.spec.ts +++ b/frontend/e2e/auth-flow.spec.ts @@ -81,6 +81,52 @@ test('login then logout flips the layout between authenticated and anonymous', a await expect(page.getByTestId('nav-login')).toBeVisible(); }); +test('login redirects to a safe ?next= target after authenticating', async ({ page }) => { + await stubAnonymousThenAuthenticated(page); + + // Arrive at /login carrying a same-origin ?next= (the shape the layout + // produces when bouncing an unauthenticated user off a gated page). + await page.goto('/login?next=%2Fsearch'); + await expect(page.getByTestId('nav-login')).toBeVisible(); + await page.getByTestId('login-username').fill('alice'); + await page.getByTestId('login-password').fill('hunter2hunter2'); + await page.getByTestId('login-submit').click(); + + // Lands on the requested page, not the default root. + await expect(page).toHaveURL(/\/search$/); +}); + +test('login ignores an unsafe ?next= and falls back to /', async ({ page }) => { + await stubAnonymousThenAuthenticated(page); + + // A protocol-relative open-redirect attempt must be discarded. + await page.goto('/login?next=%2F%2Fevil.com'); + await expect(page.getByTestId('nav-login')).toBeVisible(); + await page.getByTestId('login-username').fill('alice'); + await page.getByTestId('login-password').fill('hunter2hunter2'); + await page.getByTestId('login-submit').click(); + + // Authenticated and on the site root — never navigated off-origin. + await expect(page).toHaveURL(/\/$/); + await expect(page.getByTestId('session-user')).toContainText('alice'); +}); + +test('login rejects the %09 control-char open-redirect bypass', async ({ page }) => { + await stubAnonymousThenAuthenticated(page); + + // "/\t/evil.com" passes a naive startsWith('/') && !startsWith('//') + // guard but collapses to "//evil.com" once the browser strips the tab. + await page.goto('/login?next=%2F%09%2Fevil.com'); + await expect(page.getByTestId('nav-login')).toBeVisible(); + await page.getByTestId('login-username').fill('alice'); + await page.getByTestId('login-password').fill('hunter2hunter2'); + await page.getByTestId('login-submit').click(); + + // Stayed on-origin at the root, authenticated. + await expect(page).toHaveURL(/\/$/); + await expect(page.getByTestId('session-user')).toContainText('alice'); +}); + test('login surfaces the API error message on bad credentials', async ({ page }) => { await page.route('**/api/v1/auth/me', async (route) => { await route.fulfill({ diff --git a/frontend/package.json b/frontend/package.json index 29cbfac..756f3a1 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.90.5", + "version": "0.90.6", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/lib/safeNext.test.ts b/frontend/src/lib/safeNext.test.ts new file mode 100644 index 0000000..49ba3af --- /dev/null +++ b/frontend/src/lib/safeNext.test.ts @@ -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('/'); + }); +}); diff --git a/frontend/src/lib/safeNext.ts b/frontend/src/lib/safeNext.ts new file mode 100644 index 0000000..ef1468d --- /dev/null +++ b/frontend/src/lib/safeNext.ts @@ -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; +} diff --git a/frontend/src/routes/login/+page.svelte b/frontend/src/routes/login/+page.svelte index 21f3394..a8c79b2 100644 --- a/frontend/src/routes/login/+page.svelte +++ b/frontend/src/routes/login/+page.svelte @@ -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 { diff --git a/frontend/src/routes/register/+page.svelte b/frontend/src/routes/register/+page.svelte index 5d49473..2493a25 100644 --- a/frontend/src/routes/register/+page.svelte +++ b/frontend/src/routes/register/+page.svelte @@ -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 {