From 12302445fde637ae89efb5fc4eff2cb36622ce29 Mon Sep 17 00:00:00 2001 From: "Lucas.Xu" Date: Tue, 3 Mar 2026 13:33:50 +0800 Subject: [PATCH 1/3] fix: prevent open redirect in post-auth navigation Add isSafeRedirectUrl() to validate that redirectTo targets only same-origin URLs. Apply the guard in afterAuth() and LoginPage.tsx to block external-domain redirects (CWE-601, issue #8551). --- .../session/__tests__/sign_in.test.ts | 154 ++++++++++++++++++ src/application/session/sign_in.ts | 37 ++++- src/pages/LoginPage.tsx | 5 + 3 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 src/application/session/__tests__/sign_in.test.ts 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..c2820400 --- /dev/null +++ b/src/application/session/__tests__/sign_in.test.ts @@ -0,0 +1,154 @@ +import { afterAuth, isSafeRedirectUrl } 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 for root path', () => { + localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/')); + 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(); + }); +}); diff --git a/src/application/session/sign_in.ts b/src/application/session/sign_in.ts index 903816f2..fd3c62d3 100644 --- a/src/application/session/sign_in.ts +++ b/src/application/session/sign_in.ts @@ -40,13 +40,43 @@ export function withSignIn() { }; } +/** + * 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 = decodeURIComponent(redirectTo); + + if (!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 +87,9 @@ export function afterAuth() { // Don't redirect to user-specific pages from previous sessions window.location.href = '/app'; } else if (pathname === '/' || !pathname) { - url.pathname = '/app'; - window.location.href = url.toString(); + window.location.href = '/app'; } 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..b884b695 100644 --- a/src/pages/LoginPage.tsx +++ b/src/pages/LoginPage.tsx @@ -9,6 +9,7 @@ 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 } from '@/application/session/sign_in'; import { AFConfigContext } from '@/components/main/app.hooks'; function LoginPage() { @@ -29,6 +30,10 @@ function LoginPage() { if (isAuthenticated && redirectTo) { const decodedRedirect = decodeURIComponent(redirectTo); + if (!isSafeRedirectUrl(decodedRedirect)) { + return; + } + if (decodedRedirect !== window.location.href) { window.location.href = decodedRedirect; } From 5a6840b5ddc4833f9d417554e282331f32d0cab3 Mon Sep 17 00:00:00 2001 From: "Lucas.Xu" Date: Tue, 3 Mar 2026 13:59:27 +0800 Subject: [PATCH 2/3] fix: sanitize unsafe redirectTo param in-place on login page Replace silent block with in-place URL correction: strip the unsafe redirectTo param via setSearchParams while preserving all other query params (email, action, etc.), so the login flow continues uninterrupted. --- src/pages/LoginPage.tsx | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/pages/LoginPage.tsx b/src/pages/LoginPage.tsx index b884b695..a00218af 100644 --- a/src/pages/LoginPage.tsx +++ b/src/pages/LoginPage.tsx @@ -13,7 +13,7 @@ import { isSafeRedirectUrl } 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'; @@ -22,6 +22,20 @@ function LoginPage() { const isAuthenticated = useContext(AFConfigContext)?.isAuthenticated || false; + // Strip unsafe redirectTo from URL immediately, preserving all other params + useEffect(() => { + if (!redirectTo) return; + + const decodedRedirect = decodeURIComponent(redirectTo); + + if (!isSafeRedirectUrl(decodedRedirect)) { + setSearch((prev) => { + prev.delete('redirectTo'); + return prev; + }); + } + }, [redirectTo, setSearch]); + useEffect(() => { if (action === LOGIN_ACTION.CHANGE_PASSWORD || force) { return; @@ -30,10 +44,6 @@ function LoginPage() { if (isAuthenticated && redirectTo) { const decodedRedirect = decodeURIComponent(redirectTo); - if (!isSafeRedirectUrl(decodedRedirect)) { - return; - } - if (decodedRedirect !== window.location.href) { window.location.href = decodedRedirect; } From e7320b4ec9e736ffbafdbad335c9041aed97212e Mon Sep 17 00:00:00 2001 From: "Lucas.Xu" Date: Tue, 3 Mar 2026 14:06:51 +0800 Subject: [PATCH 3/3] fix: address Sourcery review comments - Add safeDecodeRedirectParam helper (returns null on malformed input) and use it in afterAuth and LoginPage instead of bare decodeURIComponent - Restore query-param/hash preservation for root-path redirects in afterAuth - Use immutable URLSearchParams construction in LoginPage setSearch updater --- .../session/__tests__/sign_in.test.ts | 30 +++++++++++++++++-- src/application/session/sign_in.ts | 20 +++++++++++-- src/pages/LoginPage.tsx | 16 +++++----- 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/src/application/session/__tests__/sign_in.test.ts b/src/application/session/__tests__/sign_in.test.ts index c2820400..853e3c20 100644 --- a/src/application/session/__tests__/sign_in.test.ts +++ b/src/application/session/__tests__/sign_in.test.ts @@ -1,4 +1,4 @@ -import { afterAuth, isSafeRedirectUrl } from '../sign_in'; +import { afterAuth, isSafeRedirectUrl, safeDecodeRedirectParam } from '../sign_in'; // Mock localStorage const localStorageMock = (() => { @@ -122,8 +122,14 @@ describe('afterAuth', () => { expect(window.location.href).toBe('/app'); }); - it('redirects to /app for root path', () => { - localStorage.setItem('redirectTo', encodeURIComponent('http://localhost/')); + 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'); }); @@ -152,3 +158,21 @@ describe('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 fd3c62d3..f4bd54be 100644 --- a/src/application/session/sign_in.ts +++ b/src/application/session/sign_in.ts @@ -40,6 +40,18 @@ 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 @@ -69,9 +81,9 @@ export function afterAuth() { clearRedirectTo(); if (redirectTo) { - const decoded = decodeURIComponent(redirectTo); + const decoded = safeDecodeRedirectParam(redirectTo); - if (!isSafeRedirectUrl(decoded)) { + if (!decoded || !isSafeRedirectUrl(decoded)) { window.location.href = '/app'; return; } @@ -87,7 +99,9 @@ export function afterAuth() { // Don't redirect to user-specific pages from previous sessions window.location.href = '/app'; } else if (pathname === '/' || !pathname) { - window.location.href = '/app'; + // Preserve query params and hash but redirect to /app path + url.pathname = '/app'; + window.location.href = url.toString(); } else { window.location.href = decoded; } diff --git a/src/pages/LoginPage.tsx b/src/pages/LoginPage.tsx index a00218af..7fbc9004 100644 --- a/src/pages/LoginPage.tsx +++ b/src/pages/LoginPage.tsx @@ -9,7 +9,7 @@ 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 } from '@/application/session/sign_in'; +import { isSafeRedirectUrl, safeDecodeRedirectParam } from '@/application/session/sign_in'; import { AFConfigContext } from '@/components/main/app.hooks'; function LoginPage() { @@ -26,12 +26,14 @@ function LoginPage() { useEffect(() => { if (!redirectTo) return; - const decodedRedirect = decodeURIComponent(redirectTo); + const decodedRedirect = safeDecodeRedirectParam(redirectTo); - if (!isSafeRedirectUrl(decodedRedirect)) { + if (!decodedRedirect || !isSafeRedirectUrl(decodedRedirect)) { setSearch((prev) => { - prev.delete('redirectTo'); - return prev; + const next = new URLSearchParams(prev); + + next.delete('redirectTo'); + return next; }); } }, [redirectTo, setSearch]); @@ -42,9 +44,9 @@ function LoginPage() { } 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; } }