From c1811fd7168dd9c4ee6fb7da354148c1aec2bf6d Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sat, 3 Oct 2026 12:33:35 -0400 Subject: [PATCH] fix(preview): bound row and sheet indices before ExcelJS, reuse merges per tile, cap format decimals ExcelJS stores a row at _rows[r - 1] and a sheet at _worksheets[sheetId], and walks or slices those arrays up to the largest index, so the index a row or sheet claims is a cost of its own. Admission now reads each tag's attributes in order and refuses an r outside 1-1048576 (absent r is fine), and a counter for the resolved xl/workbook.xml reads every tag and refuses one that does not parse or whose sheetId is not plain digits up to LIMITS.maxSheetId (65535). sendTile no longer reads sheet.model, which rebuilt every row and cell model on each tile: the merges read in worksheetMetadata are kept in mergesById next to populatedRowsById, replaced on load and cleared on dispose. Number formats cap decimals at 30, as Excel does; toLocaleString throws a RangeError above 100 and the whole grid was replaced by the error. Docs: CLAUDE.md and architecture-invariants describe both bounds and the merge reuse. SPREADSHEET_ASSET_VERSION is recomputed for the edited worker and core. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/public/spreadsheet-preview-worker.js | 14 ++++-- src/web/public/spreadsheet-preview.js | 2 +- src/web/public/spreadsheet-xlsx-core.js | 44 +++++++++++++++-- test/spreadsheet-preview-worker.test.ts | 50 ++++++++++++++++++++ test/spreadsheet-xlsx-core.test.ts | 48 +++++++++++++++++++ 7 files changed, 153 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 640c6be6..e74878e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -289,7 +289,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Attachments** (live external document references; all wiring in `file-routes.ts`): a **registry** maps a stable `attachmentId` to a realpath-resolved, extension-allowlisted absolute path, so browser requests never carry arbitrary absolute paths. ⚠️ The **magic-link scanner** (`codeman://attach?...` in terminal output) is **prompt-injectable**, so its scan path is force-confined to the session workspace; a hostile prompt could otherwise exfiltrate arbitrary host files over SSE. The security gate is an extension **allowlist**, not a blocklist. `document-conversion-limiter.ts` caps converter spawns globally: without it, N large docs detected at once fork N multi-minute processes, which is a resource-exhaustion vector. → [architecture-invariants#attachments](docs/architecture-invariants.md#attachments) -**File-path links (terminal + chat)**: a path an agent prints is clickable on BOTH surfaces and opens the file-preview overlay. ⚠️ ONE pattern (`FILE_PATH_LINK_PATTERN` / `absoluteFilePathPattern()` in constants.js) feeds the xterm link provider AND `_linkifyFilePaths()`, a fresh instance per call (`lastIndex`). The chat linkifier walks TEXT NODES with DOM APIs, never rebuilds sanitized markup as a string. ⚠️ An out-of-workspace path goes through the ATTACHMENT routes (`POST /api/sessions/:id/attachments` with `notify: false`), never by widening `file-content`/`file-raw` or `file-stream-manager`'s `tail -f` allowlist. ⚠️ `TEXT_ATTACHMENT_EXTENSIONS` IS `EDITABLE_EXTENSIONS` (never a second list), and widening READ must never widen RUN: `html`/`htm`/`svg` stay download-only, other text is inert `text/plain`+`nosniff`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) +**File-path links (terminal + chat)**: a path an agent prints is clickable on BOTH surfaces and opens the file-preview overlay. ⚠️ ONE pattern (`FILE_PATH_LINK_PATTERN` / `absoluteFilePathPattern()` in constants.js) feeds the xterm link provider AND `_linkifyFilePaths()`, a fresh instance per call (`lastIndex`). The chat linkifier walks TEXT NODES with DOM APIs, never rebuilds sanitized markup as a string. ⚠️ An out-of-workspace path goes through the ATTACHMENT routes (`POST /api/sessions/:id/attachments` with `notify: false`), never by widening `file-content`/`file-raw` or `file-stream-manager`'s `tail -f` allowlist. ⚠️ `TEXT_ATTACHMENT_EXTENSIONS` IS `EDITABLE_EXTENSIONS` (never a second list), and widening READ must never widen RUN: `html`/`htm`/`svg` stay download-only, other text is inert `text/plain`+`nosniff`. Media extensions are single-sourced in `attachment-registry.ts`. ⚠️ **XLSX previews parse in the BROWSER**, never on the server: `spreadsheet-preview.js` fetches the raw route with `?preview=true` (413 above `MAX_XLSX_BROWSER_PREVIEW_BYTES`, 10 MB) and hands the bytes to `spreadsheet-preview-worker.js`, the only place the pinned `exceljs`/`fflate` vendor bundles load (never on page load). `admitXlsx()` caps the ZIP before ExcelJS runs, counting what ExcelJS will EXPAND as well as what it reads (a merge costs its area, a `` past 16384 is refused, validations are never parsed via `ignoreNodes`, and defined names are never expanded: the worker stubs `_definedNames.model`). ⚠️ The INDEX a row or sheet claims is bounded too, since ExcelJS allocates and walks up to it: a `` outside 1-1048576 and an `xl/workbook.xml` `` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ Admission keys every entry on the name ExcelJS will SEE (`excelJsEntryName()`: JSZip's `.`/`..`/empty-segment resolution, then one leading `/` stripped), refuses two entries that land on one name, and treats anything matching ExcelJS's UNANCHORED `xl/worksheets/sheet.xml` as a worksheet, so `/xl/...` or `xl/./...` cannot skip a counter, and ExcelJS parses only a STORE-only archive rebuilt from the entries admission inflated (`buildAdmittedArchive()`), never the fetched bytes; cell text goes through `textContent`, formulas are never evaluated. Bumping either package or editing the worker/core changes `SPREADSHEET_ASSET_VERSION`, which `npm run check:public-assets` pins. xls/ods stay download-only. → [architecture-invariants#file-path-links-terminal--response-viewer](docs/architecture-invariants.md#file-path-links-terminal--response-viewer) **Filesystem path picker** (Link Existing "Browse" + the mobile keyboard's `📁 Path` key): lazy one-directory browsing via `GET /api/filesystem/browse`, with `GET /api/filesystem/preview` for the tapped file. Inserts the path **without** Enter, so the prompt is never submitted; the sibling `⌫ All` key clears only the unsent prompt and must never send the agent's `/clear`. ⚠️ This is a **second file-serving surface and inherits neither the attachment confinement nor its ownership scoping** — it allowlists Home, `CASES_DIR`, `/mnt/d` and `CODEMAN_FILE_PICKER_ROOTS`, blocks sensitive trees, and rejects symlink escapes **after** `realpath`. ⚠️ The optional `sessionId` is an ownership boundary that must be `canAccessOwned`-checked by hand (it does not go through `findSessionOrFail`), and in multi-user mode a non-admin gets only their own `userSpacePath` as a root: per-user spaces live INSIDE `homedir()`, so a `Home` root exposes every other user's workspace. Previews go through the same global conversion limiter, and Markdown/TXT/JSON are served as inert `text/plain`. → [architecture-invariants#filesystem-path-picker](docs/architecture-invariants.md#filesystem-path-picker) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index a701584b..fdd9eb02 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). Defined names expand the same way and live in `xl/workbook.xml`, which admission never scans: ExcelJS's `DefinedNames` model setter builds one object per cell of every range (a whole-sheet name exhausted a 4 GB heap), so right after `new ExcelJS.Workbook()` the worker replaces `_definedNames.model` with an `Object.defineProperty` stub whose getter returns `[]` and whose setter drops the value. Print areas and titles are split off in the workbook xform's reconcile, before that setter runs, so they are unaffected, and `defineProperty` throws if a future ExcelJS renames `_definedNames` rather than silently expanding again. ⚠️ Every name admission uses is the one ExcelJS will SEE, not the stored one: JSZip resolves each entry name on load (`utils.resolve`: `.` and empty middle segments dropped, `..` pops a segment) and ExcelJS then strips one leading `/` and matches worksheets with an UNANCHORED `xl/worksheets/sheet.xml`. Checking the stored name once let `/xl/worksheets/sheet1.xml`, `xl/./...`, `xl//...`, `xl/xl/worksheets/sheet1.xml` and `xl/worksheets/sheet1.xml.x` skip every counter. `excelJsEntryName()` mirrors both steps; admission refuses two entries that resolve to one name, keys `inflatedEntries` (so the rebuilt archive) on the resolved name, picks the worksheet and styles counters from it, and counts any name matching ExcelJS's unanchored worksheet pattern as a worksheet. The local-versus-central consistency checks still compare the stored names. 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. +**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). Defined names expand the same way and live in `xl/workbook.xml`, where admission reads only the `` ids: ExcelJS's `DefinedNames` model setter builds one object per cell of every range (a whole-sheet name exhausted a 4 GB heap), so right after `new ExcelJS.Workbook()` the worker replaces `_definedNames.model` with an `Object.defineProperty` stub whose getter returns `[]` and whose setter drops the value. Print areas and titles are split off in the workbook xform's reconcile, before that setter runs, so they are unaffected, and `defineProperty` throws if a future ExcelJS renames `_definedNames` rather than silently expanding again. ⚠️ Every name admission uses is the one ExcelJS will SEE, not the stored one: JSZip resolves each entry name on load (`utils.resolve`: `.` and empty middle segments dropped, `..` pops a segment) and ExcelJS then strips one leading `/` and matches worksheets with an UNANCHORED `xl/worksheets/sheet.xml`. Checking the stored name once let `/xl/worksheets/sheet1.xml`, `xl/./...`, `xl//...`, `xl/xl/worksheets/sheet1.xml` and `xl/worksheets/sheet1.xml.x` skip every counter. `excelJsEntryName()` mirrors both steps; admission refuses two entries that resolve to one name, keys `inflatedEntries` (so the rebuilt archive) on the resolved name, picks the worksheet and styles counters from it, and counts any name matching ExcelJS's unanchored worksheet pattern as a worksheet. The local-versus-central consistency checks still compare the stored names. 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 reads each ``'s attributes the same way. ⚠️ The INDEX a row or sheet claims is a cost too, not just how many there are: ExcelJS stores a row at `_rows[r - 1]` and walks `_rows` up to the largest index on every `eachRow` and `sheet.model` (five rows plus one `` took 5 s to load and 1.7 s per tile), and stores a sheet at `_worksheets[sheetId]`, which its `worksheets` getter slices and sorts (`sheetId="30000000"` on a one-cell workbook took 1.6 s and 557 MB). So a `` that is not plain digits in 1 to 1,048,576 is refused (an absent `r` is fine), and a counter for the resolved `xl/workbook.xml` reads every `` tag in order and refuses one that does not parse or whose `sheetId` is not plain digits up to `LIMITS.maxSheetId` (65535; real ids are small, and the `(?=[\s/>])` lookahead keeps the `` container out). The counter also counts 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`. ⚠️ `sendTile` must never read `sheet.model`, which rebuilds every row and cell model (10-19 ms per tile on a 100k-cell sheet, once per animation frame while scrolling): each sheet's merges are read once in `worksheetMetadata` and kept in `mergesById` next to `populatedRowsById` (replaced on load, cleared on dispose), and a worker test makes the `model` getter throw before requesting a tile. Number formats cap decimals at 30, as Excel does, since `toLocaleString` throws above 100. 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 68f13616..680e2656 100644 --- a/src/web/public/spreadsheet-preview-worker.js +++ b/src/web/public/spreadsheet-preview-worker.js @@ -25,6 +25,9 @@ const core = self.CodemanSpreadsheetXlsxCore; let workbook = null; let sheetsById = new Map(); let populatedRowsById = new Map(); +// Merges read once at load: `sheet.model` rebuilds every row and cell model, +// which is far too much to pay on every tile. +let mergesById = new Map(); let normalizedStyles = []; let styleIds = new Map(); let themePalette = core.DEFAULT_THEME_PALETTE; @@ -100,6 +103,7 @@ function worksheetMetadata(sheet) { } return { populatedRows, + merges, metadata: { id: String(sheet.id), name: sheet.name, @@ -126,8 +130,8 @@ async function loadWorkbook(bytes) { if (!self.ExcelJS) importScripts(`vendor/exceljs.min.js${spreadsheetAssetQuery}`); const nextWorkbook = new self.ExcelJS.Workbook(); // ExcelJS's DefinedNames model setter expands every range into one object per - // cell (a whole-sheet name exhausts the heap), and admission never scans - // xl/workbook.xml. The preview never shows defined names, so they are not + // cell (a whole-sheet name exhausts the heap), and admission reads only the + // `` ids in xl/workbook.xml, never defined names. The preview never shows defined names, so they are not // stored at all; print areas and titles are split off before this setter runs. // defineProperty throws if a future ExcelJS renames `_definedNames`, rather // than silently expanding again. @@ -142,6 +146,7 @@ async function loadWorkbook(bytes) { }); const nextSheets = new Map(); const nextRows = new Map(); + const nextMerges = new Map(); normalizedStyles = []; styleIds = new Map(); themePalette = core.parseThemePalette(readThemeXml(nextWorkbook, admission.entries)); @@ -152,11 +157,13 @@ async function loadWorkbook(bytes) { const metadata = sheetResult.metadata; nextSheets.set(metadata.id, sheet); nextRows.set(metadata.id, sheetResult.populatedRows); + nextMerges.set(metadata.id, sheetResult.merges); sheets.push(metadata); } workbook = nextWorkbook; sheetsById = nextSheets; populatedRowsById = nextRows; + mergesById = nextMerges; self.postMessage({ type: 'metadata', sheets, @@ -211,7 +218,7 @@ function sendTile(message) { addCell(cell); }); } - const merges = core.intersectingMerges(Array.from(sheet.model?.merges || []), range); + const merges = core.intersectingMerges(mergesById.get(String(message.sheetId)) || [], range); for (const merge of merges) { const anchor = core.parseRange(merge); if (anchor) addCell(sheet.getCell(anchor.r1, anchor.c1)); @@ -236,6 +243,7 @@ self.onmessage = async (event) => { workbook = null; sheetsById = new Map(); populatedRowsById = new Map(); + mergesById = new Map(); themePalette = core.DEFAULT_THEME_PALETTE; } } catch (error) { diff --git a/src/web/public/spreadsheet-preview.js b/src/web/public/spreadsheet-preview.js index b30b7a56..afad0882 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 = '2bb0eff45e39'; + const SPREADSHEET_ASSET_VERSION = '5bd72f3f823f'; 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 498acce5..7daf0a12 100644 --- a/src/web/public/spreadsheet-xlsx-core.js +++ b/src/web/public/spreadsheet-xlsx-core.js @@ -29,6 +29,10 @@ maxRowsPerSheet: 100000, maxMergesPerSheet: 5000, maxStyles: 5000, + // ExcelJS stores each sheet at `_worksheets[sheetId]`, so the id is an array + // length. Excel numbers sheets from 1 and never reuses an id, so real ids + // stay small; 65535 leaves room for heavy editing at a negligible cost. + maxSheetId: 65535, }); const MAX_ROW = 1048576; const MAX_COL = 16384; @@ -216,6 +220,26 @@ } } + // ExcelJS stores each row at `_rows[r - 1]`, and eachRow and `sheet.model` + // walk every index up to the largest, so the index a row CLAIMS is a cost. + function checkRowIndex(attributes) { + const value = attributes.get('r'); + if (value === undefined) return; + if (!/^[0-9]+$/.test(value) || Number(value) < 1 || Number(value) > MAX_ROW) { + fail('malformed', `Worksheet row index is outside 1-${MAX_ROW}`); + } + } + + // ExcelJS stores each sheet at `_worksheets[sheetId]` (an absent id parses to + // NaN, a plain property, so it is harmless). + function checkSheetId(attributes, limits) { + const value = attributes.get('sheetId'); + if (value === undefined) return; + if (!/^[0-9]+$/.test(value) || Number(value) > limits.maxSheetId) { + fail('malformed', `Workbook sheetId is not a number up to ${limits.maxSheetId}`); + } + } + function createXmlCounter(name, counts, limits) { let tail = ''; const decoder = new TextDecoder(); @@ -225,6 +249,7 @@ const excelJsName = excelJsEntryName(name); const worksheet = isWorksheetName(excelJsName); const styles = excelJsName === 'xl/styles.xml'; + const workbook = excelJsName === 'xl/workbook.xml'; const addCells = (cells) => { sheetCells += cells; counts.cells += cells; @@ -233,7 +258,7 @@ }; return { push(chunk, final) { - if (!worksheet && !styles) return; + if (!worksheet && !styles && !workbook) return; const text = tail + decoder.decode(chunk, { stream: !final }); // `<` can never appear inside an attribute value, so every tag before the // last `<` is complete. Carry everything from that `<` into the next scan @@ -248,9 +273,13 @@ 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)) { + for (const match of scan.matchAll(/<(mergeCell|col|row)(?=[\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] === 'row') { + checkRowIndex(attributes); + continue; + } if (match[1] === 'col') { checkColumnSpan(attributes); continue; @@ -262,6 +291,14 @@ addCells(mergeArea(attributes)); } } + if (workbook) { + // `` is the container; the lookahead keeps it from matching. + for (const match of scan.matchAll(/])/g)) { + const attributes = readTagAttributes(scan, match.index + match[0].length); + if (!attributes) fail('malformed', 'Workbook has a whose attributes do not parse'); + checkSheetId(attributes, limits); + } + } if (styles) { // Every counts, cellStyleXfs included: tracking which list a tag // sits in can be desynced by a closing tag inside an XML comment. @@ -548,7 +585,8 @@ return { text: `${yyyy}-${mm}-${dd} ${hh}:${minutes}` }; } const percent = code.includes('%'); - const decimals = code.match(/\.([0#]+)/)?.[1].length || 0; + // toLocaleString throws above 100 fraction digits; Excel itself caps at 30. + const decimals = Math.min(code.match(/\.([0#]+)/)?.[1].length || 0, 30); const numericPattern = /^[€£¥$]?[#,0]+(?:\.[0#]+)?%?$/; if (numericPattern.test(code)) { const currency = /^[€£¥$]/.exec(code)?.[0] || ''; diff --git a/test/spreadsheet-preview-worker.test.ts b/test/spreadsheet-preview-worker.test.ts index 18dcccb6..da75b871 100644 --- a/test/spreadsheet-preview-worker.test.ts +++ b/test/spreadsheet-preview-worker.test.ts @@ -696,3 +696,53 @@ describe('spreadsheet preview worker: defined names', () => { expect(harness.peek('Object.keys(workbook.definedNames.matrixMap).length')).toBe(0); }); }); + +describe('spreadsheet preview worker: indices a row or sheet claims', () => { + // ExcelJS stores a row at `_rows[r - 1]`, and eachRow and `sheet.model` walk + // every index up to the largest, so one far row made each tile cost seconds. + it('refuses a past the last Excel row before ExcelJS loads', async () => { + const rows = + Array.from({ length: 4 }, (_, i) => `${i + 2}`).join('') + + '6'; + const bytes = await emptyRowsWorkbook(rows); + const harness = createHarness(); + await harness.send({ type: 'load', bytes }); + expect(harness.messages.at(-1)).toMatchObject({ type: 'error', code: 'malformed' }); + expect(harness.imports.some((url) => url.includes('exceljs'))).toBe(false); + }, 60_000); + + // ExcelJS stores a sheet at `_worksheets[sheetId]`; 30,000,000 took 1.6 s + // and 557 MB on a one-cell workbook. + it('refuses a sheetId above the cap in xl/workbook.xml before ExcelJS loads', async () => { + const workbook = new ExcelJS.Workbook(); + workbook.addWorksheet('Data').getCell('A1').value = 'one'; + const entries = fflate.unzipSync(new Uint8Array(await workbook.xlsx.writeBuffer())); + const book = fflate.strFromU8(entries['xl/workbook.xml']); + const patched = book.replace('sheetId="1"', 'sheetId="30000000"'); + expect(patched).not.toBe(book); + entries['xl/workbook.xml'] = fflate.strToU8(patched); + const harness = createHarness(); + await harness.send({ type: 'load', bytes: toArrayBuffer(fflate.zipSync(entries)) }); + expect(harness.messages.at(-1)).toMatchObject({ type: 'error', code: 'malformed' }); + expect(harness.imports.some((url) => url.includes('exceljs'))).toBe(false); + }, 60_000); +}); + +describe('spreadsheet preview worker: tiles reuse the merges read at load', () => { + // `sheet.model` rebuilds every row and cell model; a tile must not pay that. + it('serves a tile with its merge without touching sheet.model', async () => { + const harness = createHarness(); + const metadata = await loadMetadata(harness, await fixture()); + const sheetId = metadata.sheets[0].id; + harness.peek( + `Object.defineProperty(sheetsById.get(${JSON.stringify(sheetId)}), 'model', { + configurable: true, + get() { throw new Error('sheet.model touched'); }, + })` + ); + const tile = await requestTile(harness, sheetId, { r1: 4, c1: 2, r2: 4, c2: 3 }); + expect(tile.type, JSON.stringify(tile)).toBe('tile'); + expect(tile.merges).toEqual(['A4:C4']); + expect(tile.cells).toContainEqual(expect.objectContaining({ row: 4, col: 1, text: 'Merged' })); + }); +}); diff --git a/test/spreadsheet-xlsx-core.test.ts b/test/spreadsheet-xlsx-core.test.ts index af5dfa5c..0c16bf46 100644 --- a/test/spreadsheet-xlsx-core.test.ts +++ b/test/spreadsheet-xlsx-core.test.ts @@ -283,6 +283,49 @@ describe('spreadsheet XLSX core', () => { ).toBe(0); }); + // ExcelJS stores a row at `_rows[r - 1]` and walks `_rows` up to the largest + // index on every eachRow and `sheet.model`, so the index a row CLAIMS is cost. + it('refuses a that is not plain digits in 1-1048576 and a whose attributes do not parse', () => { + const sheet = (rowTag: string) => + workbookZip(`${rowTag}`); + for (const r of ['50000000', '1048577', '0', '1a', '-1', '1e3', ' 2', '']) { + expect(() => core.admitXlsx(sheet(``), fflate), r).toThrowError(/row index/i); + } + expect(() => core.admitXlsx(sheet(''), fflate)).toThrowError(/do not parse/i); + // A quoted fake `r` cannot stand in for the real one. + expect(() => core.admitXlsx(sheet(``), fflate)).toThrowError(/row index/i); + for (const ok of ['', '', '']) { + expect(() => core.admitXlsx(sheet(ok), fflate), ok).not.toThrow(); + } + }); + + // ExcelJS stores a sheet at `_worksheets[sheetId]`, and the `worksheets` + // getter slices and sorts that array, so a large id allocates a huge array. + it('refuses a in xl/workbook.xml that is not plain digits or is above the cap', () => { + expect(core.LIMITS.maxSheetId).toBe(65535); + const book = (sheets: string, name = 'xl/workbook.xml') => { + const entries = fflate.unzipSync(workbookZip()); + delete entries['xl/workbook.xml']; + entries[name] = fflate.strToU8(`${sheets}`); + return fflate.zipSync(entries); + }; + const one = (id: string) => ``; + for (const id of ['30000000', '65536', '1a', '-1', '1e3', ' 1', '']) { + expect(() => core.admitXlsx(book(one(id)), fflate), id).toThrowError(/sheetId/i); + } + // Every is read, not just the first; the name is the one ExcelJS sees. + expect(() => core.admitXlsx(book(one('1') + one('30000000')), fflate)).toThrowError(/sheetId/i); + expect(() => core.admitXlsx(book(one('30000000'), '/xl/./workbook.xml'), fflate)).toThrowError(/sheetId/i); + expect(() => core.admitXlsx(book(''), fflate)).toThrowError(/do not parse/i); + expect(() => core.admitXlsx(book(``), fflate)).toThrowError(/sheetId/i); + // `` is the container, never a sheet; an absent sheetId is harmless. + expect(() => core.admitXlsx(book(one('1') + one('65535') + ''), fflate)).not.toThrow(); + // Only the workbook part is read this way. + const counts = { cells: 0, merges: 0, styles: 0, rows: 0 }; + const other = core.createXmlCounter('xl/other.xml', counts as never, core.LIMITS); + expect(() => other.push(fflate.strToU8(one('30000000')), true)).not.toThrow(); + }); + 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 }); @@ -344,6 +387,11 @@ describe('spreadsheet XLSX core', () => { expect(core.formatCellValue(7, '[Red][<0]0.0')).toMatchObject({ text: '7', warning: expect.any(String) }); expect(core.formatCellValue({ formula: 'SUM(A1:A2)' }, 'General')).toMatchObject({ text: '=SUM(A1:A2)' }); }); + + // toLocaleString throws a RangeError above 100 fraction digits; Excel caps at 30. + it('caps a number format at 30 decimals instead of throwing', () => { + expect(core.formatCellValue(1.5, `0.${'0'.repeat(120)}`).text).toBe(`1.5${'0'.repeat(29)}`); + }); }); const themeXml = `