Merge pull request #20 from CodeNeedsCoffee/fix-cross-tab-session-refresh-race

 fix cross-tab session refresh race condition

Author: NeverEndingCode

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

client/package-lock.json

@@ -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;
     }
   })();

client/src/game/api.js

@@ -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

docs/authentication-methods.md

@@ -1,6 +1,6 @@
 {
   "name": "rackstack-server",
-  "version": "1.11.1",
+  "version": "1.11.2",
   "private": true,
   "type": "module",
   "scripts": {

package-lock.json

@@ -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 });
+  });
+});