diff --git a/src/routes/auth/login/+page.server.ts b/src/routes/auth/login/+page.server.ts index ce5f4f4..f4f51d5 100644 --- a/src/routes/auth/login/+page.server.ts +++ b/src/routes/auth/login/+page.server.ts @@ -1,8 +1,9 @@ import { redirect } from '@sveltejs/kit'; import type { PageServerLoad } from './$types'; +import { safeRedirect } from './safe-redirect'; export const load: PageServerLoad = async ({ locals, url }) => { if (locals.session?.userType === 'org') { - redirect(303, url.searchParams.get('redirect') ?? '/portal/dashboard'); + redirect(303, safeRedirect(url.searchParams.get('redirect'))); } }; diff --git a/src/routes/auth/login/+page.svelte b/src/routes/auth/login/+page.svelte index 0d6a154..b796b2c 100644 --- a/src/routes/auth/login/+page.svelte +++ b/src/routes/auth/login/+page.svelte @@ -8,14 +8,16 @@ import { goto } from '$app/navigation'; import { page } from '$app/state'; import { resolve } from '$app/paths'; + import { safeRedirect } from './safe-redirect'; import type { PageData } from './$types'; let { data }: { data: PageData } = $props(); - const redirectTo = $derived(page.url.searchParams.get('redirect') ?? '/portal/dashboard'); + const redirectTo = $derived(page.url.searchParams.get('redirect')); function handleSuccess() { - goto(resolve(redirectTo as '/portal/dashboard')); + // The `redirect` param is attacker-controlled — validate it before navigating. + goto(resolve(safeRedirect(redirectTo) as '/portal/dashboard')); } diff --git a/src/routes/auth/login/safe-redirect.ts b/src/routes/auth/login/safe-redirect.ts new file mode 100644 index 0000000..0db8b31 --- /dev/null +++ b/src/routes/auth/login/safe-redirect.ts @@ -0,0 +1,40 @@ +// The post-login `redirect` query param is attacker-controlled, so it must +// never be handed to `redirect()`/`goto()` without validation — otherwise it +// is an open redirect (an attacker can send `?redirect=https://evil.example` +// or the protocol-relative `?redirect=//evil.example` to bounce a logged-in +// user off-site). Only same-origin, path-absolute targets are allowed. + +export const DEFAULT_REDIRECT = '/portal/dashboard'; + +/** + * True if `value` contains any C0 control character (U+0000–U+001F, incl. + * TAB/LF/CR). Browsers strip these from URLs before navigating, so a value + * like `/\t/evil.example` or `/\n//evil.example` would slip past the slash + * checks below and then normalise to a protocol-relative, off-origin URL. + * Strip-then-parse is the classic open-redirect bypass, so we reject any such + * value up front. (Expressed as a char-code scan to keep literal control + * characters out of the source.) + */ +function hasControlChar(value: string): boolean { + for (let i = 0; i < value.length; i++) { + if (value.charCodeAt(i) <= 0x1f) return true; + } + return false; +} + +/** + * Return a safe post-login redirect target. A value is only accepted when it + * begins with a single `/` (a same-origin absolute path). Everything else — + * empty/missing values, values containing control characters, absolute URLs + * (`https://…`), protocol-relative URLs (`//host`) and the backslash variant + * browsers normalise to them (`/\host`) — falls back to {@link DEFAULT_REDIRECT}. + */ +export function safeRedirect(target: string | null | undefined): string { + if (!target) return DEFAULT_REDIRECT; + if (hasControlChar(target)) return DEFAULT_REDIRECT; + if (!target.startsWith('/')) return DEFAULT_REDIRECT; + // Reject protocol-relative URLs. Browsers treat `\` as `/`, so `/\host` + // escapes the origin just like `//host` does. + if (target[1] === '/' || target[1] === '\\') return DEFAULT_REDIRECT; + return target; +} diff --git a/tests/unit/safe-redirect.test.ts b/tests/unit/safe-redirect.test.ts new file mode 100644 index 0000000..d40f8ec --- /dev/null +++ b/tests/unit/safe-redirect.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from 'vitest'; +import { safeRedirect, DEFAULT_REDIRECT } from '../../src/routes/auth/login/safe-redirect'; + +describe('safeRedirect', () => { + it('accepts same-origin absolute paths', () => { + expect(safeRedirect('/portal/dashboard')).toBe('/portal/dashboard'); + expect(safeRedirect('/portal/settings')).toBe('/portal/settings'); + expect(safeRedirect('/')).toBe('/'); + expect(safeRedirect('/a?b=c#d')).toBe('/a?b=c#d'); + }); + + it('falls back for missing values', () => { + expect(safeRedirect(null)).toBe(DEFAULT_REDIRECT); + expect(safeRedirect(undefined)).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('')).toBe(DEFAULT_REDIRECT); + }); + + it('rejects values that do not start with a slash', () => { + expect(safeRedirect('portal/dashboard')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('https://evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('http://evil.example/path')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('javascript:alert(1)')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect(' /portal/dashboard')).toBe(DEFAULT_REDIRECT); + }); + + it('rejects protocol-relative URLs', () => { + expect(safeRedirect('//evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('//evil.example/path')).toBe(DEFAULT_REDIRECT); + }); + + it('rejects the backslash variant browsers normalise to protocol-relative', () => { + expect(safeRedirect('/\\evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('/\\/evil.example')).toBe(DEFAULT_REDIRECT); + }); + + it('rejects control chars browsers strip before navigating (TAB/LF/CR)', () => { + // Browsers drop these, so `/\t/evil.example` etc. would normalise to + // `//evil.example` (off-origin) if they slipped past the slash checks. + expect(safeRedirect('/\t/evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('/\n/evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('/\r/evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('/\t\\evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('\t//evil.example')).toBe(DEFAULT_REDIRECT); + expect(safeRedirect('/foo\nbar')).toBe(DEFAULT_REDIRECT); + }); +});