Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/routes/auth/login/+page.server.ts
Original file line number Diff line number Diff line change
@@ -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')));
}
};
6 changes: 4 additions & 2 deletions src/routes/auth/login/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
40 changes: 40 additions & 0 deletions src/routes/auth/login/safe-redirect.ts
Original file line number Diff line number Diff line change
@@ -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;
}
46 changes: 46 additions & 0 deletions tests/unit/safe-redirect.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
Loading