fix(auth): decide a single winner when two refreshes race one token

.changeset/tall-moons-rotate.md

@@ -0,0 +1,23 @@
+---
+'seamless-auth-api': patch
+---
+
+Refresh token reuse detection can no longer be defeated by racing it.
+
+Rotation read the session, checked it had not already been rotated, created the replacement
+and linked the two, in four statements with no transaction, row lock or conditional write.
+Two refreshes carrying the same token both passed the check and both wrote the link, and
+the second write won. Both callers ended up with working refresh tokens, and one
+replacement was live while reachable from nothing, so the chain revocation that reuse
+detection triggers walked straight past it. Someone who copied a refresh token and raced
+the legitimate client kept a session that the revocation triggered by that theft could not
+reach, until its own absolute expiry.
+
+The link is now claimed conditional on it still being unset, in one statement the database
+serialises. The rotation that loses revokes the replacement it made, reloads the session so
+the chain walk follows the link the winner wrote, revokes the chain from there and answers
+`401 refresh_token_reused`, which is what an already rotated token has always answered.
+
+Two legitimate refreshes racing each other now end the session chain, the same as
+presenting a rotated token twice in sequence. A client that fires concurrent refreshes of a
+single token will sign its user out.

docs/security-posture.md

@@ -369,6 +369,40 @@ The refusal is recorded as one `request_suspicious` auth event with the real cli
 agent, and the rejected origin in an `origin` metadata field. It used to be recorded with the origin
 string in the `ipAddress` field, which made the trail hard to read.
 
