fix(preview): index rows and cells by their present keys, cap theme size

ExcelJS keeps a row's cells at `_cells[col - 1]`, so a row whose only
cell sits in XFD is a dictionary-mode array that `eachCell` and
`hasValues` (behind `eachRow`) walk to index 16,384. The worker walked
each row four times at load and once per tile, so a small file of
far-column rows took seconds to load and to tile.

`worksheetMetadata` now builds each sheet's row and cell index from
`Object.keys(sheet._rows)` and `Object.keys(row._cells)`, sorted
numerically, skipping falsy and Null-type cells exactly as
`eachCell({ includeEmpty: false })` does and keeping a row only when it
holds one such cell (`hasValues`). Styles, extent and row heights come
from that one pass and merges from `sheet._merges`; `sendTile` reads
each row's cells from the index.

`parseThemePalette` returns the default palette for a theme above
64 * 1024 characters, since its patterns are quadratic on unclosed tags.
This commit is contained in:
Aamer Akhter
2026-10-03 17:21:41 -04:00
parent c1811fd716
commit 7d3e27fb6d
7 changed files with 193 additions and 23 deletions
+1 -1
View File
@@ -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 `<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. ⚠️ 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. ⚠️ 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)
File diff suppressed because one or more lines are too long
+55 -20
View File
@@ -24,6 +24,8 @@ importScripts(`vendor/fflate.min.js${spreadsheetAssetQuery}`, `spreadsheet-xlsx-
const core = self.CodemanSpreadsheetXlsxCore;
let workbook = null;
let sheetsById = new Map();
// Per sheet: its populated rows in order, each with its populated cells in
// column order, built once at load from the keys that exist (`populatedRowIndex`).
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.
@@ -78,23 +80,56 @@ function normalizeStyle(cell) {
return id;
}
// Ascending numeric own keys of a sparse array. ExcelJS keeps rows at
// `_rows[r - 1]` and a row's cells at `_cells[col - 1]`, and one far index puts
// the array in dictionary mode, where its own `eachRow`, `eachCell` and
// `hasValues` (forEach/some) visit every index up to the largest: a single XFD
// cell per row costs 16,384 steps a row. Walking the keys that exist does not.
function presentIndices(sparse) {
const indices = [];
for (const key of Object.keys(sparse || [])) {
const index = Number(key);
if (Number.isInteger(index) && index >= 0) indices.push(index);
}
return indices.sort((a, b) => a - b);
}
// The rows and cells `sheet.eachRow({ includeEmpty: false })` and
// `row.eachCell({ includeEmpty: false })` would visit, in the same order: a
// cell counts when it exists and its type is not `ValueType.Null`, and a row
// counts when it holds at least one such cell (ExcelJS's `row.hasValues`).
function populatedRowIndex(sheet) {
const nullType = self.ExcelJS.ValueType.Null;
const rows = [];
for (const rowIndex of presentIndices(sheet._rows)) {
const row = sheet._rows[rowIndex];
if (!row) continue;
const cells = [];
for (const cellIndex of presentIndices(row._cells)) {
const cell = row._cells[cellIndex];
if (cell && cell.type !== nullType) cells.push(cell);
}
if (cells.length > 0) rows.push({ number: row.number, row, cells });
}
return rows;
}
function worksheetMetadata(sheet) {
const cellRefs = [];
const populatedRows = [];
sheet.eachRow({ includeEmpty: false }, (row) => {
populatedRows.push(row.number);
row.eachCell({ includeEmpty: false }, (cell) => {
const rowOverrides = [];
const populatedRows = populatedRowIndex(sheet);
for (const { number, row, cells } of populatedRows) {
for (const cell of cells) {
cellRefs.push(cell.address);
normalizeStyle(cell);
});
});
const merges = Array.from(sheet.model?.merges || []);
}
if (row.hidden) rowOverrides.push([number, 0]);
else if (row.height) rowOverrides.push([number, Math.min(546, Math.max(0, row.height * (4 / 3)))]);
}
// `sheet.model` rebuilds every row and cell model, so merges come straight
// from ExcelJS's own merge map, in the order the model getter would list them.
const merges = Object.values(sheet._merges || {}).map((merge) => merge.range);
const extent = core.deriveExtent(cellRefs, merges);
const rowOverrides = [];
sheet.eachRow({ includeEmpty: false }, (row) => {
if (row.hidden) rowOverrides.push([row.number, 0]);
else if (row.height) rowOverrides.push([row.number, Math.min(546, Math.max(0, row.height * (4 / 3)))]);
});
const columnOverrides = [];
for (let col = 1; col <= extent.cols; col += 1) {
const column = sheet.getColumn(col);
@@ -207,16 +242,16 @@ function sendTile(message) {
});
};
const populatedRows = populatedRowsById.get(String(message.sheetId)) || [];
for (const rowNumber of populatedRows) {
for (const populated of populatedRows) {
if (truncated) break;
if (rowNumber < range.r1) continue;
if (rowNumber > range.r2) break;
const row = sheet.getRow(rowNumber);
if (row.hidden) continue;
row.eachCell({ includeEmpty: false }, (cell) => {
if (cell.col < range.c1 || cell.col > range.c2) return;
if (populated.number < range.r1) continue;
if (populated.number > range.r2) break;
if (populated.row.hidden) continue;
for (const cell of populated.cells) {
if (cell.col < range.c1) continue;
if (cell.col > range.c2) break;
addCell(cell);
});
}
}
const merges = core.intersectingMerges(mergesById.get(String(message.sheetId)) || [], range);
for (const merge of merges) {
+1 -1
View File
@@ -20,7 +20,7 @@
(function initSpreadsheetPreview(global) {
'use strict';
const SPREADSHEET_ASSET_VERSION = '5bd72f3f823f';
const SPREADSHEET_ASSET_VERSION = '6e843e269018';
const MAX_PREVIEW_BYTES = 10 * 1024 * 1024;
const DEFAULT_TIMEOUT_MS = 20000;
const MAX_SCROLL_PX = 8000000;
+8
View File
@@ -744,9 +744,17 @@
return undefined;
}
// Real theme1.xml files are under 10 KB. The scheme and slot patterns below
// rescan to the end of the text for every opening tag that has no close, so a
// padded theme is quadratic (1 MB of `<a:clrScheme>` took 21.8 s); above this
// many characters (UTF-16 code units of the decoded XML) the default palette
// is used instead.
const MAX_THEME_XML_CHARS = 64 * 1024;
function parseThemePalette(xml) {
const palette = DEFAULT_THEME_PALETTE.slice();
const text = typeof xml === 'string' ? xml : '';
if (text.length > MAX_THEME_XML_CHARS) return palette;
const scheme = /<(?:[A-Za-z0-9_]+:)?clrScheme\b[^>]*>([\s\S]*?)<\/(?:[A-Za-z0-9_]+:)?clrScheme\s*>/.exec(text);
if (!scheme) return palette;
// Fresh pattern per call: a shared /g regex would carry lastIndex across calls.
+110
View File
@@ -746,3 +746,113 @@ describe('spreadsheet preview worker: tiles reuse the merges read at load', () =
expect(tile.cells).toContainEqual(expect.objectContaining({ row: 4, col: 1, text: 'Merged' }));
});
});
/** Row and Worksheet prototypes of the ExcelJS build the harness hands the worker. */
function excelJsPrototypes(): { row: Record<string, unknown>; sheet: Record<string, unknown> } {
const probe = new ExcelJS.Workbook().addWorksheet('probe');
return { row: Object.getPrototypeOf(probe.getRow(1)), sheet: Object.getPrototypeOf(probe) };
}
/** Make ExcelJS's dense row, cell and model walks throw until the returned restore runs. */
function forbidDenseWalks(): () => void {
const { row, sheet } = excelJsPrototypes();
const saved = [
[sheet, 'eachRow', Object.getOwnPropertyDescriptor(sheet, 'eachRow')],
[row, 'eachCell', Object.getOwnPropertyDescriptor(row, 'eachCell')],
[row, 'hasValues', Object.getOwnPropertyDescriptor(row, 'hasValues')],
] as const;
for (const [target, name] of saved) {
Object.defineProperty(target, name, {
configurable: true,
get() {
throw new Error(`${name} touched`);
},
});
}
// `sheet.model` rebuilds every row and cell model the same dense way; load
// still needs its setter, so only reading it throws.
const model = Object.getOwnPropertyDescriptor(sheet, 'model')!;
Object.defineProperty(sheet, 'model', {
configurable: true,
get() {
throw new Error('model touched');
},
set: model.set,
});
return () => {
for (const [target, name, descriptor] of saved) Object.defineProperty(target, name, descriptor!);
Object.defineProperty(sheet, 'model', model);
};
}
describe('spreadsheet preview worker: rows and cells are indexed by their present keys', () => {
// ExcelJS keeps a row's cells at `_cells[col - 1]`; one far-column cell makes
// eachCell and hasValues (behind eachRow) visit every index up to 16,384.
it('loads and serves a tile without ExcelJS eachRow, eachCell, hasValues or sheet.model', async () => {
const harness = createHarness();
const bytes = await fixture();
const restore = forbidDenseWalks();
try {
const metadata = await loadMetadata(harness, bytes);
expect(metadata.sheets[0]).toMatchObject({ rows: 4, cols: 3 });
expect(metadata.sheets[0].rowOverrides).toContainEqual([2, 40]);
expect(metadata.sheets[0].merges).toEqual(['A4:C4']);
const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 4, c2: 3 });
expect(tile.type, JSON.stringify(tile)).toBe('tile');
expect(
(tile.cells as Array<{ row: number; col: number; text: string }>).map(({ row, col, text }) => [row, col, text])
).toEqual([
[1, 1, 'Revenue'],
[2, 2, '$1,234.50'],
[3, 3, '$1,234.50'],
[4, 1, 'Merged'],
]);
expect(tile.merges).toEqual(['A4:C4']);
} finally {
restore();
}
});
// eachCell and eachRow skip a cell whose value is Null (a styled empty `<c/>`),
// and a row holding only such cells; the key walk must skip them the same way.
it('skips value-less cells and the rows that hold only them, as ExcelJS eachRow/eachCell do', async () => {
const bytes = await emptyRowsWorkbook(
'<row r="3" ht="30" customHeight="1"><c r="E3" s="1"/></row><row r="4"><c r="B4" s="1"/><c r="C4"><v>7</v></c></row>'
);
const harness = createHarness();
const metadata = await loadMetadata(harness, bytes);
// The styled empty cells really exist in ExcelJS, as Null-type cells.
expect(harness.peek('sheetsById.values().next().value.findCell(3, 5)?.type')).toBe(0);
expect(harness.peek('sheetsById.values().next().value.findCell(4, 2)?.type')).toBe(0);
expect(metadata.sheets[0]).toMatchObject({ rows: 4, cols: 3 });
expect(metadata.sheets[0].rowOverrides).toEqual([]);
const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 1, r2: 4, c2: 5 });
expect((tile.cells as Array<{ row: number; col: number }>).map(({ row, col }) => [row, col])).toEqual([
[1, 1],
[4, 3],
]);
});
it('loads and tiles 3,000 rows that each hold one XFD cell', async () => {
const rows = Array.from(
{ length: 3_000 },
(_, i) => `<row r="${i + 2}" ht="0.01" customHeight="1"><c r="XFD${i + 2}"><v>${i + 2}</v></c></row>`
).join('');
const bytes = await emptyRowsWorkbook(rows);
const harness = createHarness();
const restore = forbidDenseWalks();
try {
const started = performance.now();
const metadata = await loadMetadata(harness, bytes);
expect(metadata.sheets[0]).toMatchObject({ rows: 3_001, cols: 16_384 });
expect(metadata.sheets[0].rowOverrides).toHaveLength(3_000);
const tile = await requestTile(harness, metadata.sheets[0].id, { r1: 1, c1: 16_380, r2: 3_001, c2: 16_384 });
expect(tile.type, JSON.stringify(tile).slice(0, 200)).toBe('tile');
expect(tile.cells).toHaveLength(2_500);
expect(tile.cells[0]).toMatchObject({ row: 2, col: 16_384, text: '2' });
expect(performance.now() - started).toBeLessThan(5_000);
} finally {
restore();
}
}, 60_000);
});
+17
View File
@@ -412,6 +412,23 @@ const themeXml = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
</a:clrScheme></a:themeElements></a:theme>`;
describe('spreadsheet XLSX colour resolution', () => {
// The clrScheme and slot patterns rescan to the end of the text for every
// unclosed opening tag, so a padded theme is quadratic.
it('uses the default palette for a theme above 64 KB, even a 1 MB run of unclosed clrScheme tags', () => {
const padded = '<a:clrScheme>'.repeat(Math.ceil((1024 * 1024) / 13));
expect(padded.length).toBeGreaterThanOrEqual(1024 * 1024);
const started = performance.now();
expect(core.parseThemePalette(padded)).toEqual(core.DEFAULT_THEME_PALETTE);
expect(performance.now() - started).toBeLessThan(1_000);
// The cap is on characters: a real theme padded to exactly 64 KB still parses, one more does not.
const fill = (length: number) =>
themeXml.replace('</a:theme>', `<!--${' '.repeat(length - themeXml.length - 7)}--></a:theme>`);
expect(fill(64 * 1024)).toHaveLength(64 * 1024);
expect(core.parseThemePalette(fill(64 * 1024))[4]).toBe('#ff0000');
expect(core.parseThemePalette(fill(64 * 1024 + 1))).toEqual(core.DEFAULT_THEME_PALETTE);
});
it('parses a theme palette into styles.xml index order, swapping lt/dk against clrScheme order', () => {
const palette = core.parseThemePalette(themeXml);
expect(palette).toHaveLength(12);