Harden storage transactions against path drift#145
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughadds platform-aware storage path normalization and comparison utilities to detect path drift during transactions. transaction handlers now capture storage path in state and validate it at export time, preventing exports when paths change. introduces new optional parameters to internal functions to support custom storage paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
detailed notes for reviewwindows path edge cases: carefully verify transaction state lifecycle: the export blocking logic: snapshot tie-breaking: regression test coverage: tests cover exception handling, storage reloads, and combined transactions, but verify there's no gap for concurrent transaction attempts or race conditions between path checks and file operations. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 review |
|
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function normalizeStorageComparisonPath(path: string): string { | ||
| const resolved = resolvePath(path); | ||
| if (process.platform !== "win32") { | ||
| return resolved; | ||
| } | ||
| return resolved.replaceAll("\\", "/").toLowerCase(); | ||
| } | ||
|
|
||
| function areEquivalentStoragePaths(left: string, right: string): boolean { | ||
| return ( | ||
| normalizeStorageComparisonPath(left) === normalizeStorageComparisonPath(right) | ||
| ); | ||
| } |
There was a problem hiding this comment.
normalizeStorageComparisonPath calls resolvePath (which uses path.resolve) — that resolves . and .. components but does not dereference symlinks. if the stored transaction path was captured through a symlink (~/.codex/multi-auth → /Users/foo/.codex/multi-auth) and the active path comes in as the real path (or vice versa), areEquivalentStoragePaths returns false and exportAccounts throws a spurious mismatch error. on windows token storage paths this is less common, but on macos /var → /private/var symlinks are a well-known trap. using fs.realpathSync (with a try/catch for missing paths) would make the comparison robust. worth at least a code comment noting the symlink caveat.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 384-396
Comment:
**symlink aliases can fool path equivalence on posix**
`normalizeStorageComparisonPath` calls `resolvePath` (which uses `path.resolve`) — that resolves `.` and `..` components but does **not** dereference symlinks. if the stored transaction path was captured through a symlink (`~/.codex/multi-auth → /Users/foo/.codex/multi-auth`) and the active path comes in as the real path (or vice versa), `areEquivalentStoragePaths` returns `false` and `exportAccounts` throws a spurious mismatch error. on windows token storage paths this is less common, but on macos `/var` → `/private/var` symlinks are a well-known trap. using `fs.realpathSync` (with a try/catch for missing paths) would make the comparison robust. worth at least a code comment noting the symlink caveat.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Tightens storage transaction handling when the resolved storage path changes mid-transaction.
What changed
Validation
npm run lintnpm run typechecknpx vitest run test/storage.test.ts --testNamePattern="fails fast when export is called from an active transaction after the storage path changes|allows equivalent Windows-style storage paths during export from an active transaction|reloads fresh storage after a transaction handler throws|persists transaction updates to the original storage path after path drift|reloads fresh storage after a transaction handler returns successfully|reloads fresh storage after a combined transaction handler throws|reloads fresh storage after a combined transaction handler returns successfully"Supersedes part of #139.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr hardens storage transaction semantics against mid-transaction path drift by pinning
loadAccountsInternalandsaveAccountsUnlockedto the path captured at transaction start, fast-failingexportAccountswhen it detects a path mismatch via the newareEquivalentStoragePathshelper (windows-aware), and marking transactions inactive in afinallyblock so stale context can't leak to post-transaction reads.normalizeStorageComparisonPath/areEquivalentStoragePaths: correct design for windows case-insensitivity, but usespath.resolvenotfs.realpath— symlink aliases (e.g. macos/var → /private/var) can produce a false mismatch and causeexportAccountsto throw a spurious error against valid token storagelatestValidSnapshotgains a stable index tiebreaker whenmtimeMsvalues are equal — clean fix, well testedstate.active = falseinfinallyfor both transaction functions — correct; prevents stale snapshot from being visible toexportAccountscalled after a throwwithAccountAndFlaggedStorageTransaction; the windowsreplaceAll("\\", "/")branch is never actually exercised on posix ci sincepath.resolvenever emits backslashes on linuxConfidence Score: 3/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[withAccountStorageTransaction / withAccountAndFlaggedStorageTransaction] --> B[withStorageLock] B --> C[storagePath = getStoragePath at start] C --> D[loadAccountsInternal pinned to storagePath] D --> E[transactionSnapshotContext.run state active=true] E --> F[handler current persist] F -->|persist called| G[saveAccountsUnlocked pinned to storagePath] G --> H[state.snapshot updated] F -->|success or throw| I[finally: state.active = false] I --> J[transaction done] K[exportAccounts called] --> L{transactionState?.active?} L -->|no| M[new withAccountStorageTransaction fresh load] L -->|yes| N{areEquivalentStoragePaths transaction vs active?} N -->|paths match| O[use transactionState.snapshot] N -->|paths differ| P[throw path mismatch error] style P fill:#f55,color:#fff style O fill:#5a5,color:#fff style M fill:#5a5,color:#fffPrompt To Fix All With AI
Last reviewed commit: "Harden storage trans..."