fix(auth): lock credential profile mutations#272
Conversation
|
✅ This PR is linked to an issue assigned to @naufalfx805-source — thanks! The |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesCredential profile writes and deletions now use an exclusive lock around the full read-modify-write cycle, including retries, stale-lock recovery, token-checked cleanup, and timeout handling. Tests cover stale-lock recovery and missing-profile file absence. Credential mutation locking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProfileMutation as writeProfile/deleteProfile
participant Lock as credentials.lock
participant Credentials as credentials file
Caller->>ProfileMutation: Request profile mutation
ProfileMutation->>Lock: Acquire exclusive lock
Lock-->>ProfileMutation: Grant or reclaim stale lock
ProfileMutation->>Credentials: Read, mutate, and atomically write
ProfileMutation->>Lock: Release matching lock
ProfileMutation-->>Caller: Complete mutation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/credentials.ts (2)
285-286: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Math.random()used for the lock token.Static analysis flags
Math.random()in token construction. Here the token only arbitrates ownership of a local0o600advisory lock file (not a cross-trust-boundary secret), so predictability isn't a meaningful exploit path, butcrypto.randomUUID()is a trivial drop-in that removes the finding and gives stronger uniqueness guarantees thanMath.random().toString(16).Proposed fix
- const token = `${process.pid}:${Date.now()}:${Math.random().toString(16).slice(2)}`; + const token = `${process.pid}:${Date.now()}:${randomUUID()}`;(requires importing
randomUUIDfromnode:crypto)🤖 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/lib/credentials.ts` around lines 285 - 286, Replace the Math.random-based portion of the lock token in the credentials locking flow with randomUUID imported from node:crypto. Preserve the existing process ID and timestamp components and keep the token used by the local advisory lock unchanged otherwise.Source: Linters/SAST tools
275-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
mkdirSynccall.
writeCredentialsAtomic'smkdirSync(dirname(path), ...)duplicates the one already performed inmutateCredentialsFile(Line 263) immediately before the lock is acquired. Since this function's only visible caller ismutateCredentialsFile, the second call is redundant work on every write.🤖 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/lib/credentials.ts` around lines 275 - 281, Remove the redundant mkdirSync(dirname(path), ...) call from writeCredentialsAtomic, relying on mutateCredentialsFile to create the parent directory before acquiring the lock. Leave the atomic write, rename, and permission enforcement behavior unchanged.
🤖 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/lib/credentials.ts`:
- Around line 283-342: Prevent stale-lock reclamation from allowing a live
holder to write after ownership is stolen. Update mutateCredentialsFile to
verify the lock token still matches immediately before writeCredentialsAtomic,
and abort without writing if ownership was lost; retain releaseCredentialsLock’s
token check and existing reclaim behavior for genuinely stale or dead locks.
---
Nitpick comments:
In `@src/lib/credentials.ts`:
- Around line 285-286: Replace the Math.random-based portion of the lock token
in the credentials locking flow with randomUUID imported from node:crypto.
Preserve the existing process ID and timestamp components and keep the token
used by the local advisory lock unchanged otherwise.
- Around line 275-281: Remove the redundant mkdirSync(dirname(path), ...) call
from writeCredentialsAtomic, relying on mutateCredentialsFile to create the
parent directory before acquiring the lock. Leave the atomic write, rename, and
permission enforcement behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 200bebb9-6f4b-4755-897a-66033eb9499f
📒 Files selected for processing (2)
src/lib/credentials.test.tssrc/lib/credentials.ts
Summary
Fixes #77
Claim
Test plan
Summary by CodeRabbit
Bug Fixes
Tests