feat(oauth): grant_type=refresh_token exchange at /oauth/token#153
Merged
feat(oauth): grant_type=refresh_token exchange at /oauth/token#153
Conversation
Introduces a `family_id` column on `refresh_tokens` plus a covering index. Every rotation preserves the column so that replay of a revoked token can be attributed to — and revoke — an entire family atomically (OAuth 2.1 §4.3.1 / RFC 9700 §2.2.2). Repository gains two siblings to the existing revoke API: `findByTokenHashIncludingRevoked` (so replay is detectable at all) and `revokeFamily` (revokes every non-revoked row sharing a family_id in one statement, preserving the original revokedReason for audit).
Dispatches a third grant type at the OAuth token endpoint. New `handleRefreshToken` sibling to authorization_code/client_credentials: - Looks up the presented token regardless of revoked/expired state so replay is detectable. - Rejects cross-client presentation (invalid_grant) before any family revocation fires, so replay on one client can never fan out revocation on another. - Replay detection: a revoked-but-known token revokes its entire `family_id` (OAuth 2.1 §4.3.1, RFC 9700 §2.2.2) and the request fails invalid_grant. - Rotation: the presented token is revoked as 'rotated' BEFORE the new token is persisted — no two tokens in the same chain link can be live at once. - Down-scoping honoured (subset of original scopes); any scope outside the original set is rejected as invalid_scope. - Public clients (`token_endpoint_auth_method: 'none'`) present only client_id and are bound by refresh-token ownership; a new `authenticateClientForRefresh` helper handles the split. Audit logs carry `familyId` on success, and a security-event row is written when a family is revoked on replay. Authorization-code initial issuance and /auth/login now seed a new family_id per token so rotations can inherit it cleanly.
… surface `/auth/refresh` predated the OAuth-compliant token endpoint; it issued refresh tokens that /oauth/token could not accept, leaving a bifurcated refresh story. With `grant_type=refresh_token` landing at /oauth/token, the legacy endpoint is redundant — and keeping two surfaces with different rotation/ownership rules is a footgun. Audit confirmed no external callers (only the route's own tests + MVP docs mention the path). The now-unused `refreshSchema`/`refreshResponseSchema` Zod shapes are also removed. `REFRESH_RATE_LIMIT` env var stays in place for potential future use.
|
View your CI Pipeline Execution ↗ for commit b0164cd
☁️ Nx Cloud last updated this comment at |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the OAuth 2.1 refresh_token grant type, introducing refresh token rotation and family-based replay detection. It adds a familyId to the refresh token schema and database, updates the token exchange endpoint to handle refresh grants for both public and confidential clients, and provides comprehensive test coverage for these security flows. Feedback includes addressing a high-severity race condition in the token rotation logic to ensure atomicity and simplifying redundant logic in the client authentication helper.
Comment on lines
+489
to
+702
| async function handleRefreshToken( | ||
| ctx: HandlerContext<TokenExchangeRefreshBody> | ||
| ): Promise<TokenExchangeResponse> { | ||
| const { fastify, request, client, body } = ctx; | ||
|
|
||
| if (!client.grantTypes.includes('refresh_token')) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: null, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { error: 'unauthorized_client', grantType: 'refresh_token' }, | ||
| }); | ||
| throw new UnauthorizedClientError(); | ||
| } | ||
|
|
||
| const presentedHash = fastify.jwtUtils.hashRefreshToken(body.refresh_token); | ||
|
|
||
| // Fetch the row without any active-state filter so replay of a | ||
| // revoked token is detectable; downstream checks enforce liveness. | ||
| const storedToken = | ||
| await fastify.repositories.refreshTokens.findByTokenHashIncludingRevoked(presentedHash); | ||
|
|
||
| if (!storedToken) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: null, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { error: 'invalid_grant: refresh token not recognised' }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
|
|
||
| // Ownership check — before any replay logic, before any revocation. | ||
| // Cross-client replay must never be able to fan out family revocation | ||
| // on another client's tokens. | ||
| if (storedToken.oauthClientId !== client.id) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: storedToken.userId, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { | ||
| error: 'invalid_grant: refresh token bound to different client', | ||
| presentingClientId: client.id, | ||
| ownerClientId: storedToken.oauthClientId, | ||
| }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
|
|
||
| // Replay detection — a revoked token in the correct family MUST trigger | ||
| // family-wide revocation per RFC 9700 §2.2.2 / OAuth 2.1 §4.3.1. | ||
| if (storedToken.revoked) { | ||
| const familyRevokedCount = await fastify.repositories.refreshTokens.revokeFamily( | ||
| storedToken.familyId, | ||
| 'replay_detected' | ||
| ); | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: storedToken.userId, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'security', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { | ||
| error: 'invalid_grant: refresh token replay detected', | ||
| familyId: storedToken.familyId, | ||
| familyTokensRevoked: familyRevokedCount, | ||
| }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
|
|
||
| if (storedToken.expiresAt <= Date.now()) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: storedToken.userId, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { error: 'invalid_grant: refresh token expired' }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
|
|
||
| // Load the subject user; reject if missing or disabled (RFC 9700 §4.14). | ||
| const user = await fastify.repositories.users.findById(storedToken.userId); | ||
| if (!user) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: storedToken.userId, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { error: 'invalid_grant: user not found' }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
| if (!user.enabled) { | ||
| await fastify.repositories.refreshTokens.revoke(storedToken.id, 'user_disabled'); | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: user.id, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { error: 'invalid_grant: user_disabled' }, | ||
| }); | ||
| throw new InvalidGrantError('Invalid or expired refresh token'); | ||
| } | ||
|
|
||
| // Down-scoping: request narrower scope or keep original set. | ||
| // Upscoping (any scope not in the original set) → invalid_scope. | ||
| let grantedScopes: string[]; | ||
| if (body.scope !== undefined) { | ||
| const requested = body.scope.split(/\s+/).filter((s) => s.length > 0); | ||
| const extraneous = requested.filter((s) => !storedToken.scopes.includes(s)); | ||
| if (extraneous.length > 0) { | ||
| await fastify.repositories.auditLogs.create({ | ||
| userId: user.id, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.failure', | ||
| eventType: 'token', | ||
| success: false, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { | ||
| error: 'invalid_scope: upscoping rejected', | ||
| extraneous, | ||
| originalScopes: storedToken.scopes, | ||
| }, | ||
| }); | ||
| throw new InvalidScopeError(`${extraneous.join(' ')} not in original refresh token scope`); | ||
| } | ||
| grantedScopes = requested; | ||
| } else { | ||
| grantedScopes = storedToken.scopes; | ||
| } | ||
|
|
||
| // Rotate: revoke the presented token FIRST so a concurrent replay | ||
| // cannot also succeed. We then issue a new token carrying the same | ||
| // `familyId`. (The unique index on `token_hash` protects against the | ||
| // degenerate case of a collision.) | ||
| await fastify.repositories.refreshTokens.revoke(storedToken.id, 'rotated'); | ||
|
|
||
| const { token: newRefreshToken, tokenHash: newRefreshTokenHash } = | ||
| fastify.jwtUtils.generateRefreshToken(); | ||
| const refreshTokenExpiresAt = Date.now() + fastify.jwtUtils.getRefreshTokenLifespan() * 1000; | ||
|
|
||
| await fastify.repositories.refreshTokens.create({ | ||
| userId: user.id, | ||
| oauthClientId: client.id, | ||
| tokenHash: newRefreshTokenHash, | ||
| familyId: storedToken.familyId, | ||
| previousTokenHash: presentedHash, | ||
| expiresAt: refreshTokenExpiresAt, | ||
| scopes: grantedScopes, | ||
| }); | ||
|
|
||
| const scopeString = grantedScopes.length > 0 ? grantedScopes.join(' ') : undefined; | ||
|
|
||
| const accessToken = await fastify.jwtUtils.signAccessToken({ | ||
| sub: user.id, | ||
| email: user.email, | ||
| email_verified: user.emailVerified, | ||
| clientId: client.clientId, | ||
| scope: scopeString, | ||
| aud: resolveAudience(client), | ||
| }); | ||
|
|
||
| const accessTokenExpiresIn = fastify.jwtUtils.getAccessTokenLifespan(); | ||
|
|
||
| await fastify.repositories.auditLogs.create({ | ||
| userId: user.id, | ||
| oauthClientId: client.id, | ||
| event: 'oauth.token.exchange.success', | ||
| eventType: 'token', | ||
| success: true, | ||
| ipAddress: request.ip, | ||
| userAgent: request.headers['user-agent'] || null, | ||
| metadata: { | ||
| grantType: 'refresh_token', | ||
| familyId: storedToken.familyId, | ||
| rotatedFromTokenId: storedToken.id, | ||
| scope: scopeString, | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| access_token: accessToken, | ||
| refresh_token: newRefreshToken, | ||
| expires_in: accessTokenExpiresIn, | ||
| token_type: 'Bearer' as const, | ||
| ...(scopeString ? { scope: scopeString } : {}), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The handleRefreshToken function is vulnerable to a race condition during token rotation. Multiple concurrent requests using the same refresh token can bypass the revoked check at line 552 because the check is based on a stale read from line 513. This could result in multiple valid refresh tokens being issued for the same family link, violating the security invariants of refresh token rotation.
To prevent this, you should wrap the entire flow (from the initial token lookup to the creation of the new token) in a database transaction. Additionally, the revocation of the current token (line 650) must be atomic and conditional, ensuring it only succeeds if the token is not already revoked. Following the organization's general rules, either use an atomic update with built-in checks in the WHERE clause or wrap the operations in a transaction.
References
- To prevent race conditions in read-then-update scenarios (e.g., checking and then using a token), either use an atomic update with built-in checks in the WHERE clause, or wrap the separate read and update operations in a database transaction.
Comment on lines
+158
to
+162
| const hasBasic = /^Basic\s/i.test(request.headers.authorization ?? ''); | ||
| if (hasBasic || bodyClientSecret) { | ||
| const creds = extractClientCredentials(request, bodyClientId, bodyClientSecret); | ||
| return authenticateClient(fastify, realmId, creds); | ||
| } |
There was a problem hiding this comment.
The hasBasic check and subsequent call to extractClientCredentials are somewhat redundant. extractClientCredentials already internally calls parseBasicAuthHeader and handles the logic for determining the authentication method. You could simplify this by directly calling extractClientCredentials and catching the error if no credentials (secret) are found, which would indicate a potential public client path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #151.
Summary
grant_type=refresh_tokendispatch at/oauth/tokenper RFC 6749 §6 with required rotation (revoke-old + mint-new) and token-family replay detection.refresh_tokens.family_id uuid NOT NULLvia migration0002_add_refresh_tokens_family_id.sql(legacy rows backfilled withuuidv7()— one row = singleton family, correct for pre-rotation tokens). Repository gainsfindByTokenHashIncludingRevokedand atomicrevokeFamily./auth/refresh— grep confirmed no external callers, keeping two surfaces with diverging rotation semantics was the footgun the issue calls out.invalid_scope; cross-client refresh rejected withinvalid_grant; public clients supported (bound by refresh-token ownership, noclient_secret).Caveats for reviewers
0002. Pick one landing order; this one is the smallest schema delta and self-contained on the tokens table — suggest landing it at0002and renumbering the others after.handleRefreshTokenis load-bearing: ownership check runs before replay detection so cross-client replay can't fan out revocation onto the owner's family; rotation-revoke runs before the new-token insert.REFRESH_RATE_LIMIT/_WINDOWenv vars are present but not yet wired — follow-up if a grant-specific rate limit is wanted.Test plan
pnpm nx run-many -t test lint typecheck -p auth-server,infra-db— green