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:

- <mergeCell>/<col> 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 <row>, cells or not, so <row> 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 <xf> without tracking which list it sits in,
since a </cellXfs> inside a comment desynced that state.
This commit is contained in:
Aamer Akhter
2026-10-01 21:39:26 -04:00
parent e85b4f34dd
commit d65ee4f89d
6 changed files with 160 additions and 31 deletions
+1 -1
View File
@@ -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 `<mergeCell>`, per address of a `<dataValidation sqref>` and per index up to `<col max>`, 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 `<col>` 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 `<mergeCell>`, per address of a `<dataValidation sqref>` and per index up to `<col max>`, 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 `<col>` whose `min`/`max` is past 16384, and the worker loads with `ignoreNodes: ['dataValidations']` (the preview never shows validations). The counter reads `<mergeCell>`/`<col>` 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 `<row>` (ExcelJS keeps one Row object per element, cells or not) against per-sheet and total row caps, and every `<xf>` in styles.xml with no list-tracking state (a `</cellXfs>` 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
+6 -2
View File
@@ -127,8 +127,12 @@ async function loadWorkbook(bytes) {
const nextWorkbook = new self.ExcelJS.Workbook();
// ExcelJS expands every address of a `<dataValidation sqref>` 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 = [];
+1 -1
View File
@@ -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;
+58 -26
View File
@@ -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 `<col max>`, 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(/<c(?:\s|>)/g) || []).length);
for (const [tag] of scan.matchAll(/<mergeCell(?:\s[^>]*)?>/g)) {
// ExcelJS keeps a Row object for every <row>, with or without cells.
const rows = (scan.match(/<row(?=[\s/>])/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(/<col(?:\s[^>]*)?>/g)) checkColumnSpan(tag);
}
if (styles) {
const tokens = scan.match(/<cellXfs(?:\s|>)|<\/cellXfs\s*>|<xf(?:\s|\/?>)/g) || [];
for (const token of tokens) {
if (token.startsWith('<cellXfs')) inCellXfs = true;
else if (token.startsWith('</cellXfs')) inCellXfs = false;
else if (inCellXfs) counts.styles += 1;
}
// Every <xf> 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(/<xf(?=[\s/>])/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;
+52
View File
@@ -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',
`<mergeCells count="1"><mergeCell x=' ref="A1"' ref="A1:CV30000"/></mergeCells>`,
'cell-limit',
],
[
'a quoted > before the real col max',
'before-sheetData',
'<cols><col x=">" min="1" max="3000000"/></cols>',
'malformed',
],
[
'a quoted fake max before the real col max',
'before-sheetData',
`<cols><col x=' max="1"' min="1" max="3000000"/></cols>`,
'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 <row>, 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) => `<row r="${i + 2}"/>`).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<ArrayBuffer> {
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('</sheetData>', `${extraRows}</sheetData>`);
expect(patched).not.toBe(sheet);
entries['xl/worksheets/sheet1.xml'] = fflate.strToU8(patched);
return toArrayBuffer(fflate.zipSync(entries));
}
+42 -1
View File
@@ -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 = '<sheetView workbookViewId="0"/>'.repeat(12);
const cases: Array<[string, RegExp]> = [
[`<worksheet>${pad}<cols><col min="1" max="99999"/></cols>${pad}</worksheet>`, /column max/i],
[`<worksheet>${pad}<cols><col x=">" min="1" max="99999"/></cols>${pad}</worksheet>`, /column max/i],
[`<worksheet>${pad}<mergeCells><mergeCell ref="A1:CV30000"/></mergeCells>${pad}</worksheet>`, /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(`<mergeCell x=' ref="A1"' ref="A1:CV30000"/>`)).toThrowError(/cells limit/i);
expect(() => push('<col x=">" min="1" max="3000000"/>')).toThrowError(/column max/i);
expect(() => push(`<col x=' max="1"' min="1" max="3000000"/>`)).toThrowError(/column max/i);
// Anything the walk cannot read up to `>` is refused, as is a repeated name.
expect(() => push('<mergeCell ref="A1:B2" junk/>')).toThrowError(/do not parse/i);
expect(() => push('<mergeCell ref="A1" ref="A1:CV30000"/>')).toThrowError(/do not parse/i);
expect(() => push('<cols><col min="1" max="3" width="9"></col></cols><mergeCell ref="A1:B2" />')).not.toThrow();
});
it('counts every <row>, empty or not, against per-sheet and total caps', () => {
const rows = (n: number) => '<worksheet><sheetData>' + '<row r="1"/>'.repeat(n) + '</sheetData></worksheet>';
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);
// <rowBreaks>/<rows...> style names are not rows.
expect(
(core.admitXlsx(workbookZip('<worksheet><rowBreaks/></worksheet>'), fflate) as { counts: { rows: number } })
.counts.rows
).toBe(0);
});
it('counts every <xf> in styles.xml, so a </cellXfs> 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 = '<styleSheet><cellXfs><!-- </cellXfs> -->' + '<xf/>'.repeat(200) + '</cellXfs></styleSheet>';
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);