Skip to content

feat(preview): render XLSX spreadsheets in the file-preview overlay - #502

Open
aakhter wants to merge 1 commit into
Ark0N:masterfrom
aakhter:pr/xlsx-preview
Open

aakhter wants to merge 1 commit into
Ark0N:masterfrom
aakhter:pr/xlsx-preview

Conversation

@aakhter

@aakhter aakhter commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

What

.xlsx files were download-only. This adds a read-only preview in the file-preview overlay: sheet tabs, number formats, merged cells and theme colours, virtualized so large sheets scroll smoothly. It works for workspace files, attachments, and xlsx paths printed in the terminal (added to the file-path link pattern and FILE_PREVIEW_EXTENSIONS). .xls and .ods stay download-only, since ExcelJS only reads xlsx; tests pin both.

How

  • All parsing happens in the browser, in a Web Worker (spreadsheet-preview-worker.js + spreadsheet-xlsx-core.js) using exceljs@4.4.0 and fflate@0.8.2 (both MIT, pinned exactly as devDependencies and copied into vendor/ by postinstall and the build, like the other vendored bundles). The server does no parsing.
  • Server side xlsx only joins the existing attachment allowlist and file-content classification, so every request still goes through the existing confinement (resolveFileTarget, resolveServableAttachmentPath). There is no new path handling. ?preview=true is capped at 10 MB (413 above it) on both file-raw and the attachment raw route; downloads are unchanged.
  • Overlay wiring is two small methods on the existing preview (_openSpreadsheetPreview / _disposeSpreadsheetPreview), torn down from _stopFilePreviewMedia on open and close. The worker is created via CodemanBase.url and loads its scripts by relative URL, so --base-url mounts work.

Safety

The workbook is untrusted input:

  • Checked before ExcelJS loads (admitXlsx): at most 5000 ZIP entries, 64 MB inflated in total, 32 MB per entry, a 100:1 compression ratio, 50 worksheets, 250k cells (100k per sheet), 5000 merges per sheet and 5000 styles. Encrypted and ZIP64 files are refused. There is also a 20 s timeout.
  • Rendering: cell text and sheet names are written with textContent. The generated style block only accepts validated #rrggbb colours and a fixed keyword set. At most 2500 cells are drawn per tile.
  • Nothing is evaluated or fetched: formulas show their cached result (or the formula text), and external links, charts, drawings and macros only produce a "not shown" notice.

Cost

Page load gains only spreadsheet-preview.js (5.0 KB gz). Opening a spreadsheet then loads the worker (3.0 KB), core (7.4 KB), fflate (12.5 KB) and exceljs (256 KB gz, 948 KB raw), about 284 KB gz in total. ExcelJS is only fetched after the workbook passes the checks. check:public-assets enforces a 1.1 MB vendor budget, and a content-hash SPREADSHEET_ASSET_VERSION cache-busts the worker.

dependency-security.test.ts gains one exact exemption: exceljs pins uuid@8.3.2 (our own uuid stays >= 14). The advisory is MODERATE, outside that suite's CRITICAL/HIGH policy, and covers v3/v5/v6 with a caller-supplied buffer, while exceljs only calls v4(). The browser also loads exceljs's own prebuilt dist bundle, and nothing server-side imports exceljs.

Testing

  • 43 new tests: xlsx core 20, worker 7, renderer/overlay 10, assets 5, plus 1 Chromium test under a strict CSP (added to BROWSER_TEST_GLOBS). Also 6 new route tests across file-routes and the attachment path guard.
  • Mutation-checked. Each of these fails a test:
    • textContent swapped for innerHTML
    • xlsx removed from the allowlist
    • the classification change reverted
    • the preview cap removed
    • the compression-ratio cap removed
    • the workbook checks skipped
    • no dispose on close
    • the formula-text path broken
  • Related suites pass unchanged, including file-routes, the attachment path guard, sw-precache, base-path and dependency-security.
  • typecheck, lint, check:frontend-syntax, format:check, check:public-assets, check:lockfile and build are clean.
  • Checked in a real browser on an isolated instance: both sheet tabs rendered, a cell containing <img onerror> displayed as text with no image element created, no console errors, and none of the four preview requests were made until a spreadsheet was opened.

