fix(oauth): make device code and refresh token single-use

[appleboy]

Summary

Closes two concurrency races that let a single credential be redeemed more than once on /oauth/token. A refresh token in rotation mode and an authorized device code are now both truly single-use: concurrent redemptions of the same credential result in at most one new token pair, and the losing caller gets a clear OAuth error instead of a second set of valid credentials.

AI Authorship

• No AI was used in this PR
• AI was used. Details:
  • Tool / model: Claude Opus 4.8 (Claude Code)
  • AI-authored files: all 10 files in the diff (production code, tests, and regenerated mocks)
  • Human line-by-line reviewed: none yet — human review is still pending. Please review the production paths closely (see Reviewer guide).
  • Automated checks run: make generate && make fmt && make lint && make test (all green), plus an automated multi-agent simplify/cleanup pass.

Change classification

• Core code (broad impact — needs line-by-line review)
• Leaf node

This sits on the OAuth token issuance/refresh hot path and changes a shared store interface (DeviceCodeStore.DeleteDeviceCodeByID signature) used across services.

Plan reference

Implements the goal of plan.md (batch B, issues #4 + #13): make refresh-token rotation and device-code exchange atomic single-use.

• build(deps): Bump actions/setup-go from 5 to 6 #4 — refresh rotation: the old refresh token was revoked with an unconditional UPDATE … WHERE id=?, and the IsActive() status check ran in-memory outside the transaction. Two concurrent refreshes with the same active token both passed the check and each minted a valid token chain.
• feat: introduce pluggable token providers with selectable validation modes #13 — device code: ExchangeDeviceCode issued tokens first and deleted the device code last (swallowing the delete error), so two concurrent exchanges both issued tokens.

What changed

• Device code (token_exchange.go): the device code is now consumed via an atomic delete that returns rows-affected, before token issuance. RowsAffected == 0 → reject with access_denied. Mirrors the existing auth-code flow (MarkAuthorizationCodeUsed runs before issuance). DeleteDeviceCodeByID now returns (int64, error).
• Refresh rotation (token_refresh.go): the old refresh token is revoked inside the transaction via a new compare-and-swap store method RevokeRefreshTokenIfActive (UPDATE … WHERE id=? AND status='active'). The loser sees RowsAffected == 0, rolls back its transaction, and returns invalid_grant.
• No family revoke on same-instant races (deliberate): a same-millisecond duplicate is most often a legitimate client retry/double-submit, not theft. Genuine delayed replay of an already-rotated token is still caught by the existing !IsActive() → revokeTokenFamilyWithAudit path, which is unchanged.
• UpdateTokenStatus is intentionally left unconditional — the admin enable/disable/revoke paths depend on it, which is why a separate CAS method was added rather than guarding the shared one.

Verification

• Unit tests (store layer): RevokeRefreshTokenIfActive (active→1, revoked/disabled/missing→0) and DeleteDeviceCodeByID (exists→1, already-deleted→0)
• Concurrent service-level tests: 2 goroutines redeeming the same device code / refresh token → asserts exactly one success, loser gets the right error, only one token chain persists, family not revoked
• At least 3 e2e/service tests (1 happy path via existing rotation tests + 2 concurrent-race error paths)
• Stress / soak test: N/A — race tests use a barrier-synchronized goroutine pair against real SQLite; also pass under -race
• Manual verification (optional): start server with ENABLE_TOKEN_ROTATION=true, fire two concurrent curl requests with the same refresh token at /oauth/token → expect one 200 and one 4xx.

Verifiability check

• Inputs/outputs documented in method and call-site comments
• Reviewer can judge correctness from the CAS WHERE clauses + rows-affected handling and the concurrency tests
• Failures surface as standard OAuth errors and existing token-refresh metrics

Security check

• No secrets in code
• External inputs (refresh token, device code, client_id/secret) validated on the existing paths ahead of these changes
• Single-use enforcement is the security property under test; covered by concurrent tests
• Rate limits already applied to /oauth/token (unchanged)
• Errors return opaque OAuth codes (invalid_grant / access_denied), no internal leakage

Risk & rollback

• Risk: the device-code fix moves consumption before issuance, so a signing/persist failure now burns the device code and the CLI must restart the flow — an accepted trade-off, consistent with the auth-code flow. A missed call-site for the changed DeleteDeviceCodeByID signature would fail to compile (caught by build).
• Rollback: single self-contained commit, no schema/migration changes. git revert fully restores prior behavior with no data-compatibility concerns.

Reviewer guide

• Read carefully: internal/store/token.go (RevokeRefreshTokenIfActive CAS), internal/services/token_refresh.go (transaction CAS + rollback semantics, no-family-revoke decision), internal/services/token_exchange.go (consume-before-issue ordering), internal/store/device_code.go (signature change).
• Spot-check OK: internal/mocks/mock_store.go (regenerated by mockgen, not hand-edited), internal/core/store.go (interface signatures), the test files (verify the concurrency assertions match the intended invariants).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 35.55556% with 29 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/mocks/mock_store.go	0.00%	26 Missing ⚠️
internal/services/token_refresh.go	60.00%	1 Missing and 1 partial ⚠️
internal/services/device.go	50.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

Fixes two TOCTOU concurrency races on /oauth/token so that an authorized device code and a rotation-mode refresh token are truly single-use. Both flows now consume the credential via a compare-and-swap DB update (rows-affected) before issuing new tokens; the losing concurrent caller gets a clear OAuth error and no new token chain is minted.

Changes:

• Device code is deleted atomically before token issuance in ExchangeDeviceCode; DeleteDeviceCodeByID now returns (rowsAffected, error) and a 0 rows-affected result maps to ErrAccessDenied.
• Refresh-token rotation now uses a new CAS store method RevokeRefreshTokenIfActive (WHERE id=? AND status='active') inside the transaction; the loser rolls back and returns invalid_grant (deliberately no family revoke for same-instant races).
• Interface, mocks, and tests updated; new concurrency tests (token_single_use_test.go) plus store-level CAS/delete tests assert exactly-one-winner semantics.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/core/store.go	Updates DeleteDeviceCodeByID signature and adds RevokeRefreshTokenIfActive to the writer interface.
internal/store/token.go	Implements the CAS RevokeRefreshTokenIfActive (active → revoked guarded update).
internal/store/device_code.go	DeleteDeviceCodeByID now returns rows-affected for single-use enforcement.
internal/services/token_exchange.go	Consumes device code via atomic delete before token issuance; rejects 0-rows as ErrAccessDenied.
internal/services/token_refresh.go	Rotation path revokes old refresh token via in-transaction CAS; loser returns ErrInvalidRefreshToken.
internal/services/device.go	Adapts DeleteDeviceCodeByID call sites to the new two-value signature.
internal/services/token_single_use_test.go	New concurrent tests proving exactly-one-winner for device-code exchange and refresh rotation.
internal/store/token_test.go	Adds unit tests covering active/revoked/disabled/missing branches of RevokeRefreshTokenIfActive.
internal/store/device_code_test.go	Adds rows-affected assertions and an "already deleted" case.
internal/mocks/mock_store.go	Regenerated mocks for the changed/new store methods.

Files not reviewed (1)

• internal/mocks/mock_store.go: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.