Revoke every refresh token when a used one is replayed

[diveshpatil9104]

Before this, replaying a used refresh token got a 401 and a log line, and the thief's copy kept working.

Now a replay revokes every refresh token the user holds, through the existing DeleteRefreshTokensForUser. I chose that over a per-login family id because only the Android app and the simulator use refresh tokens, and a driver has one device, so both are the same set. A family id would cost a migration for no gain here.

The handler finds a spent token in two places and both revoke: the UsedAt check, and the !rotated branch where a concurrent request won the compare-and-set. Revoking only in the first leaves a hole. A thief racing the real client can win the rotation and get token B, the client logs in again, and nobody ever replays B, so it is never revoked. This only works because #129 made DeleteRefreshTokensForUser take the user lock, so a revoke that lands mid-rotation deletes the replacement too.

A failed revoke returns 500, not 401. A 401 would send the real client to log in again while the thief's copy survived, and a retry after a 500 tries the revocation again. Otherwise the caller sees the same 401 as before, and the revocation is logged at Warn with sub and ip.

Out of scope: the thief's current access token works until it expires (24h today), since the server doesn't know which jti the thief holds.

Mutation checks: removing the revoke from the UsedAt branch fails TestHandleRefresh_RejectsReusedToken and TestRefresh_ReplayRevokesEveryToken, removing it from the !rotated branch fails TestHandleRefresh_ConcurrentLossRevokes, and returning 401 on a failed revoke fails TestHandleRefresh_ReuseRevokeFailureIs500 (one subtest per branch).

go build, vet, fmt and test are clean, and with DATABASE_URL set the store and refresh tests ran (135 passed, 0 skipped). #134 isn't merged, so CI skips the new DB test until it is.

Part of #118

Summary by CodeRabbit

• Security
  • Reusing a refresh token that has already been used now revokes all refresh tokens for that account, including tokens from other sessions. Those sessions must sign in again after their existing access tokens expire.
  • If revoking refresh tokens fails, the refresh request returns a server error instead of an unauthorized response.
  • A refresh-token rotation that loses a concurrent update also revokes the account’s refresh tokens.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: b5ea6c84-0c21-4a56-b549-e26bd91d5668

📥 Commits

Reviewing files that changed from the base of the PR and between b9f2790 and 92703e8.

📒 Files selected for processing (5)

• README.md
• auth_refresh.go
• auth_refresh_test.go
• openapi.yaml
• store_refresh_tokens_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Refresh-token replay now revokes all refresh tokens held by the user. The handler returns 500 if revocation fails. Tests and documentation cover replay, concurrent rotation loss, and the effect on existing access tokens.

Changes

Refresh-token replay revocation

Layer / File(s)	Summary
Replay revocation handling auth_refresh.go	RefreshStore now requires refresh-token deletion. When the handler detects a spent token or loses a concurrent rotation, it deletes all refresh tokens for that user. It returns 500 if deletion fails.
Replay behavior validation and documentation auth_refresh_test.go, store_refresh_tokens_test.go, README.md, openapi.yaml	Tests verify revocation after replay and concurrent rotation loss, deletion-failure responses, and that normal rotation or rejection of an unknown or expired token does not revoke sessions. Documentation describes replay revocation and notes that issued access tokens remain valid until expiry.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant RefreshHandler
  participant RefreshStore
  Client->>RefreshHandler: Submit a spent refresh token
  RefreshHandler->>RefreshStore: Delete all refresh tokens for the user
  RefreshStore-->>RefreshHandler: Return deletion result
  RefreshHandler-->>Client: Return 401, or 500 if deletion failed

Loading

Merge Risk: ⚪ Minimal · up to 92703

Replay revokes the affected user’s refresh sessions, and a revocation failure is reported as a server error. No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 92703

The change improves replay containment and limits deletion to the token owner's refresh sessions. Account-wide invalidation on benign retries is deliberate, and already-issued access tokens remain usable until expiry. The core rotation and deletion ordering is supported, but overlapping fresh logins, client recovery behavior, and deployment conditions are not fully demonstrated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The maximum direct deletion scope is all refresh-token rows for the presented token's owner, including independent logins and rotation replacements. Integration coverage verifies that another user's token survives.

Security Findings and Attack Paths

• observed — Possession of a retained spent token now permits account-wide refresh invalidation without password authentication. A benign retry after a lost response has the same effect. This is the documented containment policy, not an established cross-owner vulnerability; successful deletion also removes the token row that enabled subsequent lookups.

Trust Boundaries and Controls

• observed — The handler limits request size, validates JSON input, and applies the shared per-IP limiter when configured before resolving the token. Deletion authority is then derived from persisted ownership. Fresh login remains a separate boundary requiring password verification and an active account.

Resilience and Maintainability Implications

• inferred — The rotation lock proof does not cover every recovery interleaving. Login creation uses a direct insert after independent authentication, and requests that already captured a spent row can perform later deletions that invalidate newly issued refresh tokens. Once captured requests finish and the trigger row is gone, the same old token cannot initiate further deletion through a new lookup. Exact concurrent-login ordering and client recovery behavior remain unproven.

Hardening Proposals

• proposed — Define the recovery boundary for overlapping replay and fresh-login requests, including client single-flight refresh handling and reauthentication after account-wide invalidation. This would make the intended security-versus-availability tradeoff explicit without treating the current policy as a verified vulnerability.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: revoking every refresh token when a used token is replayed.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[aaronbrethorst]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[aaronbrethorst]

Thanks, Divesh. Revoking on both the UsedAt != nil path and the lost-race path is right, since the lost race is the case where a thief could otherwise keep a fresh token. Returning 500 when revocation fails, so a retry tries again, is a careful touch. Merging.

One thing to settle before the Android app starts refreshing: with no grace window, a client that retries a refresh after losing the response logs the driver out everywhere. Please document single-flight refresh for client authors. That's a follow-up, not a blocker.