From 54418362743d6ec8c60016fe2b838e1e32084923 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 13 Jul 2026 19:43:48 -0400 Subject: [PATCH] fix(web): harden auth session teardown per /sh-security-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AUTH-L1 (confirmed medium): logout() now clears the react-query cache — the singleton cache survived SPA logout, serving the previous principal's cached GETs to the next login in the same tab for up to staleTime with no server round-trip. - AUTH-L3 (confirmed low): isTokenValid decodes base64url before atob — valid Cognito JWTs containing '-'/'_' in the payload segment were misclassified as expired (login lockout/loop; inherited from the old authSlice). - AUTH-L2 (unverified, hardened anyway): 401 interceptor broadcasts AUTH_SESSION_CLEARED_EVENT so AuthProvider drops in-memory state synchronously, restoring the old Redux atomic-clear semantics. - INJ-1 (unverified, hardened anyway): Authorization header only set when the stored token is a string. Each fix pinned by a test; 63 vitest green, tsc clean. --- web/src/lib/api/__tests__/client.test.ts | 12 +++++++++ web/src/lib/api/client.ts | 9 ++++--- .../lib/auth/__tests__/authStorage.test.ts | 11 ++++++++ web/src/lib/auth/authStorage.ts | 7 +++++- web/src/providers/AuthProvider.tsx | 23 +++++++++++++++-- .../providers/__tests__/AuthProvider.test.tsx | 25 ++++++++++++++++++- 6 files changed, 79 insertions(+), 8 deletions(-) diff --git a/web/src/lib/api/__tests__/client.test.ts b/web/src/lib/api/__tests__/client.test.ts index d05ac6e..b2daf07 100644 --- a/web/src/lib/api/__tests__/client.test.ts +++ b/web/src/lib/api/__tests__/client.test.ts @@ -120,6 +120,18 @@ describe('API client interceptors', () => { expect(result.headers.Authorization).toBeUndefined(); }); + it('does not attach header when the token field is not a string', () => { + sessionStorageData['proposal_system_token'] = JSON.stringify({ token: { a: 1 } }); + + const config = { + headers: {} as Record, + } as unknown as InternalAxiosRequestConfig; + + const result = interceptors.requestFulfilled(config); + + expect(result.headers.Authorization).toBeUndefined(); + }); + it('WEB-C1: does not attach header when token field is missing in parsed data', () => { sessionStorageData['proposal_system_token'] = JSON.stringify({ email: 'user@test.com' }); diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 1ab96bd..3e5ffcd 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -1,6 +1,6 @@ import axios from 'axios'; import { API_URL, STORAGE_KEY_TOKEN } from '../../constants'; -import { clearAuth } from '../auth/authStorage'; +import { AUTH_SESSION_CLEARED_EVENT, clearAuth } from '../auth/authStorage'; const apiClient = axios.create({ baseURL: API_URL, @@ -16,7 +16,7 @@ apiClient.interceptors.request.use( if (tokenData) { try { const parsed = JSON.parse(tokenData); - if (parsed?.token) { + if (typeof parsed?.token === 'string') { config.headers.Authorization = `Bearer ${parsed.token}`; } } catch { @@ -35,9 +35,10 @@ apiClient.interceptors.response.use( const { status, data } = error.response; if (status === 401) { - // Fix: WEB-M2 — clear the stored session before redirecting; the full-page - // navigation resets the AuthProvider, so context state re-derives as logged out. + // Fix: WEB-M2 — terminate the session atomically: clear storage, tell the + // AuthProvider to drop in-memory state, then hard-redirect as belt-and-braces. clearAuth(); + window.dispatchEvent(new Event(AUTH_SESSION_CLEARED_EVENT)); window.location.href = '/login'; return Promise.reject(new Error('Session expired. Please log in again.')); } diff --git a/web/src/lib/auth/__tests__/authStorage.test.ts b/web/src/lib/auth/__tests__/authStorage.test.ts index aa936e3..cdaa2ca 100644 --- a/web/src/lib/auth/__tests__/authStorage.test.ts +++ b/web/src/lib/auth/__tests__/authStorage.test.ts @@ -67,6 +67,17 @@ describe('authStorage', () => { expect(isTokenValid(createToken(-3600))).toBe(false); }); + it('isTokenValid accepts a base64url payload (JWT segments are base64url, not base64)', () => { + // '>>>' encodes to 'Pj4-' in base64url — the '-' made the old atob() call throw. + const payload = btoa(JSON.stringify({ sub: '>>>>>>', exp: Math.floor(Date.now() / 1000) + 3600 })) + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=+$/, ''); + expect(payload).toMatch(/[-_]/); + + expect(isTokenValid(`${btoa(JSON.stringify({ alg: 'HS256' }))}.${payload}.sig`)).toBe(true); + }); + it('QA-C5: isTokenValid rejects missing or malformed tokens', () => { expect(isTokenValid(null)).toBe(false); expect(isTokenValid(undefined)).toBe(false); diff --git a/web/src/lib/auth/authStorage.ts b/web/src/lib/auth/authStorage.ts index 648e1bd..ca0e7ce 100644 --- a/web/src/lib/auth/authStorage.ts +++ b/web/src/lib/auth/authStorage.ts @@ -22,12 +22,17 @@ export function clearAuth(): void { window.sessionStorage.removeItem(STORAGE_KEY_TOKEN); // WEB-C1 } +// Dispatched by non-React code (the 401 interceptor) so AuthProvider can drop +// in-memory session state synchronously, keeping storage and context atomic. +export const AUTH_SESSION_CLEARED_EVENT = 'auth:session-cleared'; + export function isTokenValid(token: string | null | undefined): boolean { if (!token) return false; try { const part = token.split('.')[1]; if (!part) return false; - const payload = JSON.parse(atob(part)); + // JWT segments are base64url; atob only accepts standard base64. + const payload = JSON.parse(atob(part.replace(/-/g, '+').replace(/_/g, '/'))); return new Date(payload.exp * 1000) > new Date(); } catch { return false; diff --git a/web/src/providers/AuthProvider.tsx b/web/src/providers/AuthProvider.tsx index 7cd96f7..c7150b0 100644 --- a/web/src/providers/AuthProvider.tsx +++ b/web/src/providers/AuthProvider.tsx @@ -3,9 +3,16 @@ // this provider only owns the in-memory state. Token acquisition (Cognito code // exchange, dev-login) stays in the auth pages — they hand the resolved AuthUser // to login(). -import { useCallback, useMemo, useState, type ReactNode } from 'react'; +import { useCallback, useEffect, useMemo, useState, type ReactNode } from 'react'; import type { AuthUser } from '@proposal-system/api-contracts'; -import { clearAuth, getAuthUser, isTokenValid, setAuthUser } from '../lib/auth/authStorage'; +import { + AUTH_SESSION_CLEARED_EVENT, + clearAuth, + getAuthUser, + isTokenValid, + setAuthUser, +} from '../lib/auth/authStorage'; +import { queryClient } from '../lib/queryClient'; import { AuthContext, type AuthContextValue } from './authContext'; export default function AuthProvider({ children }: { children: ReactNode }) { @@ -22,10 +29,22 @@ export default function AuthProvider({ children }: { children: ReactNode }) { const logout = useCallback(() => { clearAuth(); + // Drop all principal-scoped state: cached queries must not survive into the + // next login in the same tab, or query keys shared across users would serve + // the previous principal's data without hitting the server. + queryClient.clear(); setUser(null); setErrorState(null); }, []); + // The 401 interceptor clears storage outside React; mirror it here so context + // state never outlives the stored session while the redirect commits. + useEffect(() => { + const onSessionCleared = () => setUser(null); + window.addEventListener(AUTH_SESSION_CLEARED_EVENT, onSessionCleared); + return () => window.removeEventListener(AUTH_SESSION_CLEARED_EVENT, onSessionCleared); + }, []); + const setError = useCallback((message: string | null) => { setErrorState(message); setIsLoading(false); diff --git a/web/src/providers/__tests__/AuthProvider.test.tsx b/web/src/providers/__tests__/AuthProvider.test.tsx index 81ce812..7271802 100644 --- a/web/src/providers/__tests__/AuthProvider.test.tsx +++ b/web/src/providers/__tests__/AuthProvider.test.tsx @@ -13,7 +13,8 @@ import { describe, it, expect, beforeEach } from 'vitest'; import type { AuthUser } from '@proposal-system/api-contracts'; import AuthProvider from '../AuthProvider'; import { useAuthContext } from '../authContext'; -import { setAuthUser } from '../../lib/auth/authStorage'; +import { AUTH_SESSION_CLEARED_EVENT, setAuthUser } from '../../lib/auth/authStorage'; +import { queryClient } from '../../lib/queryClient'; import { STORAGE_KEY_TOKEN } from '../../constants'; function createToken(expOffsetSeconds: number): string { @@ -78,6 +79,28 @@ describe('AuthProvider', () => { expect(window.sessionStorage.getItem(STORAGE_KEY_TOKEN)).toBeNull(); }); + it('logout clears the react-query cache so the next principal cannot read cached data', () => { + queryClient.setQueryData(['proposals', 'list'], [{ id: 'p1' }]); + const { result } = renderAuth(); + + act(() => result.current.login(createUser())); + act(() => result.current.logout()); + + expect(queryClient.getQueryData(['proposals', 'list'])).toBeUndefined(); + }); + + it('drops in-memory session state when the 401 interceptor broadcasts session-cleared', () => { + const { result } = renderAuth(); + + act(() => result.current.login(createUser())); + act(() => { + window.dispatchEvent(new Event(AUTH_SESSION_CLEARED_EVENT)); + }); + + expect(result.current.user).toBeNull(); + expect(result.current.isAuthenticated).toBe(false); + }); + it('QA-C5: initializes from an existing stored session', () => { const user = createUser(); setAuthUser(user);