fix(api): invalidate auth cache on API key deletion - #3324
Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit bf7ad2b. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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 immediate cache invalidation when an API key is deleted. It updates the database query to return the deleted key's hash and introduces an InvalidateAPIKeyCache method to the AuthService interface. Feedback suggests adding a defensive nil check for the authService parameter in DeleteAPIKey to prevent potential runtime panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59f199fc18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this PR touches the API-key authentication/revocation cache path, so I'm deferring for a human look rather than auto-approving.
Reviewed the delete-key flow end-to-end: the SQL query change (RETURNING api_key_hash instead of id), the new InvalidateAPIKeyCache plumbed through both the user-facing and admin delete handlers, and the AuthService interface/impl change. Checked the async background cache-refresh path noted by the bug hunter (teamCache's refresh-ahead goroutine) for resurrecting a just-deleted key's cache entry — the PR's own notes already call out the residual replica-lag race, and it's bounded to seconds rather than the original 5-minute TTL. Test coverage (admin_api_keys_test.go) asserts the deleted key's hash is passed to InvalidateAPIKeyCache.
Extended reasoning...
Overview
The PR fixes delayed API-key revocation: deleting a team API key now returns the key's api_key_hash from the DB (via an updated sqlc query) and invalidates the corresponding Redis auth-cache entry through a new InvalidateAPIKeyCache method on the auth Service interface. Both the user-facing (DELETE /api-keys/{id}) and admin (DELETE /admin/teams/{teamID}/api-keys/{id}) delete paths are updated to call this. Generated sqlc code and the raw SQL file were regenerated to match.
Security risks
This is directly on the authentication/authorization path (API key validation and cache invalidation), so it warrants care even though the change itself is narrow. The main risk class is incomplete invalidation (revoked keys continuing to authenticate) rather than a new vulnerability being introduced. The PR description is transparent about a residual replica-lag race (a request reading from the DB read replica could theoretically re-populate the cache for a just-deleted key before replication catches up), which is a pre-existing architectural property of the read-replica + cache design, not something this PR introduces or worsens — it shrinks the exposure window from ~5 minutes to a replication-lag-scale window.
Level of scrutiny
Given this sits on an auth revocation path, I lean toward the 'do not approve' criterion for security-sensitive code even in the absence of concrete bugs. The change is small and mechanical (return a hash instead of an id, call an invalidate method), and is well-tested, but the surface (auth cache correctness) is the kind of thing where a human sign-off is worth the small extra cost.
Other factors
Test coverage is solid: the admin delete test was extended to assert the cache invalidation call with the correct hash, and a fake auth service implementation was added/extended for both the admin and dashboard-api test suites to satisfy the new interface method. No integration test was added, but the PR description explains why (the integration harness lacks admin-token/JWT plumbing for these endpoints) and the handler-level test already exercises real Postgres.
|
Addressed the P1 in b66fcaa: Considered a tombstone/negative-cache for deleted hashes as the alternative (would keep this query on the replica), but went with the primary read for simplicity — the extra primary load is bounded by the cache-miss + 1-minute refresh rates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b66fcaafd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 386c2b1e07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Deleted API keys kept authenticating for up to 5 minutes because the Redis-backed auth cache entry (keyed by the key's SHA-256 hash) was never invalidated on deletion. DeleteTeamAPIKey now returns the deleted key's hash, and team.DeleteAPIKey invalidates the corresponding auth cache entry via the new AuthService.InvalidateAPIKeyCache, so revocation takes effect immediately across all API instances (the cache is shared in Redis).
…fter-write safe After a key deletion invalidates the cache entry, a cache miss that read through the read replica could race replication lag, find the just-deleted key still present, and re-cache it for the full 5-minute TTL. Reading the API-key auth lookup from the primary closes that window (same approach as the OIDC identity lookup).
The invalidation runs after the key's DB delete has committed. If the client disconnected mid-request, the canceled request context would make the Redis delete silently fail and the revoked key would keep authenticating until the cache TTL expires. Run the invalidation on a bounded context detached from the request.
…opulation RedisCache writers (GetOrSet backfill, background refresh) hold the per-key lock across their SET, but Delete gave up on the lock after 5s and deleted anyway — a writer stalled on the backing store could then SET a pre-delete value after the DEL, repopulating stale data for a full TTL. For the auth cache this meant a revoked API key could be resurrected for another 5 minutes. Delete now waits for the lock up to the lock TTL plus a margin (bounded by the caller's ctx), so a healthy writer — whose callback is capped at RefreshTimeout, strictly less than the lock TTL — always lands its SET before the DEL. The auth invalidation timeout grows to 45s to cover that wait; lock failure still degrades to a best-effort DEL, now logged with the stale-repopulation risk. Constraints are documented on Delete and the auth invalidation path.
295c58d to
bf7ad2b
Compare
2 participants
Summary
Fixes EN-1874: deleted API keys kept authenticating for up to ~5 minutes while the dashboard promises keys are disabled immediately.
API-key auth results are cached in the shared Redis auth cache (
auth:team:<sha256-of-key>, 5-minute TTL), but the key-deletion handlers never invalidated the entry. The existingInvalidateTeamCachehelper couldn't be reused as-is because it enumerates the team's current key hashes from the DB — a just-deleted key is no longer returned.Changes
DeleteTeamAPIKeySQL query now doesRETURNING api_key_hashinstead ofid(the hash is exactly the auth cache key); sqlc code regenerated.InvalidateAPIKeyCache(ctx, hashedKey)on the authServiceinterface deletes the cached auth entry.team.DeleteAPIKeyinvalidates the returned hash after a successful DB delete, covering both callers (user-facingDELETE /api-keys/{id}and adminDELETE /admin/teams/{teamID}/api-keys/{id}). The cache is Redis-backed and shared, so one delete revokes the key across all API instances.GetTeamByHashedAPIKeynow reads from the primary DB instead of the read replica, so a cache miss racing replication lag can't re-cache a just-deleted key for another full TTL (same read-after-write reasoning as the existing OIDC identity lookup). Extra primary load is bounded by the cache miss + 1-minute refresh rates.InvalidateAPIKeyCacheruns on a bounded context detached from request cancellation, so a client disconnect after the DB delete commits can't skip the invalidation.RedisCache.Deletenow waits for the per-key writer lock (up to the lock TTL plus a margin) instead of giving up after 5s and deleting anyway — otherwise a cache writer stalled on the DB just before the deletion could SET the pre-delete value after the DEL, resurrecting the revoked key for another full TTL.Constraints / residual risk
RefreshTimeout(30s), strictly less than the lock TTL the delete waits for. If the lock cannot be obtained at all (Redis/lock-service errors or ctx expiry — no longer plain writer contention),Deletedegrades to a best-effort DEL, logs the stale-repopulation risk, and a concurrent writer may repopulate the entry until its TTL expires. Fully closing that path would need a tombstone/version mechanism inRedisCache; deferred unless it proves necessary.invalidateTimeout) if a concurrent refresh of the same key is wedged near the full 30s refresh timeout. This requires a multi-second DB stall on a single indexed point query; the typical case is milliseconds.Test plan
TestDeleteAdminTeamsTeamIDApiKeysDeletesTeamKeyto assert the deleted key's cache entry is invalidated (hash computed from the raw created key).TestRedisCache_DeleteWaitsForInflightWriterholds the write lock past the old 5s acquire cap and asserts the DEL lands after the writer's SET (verified to fail against the previousDeletebehavior).go test -racepasses forpackages/shared/pkg/cache,packages/api,packages/auth,packages/dashboard-api; golangci-lint clean.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.