fix(file-parsers): guard .doc uploads against zip-bomb memory exhaustion - #6166
Conversation
DocParser handed the raw upload straight to officeparser and then mammoth, both of which inflate every ZIP entry into memory before any app-level size cap applies. The extension is only a routing hint, so a bomb-bearing OOXML archive renamed to .doc selected the one parser that skipped the guard its docx/pptx/xlsx siblings all call. Adds assertOoxmlArchiveWithinLimits to DocParser.parseBuffer, and centrally in file-parsers parseBuffer so a future parser cannot silently opt out. The guard reads the central directory's declared sizes without decompressing, and no-ops for non-ZIP buffers, so legacy OLE .doc files are unaffected.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
The zip guard adds a second stage after declared-size/ratio checks: each DEFLATE entry is inflated with New tests cover Reviewed by Cursor Bugbot for commit bea4e75. Configure here. |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6a006c4. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6a006c4. Configure here.
|
@greptile review |
The declared uncompressed sizes in a ZIP central directory are attacker- controlled, so a bomb can under-report them and pass the size and ratio checks untouched. officeparser and mammoth only detect the mismatch after inflating the entry in full: a 498 KB archive declaring 1000 bytes per entry drove 559 MB resident through the .doc parser and 538 MB through .docx, then failed. SheetJS and officeparser reject the container earlier, so xlsx/pptx were not affected, but doc and docx both were. Each entry is now inflated during verification under a maxOutputLength bound equal to the size it declared. Node's zlib aborts the moment output would exceed that bound, so a lying entry costs only its declared size and the inflated bytes are discarded immediately; both bomb variants now reject at +0 MB across every extension. Stored entries are checked against their own compressed size, and unsupported compression methods fail closed. Verification walks the contiguous run of central-directory records rather than the EOCD's declared entry count, since that run is what a decompression library allocates per entry — a lied-down count must not hide an entry from verification. Cost is ~0.45 ms per MB of uncompressed content (22 ms for a 50 MB archive), against parse times an order of magnitude larger. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted.
|
@cursor review |
Greptile SummaryThe PR hardens uploaded-document parsing against ZIP bombs, including archives disguised with non-OOXML extensions and entries that under-report their expanded sizes.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported central/local compression-method bypass is rejected before payload processing and covered by a regression test.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/file-parsers/zip-guard.ts | Adds bounded per-entry inflation, central/local header consistency checks, and contiguous central-directory traversal; the previously reported method-mismatch bypass is rejected before payload handling. |
| apps/sim/lib/file-parsers/zip-guard.test.ts | Adds focused regression tests for under-declared sizes, unsupported methods, mismatched headers, stored-entry inconsistencies, and under-reported entry counts. |
| apps/sim/lib/file-parsers/doc-parser.ts | Runs ZIP-bomb validation before either document extraction library while preserving legacy non-ZIP .doc handling. |
| apps/sim/lib/file-parsers/index.ts | Enforces archive validation centrally for all extension-routed buffer parsing. |
| apps/sim/lib/file-parsers/doc-parser.test.ts | Verifies guarded .doc handling rejects malicious ZIP-shaped inputs before downstream libraries and still accepts valid OOXML and legacy OLE documents. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Uploaded buffer] --> B{ZIP-shaped?}
B -- No --> C[Defer to selected parser]
B -- Yes --> D[Read contiguous central-directory records]
D --> E{Declared totals and ratio within limits?}
E -- No --> R[Reject archive]
E -- Yes --> F{Central and local headers agree?}
F -- No --> R
F -- Yes --> G[Inflate each DEFLATE entry with declared-size output cap]
G --> H{Actual output matches declaration?}
H -- No --> R
H -- Yes --> C
Reviews (3): Last reviewed commit: "fix(file-parsers): charge hidden central..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The parsers disagree about which header to trust. JSZip skips the local header outright and decompresses using the central directory's method, while SheetJS's parse_local_file switches on the local header's method and inflates from there. An entry claiming STORED centrally and DEFLATE locally therefore took the guard's stored branch, skipping bounded inflation, and was still expanded downstream — a 398 KB archive hiding a 400 MB deflate payload. Verification now rejects any entry whose two headers disagree on compression method, and on declared sizes when the local header carries them (the data-descriptor flag and ZIP64 sentinels legitimately omit them, and those entries stay covered by the bounded inflate). Caught by Greptile review. All 17 real Word-produced .docx fixtures in mammoth's test data are still accepted.
…e cap sumDeclaredUncompressedSize walked only the entry count the EOCD declares, while verification walks the contiguous run of records. JSZip's readCentralDir loops on the record signature and keeps every entry it finds — a count mismatch is explicitly not an error there — so an archive that under-reported its count could hide honestly-large entries from the total-size cap and still have the parser expand them. The sum now walks the same contiguous run as the verification pass and readZipCentralDirectoryStats, and fails closed when the run is shorter than the declared count. Caught by Cursor Bugbot review.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bea4e75. Configure here.
1 participant
Summary
Closes a zip-bomb memory-exhaustion path on document upload, in two parts.
1.
.docskipped the guard entirely.DocParserhanded the raw upload straight toofficeparserand thenmammoth, without theassertOoxmlArchiveWithinLimitscall its docx/pptx/xlsx siblings all make. The extension is only a routing hint — both libraries accept an OOXML container regardless of its name — so a bomb renamed.docselected the unguarded parser and bypassed the guard that catches the identical bytes as.docx. The guard is now the first act ofDocParser.parseBuffer, and is also enforced centrally inlib/file-parsersparseBufferso a future parser can't silently opt out.2. The guard itself was bypassable by lying. The declared uncompressed sizes live in the ZIP central directory, which is attacker-controlled. Under-report them and the size and ratio checks pass untouched;
officeparser/mammothonly notice the mismatch after inflating each entry in full. Measured on a 498 KB archive declaring 1000 bytes per entry:.doc.docx.xlsx/.pptxEvery entry is now inflated during verification under a
maxOutputLengthbound equal to the size it declared. Node's zlib aborts the moment output would exceed that bound, so a lying entry costs only its declared size and the bytes are discarded immediately. Stored entries are checked against their own compressed size; unsupported compression methods fail closed. Verification walks the contiguous run of central-directory records rather than the EOCD's declared count, since that run is what a decompression library actually allocates per entry — a lied-down count must not hide an entry from verification.No limits changed: this reuses the existing
DEFAULT_OOXML_SIZE_LIMITSall four parsers already ship against.Type of Change
Testing
Both bomb variants (honest and under-declared) are now rejected at +0 MB across
.doc,.docx,.xlsx,.pptx, and.txt. Verified the new tests go red when the verification pass is disabled (4 fail) and when the.docguard call is removed (3 fail).No false positives, checked against real files rather than only synthetic ones:
.docxfixtures frommammoth's test data still accepted.docx/.doc/.pptx, and legacy OLE.docall parse unchangedmaxOutputLengthboundary confirmed to accept output equal to the cap, so honest archives are never rejectedCost: ~0.45 ms per MB of uncompressed content (1.4 ms @ 1 MB, 5.3 ms @ 10 MB, 22 ms @ 50 MB), against parse times an order of magnitude larger. The declared-size check still runs first, so a straightforward bomb is rejected at zero cost.
Gates:
tscclean,check:api-validationpassed, biome clean, 61 file-parser tests plus 1495 tests across uploads / knowledge / files API / copilot. (8lib/copilot/**test files fail to collect on a PostCSS/Tailwind config error — confirmed identical with this branch's changes fully reverted, so pre-existing and unrelated.)Checklist