xlsx files were download-only. Add a read-only, virtualized preview (sheet
tabs, number formats, merges, theme colours) parsed entirely in a browser
Web Worker with exceljs and fflate, loaded only when a spreadsheet is
opened. The workbook is checked against ZIP-bomb, entry and cell limits
before exceljs loads; cell text is written with textContent, formulas are
never evaluated and nothing referenced by the workbook is fetched. On the
server xlsx only joins the existing allowlist and classification, with a
10 MB cap on ?preview=true. xls and ods stay download-only.
@Ark0N

Ark0N commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Thanks a lot for this, @aakhter. It adds a read-only XLSX preview to the file-preview overlay, parsed entirely in a browser worker, and the overall design is exactly right: no server-side parsing, no new path handling, lazy worker-only vendor bundles, textContent rendering, an allowlisted style block, --base-url support and a content-hashed cache-bust token. Typecheck, lint, format, the asset checks, the full npm test gate and your Chromium test all pass here.

While testing it with real ExcelJS workbooks through the worker I hit three bugs that need fixing before merge:

  1. Date, rich-text, hyperlink and error cells display wrong (src/web/public/spreadsheet-xlsx-core.js:309). ExcelJS turns date-formatted cells into JS Date objects on load, so String(value) shows Mon Jan 15 2024 01:00:00 GMT+0100 (...), and in TZ=America/New_York the same cell reads Sun Jan 14 2024 ..., a day early. Rich text ({richText}), hyperlinks ({text, hyperlink}), error values ({error}) and formulas with an error result all render [object Object]. Please normalize these shapes before formatting (format Date from its UTC components per the numFmt, join richText runs, use .text / .error, recurse on result for formula and sharedFormula) and add worker round-trip tests for each, one of them under a negative-offset TZ.

  2. Filtered or dense sheets fail to preview (src/web/public/spreadsheet-preview-worker.js:179-188). sendTile() includes hidden rows, which are 0 px tall, so the viewport spans all of them, and above 2500 cells it throws tile-limit, which replaces the whole grid with an error. A 1000 x 6 sheet with 980 rows hidden by a filter fails at the renderer's default viewport, and so does a 60 x 60 filled block. Please skip hidden rows and columns in sendTile() and return a truncated tile with a warning at the cap instead of throwing (the renderer already slices to 2500). A hidden-rows worker test would pin it.

  3. admitXlsx() can be bypassed with overlapping ZIP entries (src/web/public/spreadsheet-xlsx-core.js:153-207). Admission follows local headers in file order, while ExcelJS (JSZip) follows the central directory, and the name-count check does not tie the two together. A crafted file with a stored entry that hides a full sheet1.xml, plus a small decoy sheet1.xml later in the stream, was admitted as 1 cell and 611 KB inflated; the worker then parsed 300,000 cells. Impact stays in the viewer's tab, but the PR and the new CLAUDE.md line state that admission caps the ZIP before ExcelJS runs. The simplest robust fix is to hand ExcelJS a STORE-only archive rebuilt from the entries admission already inflated (fflate.zipSync(entries, { level: 0 })); rejecting central entries whose local extents overlap also works. Please add that fixture as a regression test.

Two smaller things that fit in the same round:

  • Row and column headings take their size from CSS (64 x 20 px, styles.css:19176) rather than the axis math (spreadsheet-preview.js:216-234), so they misalign with custom widths and heights, including column B and row 2 of your own fixture. Setting width/height from the same axisOffset differences as the cells fixes it.
  • The new XLSX rule in CLAUDE.md points at docs/architecture-invariants.md#file-path-links-terminal--response-viewer, which was not updated; a short paragraph there (admission caps, worker-only vendor loading, SPREADSHEET_ASSET_VERSION) keeps the two in step. The attachments panel help (panels-ui.js:4793) and codeman attach error text (src/cli.ts:114) could also list .xlsx now.

Everything else (the route changes, the allowlist addition, the packaging and the tests) is in good shape, so once these land it is ready to merge. Thanks again for the careful work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants