Skip to content

Commit a0b1e5c

Browse files
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>
1 parent 00844b2 commit a0b1e5c

4 files changed

Lines changed: 63 additions & 3 deletions

File tree

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { redirect } from '@sveltejs/kit';
22
import type { PageServerLoad } from './$types';
3+
import { safeRedirect } from './safe-redirect';
34

45
export const load: PageServerLoad = async ({ locals, url }) => {
56
if (locals.session?.userType === 'org') {
6-
redirect(303, url.searchParams.get('redirect') ?? '/portal/dashboard');
7+
redirect(303, safeRedirect(url.searchParams.get('redirect')));
78
}
89
};

src/routes/auth/login/+page.svelte

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,16 @@
88
import { goto } from '$app/navigation';
99
import { page } from '$app/state';
1010
import { resolve } from '$app/paths';
11+
import { safeRedirect } from './safe-redirect';
1112
import type { PageData } from './$types';
1213
1314
let { data }: { data: PageData } = $props();
1415
15-
const redirectTo = $derived(page.url.searchParams.get('redirect') ?? '/portal/dashboard');
16+
const redirectTo = $derived(page.url.searchParams.get('redirect'));
1617
1718
function handleSuccess() {
18-
goto(resolve(redirectTo as '/portal/dashboard'));
19+
// The `redirect` param is attacker-controlled — validate it before navigating.
20+
goto(resolve(safeRedirect(redirectTo) as '/portal/dashboard'));
1921
}
2022
</script>
2123

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// The post-login `redirect` query param is attacker-controlled, so it must
2+
// never be handed to `redirect()`/`goto()` without validation — otherwise it
3+
// is an open redirect (an attacker can send `?redirect=https://evil.example`
4+
// or the protocol-relative `?redirect=//evil.example` to bounce a logged-in
5+
// user off-site). Only same-origin, path-absolute targets are allowed.
6+
7+
export const DEFAULT_REDIRECT = '/portal/dashboard';
8+
9+
/**
10+
* Return a safe post-login redirect target. A value is only accepted when it
11+
* begins with a single `/` (a same-origin absolute path). Everything else —
12+
* empty/missing values, absolute URLs (`https://…`), protocol-relative URLs
13+
* (`//host`) and the backslash variant browsers normalise to them (`/\host`)
14+
* — falls back to {@link DEFAULT_REDIRECT}.
15+
*/
16+
export function safeRedirect(target: string | null | undefined): string {
17+
if (!target || !target.startsWith('/')) return DEFAULT_REDIRECT;
18+
// Reject protocol-relative URLs. Browsers treat `\` as `/`, so `/\host`
19+
// escapes the origin just like `//host` does.
20+
if (target[1] === '/' || target[1] === '\\') return DEFAULT_REDIRECT;
21+
return target;
22+
}

tests/unit/safe-redirect.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { safeRedirect, DEFAULT_REDIRECT } from '../../src/routes/auth/login/safe-redirect';
3+
4+
describe('safeRedirect', () => {
5+
it('accepts same-origin absolute paths', () => {
6+
expect(safeRedirect('/portal/dashboard')).toBe('/portal/dashboard');
7+
expect(safeRedirect('/portal/settings')).toBe('/portal/settings');
8+
expect(safeRedirect('/')).toBe('/');
9+
expect(safeRedirect('/a?b=c#d')).toBe('/a?b=c#d');
10+
});
11+
12+
it('falls back for missing values', () => {
13+
expect(safeRedirect(null)).toBe(DEFAULT_REDIRECT);
14+
expect(safeRedirect(undefined)).toBe(DEFAULT_REDIRECT);
15+
expect(safeRedirect('')).toBe(DEFAULT_REDIRECT);
16+
});
17+
18+
it('rejects values that do not start with a slash', () => {
19+
expect(safeRedirect('portal/dashboard')).toBe(DEFAULT_REDIRECT);
20+
expect(safeRedirect('https://evil.example')).toBe(DEFAULT_REDIRECT);
21+
expect(safeRedirect('http://evil.example/path')).toBe(DEFAULT_REDIRECT);
22+
expect(safeRedirect('javascript:alert(1)')).toBe(DEFAULT_REDIRECT);
23+
expect(safeRedirect(' /portal/dashboard')).toBe(DEFAULT_REDIRECT);
24+
});
25+
26+
it('rejects protocol-relative URLs', () => {
27+
expect(safeRedirect('//evil.example')).toBe(DEFAULT_REDIRECT);
28+
expect(safeRedirect('//evil.example/path')).toBe(DEFAULT_REDIRECT);
29+
});
30+
31+
it('rejects the backslash variant browsers normalise to protocol-relative', () => {
32+
expect(safeRedirect('/\\evil.example')).toBe(DEFAULT_REDIRECT);
33+
expect(safeRedirect('/\\/evil.example')).toBe(DEFAULT_REDIRECT);
34+
});
35+
});

0 commit comments

Comments
 (0)