From 430f46ecbe40f3ae603bec6e66b16e034d79d149 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Wed, 29 Jul 2026 04:16:12 +0200 Subject: [PATCH] fix(import): charge each archive entry against the size budget once A ChatGPT run reads every shard twice, once to scan for assets and once to convert. Charging both passes meant a legitimate export holding more than half the decompressed limit failed partway through the second pass with a zip-bomb error, despite passing the same limit at index time. The budget bounds how much distinct data the archive can yield; a re-read yields nothing new and each read is still bounded by the per-entry cap. The aggregate guard still catches an archive that understates its sizes, which is now tested by doctoring a central directory rather than by re-reading. --- packages/api/src/import/archive.spec.ts | 68 +++++++++++++++++++++++-- packages/api/src/import/archive.ts | 60 ++++++++++++++++------ 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/packages/api/src/import/archive.spec.ts b/packages/api/src/import/archive.spec.ts index 50257ef89f..077f7faf2e 100644 --- a/packages/api/src/import/archive.spec.ts +++ b/packages/api/src/import/archive.spec.ts @@ -57,6 +57,24 @@ function buildVariedJson(entryCount: number): string { return JSON.stringify(records); } +/** + * Rewrites every central-directory record's uncompressed-size field to zero, + * leaving the actual entry data intact. yauzl indexes from the central + * directory, so this is how an archive lies about how much it will inflate to. + */ +function zeroDeclaredSizes(filepath: string): void { + const buffer = fs.readFileSync(filepath); + const CENTRAL_SIGNATURE = 0x02014b50; + const UNCOMPRESSED_SIZE_OFFSET = 24; + + for (let i = 0; i <= buffer.length - 4; i++) { + if (buffer.readUInt32LE(i) === CENTRAL_SIGNATURE) { + buffer.writeUInt32LE(0, i + UNCOMPRESSED_SIZE_OFFSET); + } + } + fs.writeFileSync(filepath, buffer); +} + afterEach(() => { while (createdDirs.length > 0) { const dir = createdDirs.pop(); @@ -105,12 +123,56 @@ describe('openArchive', () => { archive.close(); }); - it('accumulates actual decompressed bytes across repeated reads and rejects once the aggregate cap is exceeded', async () => { + /** + * The index-time sum uses the central directory's declared sizes, which an + * archive controls. Zeroing them walks the whole archive past that check, so + * the only thing standing between a lying archive and unbounded output is + * the real byte count accumulated across reads. + */ + it('accumulates real decompressed bytes even when the archive understates them', async () => { + /** DEFLATE, because yauzl requires a STORED entry's compressed and + * uncompressed sizes to match and would reject the doctored header. */ + const filepath = await writeZipEntries([ + { name: 'a.json', content: 'x'.repeat(60), compression: 'DEFLATE' }, + { name: 'b.json', content: 'y'.repeat(60), compression: 'DEFLATE' }, + ]); + zeroDeclaredSizes(filepath); + const archive = await openArchive(filepath, { maxTotalBytes: 100 }); + + expect(archive.entries.every((entry) => entry.bytes === 0)).toBe(true); + await archive.read('a.json'); + await expect(archive.read('b.json')).rejects.toThrow(ZipBombError); + + archive.close(); + }); + + /** + * A ChatGPT run reads every shard twice — once to scan for assets, once to + * convert. Charging both passes made a legitimate export over half the cap + * fail partway through the second one with a zip-bomb error. The cap bounds + * how much distinct data the archive can yield; a re-read yields nothing new + * and is still bounded individually by the per-entry cap. + */ + it('charges an entry once however many times it is read', async () => { const filepath = await writeZip({ 'a.json': 'x'.repeat(60) }); const archive = await openArchive(filepath, { maxTotalBytes: 100 }); - await archive.read('a.json'); - await expect(archive.read('a.json')).rejects.toThrow(ZipBombError); + await expect(archive.read('a.json')).resolves.toHaveLength(60); + await expect(archive.read('a.json')).resolves.toHaveLength(60); + await expect(archive.read('a.json')).resolves.toHaveLength(60); + + archive.close(); + }); + + it('charges a bare JSON upload once however many times it is read', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lc-import-reread-')); + createdDirs.push(dir); + const filepath = path.join(dir, 'export.json'); + fs.writeFileSync(filepath, 'x'.repeat(60)); + const archive = await openArchive(filepath, { maxTotalBytes: 100 }); + + await expect(archive.read('export.json')).resolves.toHaveLength(60); + await expect(archive.read('export.json')).resolves.toHaveLength(60); archive.close(); }); diff --git a/packages/api/src/import/archive.ts b/packages/api/src/import/archive.ts index 97d2b4c681..7be7c564d5 100644 --- a/packages/api/src/import/archive.ts +++ b/packages/api/src/import/archive.ts @@ -47,11 +47,39 @@ export interface Archive { type ArchiveLimits = Required; -/** Actual decompressed bytes delivered so far, shared by every `read()` - * call on one archive instance so the aggregate cap is enforced against - * real bytes rather than the (spoofable) central directory total. */ +/** + * Actual decompressed bytes delivered so far, shared by every `read()` call on + * one archive instance so the aggregate cap is enforced against real bytes + * rather than the (spoofable) central directory total. + * + * Counted once per distinct entry. A ChatGPT run reads every shard twice — + * once to scan for assets, once to convert — and charging both passes made a + * legitimate export over half the cap fail partway through the second one with + * a zip-bomb error. What the cap bounds is how much distinct data the archive + * can yield; re-reading an entry yields nothing new, and each read is still + * bounded individually by the per-entry cap. + */ interface ArchiveTotals { bytesRead: number; + counted: Set; +} + +/** Charges an entry's decompressed size against the aggregate budget the + * first time that entry is read, and throws once the budget is exceeded. */ +function chargeEntry( + name: string, + bytes: number, + limits: ArchiveLimits, + totals: ArchiveTotals, +): void { + if (totals.counted.has(name)) { + return; + } + totals.counted.add(name); + totals.bytesRead += bytes; + if (totals.bytesRead > limits.maxTotalBytes) { + throw new ZipBombError('Archive exceeds the maximum decompressed size'); + } } export function assertSafeName(name: string): void { @@ -306,22 +334,27 @@ async function openSingleFileArchive( readStream.on('data', (chunk: Buffer | string) => { const buffer = typeof chunk === 'string' ? Buffer.from(chunk) : chunk; bytes += buffer.byteLength; - totals.bytesRead += buffer.byteLength; if (bytes > limits.maxEntryBytes) { reject(new ZipBombError(`Entry ${entryName} exceeds the maximum decompressed size`)); readStream.destroy(); return; } - if (totals.bytesRead > limits.maxTotalBytes) { - reject(new ZipBombError('Archive exceeds the maximum decompressed size')); - readStream.destroy(); - return; - } chunks.push(buffer); }); readStream.on('error', reject); - readStream.on('end', () => resolve(Buffer.concat(chunks))); + readStream.on('end', () => { + /** Charged on completion, and only for the first read of this entry: + * the conversion pass re-reads what the scan already read, and + * charging both made a legitimate large export fail partway through. */ + try { + chargeEntry(entryName, bytes, limits, totals); + } catch (error) { + reject(error); + return; + } + resolve(Buffer.concat(chunks)); + }); }); } @@ -344,7 +377,7 @@ export async function openArchive( ), maxTotalBytes: options.maxTotalBytes ?? DEFAULT_MAX_TOTAL_BYTES, }; - const totals: ArchiveTotals = { bytesRead: 0 }; + const totals: ArchiveTotals = { bytesRead: 0, counted: new Set() }; if (!(await isZipFile(filepath))) { return openSingleFileArchive(filepath, limits, totals); @@ -376,10 +409,7 @@ export async function openArchive( const output = entry.compressionMethod === 0 ? raw : await inflateEntry(raw, name, limits.maxEntryBytes); - totals.bytesRead += output.byteLength; - if (totals.bytesRead > limits.maxTotalBytes) { - throw new ZipBombError('Archive exceeds the maximum decompressed size'); - } + chargeEntry(name, output.byteLength, limits, totals); return output; }