From a917b147e797b24b3d79c1a8f414fbeab7d95c1a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:43:33 -0700 Subject: [PATCH 1/2] fix(copilot): guard document-style extraction against zip bombs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../lib/copilot/vfs/document-style.test.ts | 163 ++++++++++++++++++ apps/sim/lib/copilot/vfs/document-style.ts | 15 +- 2 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 apps/sim/lib/copilot/vfs/document-style.test.ts diff --git a/apps/sim/lib/copilot/vfs/document-style.test.ts b/apps/sim/lib/copilot/vfs/document-style.test.ts new file mode 100644 index 00000000000..e50540f9221 --- /dev/null +++ b/apps/sim/lib/copilot/vfs/document-style.test.ts @@ -0,0 +1,163 @@ +/** + * @vitest-environment node + */ +import { deflateRawSync } from 'zlib' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockLoadAsync } = vi.hoisted(() => ({ mockLoadAsync: vi.fn() })) + +vi.mock('jszip', () => ({ default: { loadAsync: mockLoadAsync } })) + +import { extractDocumentStyle } from '@/lib/copilot/vfs/document-style' + +const THEME_XML = ` + + + + + + + + + + + + +` + +interface ZipEntryInput { + name: string + content: string + /** Overrides the uncompressed size written to both headers, to model a lying archive. */ + declaredUncompressedSize?: number +} + +/** + * Emit a ZIP archive byte by byte. Generating one with a library and patching it + * afterwards cannot express a declared size that never matched the payload, and + * that divergence is the whole subject of these tests. + */ +function buildZip(entries: ZipEntryInput[]): Buffer { + const locals: Buffer[] = [] + const centrals: Buffer[] = [] + let offset = 0 + + for (const entry of entries) { + const name = Buffer.from(entry.name, 'utf8') + const raw = Buffer.from(entry.content, 'utf8') + const deflated = deflateRawSync(raw) + const declared = entry.declaredUncompressedSize ?? raw.length + + const local = Buffer.alloc(30 + name.length) + local.writeUInt32LE(0x04034b50, 0) + local.writeUInt16LE(20, 4) + local.writeUInt16LE(8, 8) // deflate + local.writeUInt32LE(0, 14) // crc32 — nothing under test validates it + local.writeUInt32LE(deflated.length, 18) + local.writeUInt32LE(declared, 22) + local.writeUInt16LE(name.length, 26) + name.copy(local, 30) + locals.push(local, deflated) + + const central = Buffer.alloc(46 + name.length) + central.writeUInt32LE(0x02014b50, 0) + central.writeUInt16LE(20, 4) + central.writeUInt16LE(20, 6) + central.writeUInt16LE(8, 10) // deflate + central.writeUInt32LE(0, 16) + central.writeUInt32LE(deflated.length, 20) + central.writeUInt32LE(declared, 24) + central.writeUInt16LE(name.length, 28) + central.writeUInt32LE(offset, 42) + name.copy(central, 46) + centrals.push(central) + + offset += local.length + deflated.length + } + + const centralDirectory = Buffer.concat(centrals) + const eocd = Buffer.alloc(22) + eocd.writeUInt32LE(0x06054b50, 0) + eocd.writeUInt16LE(entries.length, 8) + eocd.writeUInt16LE(entries.length, 10) + eocd.writeUInt32LE(centralDirectory.length, 12) + eocd.writeUInt32LE(offset, 16) + + return Buffer.concat([...locals, centralDirectory, eocd]) +} + +/** Stand-in for a loaded JSZip archive, serving the parts the extractor asks for. */ +function fakeArchive(files: Record) { + return { + file: (path: string) => (files[path] ? { async: async () => files[path] } : null), + } +} + +describe('extractDocumentStyle', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('extracts theme colors and fonts from a well-formed docx', async () => { + mockLoadAsync.mockResolvedValue(fakeArchive({ 'word/theme/theme1.xml': THEME_XML })) + + const summary = await extractDocumentStyle( + buildZip([{ name: 'word/theme/theme1.xml', content: THEME_XML }]), + 'docx' + ) + + expect(mockLoadAsync).toHaveBeenCalledOnce() + expect(summary?.theme?.colors).toMatchObject({ + dk1: '000000', + lt1: 'FFFFFF', + accent1: '4472C4', + }) + expect(summary?.theme?.fonts).toEqual({ major: 'Calibri Light', minor: 'Calibri' }) + }) + + it('extracts theme data from a well-formed pptx', async () => { + mockLoadAsync.mockResolvedValue(fakeArchive({ 'ppt/theme/theme1.xml': THEME_XML })) + + const summary = await extractDocumentStyle( + buildZip([{ name: 'ppt/theme/theme1.xml', content: THEME_XML }]), + 'pptx' + ) + + expect(summary?.theme?.fonts.minor).toBe('Calibri') + }) + + it('never hands JSZip an archive declaring more expansion than the guard allows', async () => { + // JSZip would also fail this archive — but only after inflating the entry, + // which is the memory cost the guard exists to avoid. Asserting on the + // return value alone cannot tell the two apart, so assert JSZip is never + // reached. + const bomb = buildZip([ + { + name: 'word/theme/theme1.xml', + content: THEME_XML, + declaredUncompressedSize: 2 * 1024 * 1024 * 1024, + }, + ]) + + expect(await extractDocumentStyle(bomb, 'docx')).toBeNull() + expect(mockLoadAsync).not.toHaveBeenCalled() + }) + + it('never hands JSZip an archive with an implausible compression ratio', async () => { + const bomb = buildZip([ + { + name: 'word/theme/theme1.xml', + content: 'A'.repeat(400 * 1024 * 1024), + }, + ]) + + expect(await extractDocumentStyle(bomb, 'docx')).toBeNull() + expect(mockLoadAsync).not.toHaveBeenCalled() + }) + + it('returns null for a buffer that is not a ZIP archive', async () => { + expect(await extractDocumentStyle(Buffer.from('not an archive at all'), 'docx')).toBeNull() + expect(await extractDocumentStyle(Buffer.alloc(2), 'docx')).toBeNull() + expect(mockLoadAsync).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/copilot/vfs/document-style.ts b/apps/sim/lib/copilot/vfs/document-style.ts index 7edbe202fc2..29d2b120a83 100644 --- a/apps/sim/lib/copilot/vfs/document-style.ts +++ b/apps/sim/lib/copilot/vfs/document-style.ts @@ -1,11 +1,9 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' +import { assertOoxmlArchiveWithinLimits, isZipShaped } from '@/lib/file-parsers/zip-guard' const logger = createLogger('DocumentStyle') -// ZIP magic bytes: PK\x03\x04 -const ZIP_MAGIC = [0x50, 0x4b, 0x03, 0x04] - interface ThemeColors { dk1: string lt1: string @@ -389,12 +387,15 @@ export async function extractDocumentStyle( return extractPdfStyle(buffer) } - if (buffer.length < 4) return null - for (let i = 0; i < 4; i++) { - if (buffer[i] !== ZIP_MAGIC[i]) return null - } + if (!isZipShaped(buffer)) return null try { + // Reading a handful of named parts still means handing an attacker-controlled + // archive to JSZip, which inflates whatever those entries hold. Bound it with + // the same guard the document parsers use rather than trusting the entry + // sizes JSZip reports, which come from the archive itself. + assertOoxmlArchiveWithinLimits(buffer) + const JSZip = (await import('jszip')).default const zip = await JSZip.loadAsync(buffer) From 52c52be2340cb9ef674b8962971b42864ef05430 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:52:14 -0700 Subject: [PATCH 2/2] test(copilot): declare the ratio-test expansion instead of carrying it 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. --- apps/sim/lib/copilot/vfs/document-style.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/copilot/vfs/document-style.test.ts b/apps/sim/lib/copilot/vfs/document-style.test.ts index e50540f9221..372718b4703 100644 --- a/apps/sim/lib/copilot/vfs/document-style.test.ts +++ b/apps/sim/lib/copilot/vfs/document-style.test.ts @@ -144,10 +144,15 @@ describe('extractDocumentStyle', () => { }) it('never hands JSZip an archive with an implausible compression ratio', async () => { + // 400 MiB sits under the 1 GiB absolute cap, so this exercises the ratio + // check rather than the size check. Declaring the expansion rather than + // carrying it keeps the fixture a few hundred bytes — the guard reads the + // declared total, so a real payload would only cost the suite memory. const bomb = buildZip([ { name: 'word/theme/theme1.xml', - content: 'A'.repeat(400 * 1024 * 1024), + content: THEME_XML, + declaredUncompressedSize: 400 * 1024 * 1024, }, ])