diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fa0eff..5d60fd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## v1.11.2 + +- **Fixed a cross-tab logout.** Two tabs of the same session left open + long enough could each try to renew an expired login at the same moment; + the server treats that second, near-simultaneous renewal as stolen + credentials and revokes the whole session, logging out every tab even + though nothing was actually wrong. Renewals across tabs are now + serialized so this can no longer happen. + ## v1.11.1 - **Core counts you can actually read.** Legacy Cores were the one number in diff --git a/client/package-lock.json b/client/package-lock.json index 49a790a..ae8c168 100644 --- a/client/package-lock.json +++ b/client/package-lock.json @@ -1,12 +1,12 @@ { "name": "rackstack-client", - "version": "1.4.0", + "version": "1.5.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "rackstack-client", - "version": "1.4.0", + "version": "1.5.0", "dependencies": { "lucide-react": "^0.383.0", "react": "^18.3.1", diff --git a/client/src/game/api.js b/client/src/game/api.js index 79d67ed..176bdb9 100644 --- a/client/src/game/api.js +++ b/client/src/game/api.js @@ -67,31 +67,72 @@ export function __resetAuthRefreshForTests() { // forced logout, and only ever under concurrency. // // One promise, shared by every caller, same shape as server/userLock.js. +// +// That covers concurrency WITHIN one tab. It does nothing across tabs, and +// this is an idle game - the genre where players routinely leave it open in +// two or more at once, each polling independently. When two tabs' access +// tokens expire in the same window, each tab's inFlightRefresh is its own +// module-level variable, so each fires its own refresh presenting what was, +// at send time, the SAME refresh-token cookie - the exact multi-caller race +// described above, just with the callers in different browsing contexts +// instead of different promises in one. withRefreshLock is what closes that +// gap: it serialises the actual network call across tabs too. let inFlightRefresh = null; +// Web Locks name for cross-tab refresh serialisation. Same-origin only by +// construction (the API is scoped per-origin), which is exactly the set of +// tabs sharing the session cookie this is protecting. +const REFRESH_LOCK_NAME = 'rackstack:session-refresh'; + +/** + * Runs `fn` (the actual refresh network call) exclusively across every + * same-origin tab via the Web Locks API, falling back to running it directly + * when that API is unavailable - older browsers, and this project's own test + * environment, which runs under plain Node and has no navigator.locks (same + * feature-detection convention as makeActionQueue()'s window/navigator checks + * below, and game/auth.js's sessionStorage try/catch: degrade silently to + * today's per-tab-only behaviour rather than throw). + * + * A hand-rolled localStorage/BroadcastChannel mutex could do the same job but + * would need its own deadlock recovery for a tab that dies mid-refresh; Web + * Locks releases automatically when the holding context is destroyed, so + * there is nothing to recover from. + */ +function withRefreshLock(fn) { + const locks = typeof navigator !== 'undefined' ? navigator.locks : undefined; + if (!locks || typeof locks.request !== 'function') return fn(); + return locks.request(REFRESH_LOCK_NAME, fn); +} + +async function doRefreshRequest() { + try { + const res = await fetch('/auth/session/refresh', { + method: 'POST', + credentials: 'include', + // Same reason as the signinup call in game/auth.js: this is what tells + // SuperTokens to put the rotated tokens back in cookies. Refresh + // tolerates its absence better than session creation does (it infers + // the method from the tokens it was given), but a refresh that + // silently switched the session to header transport would log the + // player out on the next request, which is the same invisible failure + // one step later. + headers: { 'st-auth-mode': 'cookie' }, + }); + return res.ok; + } catch { + return false; + } +} + function refreshSession() { if (inFlightRefresh) return inFlightRefresh; inFlightRefresh = (async () => { try { - const res = await fetch('/auth/session/refresh', { - method: 'POST', - credentials: 'include', - // Same reason as the signinup call in game/auth.js: this is what tells - // SuperTokens to put the rotated tokens back in cookies. Refresh - // tolerates its absence better than session creation does (it infers - // the method from the tokens it was given), but a refresh that - // silently switched the session to header transport would log the - // player out on the next request, which is the same invisible failure - // one step later. - headers: { 'st-auth-mode': 'cookie' }, - }); - return res.ok; - } catch { - return false; + return await withRefreshLock(doRefreshRequest); } finally { - // Cleared before this promise settles, so the NEXT 401 starts a fresh - // attempt rather than re-awaiting a completed one. + // Cleared before this promise settles, so the NEXT 401 in THIS tab + // starts a fresh attempt rather than re-awaiting a completed one. inFlightRefresh = null; } })(); diff --git a/docs/authentication-methods.md b/docs/authentication-methods.md index ae91a22..1cd9dca 100644 --- a/docs/authentication-methods.md +++ b/docs/authentication-methods.md @@ -233,6 +233,16 @@ serialised so a burst of concurrent 401s produces exactly one refresh call. `GET /api/auth-info` tells the client which stack to drive, so one build serves `passport` and `supertokens` alike and the rollback stays real. +> **That serialisation now spans tabs, not just one page (v1.11.2).** SuperTokens +> rotates the refresh token on every use, so two tabs of the same session +> refreshing at once — routine for an idle game left open in several — each +> present what was, at send time, the same token; whichever the core sees +> second reads as theft and revokes the whole session, logging out every tab. +> `refreshSession()` wraps its network call in a Web Locks +> (`navigator.locks`) mutex shared by every same-origin tab, so two tabs' +> refreshes are never sent concurrently. Falls back to the old per-tab-only +> guarantee on a browser (or test runtime) without Web Locks. + > **If you touch those calls, keep the `st-auth-mode: cookie` header.** > SuperTokens chooses the session's token transfer method at creation from that > header, and **defaults to `header` when it is absent** — the session comes diff --git a/package-lock.json b/package-lock.json index 2128d17..2c77058 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "rackstack-server", - "version": "1.11.1", + "version": "1.11.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "rackstack-server", - "version": "1.11.1", + "version": "1.11.2", "dependencies": { "better-sqlite3": "^11.3.0", "cookie-parser": "^1.4.6", diff --git a/package.json b/package.json index 9bee95a..0e3abb0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rackstack-server", - "version": "1.11.1", + "version": "1.11.2", "private": true, "type": "module", "scripts": { diff --git a/tests/clientAuth.test.js b/tests/clientAuth.test.js index 2633d1f..1c8c0f5 100644 --- a/tests/clientAuth.test.js +++ b/tests/clientAuth.test.js @@ -35,14 +35,21 @@ function jsonResponse(body, { ok = true, status = 200 } = {}) { } let originalFetch; +let originalNavigator; beforeEach(() => { originalFetch = globalThis.fetch; + originalNavigator = globalThis.navigator; __resetAuthRefreshForTests(); }); afterEach(() => { globalThis.fetch = originalFetch; + // Plain reassignment can throw: the real `navigator` global is an accessor + // with no setter, and a test may have left it in that pristine state. + // Deleting first (a no-op if a test already replaced it) sidesteps that. + delete globalThis.navigator; + if (originalNavigator !== undefined) globalThis.navigator = originalNavigator; vi.restoreAllMocks(); }); @@ -377,3 +384,151 @@ describe('refresh on 401', () => { expect(res.status).toBe(401); }); }); + +// The real `navigator` global (Node 21+, and every browser) exposes +// `navigator` as an accessor with no setter, so a plain `globalThis.navigator +// = ...` throws in this ESM test file's strict mode. Delete it first, same +// as this codebase's own withRefreshLock() feature-detects its absence. +function stubNavigator(value) { + delete globalThis.navigator; + if (value !== undefined) globalThis.navigator = value; +} + +describe('cross-tab refresh coordination', () => { + it('still sends exactly one refresh for concurrent 401s when navigator.locks exists', async () => { + configureAuthRefresh({ loginFlow: 'supertokens' }); + // A trivial single-tab-shaped stub: Web Locks exists, but only one caller + // is ever asking, so it should behave exactly like having no lock at all. + stubNavigator({ locks: { request: (name, fn) => fn() } }); + + let refreshes = 0; + let refreshResolve; + const refreshGate = new Promise((resolve) => { refreshResolve = resolve; }); + let refreshed = false; + + globalThis.fetch = vi.fn(async (url) => { + if (url === '/auth/session/refresh') { + refreshes += 1; + await refreshGate; + refreshed = true; + return { ok: true, status: 200, text: async () => '' }; + } + if (!refreshed) return jsonResponse({ error: 'unauthorized' }, { ok: false, status: 401 }); + return jsonResponse({ ok: true }); + }); + + const all = Promise.all([fetchState(), fetchState(), fetchState()]); + await new Promise((r) => setTimeout(r, 10)); + refreshResolve(); + await all; + + // Proves the lock wrapper doesn't defeat the existing in-tab single-flight + // guard now that a navigator.locks happens to exist. + expect(refreshes).toBe(1); + }); + + it('serialises refreshes across tabs so neither ever overlaps the other', async () => { + // Two tabs = two independent module instances, each with its own private + // inFlightRefresh closure - exactly what two separate browsing contexts + // running the same bundle would have. vi.resetModules() + a fresh + // dynamic import gets us that inside one test file; it does not disturb + // the module instance this file's own top-level `fetchState` etc. are + // bound to (those bindings were already linked when the file loaded). + vi.resetModules(); + const tab1 = await import('../client/src/game/api.js'); + vi.resetModules(); + const tab2 = await import('../client/src/game/api.js'); + + tab1.configureAuthRefresh({ loginFlow: 'supertokens' }); + tab2.configureAuthRefresh({ loginFlow: 'supertokens' }); + + // Both "tabs" share one browser-wide Web Locks manager in reality; model + // that with one real FIFO mutex shared by both module instances (they + // already share globalThis, exactly as two tabs share it). + let queue = Promise.resolve(); + stubNavigator({ + locks: { + request: (name, fn) => { + const run = queue.then(fn, fn); + queue = run.catch(() => {}); + return run; + }, + }, + }); + + let active = 0; + let overlapped = false; + let refreshCalls = 0; + globalThis.fetch = vi.fn(async (url) => { + if (url === '/auth/session/refresh') { + refreshCalls += 1; + if (active > 0) overlapped = true; + active += 1; + // Wide enough to make an unguarded race show up reliably. + await new Promise((r) => setTimeout(r, 10)); + active -= 1; + return { ok: true, status: 200, text: async () => '' }; + } + // Both tabs' access tokens are expired - each retries its own request + // exactly once, same as every other test in this file. + return jsonResponse({ error: 'unauthorized' }, { ok: false, status: 401 }); + }); + + await Promise.all([tab1.fetchState(), tab2.fetchState()]); + + // The invariant that actually prevents SuperTokens' theft detection: the + // two tabs' refresh network calls never overlap in time. + expect(overlapped).toBe(false); + // And the fix doesn't falsely dedupe tab2's refresh away - a lock delays, + // it does not merge, so each tab still gets exactly one refresh call. + expect(refreshCalls).toBe(2); + }); + + it('falls back to per-tab-only refresh when navigator is entirely unavailable', async () => { + configureAuthRefresh({ loginFlow: 'supertokens' }); + stubNavigator(undefined); + + let refreshes = 0; + let refreshed = false; + globalThis.fetch = vi.fn(async (url) => { + if (url === '/auth/session/refresh') { + refreshes += 1; + refreshed = true; + return { ok: true, status: 200, text: async () => '' }; + } + if (!refreshed) return jsonResponse({ error: 'unauthorized' }, { ok: false, status: 401 }); + return jsonResponse({ ok: true }); + }); + + const res = await fetchState(); + + expect(refreshes).toBe(1); + expect(res).toEqual({ ok: true }); + }); + + it('falls back when navigator exists but has no locks (this test runtime today)', async () => { + configureAuthRefresh({ loginFlow: 'supertokens' }); + // Exactly the shape Node's own built-in `navigator` global has - no + // `.locks` - which is why every OTHER test in this file already exercises + // this path implicitly. This test pins it explicitly so it can't regress + // silently if a future Node/vitest version adds navigator.locks. + stubNavigator({ userAgent: 'test' }); + + let refreshes = 0; + let refreshed = false; + globalThis.fetch = vi.fn(async (url) => { + if (url === '/auth/session/refresh') { + refreshes += 1; + refreshed = true; + return { ok: true, status: 200, text: async () => '' }; + } + if (!refreshed) return jsonResponse({ error: 'unauthorized' }, { ok: false, status: 401 }); + return jsonResponse({ ok: true }); + }); + + const res = await fetchState(); + + expect(refreshes).toBe(1); + expect(res).toEqual({ ok: true }); + }); +});