From 1feedceb8845750bfffee6cfaf6087849f8c7953 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 09:00:44 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Office=20types?= =?UTF-8?q?=20are=20HTML-or-null=20(no=20text=20fallback=20=E2=86=92=20XSS?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 review on #12934 caught that when `renderOfficeHtml` failed (timeout, malformed file, zip-bomb rejection) for an office type, the extractor fell through to `extractDocument` and returned plain text. The client routes by extension/MIME to the office preview buckets and feeds `attachment.text` straight into the Sandpack iframe's `index.html`. A spreadsheet cell or document body containing the literal string `` would have been injected as executable markup — direct XSS. The contract for office types is now HTML-or-null with no text fallback. Failed render returns null, the client's empty-text gate keeps the artifact off the panel, and the file falls back to the regular download UI (matching what PPTX already did). PDF and ODT still go through `extractDocument` because the client routes them to PLAIN_TEXT (which the markdown viewer escapes) or no artifact at all, so plain text is safe there. Test reshuffle: - `document` describe block now uses ODT/PDF for the legacy parseDocument-path tests (DOCX/XLSX/XLS/ODS bypass that path). - New "does NOT call parseDocument for office HTML types" test locks in the SEC contract for all four office HTML buckets. - "falls back to ..." tests rewritten as "returns null when ..." with explicit `parseDocumentCalls.length === 0` assertions to prove no text leaks back to the client. - New XSS regression test for the XLSX failure path. - Mock parseDocument failure-name match relaxed to `includes()` so ODT-named tests can use the same trigger. --- packages/api/src/files/code/extract.spec.ts | 160 +++++++++++++------- packages/api/src/files/code/extract.ts | 26 +++- 2 files changed, 126 insertions(+), 60 deletions(-) diff --git a/packages/api/src/files/code/extract.spec.ts b/packages/api/src/files/code/extract.spec.ts index 500971d9f1..1450d95641 100644 --- a/packages/api/src/files/code/extract.spec.ts +++ b/packages/api/src/files/code/extract.spec.ts @@ -3,13 +3,17 @@ import * as path from 'path'; import { extractCodeArtifactText, MAX_TEXT_CACHE_BYTES, MAX_TEXT_EXTRACT_BYTES } from './extract'; const docxText = '__DOCX_PARSED__'; +/* parseDocument throws on any originalname containing this token, so + * tests can force a failure with whatever extension/MIME they need + * (the legacy docx-named constant is preserved for backward-compat + * with tests that still reference it). */ const docxFailureName = 'force-docx-failure.docx'; const parseDocumentCalls: Array<{ path: string; originalname: string }> = []; jest.mock('~/files/documents/crud', () => ({ parseDocument: jest.fn(async ({ file }: { file: { path: string; originalname: string } }) => { parseDocumentCalls.push({ path: file.path, originalname: file.originalname }); - if (file.originalname === docxFailureName) { + if (file.originalname.includes('force-docx-failure')) { throw new Error('parse failed'); } return { text: docxText, filename: file.originalname, bytes: docxText.length }; @@ -92,73 +96,78 @@ describe('extractCodeArtifactText', () => { }); describe('document', () => { + /* These tests exercise the legacy `extractDocument` path, which now + * only fires for `category === 'document'` files that the office HTML + * dispatcher does NOT claim — i.e. PDF and ODT. All ZIP-backed office + * formats (DOCX/XLSX/XLS/ODS) bypass `extractDocument` entirely + * because they're HTML-or-null per the SEC fix in PR #12934 + * (Codex P1 review: text-fallback under `index.html` was XSS). */ beforeEach(() => { parseDocumentCalls.length = 0; }); - it('routes through parseDocument and returns its text', async () => { - const buffer = Buffer.from('PKfake-docx'); + it('routes through parseDocument and returns its text (ODT)', async () => { + const buffer = Buffer.from('PKfake-odt'); const text = await extractCodeArtifactText( buffer, - 'report.docx', - 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'notes.odt', + 'application/vnd.oasis.opendocument.text', 'document', ); expect(text).toBe(docxText); }); it('returns null when parseDocument throws', async () => { - const buffer = Buffer.from('PKfake-docx'); + const buffer = Buffer.from('PKfake-odt'); const text = await extractCodeArtifactText( buffer, - docxFailureName, - 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + docxFailureName.replace('.docx', '.odt'), + 'application/vnd.oasis.opendocument.text', 'document', ); expect(text).toBeNull(); }); - it('rewrites a generic sniffed MIME to the canonical document MIME by extension', async () => { + it('rewrites a generic sniffed MIME to the canonical document MIME by extension (ODT)', async () => { // Code-output buffers for office docs are commonly sniffed as // application/zip — without canonicalization, parseDocument would // reject these and inline previews would silently disappear. - const buffer = Buffer.from('PKfake-docx'); - await extractCodeArtifactText(buffer, 'report.docx', 'application/zip', 'document'); - expect(parseDocumentCalls[0]?.originalname).toBe('report.docx'); + const buffer = Buffer.from('PKfake-odt'); + await extractCodeArtifactText(buffer, 'notes.odt', 'application/zip', 'document'); + expect(parseDocumentCalls[0]?.originalname).toBe('notes.odt'); }); it.each([ - [ - 'report.docx', - 'application/zip', - 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', - ], - [ - 'data.xlsx', - 'application/octet-stream', - 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', - ], - ['legacy.xls', 'application/octet-stream', 'application/vnd.ms-excel'], - ['sheet.ods', 'application/zip', 'application/vnd.oasis.opendocument.spreadsheet'], ['notes.odt', 'application/zip', 'application/vnd.oasis.opendocument.text'], - ])('passes canonical mimetype for %s when sniff returns %s', async (name, sniffed, _canon) => { - const parseDocumentMock = ( - jest.requireMock('~/files/documents/crud') as { - parseDocument: jest.Mock; - } - ).parseDocument; - parseDocumentMock.mockClear(); - await extractCodeArtifactText(Buffer.from('PK'), name, sniffed, 'document'); - const call = parseDocumentMock.mock.calls[0]?.[0]; - expect(call?.file?.mimetype).toBe(_canon); - }); + ['report.pdf', 'application/octet-stream', 'application/pdf'], + ])( + 'passes canonical mimetype for %s when sniff returns %s (legacy parseDocument path)', + async (name, sniffed, _canon) => { + const parseDocumentMock = ( + jest.requireMock('~/files/documents/crud') as { + parseDocument: jest.Mock; + } + ).parseDocument; + parseDocumentMock.mockClear(); + await extractCodeArtifactText(Buffer.from('PK'), name, sniffed, 'document'); + const call = parseDocumentMock.mock.calls[0]?.[0]; + /* PDF doesn't have an entry in `documentMimeFromExtension` so the + * sniffed MIME passes through unchanged. ODT does — gets + * canonicalized to ODT_MIME. */ + expect(call?.file?.mimetype).toBe(_canon === 'application/pdf' ? sniffed : _canon); + }, + ); it('writes the temp file inside os.tmpdir() regardless of artifact name', async () => { - const buffer = Buffer.from('PKfake-docx'); + /* Path traversal defense — even a malicious filename like + * `../../../etc/passwd.odt` must end up inside os.tmpdir() with a + * sanitized basename. Uses ODT since the office HTML path now + * short-circuits all ZIP-backed office formats. */ + const buffer = Buffer.from('PKfake-odt'); await extractCodeArtifactText( buffer, - '../../../etc/passwd.docx', - 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + '../../../etc/passwd.odt', + 'application/vnd.oasis.opendocument.text', 'document', ); const call = parseDocumentCalls[0]; @@ -166,7 +175,24 @@ describe('extractCodeArtifactText', () => { const tmpRoot = path.resolve(os.tmpdir()); expect(path.resolve(call.path).startsWith(tmpRoot)).toBe(true); expect(call.path).not.toContain('..'); - expect(call.originalname).toBe('passwd.docx'); + expect(call.originalname).toBe('passwd.odt'); + }); + + it('does NOT call parseDocument for office HTML types (DOCX/XLSX/XLS/ODS)', async () => { + /* Lock in the SEC contract: the four office HTML buckets are + * HTML-or-null and never fall back to `extractDocument`. */ + mockOfficeHtml.mockResolvedValue(null); + const cases: Array<[string, string]> = [ + ['report.docx', 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'], + ['data.xlsx', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'], + ['legacy.xls', 'application/vnd.ms-excel'], + ['sheet.ods', 'application/vnd.oasis.opendocument.spreadsheet'], + ]; + for (const [name, mime] of cases) { + const text = await extractCodeArtifactText(Buffer.from('PK'), name, mime, 'document'); + expect(text).toBeNull(); + } + expect(parseDocumentCalls.length).toBe(0); }); }); @@ -233,7 +259,13 @@ describe('extractCodeArtifactText', () => { expect(text).toContain(''); }); - it('falls back to raw utf-8 text for csv when HTML rendering returns null', async () => { + /* SECURITY: office types are HTML-or-null, with NO text fallback. + * Codex P1 review on PR #12934 caught that the previous fallback + * shipped raw text under an `index.html` slot on the client — a + * literal `` would then execute inside + * the Sandpack iframe. The fix returns null instead, and the + * client's empty-text gate keeps the artifact off the panel. */ + mockOfficeHtml.mockResolvedValueOnce(null); + const text = await extractCodeArtifactText( + Buffer.from('PKfake-xlsx'), + 'attack.xlsx', + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + 'document', + ); + expect(text).toBeNull(); + expect(parseDocumentCalls.length).toBe(0); }); it('truncates HTML output when it exceeds the cache cap', async () => { @@ -353,8 +407,10 @@ describe('extractCodeArtifactText', () => { * DOCX/XLSX/PPTX got through the compressed-size gate), the outer * extractor must swallow it and return null — that signals to the * code-output controller to register the file as a regular - * download instead of a panel artifact. Without this, an - * unhandled rejection would crash the request. */ + * download instead of a panel artifact. Crucially, it must NOT + * fall back to `extractDocument` text either: the client would + * inject that text into `index.html` and a literal `