From 5500405f2fbf819a58d27a719081b7c9a2ef15bb Mon Sep 17 00:00:00 2001 From: jubnl <66769052+jubnl@users.noreply.github.com> Date: Mon, 15 Jun 2026 07:58:20 +0200 Subject: [PATCH] fix(security): stop cross-user offline data leak on shared devices (#1176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes BLOCKER B4 — three reinforcing paths could serve one account's cached data to the next user on a shared device: - The Workbox 'api-data' cache keyed trip/user-scoped GETs by URL only (cookie-blind). Changed to NetworkOnly; offline reads come from the per-user IndexedDB cache via the repo layer instead. - IndexedDB had no per-user scoping. The Dexie connection is now scoped per user (trek-offline-u) behind a Proxy so the ~19 importers keep a stable binding; login opens the user DB, logout deletes it and returns to the anonymous DB. - logout() was fire-and-forget and racy: background flush/syncAll could re-seed the DB after the wipe. It is now async and ordered — close an auth gate, unregister sync triggers, disconnect, clear caches, delete the user DB — and flush()/syncAll() bail when the gate is closed. --- client/src/db/offlineDb.ts | 79 ++++++++++++++++++- client/src/store/authStore.ts | 49 +++++++++--- client/src/sync/authGate.ts | 18 +++++ client/src/sync/mutationQueue.ts | 3 +- client/src/sync/tripSyncManager.ts | 3 +- client/tests/unit/db/offlineDb.test.ts | 37 +++++++++ client/tests/unit/stores/authStore.test.ts | 8 +- client/tests/unit/sync/mutationQueue.test.ts | 22 ++++++ .../tests/unit/sync/tripSyncManager.test.ts | 15 ++++ client/vite.config.js | 17 ++-- 10 files changed, 223 insertions(+), 28 deletions(-) create mode 100644 client/src/sync/authGate.ts diff --git a/client/src/db/offlineDb.ts b/client/src/db/offlineDb.ts index e94ceba1..bed9909b 100644 --- a/client/src/db/offlineDb.ts +++ b/client/src/db/offlineDb.ts @@ -54,6 +54,33 @@ export interface BlobCacheEntry { // ── Dexie class ──────────────────────────────────────────────────────────────── +/** + * The offline DB is scoped per user so that one account can never read another + * account's cached data on a shared device. Anonymous (logged-out) state uses + * the base name; a logged-in user uses `trek-offline-u`. + */ +const ANON_DB_NAME = 'trek-offline'; + +function userDbName(userId: number | string): string { + return `trek-offline-u${userId}`; +} + +/** + * Best-effort read of the persisted auth snapshot so the very first DB opened on + * app load (before loadUser resolves) is already the correct per-user one — the + * PWA can render cached data offline without leaking across users. + */ +function initialDbName(): string { + try { + const raw = typeof localStorage !== 'undefined' ? localStorage.getItem('trek_auth_snapshot') : null; + if (!raw) return ANON_DB_NAME; + const id = JSON.parse(raw)?.state?.user?.id; + return id != null ? userDbName(id) : ANON_DB_NAME; + } catch { + return ANON_DB_NAME; + } +} + class TrekOfflineDb extends Dexie { trips!: Table; days!: Table; @@ -71,8 +98,8 @@ class TrekOfflineDb extends Dexie { syncMeta!: Table; blobCache!: Table; - constructor() { - super('trek-offline'); + constructor(name: string = ANON_DB_NAME) { + super(name); this.version(1).stores({ trips: 'id', @@ -97,7 +124,53 @@ class TrekOfflineDb extends Dexie { } } -export const offlineDb = new TrekOfflineDb(); +// The live instance is swapped on login/logout via reopenForUser/reopenAnonymous. +// A Proxy keeps the exported `offlineDb` binding stable for the ~19 modules that +// import it directly, while every access forwards to the current connection. +let _db = new TrekOfflineDb(initialDbName()); + +export const offlineDb = new Proxy({} as TrekOfflineDb, { + get(_target, prop) { + const value = (_db as unknown as Record)[prop]; + return typeof value === 'function' ? (value as (...args: unknown[]) => unknown).bind(_db) : value; + }, + set(_target, prop, value) { + (_db as unknown as Record)[prop] = value; + return true; + }, +}) as TrekOfflineDb; + +async function switchTo(name: string): Promise { + if (_db.name === name) { + if (!_db.isOpen()) await _db.open(); + return; + } + if (_db.isOpen()) _db.close(); + _db = new TrekOfflineDb(name); + await _db.open(); +} + +/** Point the offline DB at a specific user's scoped database (call on login). */ +export async function reopenForUser(userId: number | string): Promise { + await switchTo(userDbName(userId)); +} + +/** Point the offline DB at the anonymous database (call on logout). */ +export async function reopenAnonymous(): Promise { + await switchTo(ANON_DB_NAME); +} + +/** + * Delete the current user's scoped database entirely and return to the anonymous + * DB. Used on logout so no trace of the account's data remains on the device. + */ +export async function deleteCurrentUserDb(): Promise { + if (_db.name !== ANON_DB_NAME) { + try { await _db.delete(); } catch { /* ignore — fall through to anon */ } + } + _db = new TrekOfflineDb(ANON_DB_NAME); + await _db.open(); +} // ── Bulk upsert helpers ──────────────────────────────────────────────────────── diff --git a/client/src/store/authStore.ts b/client/src/store/authStore.ts index 8d8c342d..ccf1c04a 100644 --- a/client/src/store/authStore.ts +++ b/client/src/store/authStore.ts @@ -5,7 +5,9 @@ import { connect, disconnect } from '../api/websocket' import type { User } from '../types' import { getApiErrorMessage } from '../types' import { tripSyncManager } from '../sync/tripSyncManager' -import { clearAll } from '../db/offlineDb' +import { reopenForUser, deleteCurrentUserDb } from '../db/offlineDb' +import { setAuthed } from '../sync/authGate' +import { unregisterSyncTriggers } from '../sync/syncTriggers' import { useSystemNoticeStore } from './systemNoticeStore.js' interface AuthResponse { @@ -40,7 +42,7 @@ interface AuthState { login: (email: string, password: string) => Promise completeMfaLogin: (mfaToken: string, code: string) => Promise register: (username: string, email: string, password: string, invite_token?: string) => Promise - logout: () => void + logout: () => Promise /** Pass `{ silent: true }` to refresh the user without toggling global isLoading (avoids unmounting protected routes). */ loadUser: (opts?: { silent?: boolean }) => Promise updateMapsKey: (key: string | null) => Promise @@ -65,6 +67,19 @@ interface AuthState { // Sequence counter to prevent stale loadUser responses from overwriting fresh auth state let authSequence = 0 +/** + * Mark the session authenticated and point the offline DB at this user's scoped + * database before any background sync runs, so cached data never crosses users. + */ +async function onAuthSuccess(userId: number): Promise { + setAuthed(true) + try { + await reopenForUser(userId) + } catch (err) { + console.error('[auth] failed to open user-scoped offline DB', err) + } +} + export const useAuthStore = create()( persist( (set, get) => ({ @@ -99,6 +114,7 @@ export const useAuthStore = create()( isLoading: false, error: null, }) + await onAuthSuccess(data.user.id) connect() tripSyncManager.syncAll().catch(console.error) if (!data.user?.must_change_password) { @@ -123,6 +139,7 @@ export const useAuthStore = create()( isLoading: false, error: null, }) + await onAuthSuccess(data.user.id) connect() tripSyncManager.syncAll().catch(console.error) if (!data.user?.must_change_password) { @@ -147,6 +164,7 @@ export const useAuthStore = create()( isLoading: false, error: null, }) + await onAuthSuccess(data.user.id) connect() tripSyncManager.syncAll().catch(console.error) useSystemNoticeStore.getState().fetch() @@ -158,18 +176,27 @@ export const useAuthStore = create()( } }, - logout: () => { + logout: async () => { + // 1. Gate first so any in-flight flush/syncAll bails before we wipe the DB. + setAuthed(false) + set({ isAuthenticated: false }) + // 2. Stop background sync triggers (30s interval, WS pre-reconnect hook, listeners). + unregisterSyncTriggers() + // 3. Tear down the live connection. disconnect() useSystemNoticeStore.getState().reset() - // Tell server to clear the httpOnly cookie - fetch('/api/auth/logout', { method: 'POST', credentials: 'include' }).catch(() => {}) - // Clear service worker caches containing sensitive data + // 4. Tell server to clear the httpOnly cookie (best-effort). + await fetch('/api/auth/logout', { method: 'POST', credentials: 'include' }).catch(() => {}) + // 5. Clear service worker caches containing sensitive data. if ('caches' in window) { - caches.delete('api-data').catch(() => {}) - caches.delete('user-uploads').catch(() => {}) + await Promise.all([ + caches.delete('api-data').catch(() => {}), + caches.delete('user-uploads').catch(() => {}), + ]) } - // Purge all cached trip data from IndexedDB - clearAll().catch(console.error) + // 6. Delete this user's scoped IndexedDB and return to the anonymous DB. + await deleteCurrentUserDb().catch(console.error) + // 7. Finish clearing auth state. set({ user: null, isAuthenticated: false, @@ -189,6 +216,7 @@ export const useAuthStore = create()( isAuthenticated: true, isLoading: false, }) + await onAuthSuccess(data.user.id) connect() } catch (err: unknown) { if (seq !== authSequence) return // stale response — ignore @@ -282,6 +310,7 @@ export const useAuthStore = create()( demoMode: true, error: null, }) + await onAuthSuccess(data.user.id) connect() return data } catch (err: unknown) { diff --git a/client/src/sync/authGate.ts b/client/src/sync/authGate.ts new file mode 100644 index 00000000..ffa868a0 --- /dev/null +++ b/client/src/sync/authGate.ts @@ -0,0 +1,18 @@ +/** + * Auth gate — a single boolean the sync layer checks before touching the + * offline DB. It lets logout disable all background sync (flush / syncAll / + * periodic triggers) *before* awaiting the DB swap, so an in-flight loop can't + * re-seed the database after the user has logged out. + * + * Kept separate from authStore to avoid an import cycle + * (authStore → tripSyncManager → authStore). + */ +let _authed = false + +export function setAuthed(value: boolean): void { + _authed = value +} + +export function isAuthed(): boolean { + return _authed +} diff --git a/client/src/sync/mutationQueue.ts b/client/src/sync/mutationQueue.ts index 34cc80a5..717d6997 100644 --- a/client/src/sync/mutationQueue.ts +++ b/client/src/sync/mutationQueue.ts @@ -7,6 +7,7 @@ */ import { offlineDb } from '../db/offlineDb' import { apiClient } from '../api/client' +import { isAuthed } from './authGate' import type { QueuedMutation } from '../db/offlineDb' import type { Table } from 'dexie' @@ -88,7 +89,7 @@ export const mutationQueue = { * 4xx responses are marked failed and skipped. */ async flush(): Promise { - if (_flushing || !navigator.onLine) return + if (_flushing || !navigator.onLine || !isAuthed()) return _flushing = true // tempId → realId learned during this flush, so a dependent edit/delete // queued against an offline-created entity (still holding the negative id) diff --git a/client/src/sync/tripSyncManager.ts b/client/src/sync/tripSyncManager.ts index 4129469b..a4662b25 100644 --- a/client/src/sync/tripSyncManager.ts +++ b/client/src/sync/tripSyncManager.ts @@ -29,6 +29,7 @@ import { clearTripData, } from '../db/offlineDb' import { prefetchTilesForTrip } from './tilePrefetcher' +import { isAuthed } from './authGate' import { useSettingsStore } from '../store/settingsStore' import type { Trip, Day, Place, PackingItem, TodoItem, BudgetItem, Reservation, TripFile, Accommodation, TripMember } from '../types' @@ -134,7 +135,7 @@ export const tripSyncManager = { * No-ops when offline. */ async syncAll(): Promise { - if (_syncing || !navigator.onLine) return + if (_syncing || !navigator.onLine || !isAuthed()) return _syncing = true try { const { trips } = await tripsApi.list() as { trips: Trip[] } diff --git a/client/tests/unit/db/offlineDb.test.ts b/client/tests/unit/db/offlineDb.test.ts index 8ec509d8..d6422fac 100644 --- a/client/tests/unit/db/offlineDb.test.ts +++ b/client/tests/unit/db/offlineDb.test.ts @@ -23,6 +23,9 @@ import { upsertReservations, upsertTripFiles, upsertSyncMeta, + reopenForUser, + reopenAnonymous, + deleteCurrentUserDb, type QueuedMutation, type SyncMeta, type BlobCacheEntry, @@ -271,3 +274,37 @@ describe('offlineDb — clearAll', () => { expect(await offlineDb.places.count()).toBe(0); }); }); + +describe('offlineDb — per-user scoping (B4)', () => { + afterEach(async () => { + // Leave the suite on the anonymous DB so other tests are unaffected. + await reopenAnonymous(); + }); + + it('isolates one user\'s cached data from another', async () => { + await reopenForUser(1); + await upsertPlaces([makePlace(10, 1)]); + expect(await offlineDb.places.count()).toBe(1); + + // Switching users must not expose user 1's rows. + await reopenForUser(2); + expect(await offlineDb.places.count()).toBe(0); + + // Switching back restores user 1's data (different physical DB). + await reopenForUser(1); + expect(await offlineDb.places.get(10)).toBeDefined(); + }); + + it('deleteCurrentUserDb wipes the user DB and returns to anonymous', async () => { + await reopenForUser(5); + await upsertPlaces([makePlace(20, 1)]); + + await deleteCurrentUserDb(); + // Now on the anonymous DB — no user data. + expect(await offlineDb.places.count()).toBe(0); + + // Re-opening user 5 starts empty (DB was deleted, not just detached). + await reopenForUser(5); + expect(await offlineDb.places.count()).toBe(0); + }); +}); diff --git a/client/tests/unit/stores/authStore.test.ts b/client/tests/unit/stores/authStore.test.ts index 5b25145f..22869719 100644 --- a/client/tests/unit/stores/authStore.test.ts +++ b/client/tests/unit/stores/authStore.test.ts @@ -105,10 +105,10 @@ describe('authStore', () => { }); describe('FE-AUTH-006: logout', () => { - it('calls disconnect() and clears user state', () => { + it('calls disconnect() and clears user state', async () => { useAuthStore.setState({ user: buildUser(), isAuthenticated: true }); - useAuthStore.getState().logout(); + await useAuthStore.getState().logout(); const state = useAuthStore.getState(); expect(disconnect).toHaveBeenCalledOnce(); @@ -441,10 +441,10 @@ describe('authStore', () => { }); describe('FE-STORE-AUTH-PERSIST-001: logout resets persisted snapshot', () => { - it('snapshot has isAuthenticated:false after logout (PWA offline will redirect to login)', () => { + it('snapshot has isAuthenticated:false after logout (PWA offline will redirect to login)', async () => { useAuthStore.setState({ user: buildUser(), isAuthenticated: true }); - useAuthStore.getState().logout(); + await useAuthStore.getState().logout(); const snapshot = JSON.parse(localStorage.getItem('trek_auth_snapshot') ?? '{}'); expect(snapshot?.state?.isAuthenticated).toBe(false); diff --git a/client/tests/unit/sync/mutationQueue.test.ts b/client/tests/unit/sync/mutationQueue.test.ts index ff4b5041..8e7868ac 100644 --- a/client/tests/unit/sync/mutationQueue.test.ts +++ b/client/tests/unit/sync/mutationQueue.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import 'fake-indexeddb/auto'; import { server } from '../../helpers/msw/server'; import { http, HttpResponse } from 'msw'; +import { setAuthed } from '../../../src/sync/authGate'; import { mutationQueue, generateUUID, nextTempId } from '../../../src/sync/mutationQueue'; import { offlineDb, clearAll } from '../../../src/db/offlineDb'; import { placeRepo } from '../../../src/repo/placeRepo'; @@ -16,11 +17,13 @@ import { buildPlace, buildPackingItem } from '../../helpers/factories'; beforeEach(async () => { await clearAll(); mutationQueue._resetFlushing(); + setAuthed(true); Object.defineProperty(navigator, 'onLine', { value: true, writable: true, configurable: true }); }); afterEach(() => { vi.restoreAllMocks(); + setAuthed(false); }); // ── helpers ────────────────────────────────────────────────────────────────── @@ -215,6 +218,25 @@ describe('mutationQueue.flush — offline guard', () => { const m = await offlineDb.mutationQueue.get(id); expect(m!.status).toBe('pending'); }); + + it('does nothing when logged out (auth gate closed)', async () => { + setAuthed(false); + const id = generateUUID(); + await mutationQueue.enqueue(makeMutation({ id })); + + let called = false; + server.use( + http.post('/api/trips/1/places', () => { + called = true; + return HttpResponse.json({ place: buildPlace({ trip_id: 1 }) }); + }), + ); + + await mutationQueue.flush(); + expect(called).toBe(false); + const m = await offlineDb.mutationQueue.get(id); + expect(m!.status).toBe('pending'); + }); }); // ── pending / pendingCount ──────────────────────────────────────────────────── diff --git a/client/tests/unit/sync/tripSyncManager.test.ts b/client/tests/unit/sync/tripSyncManager.test.ts index f72c23ac..46ff3829 100644 --- a/client/tests/unit/sync/tripSyncManager.test.ts +++ b/client/tests/unit/sync/tripSyncManager.test.ts @@ -9,6 +9,7 @@ import 'fake-indexeddb/auto'; import { server } from '../../helpers/msw/server'; import { http, HttpResponse } from 'msw'; import { tripSyncManager } from '../../../src/sync/tripSyncManager'; +import { setAuthed } from '../../../src/sync/authGate'; import { offlineDb, clearAll, upsertTrip } from '../../../src/db/offlineDb'; import { buildTrip, @@ -45,6 +46,7 @@ function makeBundle(tripId: number) { beforeEach(async () => { await clearAll(); tripSyncManager._resetSyncing(); + setAuthed(true); Object.defineProperty(navigator, 'onLine', { value: true, writable: true, configurable: true }); // Stub fetch for blob caching (used by cacheFilesForTrip) vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ @@ -56,6 +58,19 @@ beforeEach(async () => { afterEach(() => { vi.restoreAllMocks(); vi.unstubAllGlobals(); + setAuthed(false); +}); + +describe('tripSyncManager.syncAll — auth gate (B4)', () => { + it('no-ops when logged out (gate closed)', async () => { + setAuthed(false); + let called = false; + server.use( + http.get('/api/trips', () => { called = true; return HttpResponse.json({ trips: [] }); }), + ); + await tripSyncManager.syncAll(); + expect(called).toBe(false); + }); }); // ── offline guard ───────────────────────────────────────────────────────────── diff --git a/client/vite.config.js b/client/vite.config.js index ee81f09c..e4be800f 100644 --- a/client/vite.config.js +++ b/client/vite.config.js @@ -48,16 +48,15 @@ export default defineConfig({ }, }, { - // API calls — prefer network, fall back to cache - // Exclude sensitive endpoints (auth, admin, backup, settings) + // API calls — network only. We deliberately do NOT cache API + // responses in the Service Worker: Workbox keys entries by URL and + // cannot vary on the httpOnly session cookie, so a shared device + // could serve one user's cached data to the next (cross-user leak). + // Offline reads are served from the per-user IndexedDB cache via the + // repo layer instead. The urlPattern is kept so these requests still + // bypass the SPA navigation fallback. urlPattern: /\/api\/(?!auth|admin|backup|settings|health).*/i, - handler: 'NetworkFirst', - options: { - cacheName: 'api-data', - expiration: { maxEntries: 200, maxAgeSeconds: 24 * 60 * 60 }, - networkTimeoutSeconds: 5, - cacheableResponse: { statuses: [200] }, - }, + handler: 'NetworkOnly', }, { // Uploaded files (photos, covers — public assets only)