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.
This commit is contained in:
Marco Beretta 2026-07-29 04:16:12 +02:00
parent 351a69a972
commit 430f46ecbe
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
2 changed files with 110 additions and 18 deletions

View file

@ -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();
});

View file

@ -47,11 +47,39 @@ export interface Archive {
type ArchiveLimits = Required<ArchiveOptions>;
/** 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<string>;
}
/** 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;
}