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