diff --git a/specs/TokenRefresh.cfg b/specs/TokenRefresh.cfg new file mode 100644 index 0000000..eb9714d --- /dev/null +++ b/specs/TokenRefresh.cfg @@ -0,0 +1,4 @@ +SPECIFICATION Spec +CONSTANT Clients = {c1, c2} +INVARIANT TypeOK +INVARIANT NoStaleOverwrite diff --git a/specs/TokenRefresh.tla b/specs/TokenRefresh.tla new file mode 100644 index 0000000..99fdaa5 --- /dev/null +++ b/specs/TokenRefresh.tla @@ -0,0 +1,120 @@ +---------------------------- MODULE TokenRefresh ---------------------------- +(***************************************************************************) +(* Model of the CURRENT design of getAccessToken in src/store/authStore.ts *) +(* (:97-120). *) +(* *) +(* getAccessToken is re-entrant with no mutex. It read-modify-writes a *) +(* single localStorage record across an await: read at :98, refresh at *) +(* :109, write at :111 (success) or :114 (failure). There are nine *) +(* independent call sites -- save, loadProject, toggleShare, *) +(* deleteCloudProject, and five in the project list / import UI -- any of *) +(* which can be in flight together (e.g. autosave firing while the user *) +(* clicks Share). *) +(* *) +(* This spec is expected to FAIL NoStaleOverwrite. *) +(***************************************************************************) +EXTENDS Naturals, FiniteSets + +CONSTANT Clients + +\* writePersisted(null) -- the record is gone and the user is signed out. +NoSession == [exists |-> FALSE, tok |-> 0] + +VARIABLES + store, \* the localStorage record (single shared cell) + pc, \* client -> "idle" | "refreshing" | "done" | "failed" + snap, \* client -> the record it read at :98, before its await + nextTok, \* source of fresh access tokens + staleWrite \* history variable: has anyone written based on a stale read? + +vars == <> + +Init == + \* The initial token is within 60s of expiry, so every caller takes the + \* refresh branch -- the case of interest. + /\ store = [exists |-> TRUE, tok |-> 1] + /\ pc = [c \in Clients |-> "idle"] + /\ snap = [c \in Clients |-> [exists |-> TRUE, tok |-> 1]] + /\ nextTok = 2 + /\ staleWrite = FALSE + +\* :98-99 -- no record at all. +ReadNoSession(c) == + /\ pc[c] = "idle" + /\ ~store.exists + /\ pc' = [pc EXCEPT ![c] = "failed"] + /\ UNCHANGED <> + +\* :98 then :101 -- snapshot the record and start refreshing. Nothing marks +\* the store as having a refresh in flight, so every client does this +\* independently. +ReadAndRefresh(c) == + /\ pc[c] = "idle" + /\ store.exists + /\ snap' = [snap EXCEPT ![c] = store] + /\ pc' = [pc EXCEPT ![c] = "refreshing"] + /\ UNCHANGED <> + +\* A write is stale if the record moved on after this client read it -- i.e. +\* the client is clobbering someone else's newer write. +IsStale(c) == snap[c] # store + +\* :109-112 -- POST succeeded; write {...persisted, accessToken} back. Note +\* the spread is off this client's OWN snapshot, not off current storage. +RefreshSucceeds(c) == + /\ pc[c] = "refreshing" + /\ store' = [exists |-> TRUE, tok |-> nextTok] + /\ nextTok' = nextTok + 1 + /\ pc' = [pc EXCEPT ![c] = "done"] + /\ staleWrite' = (staleWrite \/ IsStale(c)) + /\ UNCHANGED snap + +\* :113-117 -- ANY failure (network blip, 5xx, rotated refresh token) wipes +\* the entire auth record and signs the user out. +RefreshFails(c) == + /\ pc[c] = "refreshing" + /\ store' = NoSession + /\ pc' = [pc EXCEPT ![c] = "failed"] + /\ staleWrite' = (staleWrite \/ IsStale(c)) + /\ UNCHANGED <> + +Terminating == + /\ \A c \in Clients : pc[c] \in {"done", "failed"} + /\ UNCHANGED vars + +Next == + \/ \E c \in Clients : + ReadNoSession(c) \/ ReadAndRefresh(c) \/ RefreshSucceeds(c) \/ RefreshFails(c) + \/ Terminating + +Spec == Init /\ [][Next]_vars + +(***************************************************************************) +(* Properties. *) +(***************************************************************************) + +TypeOK == + /\ store.exists \in BOOLEAN + /\ \A c \in Clients : pc[c] \in {"idle", "refreshing", "done", "failed"} + +\* SAFETY. No caller writes the auth record based on a read the record has +\* already moved past. getAccessToken read-modify-writes across an await and +\* never re-validates, so this is exactly the defect. +\* +\* VIOLATED: two callers snapshot the same record and refresh concurrently; +\* the first succeeds and advances the record; the second then writes -- +\* clobbering a session established after its own read. When the second +\* call fails (a blip, a 5xx, a rotated refresh token) that write is +\* writePersisted(null), signing the user out mid-save. +\* +\* Note this is deliberately NOT "a successful refresh is never followed by +\* a logout" -- that is too strong, since a later, sequential refresh may +\* legitimately discover a dead token and sign out correctly. +NoStaleOverwrite == ~staleWrite + +\* The mechanism behind the violation: nothing bounds concurrent refreshes. +\* VIOLATED trivially with two clients. +AtMostOneRefreshInFlight == + Cardinality({c \in Clients : pc[c] = "refreshing"}) <= 1 + +============================================================================= diff --git a/specs/TokenRefreshFixed.cfg b/specs/TokenRefreshFixed.cfg new file mode 100644 index 0000000..3079f80 --- /dev/null +++ b/specs/TokenRefreshFixed.cfg @@ -0,0 +1,9 @@ +SPECIFICATION Spec + +CONSTANT Clients = {c1, c2, c3} +CONSTANT NoLeader = NoLeader + +INVARIANT TypeOK +INVARIANT NoStaleOverwrite +INVARIANT AtMostOneRefreshInFlight +INVARIANT WaitersDoNotRefresh diff --git a/specs/TokenRefreshFixed.tla b/specs/TokenRefreshFixed.tla new file mode 100644 index 0000000..cb4508e --- /dev/null +++ b/specs/TokenRefreshFixed.tla @@ -0,0 +1,135 @@ +------------------------- MODULE TokenRefreshFixed -------------------------- +(***************************************************************************) +(* Corrected getAccessToken protocol. *) +(* *) +(* Two changes relative to TokenRefresh.tla: *) +(* *) +(* 1. Single-flight. The first caller to need a refresh becomes the *) +(* leader and stores its in-flight promise; concurrent callers await *) +(* that same promise instead of issuing their own refresh. At most one *) +(* refresh is ever in flight, so no caller can be signed out by *) +(* another caller's failure. *) +(* *) +(* 2. Only a definitive auth failure clears the session. invalid_grant / *) +(* 401 means the refresh token is genuinely dead -> sign out. A *) +(* network error or 5xx rejects the waiting callers but LEAVES the *) +(* record intact, so a blip mid-save no longer logs the user out. *) +(* *) +(* All three properties hold. *) +(***************************************************************************) +EXTENDS Naturals, FiniteSets + +\* NoLeader is a model value standing for "no refresh in flight". +CONSTANTS Clients, NoLeader + +NoSession == [exists |-> FALSE, tok |-> 0] + +VARIABLES + store, + pc, \* client -> "idle" | "refreshing" | "waiting" | "done" | "failed" + leader, \* the client owning the in-flight refresh, or NoLeader + snap, \* the leader's snapshot, taken when it acquired the lock + nextTok, + staleWrite \* history variable, as in TokenRefresh.tla + +vars == <> + +Init == + /\ store = [exists |-> TRUE, tok |-> 1] + /\ pc = [c \in Clients |-> "idle"] + /\ leader = NoLeader + /\ snap = [exists |-> TRUE, tok |-> 1] + /\ nextTok = 2 + /\ staleWrite = FALSE + +ReadNoSession(c) == + /\ pc[c] = "idle" + /\ ~store.exists + /\ pc' = [pc EXCEPT ![c] = "failed"] + /\ UNCHANGED <> + +\* No refresh in flight: take the lock and issue one. +AcquireAndRefresh(c) == + /\ pc[c] = "idle" + /\ store.exists + /\ leader = NoLeader + /\ leader' = c + /\ snap' = store + /\ pc' = [pc EXCEPT ![c] = "refreshing"] + /\ UNCHANGED <> + +\* A refresh is already in flight: await the SAME promise rather than +\* issuing a second one. +JoinInflight(c) == + /\ pc[c] = "idle" + /\ store.exists + /\ leader # NoLeader + /\ pc' = [pc EXCEPT ![c] = "waiting"] + /\ UNCHANGED <> + +\* Everyone sharing the in-flight promise settles together. +Settle(c, outcome) == + [x \in Clients |-> IF x = c \/ pc[x] = "waiting" THEN outcome ELSE pc[x]] + +LeaderSucceeds(c) == + /\ leader = c + /\ pc[c] = "refreshing" + /\ store' = [exists |-> TRUE, tok |-> nextTok] + /\ nextTok' = nextTok + 1 + /\ pc' = Settle(c, "done") + /\ leader' = NoLeader + /\ staleWrite' = (staleWrite \/ (snap # store)) + /\ UNCHANGED snap + +\* invalid_grant / 401: the refresh token is dead. Signing out is correct. +LeaderRejectedByProvider(c) == + /\ leader = c + /\ pc[c] = "refreshing" + /\ store' = NoSession + /\ pc' = Settle(c, "failed") + /\ leader' = NoLeader + /\ staleWrite' = (staleWrite \/ (snap # store)) + /\ UNCHANGED <> + +\* Network error / 5xx: fail the callers but KEEP the session. The old code +\* wiped the record here, turning a dropped packet into a forced re-auth. +LeaderTransientFailure(c) == + /\ leader = c + /\ pc[c] = "refreshing" + /\ pc' = Settle(c, "failed") + /\ leader' = NoLeader + /\ UNCHANGED <> + +Terminating == + /\ \A c \in Clients : pc[c] \in {"done", "failed"} + /\ UNCHANGED vars + +Next == + \/ \E c \in Clients : + \/ ReadNoSession(c) \/ AcquireAndRefresh(c) \/ JoinInflight(c) + \/ LeaderSucceeds(c) \/ LeaderRejectedByProvider(c) \/ LeaderTransientFailure(c) + \/ Terminating + +Spec == Init /\ [][Next]_vars + +(***************************************************************************) +(* Properties -- the first two are the same statements as TokenRefresh.tla.*) +(***************************************************************************) + +TypeOK == + /\ store.exists \in BOOLEAN + /\ \A c \in Clients : pc[c] \in {"idle", "refreshing", "waiting", "done", "failed"} + /\ leader \in Clients \cup {NoLeader} + +\* Same statement as TokenRefresh.tla. Holds here: the lock means no other +\* client can write between the leader's read and its own write. +NoStaleOverwrite == ~staleWrite + +AtMostOneRefreshInFlight == + Cardinality({c \in Clients : pc[c] = "refreshing"}) <= 1 + +\* A client waiting on the shared promise never issues its own refresh. +WaitersDoNotRefresh == + \A c \in Clients : pc[c] = "refreshing" => leader = c + +============================================================================= diff --git a/src/auth/oauth.ts b/src/auth/oauth.ts index b1c150f..f2250ab 100644 --- a/src/auth/oauth.ts +++ b/src/auth/oauth.ts @@ -135,6 +135,18 @@ export async function completeSignIn(): Promise { }; } +/** + * A refresh that failed. `definitive` distinguishes "this refresh token is + * dead, sign the user out" from "the network hiccuped, keep the session". + * Treating the two alike turns a dropped packet into a forced re-auth. + */ +export class RefreshError extends Error { + constructor(message: string, readonly definitive: boolean) { + super(message); + this.name = 'RefreshError'; + } +} + export async function refreshGoogleToken(refreshToken: string): Promise<{ accessToken: string; expiresAt: number }> { const res = await fetch('/api/auth/google/refresh', { method: 'POST', @@ -143,7 +155,11 @@ export async function refreshGoogleToken(refreshToken: string): Promise<{ access }); if (!res.ok) { const err = await res.json().catch(() => ({ error: res.statusText })); - throw new Error(err.error || `Refresh failed (${res.status})`); + // 400 invalid_grant / 401: the grant is genuinely gone. Anything else + // (5xx, 429, gateway errors) is transient — a fetch rejection never + // reaches here at all and is transient by construction. + const definitive = res.status === 400 || res.status === 401; + throw new RefreshError(err.error || `Refresh failed (${res.status})`, definitive); } const data = await res.json(); return { diff --git a/src/store/authStore.test.ts b/src/store/authStore.test.ts new file mode 100644 index 0000000..329d313 --- /dev/null +++ b/src/store/authStore.test.ts @@ -0,0 +1,162 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +/** + * Regression tests for single-flight token refresh. + * + * The first case is the counterexample TLC produced against the old design + * (specs/TokenRefresh.tla): two concurrent callers snapshot the same auth + * record, both refresh, and the second writes based on a read the record has + * already moved past. + */ + +const refreshGoogleToken = vi.fn(); + +class RefreshError extends Error { + constructor(message: string, readonly definitive: boolean) { + super(message); + this.name = 'RefreshError'; + } +} + +vi.mock('../auth/oauth', () => ({ + startSignIn: vi.fn(), + completeSignIn: vi.fn(), + fetchUserProfile: vi.fn(), + refreshGoogleToken: (...args: unknown[]) => refreshGoogleToken(...args), + RefreshError, +})); +vi.mock('../storage', () => ({ clearProviderCaches: vi.fn() })); + +const AUTH_KEY = 'sinter_auth'; + +/** An auth record already inside the 60s refresh window. */ +function expiringAuth(accessToken = 'old-token') { + return { + provider: 'google', + accessToken, + refreshToken: 'refresh-1', + expiresAt: Date.now() + 1_000, + user: { id: 'u1', email: 'a@b.c', name: 'A', avatar_url: '', provider: 'google' }, + }; +} + +/** + * In-memory localStorage. jsdom does not reliably expose one here, and the + * store's whole contract is about what lands in this cell, so an explicit + * stub is both more portable and easier to assert against. + */ +function makeStorageStub(): Storage { + const cell = new Map(); + return { + getItem: (k) => cell.get(k) ?? null, + setItem: (k, v) => void cell.set(k, String(v)), + removeItem: (k) => void cell.delete(k), + clear: () => cell.clear(), + key: (i) => [...cell.keys()][i] ?? null, + get length() { return cell.size; }, + } as Storage; +} + +const readStored = () => { + const raw = localStorage.getItem(AUTH_KEY); + return raw ? JSON.parse(raw) : null; +}; + +let useAuthStore: typeof import('./authStore').useAuthStore; + +beforeEach(async () => { + vi.resetModules(); + refreshGoogleToken.mockReset(); + vi.stubGlobal('localStorage', makeStorageStub()); + localStorage.setItem(AUTH_KEY, JSON.stringify(expiringAuth())); + useAuthStore = (await import('./authStore')).useAuthStore; +}); + +afterEach(() => vi.unstubAllGlobals()); + +describe('getAccessToken single-flight refresh', () => { + it('issues one refresh for concurrent callers', async () => { + refreshGoogleToken.mockResolvedValue({ accessToken: 'new-token', expiresAt: Date.now() + 3_600_000 }); + + const results = await Promise.all([ + useAuthStore.getState().getAccessToken(), + useAuthStore.getState().getAccessToken(), + useAuthStore.getState().getAccessToken(), + ]); + + // The old design fired one refresh per caller. + expect(refreshGoogleToken).toHaveBeenCalledTimes(1); + expect(results).toEqual(['new-token', 'new-token', 'new-token']); + expect(readStored().accessToken).toBe('new-token'); + }); + + it('does not sign the user out when a concurrent caller fails', async () => { + // The TLC counterexample: two callers refresh from the same snapshot, one + // succeeds, the other's outcome must not clobber the established session. + let call = 0; + refreshGoogleToken.mockImplementation(async () => { + call += 1; + if (call === 1) return { accessToken: 'new-token', expiresAt: Date.now() + 3_600_000 }; + throw new RefreshError('network down', false); + }); + + const [a, b] = await Promise.allSettled([ + useAuthStore.getState().getAccessToken(), + useAuthStore.getState().getAccessToken(), + ]); + + expect(a.status).toBe('fulfilled'); + expect(b.status).toBe('fulfilled'); + // Single-flight means the second call never issued a competing refresh, + // so there is no second outcome to wipe the record. + expect(refreshGoogleToken).toHaveBeenCalledTimes(1); + expect(readStored()).not.toBeNull(); + expect(readStored().accessToken).toBe('new-token'); + }); + + it('keeps the session on a transient failure', async () => { + refreshGoogleToken.mockRejectedValue(new RefreshError('502 Bad Gateway', false)); + + await expect(useAuthStore.getState().getAccessToken()).rejects.toThrow('502 Bad Gateway'); + + // The old design called writePersisted(null) here, turning a gateway blip + // into a forced re-auth. + expect(readStored()).not.toBeNull(); + expect(readStored().refreshToken).toBe('refresh-1'); + }); + + it('clears the session when the grant is definitively dead', async () => { + refreshGoogleToken.mockRejectedValue(new RefreshError('invalid_grant', true)); + + await expect(useAuthStore.getState().getAccessToken()).rejects.toThrow('invalid_grant'); + + expect(readStored()).toBeNull(); + expect(useAuthStore.getState().user).toBeNull(); + }); + + it('does not resurrect a record removed during the refresh', async () => { + refreshGoogleToken.mockImplementation(async () => { + // Logout lands while the refresh is in flight. + localStorage.removeItem(AUTH_KEY); + return { accessToken: 'new-token', expiresAt: Date.now() + 3_600_000 }; + }); + + await expect(useAuthStore.getState().getAccessToken()).rejects.toThrow('Signed out during refresh'); + expect(readStored()).toBeNull(); + }); + + it('allows a fresh refresh after the previous one settles', async () => { + refreshGoogleToken.mockResolvedValue({ accessToken: 'new-token', expiresAt: Date.now() + 1_000 }); + await useAuthStore.getState().getAccessToken(); + // Still inside the refresh window, so the next call refreshes again — + // the in-flight slot must have been released. + await useAuthStore.getState().getAccessToken(); + expect(refreshGoogleToken).toHaveBeenCalledTimes(2); + }); + + it('skips refresh entirely for a token that is not near expiry', async () => { + localStorage.setItem(AUTH_KEY, JSON.stringify({ ...expiringAuth('fresh'), expiresAt: Date.now() + 3_600_000 })); + await expect(useAuthStore.getState().getAccessToken()).resolves.toBe('fresh'); + expect(refreshGoogleToken).not.toHaveBeenCalled(); + }); +}); diff --git a/src/store/authStore.ts b/src/store/authStore.ts index e1fb8a8..3a5e9de 100644 --- a/src/store/authStore.ts +++ b/src/store/authStore.ts @@ -5,11 +5,15 @@ import { completeSignIn, refreshGoogleToken, fetchUserProfile, + RefreshError, } from '../auth/oauth'; import { clearProviderCaches } from '../storage'; const AUTH_KEY = 'sinter_auth'; +/** The in-flight refresh, shared by all concurrent getAccessToken callers. */ +let refreshInFlight: Promise | null = null; + export interface AuthUser { id: string; email: string; @@ -97,26 +101,55 @@ export const useAuthStore = create((set) => ({ getAccessToken: async () => { const persisted = readPersisted(); if (!persisted) throw new Error('Not signed in'); + // Refresh Google tokens that are within 60s of expiry. - if (persisted.provider === 'google' && persisted.expiresAt && Date.now() > persisted.expiresAt - 60_000) { - if (!persisted.refreshToken) { - // Refresh token missing: force re-auth. - writePersisted(null); - set({ user: null }); - throw new Error('Session expired — please sign in again'); - } + const needsRefresh = + persisted.provider === 'google' && + persisted.expiresAt && + Date.now() > persisted.expiresAt - 60_000; + if (!needsRefresh) return persisted.accessToken; + + if (!persisted.refreshToken) { + // Refresh token missing: force re-auth. + writePersisted(null); + set({ user: null }); + throw new Error('Session expired — please sign in again'); + } + + // Single-flight. There are nine independent getAccessToken call sites and + // they can overlap freely (autosave firing while the user clicks Share). + // Without this, each one issues its own refresh and each writes the shared + // localStorage record based on a snapshot taken before its await — so one + // caller's failure clobbers a session another caller just established. + // See specs/TokenRefresh.tla. + if (refreshInFlight) return refreshInFlight; + + const refreshToken = persisted.refreshToken; + refreshInFlight = (async () => { try { - const refreshed = await refreshGoogleToken(persisted.refreshToken); - const updated: PersistedAuth = { ...persisted, accessToken: refreshed.accessToken, expiresAt: refreshed.expiresAt }; - writePersisted(updated); + const refreshed = await refreshGoogleToken(refreshToken); + // Re-read rather than spreading the pre-await snapshot: the record may + // have been replaced (second tab finished a sign-in) or removed + // (logout) while this refresh was in flight, and neither should be + // resurrected. + const current = readPersisted(); + if (!current) throw new Error('Signed out during refresh'); + writePersisted({ ...current, accessToken: refreshed.accessToken, expiresAt: refreshed.expiresAt }); return refreshed.accessToken; } catch (err) { - writePersisted(null); - set({ user: null }); + // Only a dead grant clears the session. A network blip or 5xx fails + // this call but leaves the user signed in. + if (err instanceof RefreshError && err.definitive) { + writePersisted(null); + set({ user: null }); + } throw err; + } finally { + refreshInFlight = null; } - } - return persisted.accessToken; + })(); + + return refreshInFlight; }, }));