From 17dc01f0a0d336133fb9ec32e15e7388bc60ac16 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Tue, 15 Sep 2026 11:21:27 -0500 Subject: [PATCH] fix: keep session material and open redirects out of switchToOrganization Two hardening changes to the same helper. VULN-1244: the no-returnTo branch returned refreshSession's result verbatim in data(), which serialized accessToken, sealedSession (the iron-sealed blob wrapping the refresh token) and the raw Set-Cookie string into the browser-readable single-fetch payload and SSR hydration data. Return an allowlisted projection of display-safe claims instead; the session still travels via the Set-Cookie response header only. Same invariant authkitLoader has enforced since CVE-2025-55008. VULN-1264: the returnTo branch called redirect(returnTo) raw. Run it through the package's existing sanitizeReturnPathname so a request-controlled value cannot produce an off-origin redirect. Rejected values redirect to /. --- src/auth.spec.ts | 55 +++++++++++++++++++++++++++++++++++++++++++++--- src/auth.ts | 42 ++++++++++++++++++++++++++++++------ 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/src/auth.spec.ts b/src/auth.spec.ts index 3e583f3..4da719b 100644 --- a/src/auth.spec.ts +++ b/src/auth.spec.ts @@ -135,6 +135,19 @@ describe('auth', () => { }, }; + // What the browser is allowed to see: mockAuthResponse minus session material. + const safeAuth = { + user: mockAuthResponse.user, + sessionId: mockAuthResponse.sessionId, + organizationId: mockAuthResponse.organizationId, + role: mockAuthResponse.role, + roles: mockAuthResponse.roles, + permissions: mockAuthResponse.permissions, + entitlements: mockAuthResponse.entitlements, + featureFlags: mockAuthResponse.featureFlags, + impersonator: mockAuthResponse.impersonator, + }; + beforeEach(() => { refreshSession.mockResolvedValue(mockAuthResponse); }); @@ -149,7 +162,7 @@ describe('auth', () => { const result = await switchToOrganization(request, organizationId); expect(data).toHaveBeenCalledWith( - { success: true, auth: mockAuthResponse }, + { success: true, auth: safeAuth }, { headers: { 'Set-Cookie': 'new-cookie-value', @@ -157,7 +170,7 @@ describe('auth', () => { }, ); expect(result).toEqual({ - data: { success: true, auth: mockAuthResponse }, + data: { success: true, auth: safeAuth }, init: { headers: { 'Set-Cookie': 'new-cookie-value', @@ -166,6 +179,42 @@ describe('auth', () => { }); }); + it('never ships accessToken, sealedSession, or Set-Cookie in the data payload (VULN-1244)', async () => { + const result = await switchToOrganization(request, organizationId); + + const serialized = JSON.stringify(result.data); + expect(serialized).not.toContain('new-access-token'); + expect(serialized).not.toContain('sealed-session-data'); + expect(serialized).not.toContain('new-cookie-value'); + expect(result.data.auth).not.toHaveProperty('accessToken'); + expect(result.data.auth).not.toHaveProperty('sealedSession'); + expect(result.data.auth).not.toHaveProperty('headers'); + // The session still travels in the response header. + expect(result.init?.headers).toEqual({ 'Set-Cookie': 'new-cookie-value' }); + }); + + it.each([ + 'https://evil.example.com/phish', + '//evil.example.com', + '/\\evil.example.com', + '/dashboard\r\nSet-Cookie: x=y', + 'javascript:alert(1)', + ])('rejects off-origin returnTo %s and redirects to / (VULN-1264)', async (returnTo) => { + const result = await switchToOrganization(request, organizationId, { returnTo }); + + expect(redirect).toHaveBeenCalledWith('/', { headers: { 'Set-Cookie': 'new-cookie-value' } }); + assertIsResponse(result); + expect(result.headers.get('Location')).toBe('/'); + }); + + it('preserves a same-origin returnTo path with query and hash', async () => { + const returnTo = '/settings/orgs?tab=members#top'; + const result = await switchToOrganization(request, organizationId, { returnTo }); + + assertIsResponse(result); + expect(result.headers.get('Location')).toBe(returnTo); + }); + it('should redirect to returnTo when provided', async () => { const returnTo = '/dashboard'; const result = await switchToOrganization(request, organizationId, { returnTo }); @@ -293,7 +342,7 @@ describe('auth', () => { await switchToOrganization(request, organizationId); - expect(data).toHaveBeenCalledWith({ success: true, auth: mockResponseWithoutCookie }, undefined); + expect(data).toHaveBeenCalledWith({ success: true, auth: safeAuth }, undefined); }); it('omits the Set-Cookie response header on returnTo when refreshSession returns none', async () => { diff --git a/src/auth.ts b/src/auth.ts index 3cc77dc..b3256f0 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -3,6 +3,7 @@ import { getAuthorizationUrl } from './get-authorization-url.js'; import { getClaimsFromAccessToken, getSessionFromCookie, refreshSession, terminateSession } from './session.js'; import { GetAuthURLResult, NoUserInfo, UserInfo } from './interfaces.js'; import { getConfig } from './config.js'; +import { sanitizeReturnPathname } from './return-pathname.js'; /** * Build a sign-in URL and the short-lived PKCE / CSRF cookie that must travel @@ -111,8 +112,9 @@ export async function withAuth(args: LoaderFunctionArgs): Promise` to satisfy the wider // typing on AuthLoaderSuccessData without silently emitting an empty // `Set-Cookie` header if the invariant ever changes. - const setCookie = (auth.headers as Record | undefined)?.['Set-Cookie']; + const setCookie = (headers as Record | undefined)?.['Set-Cookie']; const responseHeaders = setCookie ? { 'Set-Cookie': setCookie } : undefined; - // if returnTo is provided, redirect to there + // if returnTo is provided, redirect there. Same-origin pathname only, so a + // request-controlled value can't turn this into an open redirect. if (returnTo) { - return redirect(returnTo, responseHeaders ? { headers: responseHeaders } : undefined); + return redirect(sanitizeReturnPathname(returnTo), responseHeaders ? { headers: responseHeaders } : undefined); } // otherwise return the updated auth data