fix: make API key rotation atomic and replayable - #1624
Merged
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (45)
📝 WalkthroughWalkthroughAPI 키 로테이션을 멱등적·원자적으로 처리하도록 auth-core와 auth-drizzle의 계약, 암호화 보호기, 저장소 트랜잭션, 이벤트 복구, PostgreSQL 테스트, CI 경로, 문서와 문제 코드가 확장되었습니다. Changes원자적 API 키 로테이션
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ApiKeyManager
participant DrizzleApiKeyStore
participant EventBus
Caller->>ApiKeyManager: rotate(id, idempotencyKey)
ApiKeyManager->>DrizzleApiKeyStore: atomic rotate with recovery ciphertext
DrizzleApiKeyStore-->>ApiKeyManager: rotation result
ApiKeyManager->>DrizzleApiKeyStore: claimRotationEvent
ApiKeyManager->>EventBus: publish rotation event
ApiKeyManager->>DrizzleApiKeyStore: completeRotationEvent
DrizzleApiKeyStore-->>Caller: recovered rotation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
📊 Benchmark Results✅ All benchmarks passed
Updated: 2026-07-29T20:52:06.104Z · Commit: 620894e |
kang-heewon
force-pushed
the
fix/1585-atomic-api-key-rotation
branch
from
July 29, 2026 19:23
abc838c to
fc41832
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/auth-core/src/libs/apikey/ApiKeyManager.ts`:
- Around line 203-228: Update the rotation flow around ApiKeyManager’s
store.rotate call to persist eventStatus as "pending" for every rotation,
regardless of eventBus availability. Ensure the no-publisher path produces a
diagnostic degraded signal rather than returning degraded as false, while
preserving normal publishRotationEvent behavior when an eventBus is configured.
- Around line 171-182: Update ApiKeyManager.rotate to normalize
options.idempotencyKey once with trimming, validate that normalized value, and
use it consistently for protectionContext and store.rotate. Move the
rotationProtector configuration check before store.findById so missing
configuration fails without an unnecessary lookup.
In `@packages/auth-core/src/tests/AesGcmApiKeyRotationProtector.spec.ts`:
- Around line 14-54: AesGcmApiKeyRotationProtector tests lack coverage for
fail-closed key and ciphertext validation. Extend the suite around
constructor/requireKey and decrypt to assert ApiKeyRotationProtectionProblem for
a non-32-byte key, a malformed ciphertext such as “v1.a.b.c”, and decryption
with a protector whose keyId is no longer registered.
In `@packages/auth-core/src/tests/ApiKeyManager.spec.ts`:
- Around line 616-620: Map size assertions use toHaveLength incorrectly. In
packages/auth-core/src/tests/ApiKeyManager.spec.ts lines 616-620 and 657-660,
update the _getKeys() assertions in the idempotent retry and second logical
rotation rejection tests to assert _getKeys().size equals 2 with toBe(2).
In `@packages/auth-drizzle/src/libs/DrizzleApiKeyStore.ts`:
- Around line 337-399: Make the claim transition and replacement-key lookup
atomic in the rotation-event methods shown before completeRotationEvent and in
completeRotationEvent: wrap each update and subsequent findKeyWithClient lookup
in this.db.transaction, and roll back when the replacement cannot be found so a
failed lookup does not leave processing or completed state persisted. Preserve
the existing return mapping and claim/status conditions, or replace both steps
with a single joined query that provides the same atomic behavior.
- Around line 468-481: Update the transaction in the API key deletion flow to
inspect related rows in schema.apiKeyRotations before deleting them. If any
rotation has eventStatus pending or processing, reject the deletion with the
established Problem mechanism, or otherwise record observable evidence before
destruction; do not silently delete incomplete rotation events. Preserve
deletion of completed rotations and the apiKeys row.
- Around line 262-312: Update the rotation transaction around the replacement
insert and insertedIntent conflict handling so a conflicting intent cannot
commit the newly inserted active replacement key. Either detect and return the
existing rotation before inserting the replacement, with the required deferred
foreign-key behavior for newKeyId, or delete input.replacement.id in the
conflicting return path before returning; preserve normal rotation behavior and
ensure the old key is still revoked when appropriate.
- Around line 402-424: Update releaseRotationEvent to inspect the affected-row
result from the guarded update, using returning() or the database’s row-count
mechanism. Distinguish a successful release from a claim mismatch or
already-reclaimed event, and propagate that failure through the method’s
established diagnostic or boolean contract instead of silently resolving as
success; adjust the Promise return type and callers as needed.
In `@packages/auth-drizzle/src/migrations/addApiKeyRotations.ts`:
- Around line 9-24: Update the api_key_rotations table definition in
addApiKeyRotations to explicitly name the new_key_id uniqueness constraint
api_key_rotations_new_key_unique, matching the declaration in the schema index.
Keep the uniqueness behavior unchanged and avoid relying on PostgreSQL’s
generated constraint name.
In `@packages/auth-drizzle/src/tests/DrizzleApiKeyStore.postgres.spec.ts`:
- Around line 195-248: Move the database state assertion out of the
eventBus.publish callback in the test “recovers post-commit event publication
with the same event and credential.” Have the callback only collect the observed
state alongside the event, then assert the expected old-key revoked and new-key
active state after both rotate calls, outside the manager’s degraded
error-handling path.
In
`@packages/docs/src/content/docs/api/auth-drizzle/src/classes/DrizzleApiKeyStore.md`:
- Around line 30-33: Update the DrizzleApiKeyStore API documentation for
apiKeyRotations so it is documented as required rather than optional. Explain
that the table must be created and supplied, while noting that the constructor
falls back to defaultApiKeyRotations when it is not provided; keep the existing
PgTableWithColumns type unchanged.
In `@scripts/tests/ci-workflow.spec.ts`:
- Around line 241-248: Update the “routes auth changes to the real PostgreSQL
rotation suite” test to also assert that REAL_RESOURCE_JOB injects the
AUTH_POSTGRES_URL environment variable. Keep the existing workflow path and
command assertions unchanged, and verify the env key is present alongside the
PostgreSQL test command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdb5c6ca-7bb4-4105-9352-c728004b460e
⛔ Files ignored due to path filters (2)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.changeset/atomic-api-key-rotation.md.github/workflows/ci.ymldocs/problem-code-registry.jsonpackages/auth-core/README.mdpackages/auth-core/src/index.tspackages/auth-core/src/libs/apikey/ApiKeyManager.tspackages/auth-core/src/libs/apikey/ApiKeyRotationProtector.tspackages/auth-core/src/libs/apikey/ApiKeyStore.tspackages/auth-core/src/libs/interfaces/ApiKey.tspackages/auth-core/src/libs/problems/AuthProblems.tspackages/auth-core/src/tests/AesGcmApiKeyRotationProtector.spec.tspackages/auth-core/src/tests/ApiKeyManager.spec.tspackages/auth-core/src/tests/ApiKeySecurity.spec.tspackages/auth-drizzle/README.mdpackages/auth-drizzle/package.jsonpackages/auth-drizzle/src/index.tspackages/auth-drizzle/src/libs/DrizzleApiKeyStore.tspackages/auth-drizzle/src/migrations/addApiKeyRotations.tspackages/auth-drizzle/src/schema/index.tspackages/auth-drizzle/src/tests/DrizzleApiKeyStore.postgres.spec.tspackages/auth-drizzle/src/tests/DrizzleApiKeyStore.spec.tspackages/auth-drizzle/src/tests/DrizzleProviderConformance.spec.tspackages/docs/src/content/docs/api/auth-core/src/classes/AesGcmApiKeyRotationProtector.mdpackages/docs/src/content/docs/api/auth-core/src/classes/ApiKeyManager.mdpackages/docs/src/content/docs/api/auth-core/src/classes/ApiKeyRotationConflictProblem.mdpackages/docs/src/content/docs/api/auth-core/src/classes/ApiKeyRotationProtectionProblem.mdpackages/docs/src/content/docs/api/auth-core/src/classes/ApiKeyStore.mdpackages/docs/src/content/docs/api/auth-core/src/classes/InvalidApiKeyRotationIdempotencyKeyProblem.mdpackages/docs/src/content/docs/api/auth-core/src/interfaces/ApiKeyRotationProtector.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/AesGcmApiKeyRotationProtectorOptions.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/ApiKeyRotation.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/ApiKeyRotationInput.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/ApiKeyRotationPhaseStatus.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/ApiKeyRotationProtectionContext.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/RotateApiKeyOptions.mdpackages/docs/src/content/docs/api/auth-core/src/type-aliases/RotateApiKeyResult.mdpackages/docs/src/content/docs/api/auth-core/src/variables/API_KEY_ROTATION_PROTECTOR_TOKEN.mdpackages/docs/src/content/docs/api/auth-drizzle/src/classes/DrizzleApiKeyStore.mdpackages/docs/src/content/docs/api/auth-drizzle/src/functions/addApiKeyRotations.mdpackages/docs/src/content/docs/api/auth-drizzle/src/functions/removeApiKeyRotations.mdpackages/docs/src/content/docs/api/auth-drizzle/src/type-aliases/ApiKeyRotationMigrationClient.mdpackages/docs/src/content/docs/api/auth-drizzle/src/variables/apiKeyRotations.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpublic-api-surface.snapshot.jsonscripts/tests/ci-workflow.spec.ts
kang-heewon
force-pushed
the
fix/1585-atomic-api-key-rotation
branch
from
July 29, 2026 20:12
fc41832 to
62aea59
Compare
kang-heewon
dismissed
coderabbitai[bot]’s stale review
July 29, 2026 20:35
Superseded by commit 62aea59. Valid findings were fixed and verified; remaining comments were reviewed against the published contracts, answered with rationale, and resolved. The latest CodeRabbit status check passes.
kang-heewon
force-pushed
the
fix/1585-atomic-api-key-rotation
branch
from
July 29, 2026 20:42
62aea59 to
87043dd
Compare
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
1 participant
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.
Outcome
Fixes #1585
Verification
pnpm --filter @croco/auth-core test— 160/160 passed.pnpm --filter @croco/auth-drizzle test— 62/62 passed; 8 environment-gated PostgreSQL tests also passed separately.pnpm check— 24/25 passed, 1 not applicable, 0 failed.pnpm docs:build— 115/115 tasks passed.pnpm public-api:check,pnpm problem-registry:check,pnpm release-docs:check, and changeset-required verification passed.Review gates
Deployment contract
Apply the rotation schema first, pause rotation traffic, drain every instance using the legacy save-then-revoke path, deploy the new writers, and only then resume rotation. Mixed legacy and atomic rotation writers are unsupported because a legacy writer cannot participate in the new store transaction.
Retain every rotation-protection key for as long as records encrypted with it must remain replayable.
Summary by CodeRabbit
새 기능
문서
테스트