Repository navigation
feat(file-viewer): open Word, Excel, PowerPoint and OpenDocument files - #3572
LordMelkor wants to merge 2 commits into
Conversation
Read Office, OpenDocument, RTF and iWork files through bounded format admission, XML and archive parsing. Keep the generated document, sheet, slide and preview models independent of the viewer UI. Co-authored-by: Sigil <06114f072cf21e50fcd4133ab21346cdb1f74ee16d853a9729995fa6c03a251c@buzz.block.builderlab.xyz> Signed-off-by: LordMelkor <kray@squareup.com>
Documents show with their pictures, tables and charts, and a Word file with three or more headings has an Outline. Spreadsheets show a sheet at a time with a tab for each, and Copy as TSV copies the open sheet. Presentations show each slide fitted to the tab, with speaker notes. Numbers files open as sheets too. Older .doc and .ppt files show their text only, and Pages and Keynote files the preview picture saved in them. Nothing in a file runs. Co-authored-by: Clod <c7b21872e9ddb79ca62967d0e21e6a4518a669eb04648e963cdfb2426e62fafb@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: LordMelkor <kray@squareup.com>
joahg
left a comment
There was a problem hiding this comment.
🤖 This account isn't approving UI changes. This PR adds new views to the bundled File viewer (documents, sheets, slides, charts), so it needs a design or core-team reviewer. Because these files arrive as untrusted chat attachments, I focused on security. I found no P0 or P1.
Checked:
- DOM sinks: none in
src/bundled/file-viewer: nodangerouslySetInnerHTML,innerHTML,foreignObject,srcdocoreval. Parsers build hast directly. Every tree goes throughofficeSanitizerbeforetoJsxRuntime, and that sanitizer stripsscript/style/iframe/svg, allows nostyleattribute and limitssrctohttps. - Links and pictures:
FileLinkonly opens#anchors, Buzz links and http(s) without credentials, and only on click; anything else renders as text. Remote and linked pictures, including in slides, go through the same community-media gate as Markdown. Slide<img>also setsno-referrer. - Inline styling on slides and charts: colors are hex-validated, font names are restricted to a safe character class, and paths are built from numbers.
- Hostile files:
- The zip reader inflates in small pushes with caps per part, in total and per entry count.
vbaProject, macros and embeddings are never inflated.- XML rejects DOCTYPE and ENTITY.
- CFB chains are cycle-checked.
- RTF depth and PPT history cycles are bounded.
- The 50 MB limit checks both
Content-Lengthand the streamed total. - Nothing executes macros.
@e965/xlsx0.20.3: I compared the registry tarball with the official SheetJS 0.20.3 tarball from cdn.sheetjs.com. The library files are byte-identical. Onlypackage.json(name and repository) andREADME.mddiffer, and there are no install scripts.
Findings:
- P2, DOM clobbering via bookmark ids.
office-parse.tssetsclobber: []andclobberPrefix: "", which turns off rehype-sanitize's defaultuser-content-prefix that the Markdown path keeps. Meanwhileoffice/document.ts(docxbookmarkStart, about line 916) and the ODF bookmark handling emit the file's bookmark names as rawids. A document with a bookmark named after a global the app or a library reads, such asisTauri,configor a feature-detect name, creates a named element onwindowwhile the file is open. In the Tauri build, an own property shadows it forisTauri, so I couldn't turn this into an exploit. It's still fragile. Keeping the default prefix, or prefixing Office ids yourself, closes it, andfindIdinDocumentView.tsxalready looks up the prefixed form. - P3,
@e965/xlsxis a third-party republish. It has one maintainer. The current version matches upstream, but future bumps won't be checked automatically. Consider using the official tarball URL, or adding a note to NOTICE.md or the README saying the contents were verified against upstream for this version. - P3: SheetJS
read(sheets.ts) runs synchronously on the main thread, and abort is only checked between phases. An admitted but hostile.xls,.xlsbor.numbersfile can still freeze the UI for a while. A Worker would avoid that. - P3: the worst-case memory for one file is high: 50 MB in, up to 160 MB inflated plus 64 MB of media plus the DOM. It is bounded, but heavy for a webview.
Not reviewed in depth: I traced the outputs of odp.ts, pptx.ts and document.ts (ids, hrefs, colors, fonts, paths, pictures) to the sinks above, but didn't read every line. I didn't review the tests, the fixtures or the chart rendering math, and I didn't build or run the app.
|
Instructions to get the build to run: Still working through the community contribution model for UX changes so can't provide a review timeline as of now. Additional context can be found here: |
Opens Word, Excel, PowerPoint and OpenDocument files in the bundled File viewer, a follow-up to #3338 (scope). It ports Media viewer v1.5's Office reading. It was held back from #3338 to keep that PR reviewable (joahg/buzz-app#2), and is now rebased onto
main, with Office links and pictures moved onto main'sPartMarkdown.What changes for people
.docand.pptfiles show their text and say so.How
office/reads each format into a sanitized hast tree, sheets, slides or a preview;load.tsis the only entry point (openOffice(bytes, name, signal)). The trees pass through a sanitizer schema that allows ids, a few classes and table spans, and no styles.OfficeView.tsxopens the file and renders each family;office-parts.tssplits a document tree into parts for the existingPartBuffer;SlidesView.tsxandChartView.tsxdraw slides and charts.DocumentViewexportsOutlineand ausePartJumphook, so Office documents reuse the same outline and#linkjumps.PartMarkdownexportsPictureandFileLink, so pictures and links in Office files follow the same rules as Markdown parts: community media draws and anything else is its alt text;#links move within the file, Buzz and safe links open throughMessageLink, and anything else is its text.formats.tsrecognizes Office files by extension, then by MIME type, with a 50 MB limit.State machines
Audited with the state-machines skill:
loading | ready | failed, with an AbortController that the unmount aborts.key={target}onViewerresets it for a new file.reduceBufferfrom feat(file-viewer): bundle a file viewer for shared PDFs, Markdown, text and CSV #3338, unchanged.Efficiency
Sigil's Office data layer was reviewed by GPT Astra (High) as an efficiency oracle; its last review found no blocker.
Evidence
At the head of this PR:
pnpm lint(including check-icons): clean.src/bundled/file-viewerandsrc/features/markdown: 46 files, 1,604 tests passed.file-viewer.spec.mjs, Chromium and WebKit: 22 passed (11 per engine). New cases:notes.md; exactly one loads forfoundation.docx)git diff --check: clean apart from trailing spaces inside thertf-textutil.rtffixture, which are part of that file as macOStextutilwrites it.Known limitations
🤖 Generated with Claude Code