fix(SEC-7): strict refresh-token rotation with grace window#268
Merged
Conversation
Each refresh now revokes the presented token and issues a successor (atomic transaction, preserving the session horizon). A just-rotated token replayed within a short, configurable grace window still yields an access token — covering the benign multi-tab / lost-response race — but only while the account has a live session, so logout-all and password change are not bypassed. Rotation deliberately does NOT auto-revoke sibling sessions on replay of a superseded token: that produces account-wide logouts on common benign cases (backgrounded tab, flaky mobile network) and, without token-family chaining, cannot be made false-positive-free. Superseded replays beyond the grace window are logged for out-of-band alerting and return 401. - migration 000027: rotated_at column - RotateRefreshToken (atomic revoke + issue), HasLiveRefreshToken guard - REFRESH_ROTATION_GRACE_SECONDS config knob (default 30s) - shared insertRefreshToken / respondWithAccessToken helpers - remove dead UpdateLastUsed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the /auth/refresh flow by enforcing strict refresh-token rotation (with a short replay grace window) and adding server-side observability for suspicious refresh-token reuse.
Changes:
- Add
rotated_attracking and implement atomic refresh-token rotation with a configurable grace window. - Extend refresh responses (and API docs/tests) to return the successor
refresh_token+refresh_expires_inon rotation. - Add audit logging for refresh-token reuse beyond the grace window; update scenario coverage for strict rotation.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api-scenarios/001-user-registration-auth.yaml | Updates scenario to persist and reuse the rotated successor refresh token. |
| pkg/security/types.go | Adds RotatedAt to the refresh-token model; expands RefreshResponse to optionally include rotated successor token fields. |
| pkg/security/refresh_tokens.go | Introduces rotation helpers (newTokenString, insertRefreshToken), adds HasLiveRefreshToken, and implements RotateRefreshToken. |
| pkg/security/refresh_tokens_test.go | Replaces last-used test with rotation tests (success + already-rotated behavior). |
| pkg/security/handlers.go | Enforces strict rotation in RefreshTokenHandler and adds grace-window handling for revoked tokens. |
| pkg/security/handlers_test.go | Adds handler-level tests for strict rotation, grace replay, beyond-grace 401, and grace denial after logout-all. |
| pkg/security/audit_log.go | Adds a new audit event type and helper for reuse detection logging. |
| pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.up.sql | Adds rotated_at column to refresh_token. |
| pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.down.sql | Drops rotated_at column. |
| pkg/config/env.go | Adds REFRESH_ROTATION_GRACE_SECONDS config plumbing with a default value. |
| api-doc/swagger.yaml | Documents new optional refresh fields in refresh response schema. |
| api-doc/swagger.json | Regenerates OpenAPI JSON to include new refresh response fields. |
| api-doc/docs.go | Regenerates embedded Swagger docs to include new refresh response fields. |
| .env.sample | Adds REFRESH_ROTATION_GRACE_SECONDS sample configuration. |
Files not reviewed (1)
- api-doc/docs.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- generate the access token before rotating, so a signing failure does not burn the presented refresh token - on a lost rotation race (ErrTokenAlreadyRotated), re-read the token and re-apply full validation instead of blindly granting access — a concurrent logout/password/admin revocation now correctly returns 401 - guard the rotation UPDATE with expires_at > now so a token that expired between the handler check and the UPDATE cannot be rotated Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
Completes the refresh-token hardening (batch B, lot B-3). Now that clients store the rotated token (front PR #117, in prod), the backend enforces strict rotation:
/auth/refreshrevokes the presented token and issues a successor in one transaction, preserving the original session horizon. The response carries the newrefresh_token+refresh_expires_in.REFRESH_ROTATION_GRACE_SECONDS, default 30s) still yields an access token — covering the benign multi-tab / lost-response race — but only while the account still has a live session, so logout-all and password change are never bypassed.refresh_token_reuse_detected) for out-of-band alerting, but does not auto-revoke the account's other sessions.Why no auto-revocation on reuse?
The RFC-6819 "reuse ⇒ revoke the whole family" response was implemented and then deliberately removed after review: without token-family chaining it produces account-wide logouts on common benign cases (a backgrounded tab waking after the window, a mobile client that lost the rotation response), and the timing-based detection cannot be made false-positive-free. Rotation already bounds a stolen token tightly; containment-on-reuse can be revisited as a later lot with a
family_idcolumn if desired.Changes
000027:rotated_atcolumnRotateRefreshToken(atomic revoke + issue),HasLiveRefreshTokengrace guardREFRESH_ROTATION_GRACE_SECONDSconfig knob (+.env.sample)insertRefreshToken/respondWithAccessTokenhelpers; removed deadUpdateLastUsed🤖 Generated with Claude Code