+## Refresh rotation
+
+**Posture: one rotation of a refresh token completes, decided by the database.**
+
+Refresh tokens are single use. Rotation reads the session, checks it has not already been
+rotated, creates the replacement and links the two, which are four statements. Nothing held
+that answer still between them, so two refreshes carrying the same token both passed the
+check and both wrote the link. The second write won, both callers ended up with working
+refresh tokens, and the replacement belonging to the first was live while reachable from
+nothing.
+
+That is the case reuse detection exists for, and it defeated it.
+[`revokeSessionChain`](../src/services/sessionService.ts) starts at the presented session
+and follows `replacedBySessionId` forward, so it reached the winner and stopped. Someone
+who copied a refresh token and raced the legitimate client kept a session the revocation
+triggered by that very theft could not reach, while the trail recorded
+`refresh_token_suspicious` and reported that it had fired.
+
+`claimSessionRotation` writes the link conditional on it still being unset, in one
+statement the database serialises. No affected rows means another rotation got there
+first, which is the same thing an already rotated token means, and it is answered the same
+way: the replacement this request made is revoked with `rotation_race_lost`, the session is
+reloaded so the chain walk follows the link the winner wrote, the chain is revoked from
+there, and the caller gets `401 refresh_token_reused`.
+
+Two legitimate refreshes racing each other therefore end the session chain, the same as
+presenting a rotated token twice in sequence does. A client that fires concurrent refreshes
+of one token signs its user out, which is the direction this has to fail: the alternative
+is that whoever wins the race keeps a session, and the winner is not always the client that
+should have it.
+
+The absolute lifetime that a rotation resets is separate, and is
+[issue #185](https://github.com/fells-code/seamless-auth-api/issues/185).
+
 ## Concurrent sessions per user
 
 **Posture: uncapped by default, and a cap evicts rather than refuses.**

src/controllers/authentication.ts

@@ -29,6 +29,7 @@ import {
   resolveAvailableLoginMethods,
 } from '../services/loginPolicyService.js';
 import {
+  claimSessionRotation,
   findRefreshSessionByToken,
   hardRevokeSession,
   revokeSessionChain,
@@ -528,8 +529,42 @@ export const refreshSession = async (req: Request, res: Response) => {
     idleExpiresAt,
   });
 
-  session.replacedBySessionId = newSession.id;
-  await session.save();
+  // The read, the reuse check and this link are separate statements, so two refreshes
+  // carrying the same token both reach here. Claiming the link conditionally is what
+  // decides which of them rotated: unconditionally, both completed, the second write won,
+  // and the other replacement stayed live while reachable from nothing, so the chain
+  // revocation that reuse detection triggers walked past it and left it working.
+  const rotated = await claimSessionRotation(session, newSession.id);
+
+  if (!rotated) {
+    logger.warn(
+      `Concurrent refresh detected for session ${session.id}. Another rotation claimed it first.`,
+    );
+
+    // The replacement this request made is reachable from nothing, so it is revoked
+    // rather than left behind. Its refresh token was never returned to anyone.
+    await hardRevokeSession(newSession, 'rotation_race_lost');
+
+    // Reloaded so the chain walk follows the link the winner wrote rather than the null
+    // this instance still holds, which would stop at the old session and leave the
+    // winner's session live.
+    await session.reload();
+    await revokeSessionChain(session);
+
+    await AuthEventService.log({
+      userId: user.id,
+      type: 'refresh_token_suspicious',
+      req,
+      metadata: {
+        reason: 'Refresh token rotated twice concurrently',
+        sessionId: session.id,
+        replacedBySessionId: session.replacedBySessionId,
+        abandonedSessionId: newSession.id,
+      },
+    });
+
+    return res.status(401).json({ error: 'refresh_token_reused' });
+  }
 
   const token = await signAccessToken(newSession.id, user.id, user.roles, session.organizationId);
 

src/services/sessionService.ts

@@ -99,6 +99,38 @@ export async function hardRevokeSession(session: Session, reason = 'manual_revok
   await session.save();
 }
 
+/**
+ * Points a session at its replacement, but only while nothing else has claimed it.
+ *
+ * Rotation reads the session, checks it has not been rotated, creates the replacement and
+ * writes this link, in four statements with nothing holding the answer still between
+ * them. Two refreshes carrying the same token therefore both pass the check and both
+ * write the link, and the second write wins: two working refresh tokens exist, and one
+ * replacement is reachable from nothing, so the chain revocation that reuse detection
+ * triggers walks straight past it. That is the case reuse detection exists for.
+ *
+ * The condition in the `where` is what decides a single winner, in one statement the
+ * database serialises. No affected rows means another rotation got there first, which is
+ * the same thing the reuse check answers `401` to.
+ */
+export async function claimSessionRotation(
+  session: Session,
+  replacementSessionId: string,
+): Promise<boolean> {
+  const [claimed] = await Session.update(
+    { replacedBySessionId: replacementSessionId },
+    { where: { id: session.id, replacedBySessionId: null, revokedAt: null } },
+  );
+
+  if (claimed === 0) {
+    return false;
+  }
+
+  session.replacedBySessionId = replacementSessionId;
+
+  return true;
+}
+
 export async function validateAccessToken(token: string): Promise<ValidatedAccessToken | null> {
   const payload = await verifyJwtWithKid(token, 'access');
   if (!payload) return null;

tests/integration/authentication/authentication.spec.ts

@@ -14,6 +14,7 @@ import {
   signEphemeralToken,
 } from '../../../src/lib/token';
 import {
+  claimSessionRotation,
   findRefreshSessionByToken,
   hardRevokeSession,
   revokeSessionChain,
@@ -530,6 +531,54 @@ describe('POST /refresh', () => {
     expect(res.body.refreshToken).toBe('refresh');
   });
 
+  // Two refreshes carrying the same token both pass the reuse check, because the check and
+  // the link are separate statements. Only one rotation may complete, and the one that
+  // does not is the same event the reuse check answers 401 to.
+  it('refuses the rotation that lost the race and revokes what it made', async () => {
+    const session = {
+      id: 'session-1',
+      replacedBySessionId: null,
+      revokedAt: null,
+      userId: 'user-1',
+      infraId: 'app',
+      mode: 'server',
+      userAgent: 'agent',
+      save: vi.fn(),
+      reload: vi.fn(async function (this: { replacedBySessionId: string | null }) {
+        this.replacedBySessionId = 'winning-session';
+      }),
+    };
+
+    (findRefreshSessionByToken as any).mockResolvedValue(session);
+    (User.findOne as any).mockResolvedValue(buildUser());
+    (Session.create as any).mockResolvedValue({ id: 'losing-session' });
+    (claimSessionRotation as any).mockResolvedValue(false);
+    (signAccessToken as any).mockResolvedValue('access');
+    (generateRefreshToken as any).mockReturnValue('refresh');
+    (createRefreshTokenLookup as any).mockReturnValue('refresh-lookup');
+    (getSystemConfig as any).mockResolvedValue({
+      access_token_ttl: '15m',
+      refresh_token_ttl: '1h',
+      session_idle_ttl: '8h',
+    });
+
+    const res = await request(app).post('/refresh').set('Authorization', 'Bearer refresh-token');
+
+    expect(res.status).toBe(401);
+    expect(res.body.error).toBe('refresh_token_reused');
+    expect(res.body.refreshToken).toBeUndefined();
+
+    // The replacement this request created is reachable from nothing, so it does not get
+    // to outlive the request that made it.
+    expect(hardRevokeSession).toHaveBeenCalledWith({ id: 'losing-session' }, 'rotation_race_lost');
+
+    // Reloaded first, or the chain walk stops at the old session and leaves the winner's
+    // session live, which is the whole defect.
+    expect(session.reload).toHaveBeenCalled();
+    expect(revokeSessionChain).toHaveBeenCalledWith(session);
+    expect(session.replacedBySessionId).toBe('winning-session');
+  });
+
   it('rejects a jwt-shaped bearer token with no session', async () => {
     (findRefreshSessionByToken as any).mockResolvedValue(null);
 

tests/setup/mocks.ts

@@ -136,6 +136,9 @@ vi.mock('../../src/config/getSystemConfig.js', () => ({
 }));
 
 vi.mock('../../src/services/sessionService.js', () => ({
+  // Defaults to winning the rotation race, which is what a lone refresh does. A spec
+  // exercising two concurrent refreshes sets its own value.
+  claimSessionRotation: vi.fn(async () => true),
   validateAccessToken: vi.fn(),
   validateBearerToken: vi.fn(),
   validateSessionRecord: vi.fn(),

tests/unit/services/sessionService.spec.ts

@@ -7,6 +7,7 @@ vi.mock('../../../src/models/sessions', () => ({
     findByPk: vi.fn(),
     findOne: vi.fn(),
     findAll: vi.fn(),
+    update: vi.fn(),
   },
 }));
 
@@ -316,6 +317,37 @@ describe('sessionService', () => {
     expect(await validateAccessToken('token')).toBeNull();
   });
 
+  // Rotation reads, checks and links in separate statements, so two refreshes carrying the
+  // same token both reach the link. The condition in the where clause is what decides
+  // which of them rotated, in one statement the database serialises.
+  it('claims the rotation link only while it is still unset', async () => {
+    const { Session } = await import('../../../src/models/sessions');
+    const session = buildSession({ id: 'session-1', replacedBySessionId: null });
+
+    (Session.update as any).mockResolvedValue([1]);
+
+    const { claimSessionRotation } = await import('../../../src/services/sessionService');
+
+    expect(await claimSessionRotation(session as any, 'session-2')).toBe(true);
+    expect(Session.update).toHaveBeenCalledWith(
+      { replacedBySessionId: 'session-2' },
+      { where: { id: 'session-1', replacedBySessionId: null, revokedAt: null } },
+    );
+    expect(session.replacedBySessionId).toBe('session-2');
+  });
+
+  it('reports the rotation lost when another one claimed the link first', async () => {
+    const { Session } = await import('../../../src/models/sessions');
+    const session = buildSession({ id: 'session-1', replacedBySessionId: null });
+
+    (Session.update as any).mockResolvedValue([0]);
+
+    const { claimSessionRotation } = await import('../../../src/services/sessionService');
+
+    expect(await claimSessionRotation(session as any, 'session-2')).toBe(false);
+    expect(session.replacedBySessionId).toBeNull();
+  });
+
   it('revokes session immediately', async () => {
     const session = buildSession();