diff --git a/src/application/session/__tests__/sign_in.test.ts b/src/application/session/__tests__/sign_in.test.ts new file mode 100644 index 00000000..853e3c20 --- /dev/null +++ b/src/application/session/__tests__/sign_in.test.ts @@ -0,0 +1,178 @@ +import { afterAuth, isSafeRedirectUrl, safeDecodeRedirectParam } from '../sign_in'; + +// Mock localStorage +const localStorageMock = (() => { + let store: Record = {}; + return { + getItem: (key: string) => store[key] ?? null, + setItem: (key: string, value: string) => { + store[key] = value; + }, + removeItem: (key: string) => { + delete store[key]; + }, + clear: () => { + store = {}; + }, + }; +})(); + +Object.defineProperty(window, 'localStorage', { value: localStorageMock }); + +// Mock window.location (jsdom blocks direct href assignment) +let hrefValue = 'http://localhost/login'; +Object.defineProperty(window, 'location', { + writable: true, + value: { + get href() { + return hrefValue; + }, + set href(v: string) { + hrefValue = v; + }, + origin: 'http://localhost', + }, +}); + +beforeEach(() => { + localStorageMock.clear(); + hrefValue = 'http://localhost/login'; +}); + +describe('isSafeRedirectUrl', () => { + describe('safe URLs', () => { + it('returns true for a simple relative path', () => { + expect(isSafeRedirectUrl('/app')).toBe(true); + }); + + it('returns true for a relative path with query string', () => { + expect(isSafeRedirectUrl('/app?tab=settings')).toBe(true); + }); + + it('returns true for a relative path with nested segments', () => { + expect(isSafeRedirectUrl('/workspace/settings')).toBe(true); + }); + + it('returns true for an absolute URL matching window.location.origin', () => { + expect(isSafeRedirectUrl('http://localhost/app')).toBe(true); + }); + + it('returns true for an absolute URL with the same origin but a deep path', () => { + expect(isSafeRedirectUrl('http://localhost/workspace/abc/view/def')).toBe(true); + }); + }); + + describe('unsafe URLs', () => { + it('returns false for an absolute URL with a different origin', () => { + expect(isSafeRedirectUrl('https://evil.com')).toBe(false); + }); + + it('returns false for an absolute URL with a different subdomain', () => { + expect(isSafeRedirectUrl('https://phishing.appflowy.com')).toBe(false); + }); + + it('returns false for a protocol-relative URL', () => { + expect(isSafeRedirectUrl('//evil.com')).toBe(false); + }); + + it('returns false for a javascript: URL', () => { + expect(isSafeRedirectUrl('javascript:alert(1)')).toBe(false); + }); + + it('returns false for a data: URL', () => { + expect(isSafeRedirectUrl('data:text/html,')).toBe(false); + }); + + it('returns false for an empty string', () => { + expect(isSafeRedirectUrl('')).toBe(false); + }); + + it('returns false for a malformed string', () => { + expect(isSafeRedirectUrl('not a url at all %%')).toBe(false); + }); + + it('returns false for an http URL on a different host', () => { + expect(isSafeRedirectUrl('http://attacker.com/app')).toBe(false); + }); + }); +}); + +describe('afterAuth', () => { + it('redirects to /app when no redirectTo is stored', () => { + afterAuth(); + expect(window.location.href).toBe('/app'); + }); + + it('redirects to /app when stored redirectTo is an external URL', () => { + localStorage.setItem('redirectTo', encodeURIComponent('https://evil.com')); + afterAuth(); + expect(window.location.href).toBe('/app'); + }); + + it('redirects to /app when stored redirectTo is a protocol-relative URL', () => { + localStorage.setItem('redirectTo', encodeURIComponent('//evil.com/attack')); + afterAuth(); + expect(window.location.href).toBe('/app'); + }); + + it('redirects to /app when stored redirectTo contains a UUID path', () => { + const uuidPath = 'http://localhost/app/550e8400-e29b-41d4-a716-446655440000'; + localStorage.setItem('redirectTo', encodeURIComponent(uuidPath)); + afterAuth(); + expect(window.location.href).toBe('/app'); + }); + + it('redirects to /app path while preserving query params for root path', () => { + localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/?foo=bar')); + afterAuth(); + expect(window.location.href).toBe('http://localhost/app?foo=bar'); + }); + + it('redirects to /app when stored redirectTo has malformed encoding', () => { + localStorage.setItem('redirectTo', '%zz'); + afterAuth(); + expect(window.location.href).toBe('/app'); + }); + + it('follows a safe relative path', () => { + localStorage.setItem('redirectTo', encodeURIComponent('/settings')); + afterAuth(); + expect(window.location.href).toBe('/settings'); + }); + + it('follows a safe absolute same-origin URL', () => { + localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/settings')); + afterAuth(); + expect(window.location.href).toBe('http://localhost/settings'); + }); + + it('clears localStorage after execution regardless of outcome', () => { + localStorage.setItem('redirectTo', encodeURIComponent('https://evil.com')); + afterAuth(); + expect(localStorage.getItem('redirectTo')).toBeNull(); + }); + + it('clears localStorage even for a safe redirect', () => { + localStorage.setItem('redirectTo', encodeURIComponent('/settings')); + afterAuth(); + expect(localStorage.getItem('redirectTo')).toBeNull(); + }); +}); + +describe('safeDecodeRedirectParam', () => { + it('decodes a valid percent-encoded string', () => { + expect(safeDecodeRedirectParam('https%3A%2F%2Fevil.com')).toBe('https://evil.com'); + }); + + it('returns the string unchanged when nothing is encoded', () => { + expect(safeDecodeRedirectParam('/settings')).toBe('/settings'); + }); + + it('returns null for a malformed percent-encoded sequence', () => { + expect(safeDecodeRedirectParam('%zz')).toBeNull(); + }); + + it('returns null for a lone percent sign', () => { + expect(safeDecodeRedirectParam('%')).toBeNull(); + }); +}); diff --git a/src/application/session/sign_in.ts b/src/application/session/sign_in.ts index 903816f2..f4bd54be 100644 --- a/src/application/session/sign_in.ts +++ b/src/application/session/sign_in.ts @@ -40,13 +40,55 @@ export function withSignIn() { }; } +/** + * Decodes a percent-encoded redirect parameter, returning null on malformed input + * so that bad values are always treated as unsafe rather than crashing. + */ +export function safeDecodeRedirectParam(value: string): string | null { + try { + return decodeURIComponent(value); + } catch { + return null; + } +} + +/** + * Returns true only if the URL is safe to redirect to after authentication. + * Safe means: a relative path (starts with "/" but NOT "//") OR + * an absolute URL whose origin matches window.location.origin. + */ +export function isSafeRedirectUrl(url: string): boolean { + if (!url) return false; + + // Relative path — safe (but "//evil.com" is protocol-relative, not safe) + if (url.startsWith('/') && !url.startsWith('//')) { + return true; + } + + // Absolute URL — only safe if same origin + try { + const parsed = new URL(url); + + return parsed.origin === window.location.origin; + } catch { + return false; + } +} + export function afterAuth() { const redirectTo = getRedirectTo(); clearRedirectTo(); if (redirectTo) { - const url = new URL(decodeURIComponent(redirectTo)); + const decoded = safeDecodeRedirectParam(redirectTo); + + if (!decoded || !isSafeRedirectUrl(decoded)) { + window.location.href = '/app'; + return; + } + + const url = new URL(decoded, window.location.origin); const pathname = url.pathname; // Check if URL contains workspace/view UUIDs (user-specific paths) @@ -57,10 +99,11 @@ export function afterAuth() { // Don't redirect to user-specific pages from previous sessions window.location.href = '/app'; } else if (pathname === '/' || !pathname) { + // Preserve query params and hash but redirect to /app path url.pathname = '/app'; window.location.href = url.toString(); } else { - window.location.href = url.toString(); + window.location.href = decoded; } } else { window.location.href = '/app'; diff --git a/src/pages/LoginPage.tsx b/src/pages/LoginPage.tsx index 14058ee1..7fbc9004 100644 --- a/src/pages/LoginPage.tsx +++ b/src/pages/LoginPage.tsx @@ -9,10 +9,11 @@ import { LOGIN_ACTION } from '@/components/login/const'; import { EnterPassword } from '@/components/login/EnterPassword'; import { ForgotPassword } from '@/components/login/ForgotPassword'; import { SignUpPassword } from '@/components/login/SignUpPassword'; +import { isSafeRedirectUrl, safeDecodeRedirectParam } from '@/application/session/sign_in'; import { AFConfigContext } from '@/components/main/app.hooks'; function LoginPage() { - const [search] = useSearchParams(); + const [search, setSearch] = useSearchParams(); const action = search.get('action') || ''; const email = search.get('email') || ''; const force = search.get('force') === 'true'; @@ -21,15 +22,31 @@ function LoginPage() { const isAuthenticated = useContext(AFConfigContext)?.isAuthenticated || false; + // Strip unsafe redirectTo from URL immediately, preserving all other params + useEffect(() => { + if (!redirectTo) return; + + const decodedRedirect = safeDecodeRedirectParam(redirectTo); + + if (!decodedRedirect || !isSafeRedirectUrl(decodedRedirect)) { + setSearch((prev) => { + const next = new URLSearchParams(prev); + + next.delete('redirectTo'); + return next; + }); + } + }, [redirectTo, setSearch]); + useEffect(() => { if (action === LOGIN_ACTION.CHANGE_PASSWORD || force) { return; } if (isAuthenticated && redirectTo) { - const decodedRedirect = decodeURIComponent(redirectTo); + const decodedRedirect = safeDecodeRedirectParam(redirectTo); - if (decodedRedirect !== window.location.href) { + if (decodedRedirect && decodedRedirect !== window.location.href) { window.location.href = decodedRedirect; } }