fix(preview): cap cell text, bound merges workbook-wide, refuse runaway number formats

This commit is contained in:
Aamer Akhter
2026-10-04 20:15:09 -04:00
parent ab96e74e69
commit 0ae5ce017a
6 changed files with 319 additions and 12 deletions
+1 -1
View File
@@ -292,7 +292,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 `<col>` 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 `<row r>` outside 1-1048576 and an `xl/workbook.xml` `<sheet sheetId>` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ The COLUMN index costs the same way (a row's cells live at `_cells[col - 1]`, so one `XFD` cell makes ExcelJS's `eachRow`/`eachCell`/`hasValues` visit 16,384 slots per row): `worksheetMetadata` builds each sheet's row and cell index from the keys that exist (`Object.keys` of `_rows` and `_cells`, skipping falsy and `Null`-type cells as `eachCell({ includeEmpty: false })` does), reads row heights in that pass and merges from `sheet._merges`, and `sendTile` reads cells from that index (`populatedRowsById`); never call ExcelJS's dense `eachRow`/`eachCell` there. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters), since its patterns are quadratic on unclosed tags. ⚠️ 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<N>.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 `<col>` 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 `<row r>` outside 1-1048576 and an `xl/workbook.xml` `<sheet sheetId>` above `LIMITS.maxSheetId` (65535) are refused, and `sendTile` reuses the merges read at load (`mergesById`), never `sheet.model`, which rebuilds the whole sheet. ⚠️ The COLUMN index costs the same way (a row's cells live at `_cells[col - 1]`, so one `XFD` cell makes ExcelJS's `eachRow`/`eachCell`/`hasValues` visit 16,384 slots per row): `worksheetMetadata` builds each sheet's row and cell index from the keys that exist (`Object.keys` of `_rows` and `_cells`, skipping falsy and `Null`-type cells as `eachCell({ includeEmpty: false })` does), reads row heights in that pass and merges from `sheet._merges`, and `sendTile` reads cells from that index (`populatedRowsById`); never call ExcelJS's dense `eachRow`/`eachCell` there. `parseThemePalette` returns the default palette for a theme above 64 KB (64 * 1024 characters), since its patterns are quadratic on unclosed tags. ⚠️ Merges are capped per sheet (`LIMITS.maxMergesPerSheet`, 2,000) AND workbook-wide (`LIMITS.maxMerges`, 10,000), refused as `merge-limit` before ExcelJS loads, since ExcelJS's `_mergeCellsInternal` checks each merge against every earlier one on its sheet (quadratic). Every `<numFmt>` in `xl/styles.xml` is read with `readTagAttributes()` and refused (`number-format`) when its decoded `formatCode` is over 255 characters or has a `[` after its last `]`: ExcelJS's `isDateFmt` rescans to the end of the code per unclosed `[` once per numeric cell, and the code is echoed into the notice bar. ⚠️ `formatCellValue` caps every cell's display text at `LIMITS.maxCellTextChars` (1,000, ellipsis, never splitting a surrogate pair) for every value shape (rich text, hyperlink text, formula source, errors), since structured clone copies each tile cell's whole string to the page and the load timeout no longer covers tiles. ⚠️ 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<N>.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)