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 = `