From d65ee4f89dc390ffb029caa4ad788c6c7b1ba807 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Thu, 1 Oct 2026 21:39:26 -0400 Subject: [PATCH] fix(preview): parse admission tag attributes in order and count empty rows XML allows a raw `>` and the other quote character inside an attribute value, so a first-match search for `ref=`/`max=` could be fed a fake value from an earlier attribute while saxes read the real one: - / attributes are now read in order from the tag name with a sticky regex that consumes each quoted value whole. A tag whose attributes do not parse up to `>`, or that repeats a name, is refused. - The chunk carry keeps everything from the last `<`, which can never appear inside an attribute value, instead of comparing against the last `>`. ExcelJS keeps a Row object for every , cells or not, so tags now count against per-sheet (100k) and total (250k) caps with their own row-limit code, and the worker passes maxRows as a per-sheet backstop. styles.xml counts every without tracking which list it sits in, since a inside a comment desynced that state. --- docs/architecture-invariants.md | 2 +- src/web/public/spreadsheet-preview-worker.js | 8 +- src/web/public/spreadsheet-preview.js | 2 +- src/web/public/spreadsheet-xlsx-core.js | 84 ++++++++++++++------ test/spreadsheet-preview-worker.test.ts | 52 ++++++++++++ test/spreadsheet-xlsx-core.test.ts | 43 +++++++++- 6 files changed, 160 insertions(+), 31 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 221c7e01..fd313262 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -362,7 +362,7 @@ A file path an agent prints is a link on both surfaces it can appear on, and cli ⚠️ **The preview overlay must outrank the panel that launched it.** `.file-preview-overlay` sits at `z-index: 5100`, above the response viewer (5000) and its backdrop (4999); at its historical 2000 a path clicked in the chat opened the overlay *behind* the chat, which reads as a dead link. It stays below the toast/picker band (10000+) so a "Saved" toast still lands on top. -**XLSX previews parse in the browser worker, and ExcelJS only ever sees what admission checked.** An `.xlsx` joins the raw routes like any other file (`?preview=true` caps it at 10 MB with a 413); the server never parses it. `spreadsheet-preview.js` hands the bytes to `spreadsheet-preview-worker.js`, the ONLY place the pinned `exceljs`/`fflate` vendor bundles load: fflate and `spreadsheet-xlsx-core.js` at worker start, ExcelJS only after `admitXlsx()` passes, and the page itself never loads either. `admitXlsx()` streams every entry through fflate and enforces the entry count, per-entry and total inflated bytes (64 MB), compression ratio, worksheet, cell, merge and style caps on the actual inflated output, refusing (never truncating) a workbook that trips one. ⚠️ Admission walks LOCAL headers while ExcelJS (JSZip) reads the CENTRAL directory, so overlapping entries (a stored entry hiding a whole `sheet1.xml`, a one-cell decoy later in the stream) once let a file be admitted as 1 cell and parsed as 300k. The worker therefore never hands ExcelJS the fetched bytes: it gets `buildAdmittedArchive()`, a STORE-only `fflate.zipSync(entries, { level: 0 })` of exactly the entries admission inflated, and a name streamed twice is refused. That costs one transient copy of the inflated entries (bounded by the same 64 MB cap) and is pinned by the overlapping-entry fixture in `test/spreadsheet-preview-worker.test.ts`. ⚠️ Admission also has to bound what ExcelJS EXPANDS, not just what it reads: ExcelJS 4.4.0 builds one object per covered cell of a ``, per address of a `` and per index up to ``, so a 6.5 KB file admitted as one cell once cost a gigabyte. The counter therefore charges each merge its full AREA against the cell caps (an unparseable `ref` is refused), refuses a `` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). The counter reads tags whole across inflate-chunk edges (it scans only up to the last complete tag and carries the rest), and a central-directory `compressedSize` that runs past the file is refused, since the ratio cap divides by it. Fixtures for each are in `test/spreadsheet-preview-worker.test.ts` and `test/spreadsheet-xlsx-core.test.ts`. The worker is a stable URL cache-busted by `SPREADSHEET_ASSET_VERSION` in `spreadsheet-preview.js`, the content hash of the worker, the core and both vendor bundles; `npm run check:public-assets` fails when it drifts, so editing any of those (or bumping either package) means updating the token, or a deploy pairs a new worker with a year-cached old core. +**XLSX previews parse in the browser worker, and ExcelJS only ever sees what admission checked.** An `.xlsx` joins the raw routes like any other file (`?preview=true` caps it at 10 MB with a 413); the server never parses it. `spreadsheet-preview.js` hands the bytes to `spreadsheet-preview-worker.js`, the ONLY place the pinned `exceljs`/`fflate` vendor bundles load: fflate and `spreadsheet-xlsx-core.js` at worker start, ExcelJS only after `admitXlsx()` passes, and the page itself never loads either. `admitXlsx()` streams every entry through fflate and enforces the entry count, per-entry and total inflated bytes (64 MB), compression ratio, worksheet, cell, merge and style caps on the actual inflated output, refusing (never truncating) a workbook that trips one. ⚠️ Admission walks LOCAL headers while ExcelJS (JSZip) reads the CENTRAL directory, so overlapping entries (a stored entry hiding a whole `sheet1.xml`, a one-cell decoy later in the stream) once let a file be admitted as 1 cell and parsed as 300k. The worker therefore never hands ExcelJS the fetched bytes: it gets `buildAdmittedArchive()`, a STORE-only `fflate.zipSync(entries, { level: 0 })` of exactly the entries admission inflated, and a name streamed twice is refused. That costs one transient copy of the inflated entries (bounded by the same 64 MB cap) and is pinned by the overlapping-entry fixture in `test/spreadsheet-preview-worker.test.ts`. ⚠️ Admission also has to bound what ExcelJS EXPANDS, not just what it reads: ExcelJS 4.4.0 builds one object per covered cell of a ``, per address of a `` and per index up to ``, so a 6.5 KB file admitted as one cell once cost a gigabyte. The counter therefore charges each merge its full AREA against the cell caps (an unparseable `ref` is refused), refuses a `` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). The counter reads ``/`` attributes IN ORDER from the tag name with a sticky regex that consumes each quoted value whole (a raw `>` or the other quote character is legal inside a value, so a first-match search could be fed a fake `ref`/`max`), and refuses a tag whose attributes do not parse up to `>` or repeat a name. It counts every `` (ExcelJS keeps one Row object per element, cells or not) against per-sheet and total row caps, and every `` in styles.xml with no list-tracking state (a `` inside a comment used to desync it). It carries everything from the last `<` into the next inflate chunk, since `<` can never appear inside an attribute value, and a central-directory `compressedSize` that runs past the file is refused, since the ratio cap divides by it. Fixtures for each are in `test/spreadsheet-preview-worker.test.ts` and `test/spreadsheet-xlsx-core.test.ts`. The worker is a stable URL cache-busted by `SPREADSHEET_ASSET_VERSION` in `spreadsheet-preview.js`, the content hash of the worker, the core and both vendor bundles; `npm run check:public-assets` fails when it drifts, so editing any of those (or bumping either package) means updating the token, or a deploy pairs a new worker with a year-cached old core. ### Filesystem path picker diff --git a/src/web/public/spreadsheet-preview-worker.js b/src/web/public/spreadsheet-preview-worker.js index 51978a52..a1c7d7e2 100644 --- a/src/web/public/spreadsheet-preview-worker.js +++ b/src/web/public/spreadsheet-preview-worker.js @@ -127,8 +127,12 @@ async function loadWorkbook(bytes) { const nextWorkbook = new self.ExcelJS.Workbook(); // ExcelJS expands every address of a `` into its own // object (a whole-column dropdown is a million), and the preview never shows - // validations, so they are not parsed at all. - await nextWorkbook.xlsx.load(admitted, { ignoreNodes: ['dataValidations'] }); + // validations, so they are not parsed at all. `maxRows` is a per-sheet + // backstop behind admission's row count, which also caps the workbook total. + await nextWorkbook.xlsx.load(admitted, { + ignoreNodes: ['dataValidations'], + maxRows: core.LIMITS.maxRowsPerSheet, + }); const nextSheets = new Map(); const nextRows = new Map(); normalizedStyles = []; diff --git a/src/web/public/spreadsheet-preview.js b/src/web/public/spreadsheet-preview.js index 1f40b864..26c53b6f 100644 --- a/src/web/public/spreadsheet-preview.js +++ b/src/web/public/spreadsheet-preview.js @@ -20,7 +20,7 @@ (function initSpreadsheetPreview(global) { 'use strict'; - const SPREADSHEET_ASSET_VERSION = '078d54b1055a'; + const SPREADSHEET_ASSET_VERSION = 'ac0c75e6303e'; const MAX_PREVIEW_BYTES = 10 * 1024 * 1024; const DEFAULT_TIMEOUT_MS = 20000; const MAX_SCROLL_PX = 8000000; diff --git a/src/web/public/spreadsheet-xlsx-core.js b/src/web/public/spreadsheet-xlsx-core.js index 474922a0..5302d088 100644 --- a/src/web/public/spreadsheet-xlsx-core.js +++ b/src/web/public/spreadsheet-xlsx-core.js @@ -23,6 +23,8 @@ maxWorksheets: 50, maxCells: 250000, maxCellsPerSheet: 100000, + maxRows: 250000, + maxRowsPerSheet: 100000, maxMergesPerSheet: 5000, maxStyles: 5000, }); @@ -125,24 +127,49 @@ // from turning the carried tail into quadratic re-scanning. const MAX_CARRIED_TAG = 256 * 1024; - function attribute(tag, name) { - const match = new RegExp(`\\s${name}\\s*=\\s*(?:"([^"]*)"|'([^']*)')`).exec(tag); - return match ? (match[1] ?? match[2]) : null; + // XML attribute syntax, read in order from just after the tag name. A quoted + // value may hold a raw `>` or the other quote character, so a value is always + // consumed whole; a tag is only understood if this walk reaches its `>`. + const XML_SPACE = '[ \\t\\r\\n]'; + const ATTRIBUTE = new RegExp( + `${XML_SPACE}+([^ \\t\\r\\n=/>]+)${XML_SPACE}*=${XML_SPACE}*(?:"([^"]*)"|'([^']*)')`, + 'y' + ); + const TAG_END = new RegExp(`${XML_SPACE}*/?>`, 'y'); + + /** + * Parse the attributes of the tag whose name ends at `from` in `text`. + * Returns the attributes, or null when they do not parse cleanly up to the + * tag's closing `/>` or `>` (or a name repeats, which XML forbids). + */ + function readTagAttributes(text, from) { + const attributes = new Map(); + let at = from; + for (;;) { + ATTRIBUTE.lastIndex = at; + const match = ATTRIBUTE.exec(text); + if (!match) break; + if (attributes.has(match[1])) return null; + attributes.set(match[1], match[2] ?? match[3]); + at = ATTRIBUTE.lastIndex; + } + TAG_END.lastIndex = at; + return TAG_END.test(text) ? attributes : null; } // ExcelJS expands a merge into one cell object per covered cell at load time, // so a merge costs its AREA, not one tag. - function mergeArea(tag) { - const range = parseRange(attribute(tag, 'ref')); + function mergeArea(attributes) { + const range = parseRange(attributes.get('ref')); if (!range) fail('malformed', 'Worksheet has a merged range that does not parse'); return (Math.abs(range.r2 - range.r1) + 1) * (Math.abs(range.c2 - range.c1) + 1); } // ExcelJS builds one column object for every index up to ``, unclamped. - function checkColumnSpan(tag) { + function checkColumnSpan(attributes) { for (const name of ['min', 'max']) { - const value = attribute(tag, name); - if (value === null) continue; + const value = attributes.get(name); + if (value === undefined) continue; const index = Number(value); if (!Number.isInteger(index) || index < 1 || index > MAX_COL) { fail('malformed', `Worksheet column ${name} is outside 1-${MAX_COL}`); @@ -155,7 +182,7 @@ const decoder = new TextDecoder(); let sheetCells = 0; let sheetMerges = 0; - let inCellXfs = false; + let sheetRows = 0; const worksheet = /^xl\/worksheets\/[^/]+\.xml$/i.test(name); const styles = name === 'xl/styles.xml'; const addCells = (cells) => { @@ -168,32 +195,37 @@ push(chunk, final) { if (!worksheet && !styles) return; const text = tail + decoder.decode(chunk, { stream: !final }); - // Scan only up to a tag boundary: a tag cut by a chunk edge is carried - // whole into the next scan, so its attributes are read in one piece. - let safeEnd = text.length; - if (!final) { - const open = text.lastIndexOf('<'); - if (open > text.lastIndexOf('>')) safeEnd = open; - } + // `<` can never appear inside an attribute value, so every tag before the + // last `<` is complete. Carry everything from that `<` into the next scan + // so a tag cut by a chunk edge is always read in one piece. + const safeEnd = final ? text.length : Math.max(0, text.lastIndexOf('<')); const scan = text.slice(0, safeEnd); if (worksheet) { addCells((scan.match(/)/g) || []).length); - for (const [tag] of scan.matchAll(/]*)?>/g)) { + // ExcelJS keeps a Row object for every , with or without cells. + const rows = (scan.match(/])/g) || []).length; + sheetRows += rows; + counts.rows += rows; + if (sheetRows > limits.maxRowsPerSheet || counts.rows > limits.maxRows) + fail('row-limit', 'Workbook exceeds the rows limit'); + for (const match of scan.matchAll(/<(mergeCell|col)(?=[\s/>])/g)) { + const attributes = readTagAttributes(scan, match.index + match[0].length); + if (!attributes) fail('malformed', `Worksheet has a <${match[1]}> whose attributes do not parse`); + if (match[1] === 'col') { + checkColumnSpan(attributes); + continue; + } sheetMerges += 1; counts.merges += 1; if (sheetMerges > limits.maxMergesPerSheet) fail('merge-limit', 'Worksheet exceeds the merged ranges limit'); - addCells(mergeArea(tag)); + addCells(mergeArea(attributes)); } - for (const [tag] of scan.matchAll(/]*)?>/g)) checkColumnSpan(tag); } if (styles) { - const tokens = scan.match(/)|<\/cellXfs\s*>|)/g) || []; - for (const token of tokens) { - if (token.startsWith(' counts, cellStyleXfs included: tracking which list a tag + // sits in can be desynced by a closing tag inside an XML comment. + counts.styles += (scan.match(/])/g) || []).length; if (counts.styles > limits.maxStyles) fail('style-limit', 'Workbook exceeds the cell styles limit'); } tail = text.slice(safeEnd); @@ -210,7 +242,7 @@ for (const entry of directory.entries) expectedEntries.set(entry.name, (expectedEntries.get(entry.name) || 0) + 1); const streamedEntries = new Map(); const inflatedEntries = Object.create(null); - const counts = { worksheets: 0, cells: 0, merges: 0, styles: 0 }; + const counts = { worksheets: 0, cells: 0, rows: 0, merges: 0, styles: 0 }; const features = new Set(); let totalInflated = 0; let seenEntries = 0; diff --git a/test/spreadsheet-preview-worker.test.ts b/test/spreadsheet-preview-worker.test.ts index 8986918d..e1812ee4 100644 --- a/test/spreadsheet-preview-worker.test.ts +++ b/test/spreadsheet-preview-worker.test.ts @@ -573,3 +573,55 @@ describe('spreadsheet preview worker: admission bounds what ExcelJS expands', () expect(tile.cells).toEqual([expect.objectContaining({ row: 1, col: 1, text: 'one' })]); }, 30_000); }); + +describe('spreadsheet preview worker: quoted attribute values and empty rows', () => { + // XML allows a raw `>` and the other quote character inside an attribute + // value. Each of these was admitted before and made ExcelJS build millions of + // cells or columns. + it.each([ + [ + 'a quoted fake ref before the real merge ref', + 'after-sheetData', + ``, + 'cell-limit', + ], + [ + 'a quoted > before the real col max', + 'before-sheetData', + '', + 'malformed', + ], + [ + 'a quoted fake max before the real col max', + 'before-sheetData', + ``, + 'malformed', + ], + ] as const)('refuses %s before ExcelJS loads', async (_label, where, xml, code) => { + const harness = createHarness(); + await harness.send({ type: 'load', bytes: await sheetWithInjectedXml(where, xml) }); + expect(harness.messages.at(-1)).toMatchObject({ type: 'error', code }); + expect(harness.imports.some((url) => url.includes('exceljs'))).toBe(false); + }); + + // ExcelJS keeps a Row object for every , so cell-less rows cost memory + // too: three sheets of a million empty rows sat inside every cell cap. + it('refuses a sheet of empty rows past the row cap before ExcelJS loads', async () => { + const harness = createHarness(); + const rows = Array.from({ length: 100_001 }, (_, i) => ``).join(''); + await harness.send({ type: 'load', bytes: await emptyRowsWorkbook(rows) }); + expect(harness.messages.at(-1)).toMatchObject({ type: 'error', code: 'row-limit' }); + expect(harness.imports.some((url) => url.includes('exceljs'))).toBe(false); + }, 30_000); +}); + +async function emptyRowsWorkbook(extraRows: string): Promise { + const workbook = new ExcelJS.Workbook(); + workbook.addWorksheet('Data').getCell('A1').value = 'one'; + const entries = fflate.unzipSync(new Uint8Array(await workbook.xlsx.writeBuffer())); + const sheet = fflate.strFromU8(entries['xl/worksheets/sheet1.xml']); + const patched = sheet.replace('', `${extraRows}`); + expect(patched).not.toBe(sheet); + entries['xl/worksheets/sheet1.xml'] = fflate.strToU8(patched); + return toArrayBuffer(fflate.zipSync(entries)); +} diff --git a/test/spreadsheet-xlsx-core.test.ts b/test/spreadsheet-xlsx-core.test.ts index ba5bc2a2..5fa8487d 100644 --- a/test/spreadsheet-xlsx-core.test.ts +++ b/test/spreadsheet-xlsx-core.test.ts @@ -113,7 +113,7 @@ describe('spreadsheet XLSX core', () => { features: string[]; }; // The 2x2 merge costs its four covered cells on top of the one real cell. - expect(result.counts).toEqual({ worksheets: 1, cells: 5, merges: 1, styles: 1 }); + expect(result.counts).toEqual({ worksheets: 1, cells: 5, rows: 0, merges: 1, styles: 1 }); expect(result.features).toEqual(expect.arrayContaining(['charts', 'externalLinks'])); }); @@ -182,6 +182,7 @@ describe('spreadsheet XLSX core', () => { const pad = ''.repeat(12); const cases: Array<[string, RegExp]> = [ [`${pad}${pad}`, /column max/i], + [`${pad}${pad}`, /column max/i], [`${pad}${pad}`, /cells limit/i], ]; for (const [xml, pattern] of cases) { @@ -200,6 +201,46 @@ describe('spreadsheet XLSX core', () => { } }); + it('reads attributes in order, so a quoted value cannot hide or fake one', () => { + const counter = () => + core.createXmlCounter( + 'xl/worksheets/sheet1.xml', + { cells: 0, merges: 0, styles: 0, rows: 0 } as never, + core.LIMITS + ); + const push = (xml: string) => counter().push(fflate.strToU8(xml), true); + // A raw `>` or the other quote character is legal inside a value. + expect(() => push(``)).toThrowError(/cells limit/i); + expect(() => push('')).toThrowError(/column max/i); + expect(() => push(``)).toThrowError(/column max/i); + // Anything the walk cannot read up to `>` is refused, as is a repeated name. + expect(() => push('')).toThrowError(/do not parse/i); + expect(() => push('')).toThrowError(/do not parse/i); + expect(() => push('')).not.toThrow(); + }); + + it('counts every , empty or not, against per-sheet and total caps', () => { + const rows = (n: number) => '' + ''.repeat(n) + ''; + expect(() => core.admitXlsx(workbookZip(rows(4)), fflate, { maxRowsPerSheet: 3 })).toThrowError(/rows limit/i); + expect(() => core.admitXlsx(workbookZip(rows(4)), fflate, { maxRows: 3 })).toThrowError(/rows limit/i); + const admitted = core.admitXlsx(workbookZip(rows(3)), fflate, { maxRowsPerSheet: 3 }) as { + counts: { rows: number }; + }; + expect(admitted.counts.rows).toBe(3); + // / style names are not rows. + expect( + (core.admitXlsx(workbookZip(''), fflate) as { counts: { rows: number } }) + .counts.rows + ).toBe(0); + }); + + it('counts every in styles.xml, so a inside a comment cannot hide styles', () => { + const counts = { cells: 0, merges: 0, styles: 0, rows: 0 }; + const counter = core.createXmlCounter('xl/styles.xml', counts as never, { ...core.LIMITS, maxStyles: 100 }); + const xml = '' + ''.repeat(200) + ''; + expect(() => counter.push(fflate.strToU8(xml), true)).toThrowError(/styles limit/i); + }); + it('refuses an entry whose declared compressed size runs past the file', () => { const zip = workbookZip(); const view = new DataView(zip.buffer, zip.byteOffset, zip.byteLength);