From ff39e0d4fb85be474350aa0995b42622d07922b8 Mon Sep 17 00:00:00 2001 From: Angak0k Date: Sat, 4 Jul 2026 23:55:39 +0200 Subject: [PATCH 1/2] fix(SEC-7): strict refresh-token rotation with grace window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .env.sample | 1 + api-doc/docs.go | 6 + api-doc/swagger.json | 6 + api-doc/swagger.yaml | 4 + pkg/config/env.go | 6 + .../000027_refresh_token_rotated_at.down.sql | 1 + .../000027_refresh_token_rotated_at.up.sql | 4 + pkg/security/audit_log.go | 14 ++ pkg/security/handlers.go | 92 ++++++++++-- pkg/security/handlers_test.go | 98 ++++++++++++- pkg/security/refresh_tokens.go | 131 ++++++++++++++---- pkg/security/refresh_tokens_test.go | 61 +++++--- pkg/security/types.go | 14 +- .../001-user-registration-auth.yaml | 7 +- 14 files changed, 379 insertions(+), 66 deletions(-) create mode 100644 pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.down.sql create mode 100644 pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.up.sql diff --git a/.env.sample b/.env.sample index 7fbc8ef..54c0772 100644 --- a/.env.sample +++ b/.env.sample @@ -18,6 +18,7 @@ ACCESS_TOKEN_MINUTES=15 # Access token lifetime (default REFRESH_TOKEN_DAYS=1 # Refresh token lifetime (default: 1 day) REFRESH_TOKEN_REMEMBER_ME_DAYS=30 # Refresh token lifetime with "remember me" (default: 30 days) REFRESH_TOKEN_CLEANUP_INTERVAL_HOURS=24 # Cleanup interval for expired tokens (default: 24 hours) +REFRESH_ROTATION_GRACE_SECONDS=30 # Grace window for replaying a just-rotated refresh token (default: 30s) # Rate Limiting Configuration REFRESH_RATE_LIMIT_REQUESTS=10 # Requests per minute per IP (default: 10) diff --git a/api-doc/docs.go b/api-doc/docs.go index a227b08..d804d08 100644 --- a/api-doc/docs.go +++ b/api-doc/docs.go @@ -3878,6 +3878,12 @@ const docTemplate = `{ }, "expires_in": { "type": "integer" + }, + "refresh_expires_in": { + "type": "integer" + }, + "refresh_token": { + "type": "string" } } }, diff --git a/api-doc/swagger.json b/api-doc/swagger.json index e6e8c6f..e10d828 100644 --- a/api-doc/swagger.json +++ b/api-doc/swagger.json @@ -3875,6 +3875,12 @@ }, "expires_in": { "type": "integer" + }, + "refresh_expires_in": { + "type": "integer" + }, + "refresh_token": { + "type": "string" } } }, diff --git a/api-doc/swagger.yaml b/api-doc/swagger.yaml index 2758012..eadfea8 100644 --- a/api-doc/swagger.yaml +++ b/api-doc/swagger.yaml @@ -556,6 +556,10 @@ definitions: type: string expires_in: type: integer + refresh_expires_in: + type: integer + refresh_token: + type: string type: object security.RefreshTokenInput: properties: diff --git a/pkg/config/env.go b/pkg/config/env.go index 3b7b308..a129d60 100644 --- a/pkg/config/env.go +++ b/pkg/config/env.go @@ -42,6 +42,7 @@ var ( RefreshTokenDays int RefreshTokenRememberMeDays int RefreshTokenCleanupIntervalHours int + RefreshRotationGraceSeconds int RefreshRateLimitRequests int RefreshRateLimitWindowMinutes int ResendConfirmRateLimitRequests int @@ -62,6 +63,7 @@ type Config struct { RefreshTokenDays int RefreshTokenRememberMeDays int RefreshTokenCleanupIntervalHours int + RefreshRotationGraceSeconds int RefreshRateLimitRequests int RefreshRateLimitWindowMinutes int ResendConfirmRateLimitRequests int @@ -91,6 +93,7 @@ func EnvInit(envFilePath string) error { RefreshTokenDays = newConfig.RefreshTokenDays RefreshTokenRememberMeDays = newConfig.RefreshTokenRememberMeDays RefreshTokenCleanupIntervalHours = newConfig.RefreshTokenCleanupIntervalHours + RefreshRotationGraceSeconds = newConfig.RefreshRotationGraceSeconds RefreshRateLimitRequests = newConfig.RefreshRateLimitRequests RefreshRateLimitWindowMinutes = newConfig.RefreshRateLimitWindowMinutes ResendConfirmRateLimitRequests = newConfig.ResendConfirmRateLimitRequests @@ -136,6 +139,7 @@ func newConfig() Config { RefreshTokenDays: 1, RefreshTokenRememberMeDays: 30, RefreshTokenCleanupIntervalHours: 24, + RefreshRotationGraceSeconds: 30, RefreshRateLimitRequests: 10, RefreshRateLimitWindowMinutes: 1, ResendConfirmRateLimitRequests: 1, @@ -171,6 +175,8 @@ func setEnvVars(cfg *Config) { ifEnvEmpty(os.Getenv("REFRESH_TOKEN_REMEMBER_ME_DAYS"), strconv.Itoa(cfg.RefreshTokenRememberMeDays))) cfg.RefreshTokenCleanupIntervalHours, _ = strconv.Atoi( ifEnvEmpty(os.Getenv("REFRESH_TOKEN_CLEANUP_INTERVAL_HOURS"), strconv.Itoa(cfg.RefreshTokenCleanupIntervalHours))) + cfg.RefreshRotationGraceSeconds, _ = strconv.Atoi( + ifEnvEmpty(os.Getenv("REFRESH_ROTATION_GRACE_SECONDS"), strconv.Itoa(cfg.RefreshRotationGraceSeconds))) cfg.RefreshRateLimitRequests, _ = strconv.Atoi( ifEnvEmpty(os.Getenv("REFRESH_RATE_LIMIT_REQUESTS"), strconv.Itoa(cfg.RefreshRateLimitRequests))) cfg.RefreshRateLimitWindowMinutes, _ = strconv.Atoi( diff --git a/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.down.sql b/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.down.sql new file mode 100644 index 0000000..3d6c97d --- /dev/null +++ b/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.down.sql @@ -0,0 +1 @@ +ALTER TABLE refresh_token DROP COLUMN rotated_at; diff --git a/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.up.sql b/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.up.sql new file mode 100644 index 0000000..6519117 --- /dev/null +++ b/pkg/database/migration/migration_scripts/000027_refresh_token_rotated_at.up.sql @@ -0,0 +1,4 @@ +-- rotated_at marks tokens revoked by rotation (superseded by a successor), +-- as opposed to tokens revoked by logout / password change / admin action. +-- It drives the reuse-detection grace window on /auth/refresh. +ALTER TABLE refresh_token ADD COLUMN rotated_at TIMESTAMPTZ; diff --git a/pkg/security/audit_log.go b/pkg/security/audit_log.go index 5f2784f..6f79a49 100644 --- a/pkg/security/audit_log.go +++ b/pkg/security/audit_log.go @@ -20,6 +20,7 @@ const ( EventRefreshFailed AuditEventType = "refresh_failed" EventLogout AuditEventType = "logout_success" EventLogoutAll AuditEventType = "logout_all_success" + EventRefreshReuse AuditEventType = "refresh_token_reuse_detected" EventRateLimitExceeded AuditEventType = "rate_limit_exceeded" ) @@ -114,6 +115,19 @@ func AuditLogoutAll(c *gin.Context, userID uint, revokedSessions int64) { }) } +// AuditRefreshReuseDetected logs the replay of a superseded refresh token +// beyond the rotation grace window — a possible token-theft signal surfaced +// for out-of-band alerting (no automatic revocation is performed) +func AuditRefreshReuseDetected(c *gin.Context, userID uint) { + logAuditEvent(AuditEvent{ + EventType: EventRefreshReuse, + UserID: &userID, + IP: c.ClientIP(), + UserAgent: c.Request.UserAgent(), + Message: "Superseded refresh token replayed beyond grace window", + }) +} + // AuditRateLimitExceeded logs a rate limit exceeded event func AuditRateLimitExceeded(c *gin.Context, endpoint string) { logAuditEvent(AuditEvent{ diff --git a/pkg/security/handlers.go b/pkg/security/handlers.go index fceac34..510c99c 100644 --- a/pkg/security/handlers.go +++ b/pkg/security/handlers.go @@ -1,6 +1,7 @@ package security import ( + "errors" "net/http" "time" @@ -42,31 +43,92 @@ func RefreshTokenHandler(c *gin.Context) { } // 3. Validate refresh token - if refreshToken.Revoked { - AuditRefreshFailed(c, "refresh token has been revoked") - c.JSON(http.StatusUnauthorized, gin.H{"error": "Refresh token has been revoked"}) - return - } - if time.Now().After(refreshToken.ExpiresAt) { AuditRefreshFailed(c, "refresh token has expired") c.JSON(http.StatusUnauthorized, gin.H{"error": "Refresh token has expired"}) return } - // 4. Generate new access token - accessToken, err := GenerateToken(refreshToken.AccountID) - if err != nil { - AuditRefreshFailed(c, "failed to generate access token") - c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to generate access token"}) + // 4. A revoked token may still be within the rotation grace window + if refreshToken.Revoked { + handleRevokedRefreshToken(c, refreshToken) return } - // 5. Update last_used_at (ignore errors - non-blocking) - _ = UpdateLastUsed(c.Request.Context(), refreshToken.ID) + // 5. Strict rotation: revoke the presented token and issue its successor. + // Losing a concurrent-rotation race is the same benign situation as the + // grace path: answer with an access token only; the client adopts the + // successor from the winning request. + successor, err := RotateRefreshToken(c.Request.Context(), refreshToken) + switch { + case err == nil: + accessToken, genErr := GenerateToken(refreshToken.AccountID) + if genErr != nil { + AuditRefreshFailed(c, "failed to generate access token") + c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) + return + } + AuditRefreshSuccess(c, refreshToken.AccountID) + c.JSON(http.StatusOK, RefreshResponse{ + AccessToken: accessToken, + ExpiresIn: int64(config.AccessTokenMinutes * 60), + RefreshToken: successor.Token, + RefreshExpiresIn: max(0, int64(time.Until(successor.ExpiresAt).Seconds())), + }) + case errors.Is(err, ErrTokenAlreadyRotated): + respondWithAccessToken(c, refreshToken.AccountID) + default: + AuditRefreshFailed(c, "failed to rotate refresh token") + c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) + } +} + +// handleRevokedRefreshToken answers a refresh attempt with a revoked token. +// Rotation deliberately does NOT auto-revoke sibling sessions on replay: a +// superseded token replayed by a benign client (multi-tab race, a lost +// response on a flaky network, a backgrounded tab) is common, and an +// account-wide logout would be a worse outcome than a re-login. +// - rotated within the grace window AND the account still has a live +// session: benign race → issue an access token (no re-rotation) +// - otherwise: plain 401. A replay past the grace window is logged as a +// reuse signal for out-of-band alerting, but triggers no revocation. +func handleRevokedRefreshToken(c *gin.Context, refreshToken *RefreshToken) { + graceWindow := time.Duration(config.RefreshRotationGraceSeconds) * time.Second + if refreshToken.RotatedAt != nil && time.Since(*refreshToken.RotatedAt) <= graceWindow { + // Guard the grace path against a concurrent logout / password change / + // admin action: if no live session remains, the account was revoked + // and the grace must not resurrect access. + live, err := HasLiveRefreshToken(c.Request.Context(), refreshToken.AccountID) + if err != nil { + helper.LogAndSanitize(err, "refresh grace: live-token check failed") + c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) + return + } + if live { + respondWithAccessToken(c, refreshToken.AccountID) + return + } + } + + if refreshToken.RotatedAt != nil { + // Superseded token replayed beyond grace: log for alerting, no action + AuditRefreshReuseDetected(c, refreshToken.AccountID) + } else { + AuditRefreshFailed(c, "refresh token has been revoked") + } + c.JSON(http.StatusUnauthorized, gin.H{"error": helper.ErrMsgUnauthorized}) +} - // 6. Audit successful refresh and respond - AuditRefreshSuccess(c, refreshToken.AccountID) +// respondWithAccessToken issues a new access token for an already-valid +// session (rotation-race loser or benign grace replay) without rotating +func respondWithAccessToken(c *gin.Context, accountID uint) { + accessToken, err := GenerateToken(accountID) + if err != nil { + AuditRefreshFailed(c, "failed to generate access token") + c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) + return + } + AuditRefreshSuccess(c, accountID) c.JSON(http.StatusOK, RefreshResponse{ AccessToken: accessToken, ExpiresIn: int64(config.AccessTokenMinutes * 60), diff --git a/pkg/security/handlers_test.go b/pkg/security/handlers_test.go index bcb5c14..a50c0fc 100644 --- a/pkg/security/handlers_test.go +++ b/pkg/security/handlers_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/Angak0k/pimpmypack/pkg/config" "github.com/Angak0k/pimpmypack/pkg/database" "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" @@ -59,6 +60,102 @@ func TestRefreshTokenHandler_Success(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, response.AccessToken) assert.Positive(t, response.ExpiresIn) + + // Strict rotation: a successor refresh token is returned + assert.NotEmpty(t, response.RefreshToken) + assert.NotEqual(t, refreshToken.Token, response.RefreshToken) + assert.Positive(t, response.RefreshExpiresIn) + + // The presented token is now revoked-by-rotation + oldRow, err := GetRefreshToken(ctx, refreshToken.Token) + require.NoError(t, err) + assert.True(t, oldRow.Revoked) + require.NotNil(t, oldRow.RotatedAt) + + // The successor is usable + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: response.RefreshToken}, "") + assert.Equal(t, http.StatusOK, w.Code) +} + +func TestRefreshTokenHandler_GraceOnRecentRotation(t *testing.T) { + ctx := context.Background() + accountID := createTestAccount(t) + router := setupTestRouter() + + oldToken, err := CreateRefreshToken(ctx, accountID, false) + require.NoError(t, err) + + // Rotate: oldToken is superseded + w := postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + require.Equal(t, http.StatusOK, w.Code) + var rotated RefreshResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &rotated)) + + // Immediate replay of the superseded token (multi-tab race): a fresh + // access token is issued WITHOUT another rotation and without nuking + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + assert.Equal(t, http.StatusOK, w.Code) + var grace RefreshResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &grace)) + assert.NotEmpty(t, grace.AccessToken) + assert.Empty(t, grace.RefreshToken) + + // The successor is still live + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: rotated.RefreshToken}, "") + assert.Equal(t, http.StatusOK, w.Code) +} + +func TestRefreshTokenHandler_SupersededReplayBeyondGraceIs401(t *testing.T) { + ctx := context.Background() + accountID := createTestAccount(t) + router := setupTestRouter() + + oldToken, err := CreateRefreshToken(ctx, accountID, false) + require.NoError(t, err) + + // The token gets rotated by the legitimate client + w := postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + require.Equal(t, http.StatusOK, w.Code) + var rotated RefreshResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &rotated)) + + graceWindow := time.Duration(config.RefreshRotationGraceSeconds) * time.Second + + // Backdate the rotation beyond the grace window + _, err = database.DB().ExecContext(ctx, + `UPDATE refresh_token SET rotated_at = $1 WHERE token = $2`, + time.Now().Add(-2*graceWindow), hashRefreshToken(oldToken.Token), + ) + require.NoError(t, err) + + // Replaying the superseded token is rejected... + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + assert.Equal(t, http.StatusUnauthorized, w.Code) + + // ...but the legitimate successor session is NOT revoked (no auto-nuke: + // a lost response / backgrounded tab must not log the user out everywhere) + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: rotated.RefreshToken}, "") + assert.Equal(t, http.StatusOK, w.Code) +} + +func TestRefreshTokenHandler_GraceDeniedAfterLogoutAll(t *testing.T) { + ctx := context.Background() + accountID := createTestAccount(t) + router := setupTestRouter() + + oldToken, err := CreateRefreshToken(ctx, accountID, false) + require.NoError(t, err) + + // Rotate, then revoke every session (logout-all / password change) + w := postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + require.Equal(t, http.StatusOK, w.Code) + _, err = RevokeAllUserTokens(ctx, accountID) + require.NoError(t, err) + + // The just-rotated token is within its grace window, but no live session + // remains → the grace must not resurrect access + w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: oldToken.Token}, "") + assert.Equal(t, http.StatusUnauthorized, w.Code) } func TestRefreshTokenHandler_InvalidToken(t *testing.T) { @@ -116,7 +213,6 @@ func TestLogoutHandler_Success(t *testing.T) { // The revoked token must no longer refresh w = postJSON(t, router, "/auth/refresh", RefreshTokenInput{Token: refreshToken.Token}, "") assert.Equal(t, http.StatusUnauthorized, w.Code) - assert.Contains(t, w.Body.String(), "revoked") } func TestLogoutHandler_InvalidTokenStillSucceeds(t *testing.T) { diff --git a/pkg/security/refresh_tokens.go b/pkg/security/refresh_tokens.go index aa3b680..1fae840 100644 --- a/pkg/security/refresh_tokens.go +++ b/pkg/security/refresh_tokens.go @@ -15,6 +15,10 @@ import ( "github.com/Angak0k/pimpmypack/pkg/database" ) +// ErrTokenAlreadyRotated is returned when a rotation loses the race against +// a concurrent rotation of the same token +var ErrTokenAlreadyRotated = errors.New("refresh token already rotated") + // hashRefreshToken returns the hex-encoded SHA-256 of a refresh token — // the only form ever persisted, so a database leak cannot be replayed func hashRefreshToken(token string) string { @@ -22,27 +26,32 @@ func hashRefreshToken(token string) string { return hex.EncodeToString(sum[:]) } -// CreateRefreshToken creates a new refresh token for a user -func CreateRefreshToken(ctx context.Context, accountID uint, rememberMe bool) (*RefreshToken, error) { - var expiresAt time.Time - if rememberMe { - expiresAt = time.Now().Add(time.Hour * 24 * time.Duration(config.RefreshTokenRememberMeDays)) - } else { - expiresAt = time.Now().Add(time.Hour * 24 * time.Duration(config.RefreshTokenDays)) - } - +// newTokenString generates a cryptographically random refresh token +func newTokenString() (string, error) { tokenBytes := make([]byte, 32) if _, err := rand.Read(tokenBytes); err != nil { - return nil, fmt.Errorf("failed to generate random token: %w", err) + return "", fmt.Errorf("failed to generate random token: %w", err) } - tokenString := base64.URLEncoding.EncodeToString(tokenBytes) + return base64.URLEncoding.EncodeToString(tokenBytes), nil +} + +// queryRower is the QueryRowContext surface shared by *sql.DB and *sql.Tx, +// so insertRefreshToken works both standalone and inside a transaction. +type queryRower interface { + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} +// insertRefreshToken inserts a token row (storing only its hash) and returns +// the populated model with the plaintext token set for the caller to hand out +func insertRefreshToken( + ctx context.Context, q queryRower, plaintext string, accountID uint, expiresAt time.Time, +) (*RefreshToken, error) { var token RefreshToken - err := database.DB().QueryRowContext(ctx, + err := q.QueryRowContext(ctx, `INSERT INTO refresh_token (token, account_id, expires_at, created_at) VALUES ($1, $2, $3, $4) - RETURNING id, token, account_id, expires_at, created_at, last_used_at, revoked`, - hashRefreshToken(tokenString), accountID, expiresAt, time.Now(), + RETURNING id, token, account_id, expires_at, created_at, last_used_at, revoked, rotated_at`, + hashRefreshToken(plaintext), accountID, expiresAt, time.Now(), ).Scan( &token.ID, &token.Token, @@ -51,25 +60,41 @@ func CreateRefreshToken(ctx context.Context, accountID uint, rememberMe bool) (* &token.CreatedAt, &token.LastUsedAt, &token.Revoked, + &token.RotatedAt, ) - if err != nil { - return nil, fmt.Errorf("failed to create refresh token: %w", err) + return nil, fmt.Errorf("failed to insert refresh token: %w", err) } - // The database holds only the hash; the caller needs the plaintext to - // hand to the client — this is the only place it exists server-side - token.Token = tokenString - + // The database holds only the hash; the plaintext exists server-side only + // here, to be handed to the client + token.Token = plaintext return &token, nil } +// CreateRefreshToken creates a new refresh token for a user +func CreateRefreshToken(ctx context.Context, accountID uint, rememberMe bool) (*RefreshToken, error) { + var expiresAt time.Time + if rememberMe { + expiresAt = time.Now().Add(time.Hour * 24 * time.Duration(config.RefreshTokenRememberMeDays)) + } else { + expiresAt = time.Now().Add(time.Hour * 24 * time.Duration(config.RefreshTokenDays)) + } + + tokenString, err := newTokenString() + if err != nil { + return nil, err + } + + return insertRefreshToken(ctx, database.DB(), tokenString, accountID, expiresAt) +} + // GetRefreshToken retrieves a refresh token by token string func GetRefreshToken(ctx context.Context, tokenString string) (*RefreshToken, error) { var token RefreshToken err := database.DB().QueryRowContext(ctx, - `SELECT id, token, account_id, expires_at, created_at, last_used_at, revoked + `SELECT id, token, account_id, expires_at, created_at, last_used_at, revoked, rotated_at FROM refresh_token WHERE token = $1`, hashRefreshToken(tokenString), @@ -81,6 +106,7 @@ func GetRefreshToken(ctx context.Context, tokenString string) (*RefreshToken, er &token.CreatedAt, &token.LastUsedAt, &token.Revoked, + &token.RotatedAt, ) if err == sql.ErrNoRows { @@ -93,16 +119,65 @@ func GetRefreshToken(ctx context.Context, tokenString string) (*RefreshToken, er return &token, nil } -// UpdateLastUsed updates the last_used_at timestamp -func UpdateLastUsed(ctx context.Context, tokenID uint) error { - _, err := database.DB().ExecContext(ctx, - `UPDATE refresh_token SET last_used_at = $1 WHERE id = $2`, - time.Now(), tokenID, +// HasLiveRefreshToken reports whether the account still has at least one +// non-revoked, unexpired refresh token — used to deny the rotation grace +// once a logout/password-change/admin action has cleared the sessions. +func HasLiveRefreshToken(ctx context.Context, accountID uint) (bool, error) { + var exists bool + err := database.DB().QueryRowContext(ctx, + `SELECT EXISTS( + SELECT 1 FROM refresh_token + WHERE account_id = $1 AND revoked = FALSE AND expires_at > $2)`, + accountID, time.Now(), + ).Scan(&exists) + if err != nil { + return false, fmt.Errorf("failed to check for live refresh token: %w", err) + } + return exists, nil +} + +// RotateRefreshToken atomically revokes the presented token and issues its +// successor, preserving the session horizon. Returns ErrTokenAlreadyRotated +// when a concurrent rotation of the same token won the race. +func RotateRefreshToken(ctx context.Context, old *RefreshToken) (*RefreshToken, error) { + tokenString, err := newTokenString() + if err != nil { + return nil, err + } + + tx, err := database.DB().BeginTx(ctx, nil) + if err != nil { + return nil, fmt.Errorf("failed to begin rotation transaction: %w", err) + } + defer func() { _ = tx.Rollback() }() // no-op once committed + + now := time.Now() + result, err := tx.ExecContext(ctx, + `UPDATE refresh_token SET revoked = TRUE, rotated_at = $1, last_used_at = $1 + WHERE id = $2 AND revoked = FALSE`, + now, old.ID, ) if err != nil { - return fmt.Errorf("failed to update last used: %w", err) + return nil, fmt.Errorf("failed to revoke rotated token: %w", err) + } + rowsAffected, err := result.RowsAffected() + if err != nil { + return nil, fmt.Errorf("failed to check rows affected: %w", err) + } + if rowsAffected == 0 { + return nil, ErrTokenAlreadyRotated } - return nil + + successor, err := insertRefreshToken(ctx, tx, tokenString, old.AccountID, old.ExpiresAt) + if err != nil { + return nil, err + } + + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("failed to commit rotation: %w", err) + } + + return successor, nil } // RevokeRefreshToken marks a refresh token as revoked. It returns the owning diff --git a/pkg/security/refresh_tokens_test.go b/pkg/security/refresh_tokens_test.go index 38c1155..2d6ff84 100644 --- a/pkg/security/refresh_tokens_test.go +++ b/pkg/security/refresh_tokens_test.go @@ -150,24 +150,6 @@ func TestGetRefreshToken_NotFound(t *testing.T) { assert.Contains(t, err.Error(), "not found") } -func TestUpdateLastUsed(t *testing.T) { - ctx := context.Background() - accountID := createTestAccount(t) - - token, err := CreateRefreshToken(ctx, accountID, false) - require.NoError(t, err) - assert.Nil(t, token.LastUsedAt) - - time.Sleep(time.Second) // Ensure time difference - - err = UpdateLastUsed(ctx, token.ID) - require.NoError(t, err) - - updated, err := GetRefreshToken(ctx, token.Token) - require.NoError(t, err) - assert.NotNil(t, updated.LastUsedAt) -} - func TestRevokeRefreshToken_Success(t *testing.T) { ctx := context.Background() accountID := createTestAccount(t) @@ -236,6 +218,49 @@ func TestRevokeAllUserTokens(t *testing.T) { assert.Equal(t, int64(0), revoked) } +func TestRotateRefreshToken_Success(t *testing.T) { + ctx := context.Background() + accountID := createTestAccount(t) + + old, err := CreateRefreshToken(ctx, accountID, true) + require.NoError(t, err) + + successor, err := RotateRefreshToken(ctx, old) + require.NoError(t, err) + assert.NotEmpty(t, successor.Token) + assert.NotEqual(t, old.Token, successor.Token) + assert.Equal(t, accountID, successor.AccountID) + // Rotation preserves the session horizon + assert.WithinDuration(t, old.ExpiresAt, successor.ExpiresAt, time.Second) + + // The presented token is revoked and marked as rotated + oldRow, err := GetRefreshToken(ctx, old.Token) + require.NoError(t, err) + assert.True(t, oldRow.Revoked) + require.NotNil(t, oldRow.RotatedAt) + + // The successor is live + newRow, err := GetRefreshToken(ctx, successor.Token) + require.NoError(t, err) + assert.False(t, newRow.Revoked) + assert.Nil(t, newRow.RotatedAt) +} + +func TestRotateRefreshToken_AlreadyRotated(t *testing.T) { + ctx := context.Background() + accountID := createTestAccount(t) + + old, err := CreateRefreshToken(ctx, accountID, false) + require.NoError(t, err) + + _, err = RotateRefreshToken(ctx, old) + require.NoError(t, err) + + // A concurrent rotation of the same token loses the race + _, err = RotateRefreshToken(ctx, old) + require.ErrorIs(t, err, ErrTokenAlreadyRotated) +} + func TestCleanupExpiredTokens(t *testing.T) { ctx := context.Background() accountID := createTestAccount(t) diff --git a/pkg/security/types.go b/pkg/security/types.go index cf84928..c9c914c 100644 --- a/pkg/security/types.go +++ b/pkg/security/types.go @@ -14,6 +14,9 @@ type RefreshToken struct { CreatedAt time.Time LastUsedAt *time.Time Revoked bool + // RotatedAt is set when the token was revoked because a successor was + // issued (rotation), nil when revoked by logout/password/admin action + RotatedAt *time.Time } // RefreshTokenInput represents the input for refresh token endpoint @@ -30,10 +33,15 @@ type TokenPairResponse struct { RefreshExpiresIn int64 `json:"refresh_expires_in"` } -// RefreshResponse represents the response from refresh endpoint +// RefreshResponse represents the response from refresh endpoint. +// RefreshToken carries the rotated successor the client must store — the +// presented token is revoked. The fields are absent only on the short +// reuse-grace path (benign multi-tab race), where no rotation happens. type RefreshResponse struct { - AccessToken string `json:"access_token"` - ExpiresIn int64 `json:"expires_in"` + AccessToken string `json:"access_token"` + ExpiresIn int64 `json:"expires_in"` + RefreshToken string `json:"refresh_token,omitempty"` + RefreshExpiresIn int64 `json:"refresh_expires_in,omitempty"` } // LogoutAllResponse is the response of POST /v1/auth/logout-all diff --git a/tests/api-scenarios/001-user-registration-auth.yaml b/tests/api-scenarios/001-user-registration-auth.yaml index e11b8ea..e60a9be 100644 --- a/tests/api-scenarios/001-user-registration-auth.yaml +++ b/tests/api-scenarios/001-user-registration-auth.yaml @@ -183,7 +183,7 @@ scenarios: new_access_token: "{{response.access_token}}" refresh_token: "{{response.refresh_token}}" - - name: "Refresh access token using refresh token" + - name: "Refresh access token (strict rotation returns a successor refresh token)" request: method: POST endpoint: "/auth/refresh" @@ -198,8 +198,13 @@ scenarios: - type: json_path path: "$.expires_in" exists: true + - type: json_path + path: "$.refresh_token" + exists: true store: refreshed_access_token: "{{response.access_token}}" + # The presented token is revoked by rotation; use the successor from now on + refresh_token: "{{response.refresh_token}}" - name: "Verify refreshed access token works" request: From 4d1e641f997674fba415aeda038d1d17a7222b95 Mon Sep 17 00:00:00 2001 From: Angak0k Date: Sun, 5 Jul 2026 00:04:07 +0200 Subject: [PATCH 2/2] fix(SEC-7): close rotation TOCTOU races (code review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- pkg/security/handlers.go | 45 +++++++++++++++++++++++++--------- pkg/security/refresh_tokens.go | 2 +- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/pkg/security/handlers.go b/pkg/security/handlers.go index 510c99c..6dfe705 100644 --- a/pkg/security/handlers.go +++ b/pkg/security/handlers.go @@ -55,19 +55,19 @@ func RefreshTokenHandler(c *gin.Context) { return } - // 5. Strict rotation: revoke the presented token and issue its successor. - // Losing a concurrent-rotation race is the same benign situation as the - // grace path: answer with an access token only; the client adopts the - // successor from the winning request. + // 5. Generate the access token BEFORE rotating: if signing fails we must + // not have already burned (revoked) the presented refresh token. + accessToken, err := GenerateToken(refreshToken.AccountID) + if err != nil { + AuditRefreshFailed(c, "failed to generate access token") + c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) + return + } + + // 6. Strict rotation: revoke the presented token and issue its successor. successor, err := RotateRefreshToken(c.Request.Context(), refreshToken) switch { case err == nil: - accessToken, genErr := GenerateToken(refreshToken.AccountID) - if genErr != nil { - AuditRefreshFailed(c, "failed to generate access token") - c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) - return - } AuditRefreshSuccess(c, refreshToken.AccountID) c.JSON(http.StatusOK, RefreshResponse{ AccessToken: accessToken, @@ -76,13 +76,36 @@ func RefreshTokenHandler(c *gin.Context) { RefreshExpiresIn: max(0, int64(time.Until(successor.ExpiresAt).Seconds())), }) case errors.Is(err, ErrTokenAlreadyRotated): - respondWithAccessToken(c, refreshToken.AccountID) + // The row changed between the read and the rotation UPDATE (a + // concurrent rotation, a logout/password/admin revocation, or a + // just-crossed expiry). Re-read and apply the current policy rather + // than blindly granting access. + reevaluateRefreshAfterRace(c, input.Token) default: AuditRefreshFailed(c, "failed to rotate refresh token") c.JSON(http.StatusInternalServerError, gin.H{"error": helper.ErrMsgInternalServer}) } } +// reevaluateRefreshAfterRace re-reads a token that could not be rotated +// (the row was revoked/rotated/expired concurrently) and re-applies the full +// validation, so a benign rotation race still yields access while a +// concurrent revocation or expiry correctly returns 401. +func reevaluateRefreshAfterRace(c *gin.Context, tokenString string) { + latest, err := GetRefreshToken(c.Request.Context(), tokenString) + if err != nil { + AuditRefreshFailed(c, "invalid refresh token") + c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid refresh token"}) + return + } + if time.Now().After(latest.ExpiresAt) { + AuditRefreshFailed(c, "refresh token has expired") + c.JSON(http.StatusUnauthorized, gin.H{"error": "Refresh token has expired"}) + return + } + handleRevokedRefreshToken(c, latest) +} + // handleRevokedRefreshToken answers a refresh attempt with a revoked token. // Rotation deliberately does NOT auto-revoke sibling sessions on replay: a // superseded token replayed by a benign client (multi-tab race, a lost diff --git a/pkg/security/refresh_tokens.go b/pkg/security/refresh_tokens.go index 1fae840..ba80be3 100644 --- a/pkg/security/refresh_tokens.go +++ b/pkg/security/refresh_tokens.go @@ -154,7 +154,7 @@ func RotateRefreshToken(ctx context.Context, old *RefreshToken) (*RefreshToken, now := time.Now() result, err := tx.ExecContext(ctx, `UPDATE refresh_token SET revoked = TRUE, rotated_at = $1, last_used_at = $1 - WHERE id = $2 AND revoked = FALSE`, + WHERE id = $2 AND revoked = FALSE AND expires_at > $1`, now, old.ID, ) if err != nil {