From b41055b5ba899ca7d23efbed8e6c352f7500bb7b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 06:49:49 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Enforce=20extens?= =?UTF-8?q?ion-wins=20precedence=20in=20officeHtmlBucket?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on #12934 caught that the predicate's if-chain interleaved extension and MIME checks for each bucket — e.g. CSV's branch was `ext === 'csv' || CSV_MIME_PATTERN.test(mimeType)`. A `deck.pptx` shipped with `text/csv` (sandboxed tools sometimes ship generic MIMEs) matched the CSV branch BEFORE the PPTX extension branch was reached, so a binary PPTX would have been handed to `csvToHtml` to parse as text — yielding garbage or a parse exception. Restructure to a strict two-pass dispatch: an exhaustive extension table first (one lookup, all known extensions), then MIME-only fallback for extensionless / unknown-ext inputs. The doc comment's "extension wins" claim is now actually enforced by the implementation. Add 7 regression cases covering the conflicting-MIME footgun for each bucket: deck.pptx + text/csv → pptx; workbook.xlsx + text/csv → spreadsheet; legacy.xls + pptx-MIME → spreadsheet; report.docx + text/csv → docx; data.csv + docx-MIME → csv; etc. --- packages/api/src/files/documents/html.spec.ts | 26 ++++++++++++ packages/api/src/files/documents/html.ts | 42 ++++++++++++------- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/packages/api/src/files/documents/html.spec.ts b/packages/api/src/files/documents/html.spec.ts index fffa8aa011..bed14a2e57 100644 --- a/packages/api/src/files/documents/html.spec.ts +++ b/packages/api/src/files/documents/html.spec.ts @@ -262,6 +262,32 @@ describe('Office HTML producers', () => { ])('returns null for non-office (%s, %s)', (name, mime) => { expect(officeHtmlBucket(name, mime)).toBeNull(); }); + + /* Regression for Codex P2 review on PR #12934. A binary office file + * with a mismatched MIME (e.g. a tool sandbox sets `text/csv` on + * everything it ships) must NOT be re-routed to a different bucket + * just because the MIME matches a different bucket's pattern. The + * documented "extension wins" precedence is enforced by checking + * extensions exhaustively before any MIME pattern fires. */ + it.each([ + ['deck.pptx', 'text/csv', 'pptx'], + ['workbook.xlsx', 'text/csv', 'spreadsheet'], + [ + 'legacy.xls', + 'application/vnd.openxmlformats-officedocument.presentationml.presentation', + 'spreadsheet', + ], + ['sheet.ods', 'text/csv', 'spreadsheet'], + ['report.docx', 'text/csv', 'docx'], + ['report.docx', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', 'docx'], + [ + 'data.csv', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'csv', + ], + ])('extension wins over conflicting MIME: (%s, %s) → %s', (name, mime, expected) => { + expect(officeHtmlBucket(name, mime)).toBe(expected); + }); }); describe('sanitizeOfficeHtml security', () => { diff --git a/packages/api/src/files/documents/html.ts b/packages/api/src/files/documents/html.ts index 13e5826401..d4df8e30fe 100644 --- a/packages/api/src/files/documents/html.ts +++ b/packages/api/src/files/documents/html.ts @@ -663,33 +663,47 @@ function extensionOf(name: string): string { */ export type OfficeHtmlBucket = 'docx' | 'spreadsheet' | 'csv' | 'pptx'; +const OFFICE_EXTENSIONS: Record = { + docx: 'docx', + csv: 'csv', + xlsx: 'spreadsheet', + xls: 'spreadsheet', + ods: 'spreadsheet', + pptx: 'pptx', +}; + /** * Classify a file by extension OR MIME into the bucket the office HTML * dispatcher would route it to, or `null` if no producer applies. * - * Extension wins over MIME because content sniffing routinely labels these - * formats as `application/zip` or `application/octet-stream`. MIME-only - * routing is the fallback for extensionless filenames where the upstream - * supplied a useful Content-Type header. + * **Extension always wins** over MIME for any known office extension. + * Without this precedence, a `deck.pptx` whose `Content-Type` was + * mislabeled `text/csv` (a known footgun when tool sandboxes ship + * generic MIMEs) would be routed to `csvToHtml` and try to parse a + * binary PPTX as CSV — yielding garbage at best, a parse exception at + * worst. MIME-only routing is the fallback for extensionless filenames + * (or extensions outside our known set) where the upstream supplied a + * useful Content-Type header. */ export function officeHtmlBucket(name: string, mimeType: string): OfficeHtmlBucket | null { const ext = extensionOf(name); - if (ext === 'docx' || mimeType === DOCX_MIME) { + const byExtension = OFFICE_EXTENSIONS[ext]; + if (byExtension) { + return byExtension; + } + /* MIME-only fallback. Order doesn't matter here — there are no + * overlapping MIME patterns across buckets, and the extension table + * has already taken precedence above for any conflicting input. */ + if (mimeType === DOCX_MIME) { return 'docx'; } - if (ext === 'csv' || CSV_MIME_PATTERN.test(mimeType)) { + if (CSV_MIME_PATTERN.test(mimeType)) { return 'csv'; } - if ( - ext === 'xlsx' || - ext === 'xls' || - ext === 'ods' || - excelMimeTypes.test(mimeType) || - mimeType === ODS_MIME - ) { + if (excelMimeTypes.test(mimeType) || mimeType === ODS_MIME) { return 'spreadsheet'; } - if (ext === 'pptx' || mimeType === PPTX_MIME) { + if (mimeType === PPTX_MIME) { return 'pptx'; } return null;