fix(copilot): guard document-style extraction against zip bombs - #6176
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview That closes the same zip-bomb / memory-exhaustion gap left after document-parser hardening: this path still inflated attacker-controlled OOXML parts (theme, styles, presentation XML) with no declared-size bounds. Guard failures stay inside the existing New tests build malformed archives byte-by-byte (lying declared uncompressed sizes) and assert Reviewed by Cursor Bugbot for commit 52c52be. Configure here. |
Greptile SummaryThe PR protects document-style extraction from malicious OOXML archives before they reach JSZip.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported expensive fixture now uses a small payload with only an overridden declared size, and the guard rejects it before decompression.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/vfs/document-style.ts | Applies the shared archive guard before JSZip loads attacker-controlled DOCX or PPTX data. |
| apps/sim/lib/copilot/vfs/document-style.test.ts | Covers normal extraction and verifies oversized or implausibly compressed archives never reach JSZip without constructing large payloads. |
Reviews (2): Last reviewed commit: "test(copilot): declare the ratio-test ex..." | Re-trigger Greptile
extractDocumentStyle handed an attacker-controlled archive straight to JSZip and inflated named parts (word/theme/theme1.xml, word/styles.xml, ppt/presentation.xml, ppt/slideMasters/slideMaster1.xml) with no size bound, reachable from GET /api/workspaces/[id]/files/[fileId]/style and from the workspace VFS. Reading only a few entries is no protection: the bomb just has to live at one of those paths. It now calls assertOoxmlArchiveWithinLimits, the same guard the document parsers use, and the hand-rolled ZIP_MAGIC check is replaced by isZipShaped from that module — zip-guard is already shared this way by lib/uploads/archive.ts and lib/copilot/tools/handlers/upload-file-reader.ts. Checking each entry's size through JSZip instead would have been cheaper, since only a handful of parts are read, but JSZip reports the size the archive declares — the same attacker-controlled field a bomb lies about — so it would need to re-derive the guard's verification to be sound. The guard sits inside the existing try, so a rejection logs and returns null: the route already answers 422 and the VFS already returns null when no summary can be produced, and neither caller changes.
The compression-ratio case built a real 400 MiB string and deflated it synchronously, costing the parallel test runner memory and CPU for no added coverage. The guard reads the total the archive declares, so declaring the expansion exercises the same ratio path with a few-hundred-byte fixture. Still fails when the guard call is removed, and the file now runs in 286 ms instead of seconds. Caught by Greptile review.
d71466d to
52c52be
Compare
|
@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 52c52be. Configure here.
Summary
Follow-up to #6166, which fixed the document-parser paths.
extractDocumentStylewas the remaining unguarded ZIP consumer.JSZip.loadAsyncand inflated named parts (word/theme/theme1.xml,word/styles.xml,ppt/presentation.xml,ppt/slideMasters/slideMaster1.xml) with no size boundGET /api/workspaces/[id]/files/[fileId]/styleand from the workspace VFS (workspace-vfs.ts:1233)It now calls
assertOoxmlArchiveWithinLimits— the same guard the document parsers use — and the hand-rolledZIP_MAGICbyte check is replaced byisZipShapedfrom that module.zip-guardis already shared this way bylib/uploads/archive.tsandlib/copilot/tools/handlers/upload-file-reader.ts, so this is the established pattern rather than new coupling. Net effect is a deleted bespoke check plus the protection.Why not a cheaper targeted check
Bounding only the entries this path actually reads would be less work, but JSZip reports the size the archive declares — the same attacker-controlled field a bomb lies about, which is exactly the bypass #6166 had to fix. A targeted check would have to re-derive that verification to be sound, so reusing the shared guard is both cleaner and the only correct option short of duplicating it.
Error behavior
The guard sits inside the existing
try, so a rejection is logged (the guard warns with details before throwing) and the function returnsnull. The route already answers 422 with a descriptive message and the VFS already returnsnullwhen no summary can be produced — neither caller changes.Type of Change
Testing
The return value alone cannot demonstrate this fix: JSZip also rejects these archives, but only after inflating the entry — which is the memory cost the guard exists to avoid. The tests therefore assert JSZip is never handed the buffer, mirroring the approach in #6166's
doc-parsertests..docxfixtures frommammoth's test data are accepted by the guard; 13 still yield a style summary and the 4 that returnnullare pre-existing (empty.docx,endnotes.docx,external-picture.docx,utf8-bom.docxhave notheme1.xml) — confirmed each withguard=acceptsGates:
tscclean,check:api-validationpassed, biome clean, 124 tests acrosslib/copilot/vfsandlib/file-parsers.Checklist