Conversation
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.
|
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, While testing it with real ExcelJS workbooks through the worker I hit three bugs that need fixing before merge:
Two smaller things that fit in the same round:
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. |
- Normalize the value shapes ExcelJS loads before formatting: Date cells are formatted from their serial (UTC), so they no longer render as a local-time string a day early at negative UTC offsets; rich text joins its runs, hyperlinks show their text, error values show the error, and formula and shared-formula results (including error results) recurse. Excel serials are rounded to whole milliseconds so 00:05 no longer shows as 00:04. - sendTile() skips hidden rows and columns, and at the 2500-cell cap returns a truncated tile with a warning instead of failing the whole preview. - ExcelJS now parses a STORE-only archive rebuilt from exactly the entries admitXlsx() inflated and counted, never the fetched bytes. Admission walks local headers while JSZip reads the central directory, so overlapping entries could show the two readers different sheets. A duplicate local entry name is refused. The theme fallback reads the admitted entry too. - Row and column headings take their size from the same axis math as cells. - Document the admission, worker-only loading and SPREADSHEET_ASSET_VERSION rules in architecture-invariants, and list .xlsx in the attachments panel help and the `codeman attach` error text (built from the accepted list).
|
Thanks for the careful review, and for testing with real ExcelJS workbooks. All of it is addressed in 4edb7b8.
The small ones are done too: headings are sized from the same |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked every item from the first round at 4edb7b8 and all of them are fixed: the date and value shapes (with the TZ test), hidden rows and the truncated tile, the STORE-only rebuilt archive with its overlapping-entry fixture, heading sizes, and the docs and help text. Typecheck, lint, format, the asset checks, the full One more thing needs fixing before merge, in the same family as the round 1 admission bypass:
Two smaller ones that fit in the same round:
At merge time I will move the Once the admission fix lands, this is ready to merge. Thanks for the careful work on both rounds. |
ExcelJS 4.4.0 expands three constructs into one object per cell or column at load time, so a few KB admitted as one cell could cost a gigabyte: - a <mergeCell> now costs its full area against the per-sheet and total cell caps, and a ref that does not parse is refused - a <col> whose min or max is past 16384 is refused - the worker loads with ignoreNodes: ['dataValidations']; the preview never shows validations, and a whole-column dropdown took 5 s The XML counter now scans up to the last complete tag and carries the rest, so a merge or col tag cut by an inflate-chunk edge is read whole. A central-directory compressedSize that runs past the file is refused, since the ratio cap divides by it. The renderer and core axis offsets use prefix sums with a binary search instead of walking every override per call.
|
Thanks for running the worker core through the harness, those three expansions are a nasty class. All of it is addressed in bfab172.
Thanks for picking up the hash check and the |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked the round 2 items at bfab172: The new attribute reads can still be fooled, so the expansion bounds from round 2 do not hold yet. Two things need fixing before merge:
Two smaller ones that fit in the same round:
The |
…view # Conflicts: # src/web/public/constants.js
… rows XML allows a raw `>` and the other quote character inside an attribute value, so a first-match search for `ref=`/`max=` could be fed a fake value from an earlier attribute while saxes read the real one: - <mergeCell>/<col> attributes are now read in order from the tag name with a sticky regex that consumes each quoted value whole. A tag whose attributes do not parse up to `>`, or that repeats a name, is refused. - The chunk carry keeps everything from the last `<`, which can never appear inside an attribute value, instead of comparing against the last `>`. ExcelJS keeps a Row object for every <row>, cells or not, so <row> tags now count against per-sheet (100k) and total (250k) caps with their own row-limit code, and the worker passes maxRows as a per-sheet backstop. styles.xml counts every <xf> without tracking which list it sits in, since a </cellXfs> inside a comment desynced that state.
|
Thanks for the third round. The quoted-value trick is a good catch, and one the second-round fix did not anticipate. All of it is in d65ee4f, on top of a merge of current master (1.33.3; the only conflict was the extension lists in
Smaller ones:
I broke each fix on purpose and the tests catch it: a non-sticky attribute search, uncounted rows, uncounted styles and the old |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked the round 3 items at d65ee4f and they all hold:
Typecheck, lint, format, the asset checks, the full Running your worker and core through the worker-test harness again turned up three more problems. Two are in the admission layer, the same class as rounds 2 and 3, and one is a dependency pin.
I prototyped fixes 1 and 2 on your branch:
Please also extend the CLAUDE.md line and the architecture-invariants paragraph with the two new mechanisms, the way they already name One small docs item for the same round: the preview table in The |
…efined names, pin fflate 0.8.3 Admission checked ZIP entry names as stored, but JSZip (inside ExcelJS) resolves `.`, `..` and empty segments on load, and ExcelJS strips one leading `/` and matches worksheets with an unanchored pattern. Names like `/xl/worksheets/sheet1.xml` or `xl/worksheets/sheet1.xml.x` skipped every counter. Admission now computes the name ExcelJS will see for each entry, refuses two entries that resolve to the same name, keys the rebuilt archive on it, and picks the worksheet/styles counters from it. ExcelJS's DefinedNames model setter expands every range into one object per cell. The preview never shows defined names, so the worker stubs `_definedNames.model` before load. Pin fflate to 0.8.3 (GHSA-px8p-9vwx-vf98) and refresh SPREADSHEET_ASSET_VERSION.
|
Thanks @Ark0N, and thanks for prototyping these. All three are in 2d0ffb7.
The new tests fail on the previous head and pass now. Making the name mapping an identity fails five of them. CLAUDE.md and the architecture-invariants paragraph describe both new mechanisms, and |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked the round 4 items at 2d0ffb7 and all three hold:
Typecheck, lint, format, the asset and lockfile checks, the full I ran the worker and core through the worker-test harness again and found three more problems. Two are gaps in front of ExcelJS, and both are about the index a row or sheet claims rather than how many there are. The third is a cost on every tile.
One small one for the same round: in Please also:
The |
…s 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 <row> 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 <sheet> 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.
|
Thanks again for another careful round. All four are in c1811fd.
The CLAUDE.md line and the architecture-invariants paragraph now cover both bounds and the merge reuse, and Typecheck, lint, format, the asset, lockfile and browser-exclude checks, the spreadsheet and dependency tests and the Chromium test all pass. I also checked that each new test fails when its fix is reverted. |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked the round 5 items at c1811fd and all four hold:
Typecheck, lint, format, the frontend-syntax, browser-exclude, lockfile and asset checks, the full One more thing needs fixing before merge. It is the column-axis twin of the round 5 row index.
One small one for the same round:
For a follow-up, no need to hold this PR for it: ExcelJS creates every column object from 1 up to the highest column a sheet touches, so 50 sheets with The |
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.
|
Thanks for round 6, and for measuring the far-column cost in Chromium. Both items are in 7d3e27f.
I checked each new test against a mutation. Putting I've noted the column-object budget for a separate change, as you suggested, and left it out of this PR. Thanks again for sticking with this through six rounds. |
|
Thanks again, @aakhter. This PR adds a read-only XLSX preview to the file-preview overlay, parsed in a browser worker behind admission caps. I checked the round 6 items at 7d3e27f and both hold:
Typecheck, lint, format, the frontend-syntax, browser-exclude, lockfile and asset checks, the full This round I measured what still gets through to the page and to ExcelJS. Three things need fixing before merge. The first matters most, because it freezes the page itself rather than the worker.
Please also extend the CLAUDE.md line and the architecture-invariants paragraph with the text cap and the two new bounds, and refresh The |
What
.xlsxfiles 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 andFILE_PREVIEW_EXTENSIONS)..xlsand.odsstay download-only, since ExcelJS only reads xlsx; tests pin both.How
spreadsheet-preview-worker.js+spreadsheet-xlsx-core.js) usingexceljs@4.4.0andfflate@0.8.3(both MIT, pinned exactly as devDependencies and copied intovendor/by postinstall and the build, like the other vendored bundles). The server does no parsing.resolveFileTarget,resolveServableAttachmentPath). There is no new path handling.?preview=trueis capped at 10 MB (413 above it) on bothfile-rawand the attachment raw route; downloads are unchanged._openSpreadsheetPreview/_disposeSpreadsheetPreview), torn down from_stopFilePreviewMediaon open and close. The worker is created viaCodemanBase.urland loads its scripts by relative URL, so--base-urlmounts work.Safety
The workbook is untrusted input:
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.textContent. The generated style block only accepts validated#rrggbbcolours and a fixed keyword set. At most 2500 cells are drawn per tile.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-assetsenforces a 1.1 MB vendor budget, and a content-hashSPREADSHEET_ASSET_VERSIONcache-busts the worker.dependency-security.test.tsgains one exact exemption: exceljs pinsuuid@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 callsv4(). The browser also loads exceljs's own prebuiltdistbundle, and nothing server-side imports exceljs.Testing
BROWSER_TEST_GLOBS). Also 6 new route tests acrossfile-routesand the attachment path guard.textContentswapped forinnerHTMLtypecheck,lint,check:frontend-syntax,format:check,check:public-assets,check:lockfileandbuildare clean.<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.