From caab7eb1f77424fdc3ed57b13401ba8947298ce4 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Tue, 8 Sep 2026 13:37:51 -0400 Subject: [PATCH] fix(auth): decide a single winner when two refreshes race one token Rotation read the session, checked it had not 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 revokeSessionChain walked past it. That is the case reuse detection exists for. Someone who copied a refresh token and raced the legitimate client kept a session the revocation triggered by that theft could not reach, while the trail recorded refresh_token_suspicious and reported it had fired. claimSessionRotation writes the link conditional on it still being unset, in one statement the database serialises. No affected rows is the same condition an already rotated token presents, and is answered the same way: the replacement this request made is revoked, the session is reloaded so the chain walk follows the winner's link, the chain is revoked from there, and the caller gets 401 refresh_token_reused. Two legitimate refreshes racing each other now end the session chain, the same as presenting a rotated token twice in sequence. Closes #279 --- .changeset/tall-moons-rotate.md | 23 +++++++++ docs/security-posture.md | 34 +++++++++++++ resources/coverage-badge.svg | 14 +++--- src/controllers/authentication.ts | 39 ++++++++++++++- src/services/sessionService.ts | 32 ++++++++++++ .../authentication/authentication.spec.ts | 49 +++++++++++++++++++ tests/setup/mocks.ts | 3 ++ tests/unit/services/sessionService.spec.ts | 32 ++++++++++++ 8 files changed, 217 insertions(+), 9 deletions(-) create mode 100644 .changeset/tall-moons-rotate.md diff --git a/.changeset/tall-moons-rotate.md b/.changeset/tall-moons-rotate.md new file mode 100644 index 0000000..b11c630 --- /dev/null +++ b/.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. diff --git a/docs/security-posture.md b/docs/security-posture.md index 37da833..740a84c 100644 --- a/docs/security-posture.md +++ b/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.** diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index bfd39a2..8bd6c88 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 98.9% + + coverage: 99% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 98.9% - 98.9% + 99% + 99% diff --git a/src/controllers/authentication.ts b/src/controllers/authentication.ts index d02d45a..011bc66 100644 --- a/src/controllers/authentication.ts +++ b/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); diff --git a/src/services/sessionService.ts b/src/services/sessionService.ts index 677b6c2..2200315 100644 --- a/src/services/sessionService.ts +++ b/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 { + 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 { const payload = await verifyJwtWithKid(token, 'access'); if (!payload) return null; diff --git a/tests/integration/authentication/authentication.spec.ts b/tests/integration/authentication/authentication.spec.ts index ffa85ec..851253f 100644 --- a/tests/integration/authentication/authentication.spec.ts +++ b/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); diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index 4d8f64c..16282c1 100644 --- a/tests/setup/mocks.ts +++ b/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(), diff --git a/tests/unit/services/sessionService.spec.ts b/tests/unit/services/sessionService.spec.ts index 42b94e4..a3f5461 100644 --- a/tests/unit/services/sessionService.spec.ts +++ b/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();