feat(oauth): add rotating refresh tokens - #408
Conversation
|
Warning Review limit reached
Next review available in: 25 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughOAuth authorization now issues and rotates durable refresh-token families for eligible clients. Family state uses digests, exclusive locks, runtime isolation, scope/resource validation, replay revocation, and linked access-session checks. Consent-page loopback CSP handling and broad validation coverage were added. ChangesOAuth rotating refresh-token families
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthClient
participant SDKOAuthServerProvider
participant RefreshTokenFamilyRepository
participant SessionRepository
OAuthClient->>SDKOAuthServerProvider: exchange authorization code
SDKOAuthServerProvider->>RefreshTokenFamilyRepository: create refresh family
SDKOAuthServerProvider->>SessionRepository: create family access session
OAuthClient->>SDKOAuthServerProvider: exchange refresh token
SDKOAuthServerProvider->>RefreshTokenFamilyRepository: consume and rotate token
RefreshTokenFamilyRepository-->>SDKOAuthServerProvider: rotated or replay result
SDKOAuthServerProvider->>SessionRepository: create next session or revoke family sessions
SDKOAuthServerProvider-->>OAuthClient: return token response
Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
test/e2e/oauth-loopback-consent.e2e.test.ts (2)
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
waitForCallbacknever rejects.If Approve fails to redirect, the promise hangs until the 30 s test timeout, masking the real failure point. A bounded timeout with a descriptive rejection makes diagnosis faster.
♻️ Add a bounded wait
-function waitForCallback(): Promise<URL> { - return new Promise<URL>((resolve) => { - resolveCallback = resolve; - }); -} +function waitForCallback(timeoutMs = 10_000): Promise<URL> { + return new Promise<URL>((resolve, reject) => { + const timer = setTimeout(() => { + resolveCallback = undefined; + reject(new Error('Timed out waiting for the loopback callback')); + }, timeoutMs); + resolveCallback = (url) => { + clearTimeout(timer); + resolve(url); + }; + }); +}🤖 Prompt for 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. In `@test/e2e/oauth-loopback-consent.e2e.test.ts` around lines 235 - 239, Update waitForCallback to reject with a descriptive error after a bounded timeout when the callback is not received, while preserving resolveCallback for successful redirects and clearing the timeout once it resolves.
78-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting this into per-phase tests.
One ~150-line
itcovers DCR, consent CSP, PKCE exchange, scope/resource rejection, rotation, replay, and revocation. A failure anywhere aborts the rest and gives a vague signal. Sequentialits sharing state via the describe scope (orit.sequential) would localize failures without re-running the browser flow.🤖 Prompt for 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. In `@test/e2e/oauth-loopback-consent.e2e.test.ts` around lines 78 - 232, Split the monolithic test around its existing phases into sequential per-phase tests, using shared describe-scope state for the client, tokens, and authorization results so the browser consent flow is not repeated. Keep dedicated assertions for DCR/consent CSP and PKCE exchange, scope/resource rejection, refresh rotation and replay, and revocation; ensure setup dependencies run in order and retain the shared context cleanup.test/e2e/fixtures/oauth-refresh-worker.mjs (1)
18-26: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the sleep buffer out of the wait loop.
A fresh
SharedArrayBuffer/Int32Arrayper 10 ms tick is needless churn; allocate once before the loop. Otherwise the blocking-sleep approach is the right call inside this synchronous callback.♻️ Allocate the wait buffer once
if (releasePath) { const deadline = Date.now() + 10_000; + const sleepHandle = new Int32Array(new SharedArrayBuffer(4)); while (!fs.existsSync(releasePath) && Date.now() < deadline) { - Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10); + Atomics.wait(sleepHandle, 0, 0, 10); }🤖 Prompt for 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. In `@test/e2e/fixtures/oauth-refresh-worker.mjs` around lines 18 - 26, Update the releasePath wait loop to allocate the SharedArrayBuffer and Int32Array sleep buffer once before entering the loop, then reuse that buffer for each Atomics.wait call while preserving the existing timeout and release-file checks.src/auth/sdkOAuthServerProvider.refresh.test.ts (2)
326-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the helper for what it does.
readOnlyFamilyreads as "read-only family";readSoleFamily(orreadTheOnlyFamily) makes the single-family assertion intent obvious. Also consider a typed return instead ofRecord<string, any>.🤖 Prompt for 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. In `@src/auth/sdkOAuthServerProvider.refresh.test.ts` around lines 326 - 330, Rename the readOnlyFamily helper to readSoleFamily (or readTheOnlyFamily) so its single-file assertion is explicit, and update all call sites accordingly. Replace the broad Record<string, any> return type with the appropriate concrete type already used for parsed family data, if available.
111-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mockImplementationOnce+mockRestoreis redundant.
mockImplementationOncealready falls back to the original after the first call, so the follow-up assertion at Line 124 would pass even without the restore. PrefermockImplementationso the second exchange genuinely proves the spy was removed, or drop themockRestore.🤖 Prompt for 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. In `@src/auth/sdkOAuthServerProvider.refresh.test.ts` around lines 111 - 127, Simplify the spy lifecycle in the test around createSession: either use persistent mockImplementation and explicitly mockRestore before the second exchange, or remove the restore and rely on mockImplementationOnce. Ensure the follow-up exchange genuinely verifies the intended post-failure behavior without redundant setup.test/e2e/oauth-refresh-concurrency.e2e.test.ts (2)
34-51: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard provider shutdown with
try/finally.If
authCodeRepository.createorexchangeAuthorizationCodethrows,provider.shutdown()at Line 51 is skipped and storage handles/locks leak into the rest of the suite, whileafterEachstill deletes the directory underneath them.🤖 Prompt for 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. In `@test/e2e/oauth-refresh-concurrency.e2e.test.ts` around lines 34 - 51, Wrap the provider setup and authorization-code exchange flow in a try/finally block so provider.shutdown() always executes, including when authCodeRepository.create or exchangeAuthorizationCode throws. Keep the existing setup behavior unchanged while ensuring cleanup occurs before afterEach removes the temporary directory.
27-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo child-process cleanup on failure.
runWorkernever exposes the child, so iffirst.resultrejects (or the test fails mid-flight) the spawned workers keep running whileafterEachdeletestempDirbeneath them, producing confusing secondary errors and leaked processes.♻️ Track and kill children in teardown
describe('refresh token family cross-process persistence', () => { let tempDir: string | undefined; + const children: ReturnType<typeof spawn>[] = []; afterEach(() => { + for (const child of children.splice(0)) { + if (child.exitCode === null) { + child.kill('SIGKILL'); + } + } if (tempDir) { fs.rmSync(tempDir, { recursive: true, force: true }); } });-): { result: Promise<{ status: string }> } { + children: ReturnType<typeof spawn>[], +): { result: Promise<{ status: string }> } { const child = spawn( process.execPath, [ @@ { cwd: process.cwd(), stdio: ['ignore', 'pipe', 'pipe'] }, ); + children.push(child);Also applies to: 69-112
🤖 Prompt for 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. In `@test/e2e/oauth-refresh-concurrency.e2e.test.ts` around lines 27 - 31, Update the runWorker test helper and teardown to retain references to every spawned child process and kill them during afterEach before removing tempDir. Ensure cleanup runs even when first.result rejects or the test fails mid-flight, while preserving the existing worker execution behavior.docs/adr/0011-oauth-refresh-tokens-use-rotating-families.md (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this ADR with the section format.
docs/adr/0010already uses a small frontmatter header (status: accepted), but this ADR has only the title and content. Add the ADR’s status/frontmatter so it matches the established ADR layout.🤖 Prompt for 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. In `@docs/adr/0011-oauth-refresh-tokens-use-rotating-families.md` around lines 1 - 8, Update the frontmatter at the beginning of the ADR so it includes the established accepted status used by docs/adr/0010, placing it before the title while preserving the existing title and content unchanged.Source: Coding guidelines
src/auth/storage/refreshTokenFamilyRepository.ts (1)
108-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated token-state derivation.
This inline ternary re-implements
getTokenState(lines 77-88) with a slightly different lookup source (located.lookupvs a freshreadLookup). Extracting a shared private helper that accepts an optional pre-read lookup would keep the two paths from drifting.🤖 Prompt for 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. In `@src/auth/storage/refreshTokenFamilyRepository.ts` around lines 108 - 113, Update the token-state logic in the repository method containing the inline ternary to reuse a shared private helper, rather than duplicating getTokenState’s conditions. Make the helper accept an optional pre-read lookup so it can use located.lookup when available while preserving the fresh readLookup behavior in getTokenState, and ensure both paths derive identical states.src/auth/storage/fileStorageService.ts (1)
407-411: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
releaseLockthrowing from thefinallyblock can mask the operation's outcome.If the release rename fails, the thrown error replaces the operation's return value or its original error. Log and swallow instead — abandoned lock directories are already reclaimable via
reclaimAbandonedLock.♻️ Proposed change
} catch (error) { logger.error(`Failed to release storage lock ${lockPath}: ${error}`); - throw error; }Also applies to: 620-636
🤖 Prompt for 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. In `@src/auth/storage/fileStorageService.ts` around lines 407 - 411, Update the lock-release cleanup around the operation wrapper and the corresponding flow near the second release site so errors from releaseLock do not replace the operation’s return value or original exception. Catch release failures, log them, and swallow them while preserving the existing reclaimAbandonedLock behavior for abandoned lock directories.src/auth/sessionTypes.ts (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the top-level Zod UUID schema.
This project uses Zod 4, where
z.string().uuid()is deprecated; usez.uuid()here instead.♻️ Proposed change
- accessTokenIds: z.array(z.string().uuid()), + accessTokenIds: z.array(z.uuid()),🤖 Prompt for 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. In `@src/auth/sessionTypes.ts` at line 40, Update the accessTokenIds schema to use Zod 4’s top-level z.uuid() validator instead of the deprecated z.string().uuid() chain, while preserving the array-of-UUIDs validation.
🤖 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 `@src/auth/oauthAuthorizationFlow.ts`:
- Around line 18-25: The widened createSessionWithId contract is not propagated
by createOAuthAuthorizationFlowFromStorage. Update its createSessionWithId
adapter and the corresponding interface declaration to accept optional
refreshFamilyId and pass it through to sessionRepository.createWithId;
alternatively remove the optional parameter consistently from both interfaces
until supported by callers.
In `@src/auth/sdkOAuthServerProvider.ts`:
- Around line 450-466: Remove the unsynchronized findByToken/getTokenState
validation and the tokenState branches from the refresh-token flow around the
repository calls. Invoke repository.consume as the single authoritative,
lock-held decision point, then preserve the existing rotation.status ===
'replay' handling to revoke family access tokens and return the replay error;
continue the normal rotation path for successful results.
In `@src/auth/storage/fileStorageService.ts`:
- Around line 533-552: Broaden the Windows-specific tolerated error-code
allowlist in flushStorageDirectory so directory fsync failures from both
fs.openSync and fs.fsyncSync do not propagate for supported cases. Extend the
existing ['EINVAL', 'ENOTSUP', 'EPERM'] set to include EISDIR and EACCES, while
preserving rethrow behavior for other errors and non-Windows platforms.
In `@test/e2e/oauth-refresh-concurrency.e2e.test.ts`:
- Around line 56-65: Replace the fixed delay before the secondMarker assertion
with an explicit startup synchronization. Update the runWorker fixture or worker
entrypoint to emit a distinct started signal before consume, await that signal
for the second worker, then assert secondMarker is absent before releasing
releasePath. Preserve the existing Promise.all result assertion for the replay
and rotated outcomes.
---
Nitpick comments:
In `@docs/adr/0011-oauth-refresh-tokens-use-rotating-families.md`:
- Around line 1-8: Update the frontmatter at the beginning of the ADR so it
includes the established accepted status used by docs/adr/0010, placing it
before the title while preserving the existing title and content unchanged.
In `@src/auth/sdkOAuthServerProvider.refresh.test.ts`:
- Around line 326-330: Rename the readOnlyFamily helper to readSoleFamily (or
readTheOnlyFamily) so its single-file assertion is explicit, and update all call
sites accordingly. Replace the broad Record<string, any> return type with the
appropriate concrete type already used for parsed family data, if available.
- Around line 111-127: Simplify the spy lifecycle in the test around
createSession: either use persistent mockImplementation and explicitly
mockRestore before the second exchange, or remove the restore and rely on
mockImplementationOnce. Ensure the follow-up exchange genuinely verifies the
intended post-failure behavior without redundant setup.
In `@src/auth/sessionTypes.ts`:
- Line 40: Update the accessTokenIds schema to use Zod 4’s top-level z.uuid()
validator instead of the deprecated z.string().uuid() chain, while preserving
the array-of-UUIDs validation.
In `@src/auth/storage/fileStorageService.ts`:
- Around line 407-411: Update the lock-release cleanup around the operation
wrapper and the corresponding flow near the second release site so errors from
releaseLock do not replace the operation’s return value or original exception.
Catch release failures, log them, and swallow them while preserving the existing
reclaimAbandonedLock behavior for abandoned lock directories.
In `@src/auth/storage/refreshTokenFamilyRepository.ts`:
- Around line 108-113: Update the token-state logic in the repository method
containing the inline ternary to reuse a shared private helper, rather than
duplicating getTokenState’s conditions. Make the helper accept an optional
pre-read lookup so it can use located.lookup when available while preserving the
fresh readLookup behavior in getTokenState, and ensure both paths derive
identical states.
In `@test/e2e/fixtures/oauth-refresh-worker.mjs`:
- Around line 18-26: Update the releasePath wait loop to allocate the
SharedArrayBuffer and Int32Array sleep buffer once before entering the loop,
then reuse that buffer for each Atomics.wait call while preserving the existing
timeout and release-file checks.
In `@test/e2e/oauth-loopback-consent.e2e.test.ts`:
- Around line 235-239: Update waitForCallback to reject with a descriptive error
after a bounded timeout when the callback is not received, while preserving
resolveCallback for successful redirects and clearing the timeout once it
resolves.
- Around line 78-232: Split the monolithic test around its existing phases into
sequential per-phase tests, using shared describe-scope state for the client,
tokens, and authorization results so the browser consent flow is not repeated.
Keep dedicated assertions for DCR/consent CSP and PKCE exchange, scope/resource
rejection, refresh rotation and replay, and revocation; ensure setup
dependencies run in order and retain the shared context cleanup.
In `@test/e2e/oauth-refresh-concurrency.e2e.test.ts`:
- Around line 34-51: Wrap the provider setup and authorization-code exchange
flow in a try/finally block so provider.shutdown() always executes, including
when authCodeRepository.create or exchangeAuthorizationCode throws. Keep the
existing setup behavior unchanged while ensuring cleanup occurs before afterEach
removes the temporary directory.
- Around line 27-31: Update the runWorker test helper and teardown to retain
references to every spawned child process and kill them during afterEach before
removing tempDir. Ensure cleanup runs even when first.result rejects or the test
fails mid-flight, while preserving the existing worker execution behavior.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4f10ce65-0d16-4b1a-b12d-799d2f2a895b
📒 Files selected for processing (19)
docs/adr/0011-oauth-refresh-tokens-use-rotating-families.mddocs/en/guide/advanced/authentication.mddocs/zh/guide/advanced/authentication.mdsrc/auth/oauthAuthorizationFlow.tssrc/auth/sdkOAuthServerProvider.refresh.test.tssrc/auth/sdkOAuthServerProvider.test.tssrc/auth/sdkOAuthServerProvider.tssrc/auth/sessionTypes.tssrc/auth/storage/fileStorageService.test.tssrc/auth/storage/fileStorageService.tssrc/auth/storage/oauthStorageService.tssrc/auth/storage/refreshTokenFamilyRepository.tssrc/auth/storage/sessionRepository.tssrc/constants/auth.tssrc/core/runtime/runtimeIdentityService.tssrc/transport/http/server.tstest/e2e/fixtures/oauth-refresh-worker.mjstest/e2e/oauth-loopback-consent.e2e.test.tstest/e2e/oauth-refresh-concurrency.e2e.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/auth/storage/fileStorageService.ts (1)
621-635: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not report success while leaving an unreleasable live-process lock.
If the rename at Line 630 fails,
.refresh-family.lockremains owned by this still-live PID.reclaimAbandonedLock()will not reclaim it, so subsequent refresh rotations time out for the process lifetime. Retry/fallback-delete the verified lock directory and surface unrecoverable cleanup failures; extend the test to acquire the lock again after the injected failure.
src/auth/storage/fileStorageService.ts#L621-L635: recover the verified lock directory before returning, or propagate an unrecoverable release failure.src/auth/storage/fileStorageService.test.ts#L146-L155: verify a subsequentwithExclusiveLockcall succeeds after simulated cleanup failure.🤖 Prompt for 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. In `@src/auth/storage/fileStorageService.ts` around lines 621 - 635, Update fileStorageService.ts lines 621-635 in releaseLock to retry or fallback-delete the verified lock directory when fs.renameSync fails, and surface any unrecoverable cleanup failure instead of silently returning with a live lock. Extend fileStorageService.test.ts lines 146-155 to inject the cleanup failure and verify a subsequent withExclusiveLock call succeeds.
🤖 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.
Outside diff comments:
In `@src/auth/storage/fileStorageService.ts`:
- Around line 621-635: Update fileStorageService.ts lines 621-635 in releaseLock
to retry or fallback-delete the verified lock directory when fs.renameSync
fails, and surface any unrecoverable cleanup failure instead of silently
returning with a live lock. Extend fileStorageService.test.ts lines 146-155 to
inject the cleanup failure and verify a subsequent withExclusiveLock call
succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86121923-e0bf-469d-a80a-79243c2462e5
📒 Files selected for processing (11)
docs/adr/0011-oauth-refresh-tokens-use-rotating-families.mdsrc/auth/oauthAuthorizationFlow.tssrc/auth/sdkOAuthServerProvider.refresh.test.tssrc/auth/sdkOAuthServerProvider.tssrc/auth/sessionTypes.tssrc/auth/storage/fileStorageService.test.tssrc/auth/storage/fileStorageService.tssrc/auth/storage/refreshTokenFamilyRepository.tstest/e2e/fixtures/oauth-refresh-worker.mjstest/e2e/oauth-loopback-consent.e2e.test.tstest/e2e/oauth-refresh-concurrency.e2e.test.ts
💤 Files with no reviewable changes (1)
- src/auth/sdkOAuthServerProvider.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/auth/storage/fileStorageService.ts (2)
334-348: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRequire a schema for every persisted-data read.
Making
schemaoptional preserves an unvalidatedJSON.parse/cast path; malformed records can bypass expiration/type checks. Make the schema required and update callers to supply their record schema.Proposed direction
-readData<T extends ExpirableData>(filePrefix: string, id: string, schema?: ZodType<T>): T | null { +readData<T extends ExpirableData>(filePrefix: string, id: string, schema: ZodType<T>): T | null { ... - const parsedData = schema ? schema.parse(parsed) : (parsed as T); + const parsedData = schema.parse(parsed);As per coding guidelines, “Use Zod schemas for all input validation and API boundaries.”
🤖 Prompt for 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. In `@src/auth/storage/fileStorageService.ts` around lines 334 - 348, Update readData in FileStorageService to require a ZodType<T> schema instead of accepting an optional schema, and always validate parsed JSON through that schema before returning it. Update every readData caller to pass the appropriate record schema, removing the unchecked JSON.parse cast path while preserving existing expiration and error handling.Source: Coding guidelines
555-580: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftPrevent owner-less lock reclamation from granting two owners.
After
mkdirSyncat Line 557, the acquiring process can be paused for over one second before writingowner.json. Another process then tombstones the owner-less directory and acquires a new lock. The first process can resume and overwrite the new owner file at Line 567, so both operations proceed concurrently.Create
owner.jsonexclusively (flag: 'wx'), treat an existing/different owner as acquisition failure, and only remove a lock directory after re-reading and confirming this operation owns it. Add a deterministic regression test for this interleaving.Also applies to: 584-618
🤖 Prompt for 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. In `@src/auth/storage/fileStorageService.ts` around lines 555 - 580, Update tryAcquireLock and its cleanup path so owner.json is created exclusively with flag 'wx'; treat an existing owner, including one written after lock reclamation, as acquisition failure rather than overwriting it. Before removing the lock directory after initialization failure, re-read owner.json and remove the directory only when it still identifies the current operation; otherwise preserve the newer owner and return failure. Add a deterministic regression test covering the pause after mkdirSync, reclamation by another process, and prevention of concurrent ownership.
🤖 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 `@src/auth/storage/fileStorageService.test.ts`:
- Around line 146-158: Update the test around service.withExclusiveLock to
assert that failedReleaseRename is true after the first lock release, confirming
the injected .releasing rename failure actually occurred while preserving the
existing recovery and reacquisition assertions.
---
Outside diff comments:
In `@src/auth/storage/fileStorageService.ts`:
- Around line 334-348: Update readData in FileStorageService to require a
ZodType<T> schema instead of accepting an optional schema, and always validate
parsed JSON through that schema before returning it. Update every readData
caller to pass the appropriate record schema, removing the unchecked JSON.parse
cast path while preserving existing expiration and error handling.
- Around line 555-580: Update tryAcquireLock and its cleanup path so owner.json
is created exclusively with flag 'wx'; treat an existing owner, including one
written after lock reclamation, as acquisition failure rather than overwriting
it. Before removing the lock directory after initialization failure, re-read
owner.json and remove the directory only when it still identifies the current
operation; otherwise preserve the newer owner and return failure. Add a
deterministic regression test covering the pause after mkdirSync, reclamation by
another process, and prevention of concurrent ownership.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 40a2c584-d991-4f13-a127-29bd9ce562b4
📒 Files selected for processing (2)
src/auth/storage/fileStorageService.test.tssrc/auth/storage/fileStorageService.ts
1 participant
Summary
Validation
pnpm lintpnpm typecheckpnpm buildpnpm test:unit(297 files, 4,334 tests)pnpm test:e2e test/e2e/oauth-loopback-consent.e2e.test.ts(Chromium, 1 test)Existing Vitest mock-hoisting and Node deprecation warnings remain unchanged.
Closes #403
Closes #404
Summary by CodeRabbit
form-actionto a validated loopback origin.