fix(auth): harden post-login redirect validation (#125)

* fix(auth): validate post-login redirect to prevent open redirect

The post-login `redirect` query param was passed straight to SvelteKit's
`redirect()` (server load) and `goto()` (client) without validation,
allowing an attacker to craft `?redirect=https://evil.example` or the
protocol-relative `?redirect=//evil.example` to bounce a logged-in user
off-site (open redirect).

Add a shared `safeRedirect()` helper that only accepts same-origin,
path-absolute targets (must start with a single `/`), rejecting absolute
URLs, protocol-relative `//host` and the `/\host` backslash variant that
browsers normalise to it, falling back to `/portal/dashboard`. Wire it into
both the server load and `handleSuccess()`, with unit tests.

Refs #123

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(auth): reject control chars in redirect target

Browsers strip C0 control characters (TAB/LF/CR) from URLs before
navigating, so `/\t/evil.example` and `/\n//evil.example` slipped past
the slash checks and then normalised to a protocol-relative, off-origin
URL. Reject any C0 control char (U+0000-U+001F) up front, before the
slash checks. Add regression tests for the TAB/LF/CR variants.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: dobby-coder[bot]

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')));
 	}
 };

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'));
 	}
 </script>
 

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;
+}

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);
+	});
+});