diff --git a/client/src/components/Artifacts/Artifacts.tsx b/client/src/components/Artifacts/Artifacts.tsx index c9ddac3d17..0c5010633f 100644 --- a/client/src/components/Artifacts/Artifacts.tsx +++ b/client/src/components/Artifacts/Artifacts.tsx @@ -11,22 +11,11 @@ import useArtifacts from '~/hooks/Artifacts/useArtifacts'; import DownloadArtifact from './DownloadArtifact'; import ArtifactVersion from './ArtifactVersion'; import ArtifactTabs from './ArtifactTabs'; -import { TOOL_ARTIFACT_TYPES } from '~/utils/artifacts'; +import { isPreviewOnlyArtifact } from '~/utils/artifacts'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; import store from '~/store'; -/** - * Artifact types whose preview is server-rendered HTML — there's no source - * for a "code" view (the underlying file is binary). Hide the code tab and - * snap the active tab to "preview" when one of these is selected. - */ -const PREVIEW_ONLY_ARTIFACT_TYPES = new Set([ - TOOL_ARTIFACT_TYPES.DOCX, - TOOL_ARTIFACT_TYPES.SPREADSHEET, - TOOL_ARTIFACT_TYPES.PRESENTATION, -]); - const MAX_BLUR_AMOUNT = 32; const MAX_BACKDROP_OPACITY = 0.3; @@ -108,8 +97,7 @@ export default function Artifacts() { * generated HTML blob, which isn't useful. Filter the tab options and * snap the active tab when the user lands on an office artifact while * the code tab is selected. */ - const isPreviewOnly = - currentArtifact?.type != null && PREVIEW_ONLY_ARTIFACT_TYPES.has(currentArtifact.type); + const isPreviewOnly = isPreviewOnlyArtifact(currentArtifact?.type); const tabOptions = useMemo( () => (isPreviewOnly ? allTabOptions.filter((opt) => opt.value !== 'code') : allTabOptions), [allTabOptions, isPreviewOnly], diff --git a/client/src/utils/__tests__/artifacts.test.ts b/client/src/utils/__tests__/artifacts.test.ts index 2f3c99bd4c..b6c55c39a8 100644 --- a/client/src/utils/__tests__/artifacts.test.ts +++ b/client/src/utils/__tests__/artifacts.test.ts @@ -2,6 +2,7 @@ import { buildSandpackOptions, detectArtifactTypeFromFile, fileToArtifact, + isPreviewOnlyArtifact, languageForFilename, TOOL_ARTIFACT_TYPES, } from '../artifacts'; @@ -704,3 +705,33 @@ describe('fileToArtifact', () => { expect(artifact!.id).toBe('tool-artifact-index.html'); }); }); + +describe('isPreviewOnlyArtifact', () => { + /* The Artifacts panel hides the "code" tab and snaps `activeTab` to + * 'preview' when the current artifact is preview-only — i.e. the + * underlying file is binary and the generated HTML blob isn't a + * useful "code" view. Regression for review finding #6 on PR #12934. + * Without this test, removing a type from the predicate (or adding a + * non-office type) would silently leave users seeing the raw HTML + * blob in the code tab. */ + it.each([ + [TOOL_ARTIFACT_TYPES.DOCX, true], + [TOOL_ARTIFACT_TYPES.SPREADSHEET, true], + [TOOL_ARTIFACT_TYPES.PRESENTATION, true], + [TOOL_ARTIFACT_TYPES.HTML, false], + [TOOL_ARTIFACT_TYPES.REACT, false], + [TOOL_ARTIFACT_TYPES.MARKDOWN, false], + [TOOL_ARTIFACT_TYPES.MERMAID, false], + [TOOL_ARTIFACT_TYPES.CODE, false], + [TOOL_ARTIFACT_TYPES.PLAIN_TEXT, false], + ])('returns %s for type %s', (type, expected) => { + expect(isPreviewOnlyArtifact(type)).toBe(expected); + }); + + it.each([[null], [undefined], [''], ['application/pdf'], ['text/plain'], ['some/random-type']])( + 'returns false for non-artifact type %s', + (type) => { + expect(isPreviewOnlyArtifact(type)).toBe(false); + }, + ); +}); diff --git a/client/src/utils/artifacts.ts b/client/src/utils/artifacts.ts index e6cf67310c..4b36b1db69 100644 --- a/client/src/utils/artifacts.ts +++ b/client/src/utils/artifacts.ts @@ -296,6 +296,30 @@ export const TOOL_ARTIFACT_TYPES = { export type ToolArtifactType = (typeof TOOL_ARTIFACT_TYPES)[keyof typeof TOOL_ARTIFACT_TYPES]; +/** + * Artifact types whose preview is server-rendered HTML — there's no + * source for a "code" view because the underlying file is binary, and + * showing the generated HTML blob in a code editor would just be + * noise. Used by the artifacts panel to hide the code tab and snap + * the active tab to "preview" when one of these is selected. + * + * Exposed via a predicate (rather than the bare set) so callers can't + * accidentally widen the set and so the membership check is unit- + * testable without mounting the full Artifacts component. + */ +const PREVIEW_ONLY_ARTIFACT_TYPES: ReadonlySet = new Set([ + TOOL_ARTIFACT_TYPES.DOCX, + TOOL_ARTIFACT_TYPES.SPREADSHEET, + TOOL_ARTIFACT_TYPES.PRESENTATION, +]); + +export function isPreviewOnlyArtifact(type: string | null | undefined): boolean { + if (type == null) { + return false; + } + return PREVIEW_ONLY_ARTIFACT_TYPES.has(type as ToolArtifactType); +} + /** * Extension → fenced-code-block language hint for the CODE bucket. The * key is the lowercased file extension (no dot); the value is the diff --git a/packages/api/src/files/code/extract.spec.ts b/packages/api/src/files/code/extract.spec.ts index 1450d95641..60440051dc 100644 --- a/packages/api/src/files/code/extract.spec.ts +++ b/packages/api/src/files/code/extract.spec.ts @@ -322,7 +322,13 @@ describe('extractCodeArtifactText', () => { expect(parseDocumentCalls.length).toBe(0); }); - it('truncates HTML output when it exceeds the cache cap', async () => { + it('substitutes a "preview too large" banner when HTML exceeds the cache cap', async () => { + /* Regression for review finding #2 on PR #12934. The earlier + * implementation byte-truncated the producer's HTML at the + * UTF-8 boundary, which would land mid-tag and ship malformed + * markup like `
con\n…[truncated]` to the iframe. + * The new behavior swaps the entire payload for a small valid + * HTML banner — under the cap by construction. */ const huge = 'X'.repeat(MAX_TEXT_CACHE_BYTES + 5_000); mockOfficeHtml.mockResolvedValueOnce(huge); const text = await extractCodeArtifactText( @@ -332,8 +338,27 @@ describe('extractCodeArtifactText', () => { 'document', ); expect(text).not.toBeNull(); - expect(text!.endsWith('…[truncated]')).toBe(true); + // Always under the cap. expect(Buffer.byteLength(text!, 'utf-8')).toBeLessThanOrEqual(MAX_TEXT_CACHE_BYTES); + // Valid HTML doc, not byte-truncated markup. + expect(text!).toMatch(/^/); + expect(text!).toContain(''); + // No truncation marker (which would only appear on the legacy path). + expect(text!).not.toContain('…[truncated]'); + // User-facing banner content. + expect(text!).toContain('Preview exceeds the size limit'); + }); + + it('passes through HTML output unchanged when within the cache cap', async () => { + const small = '

hello

'; + mockOfficeHtml.mockResolvedValueOnce(small); + const text = await extractCodeArtifactText( + Buffer.from('PK'), + 'small.docx', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'document', + ); + expect(text).toBe(small); }); it('falls back to parseDocument for pdf (HTML producer returns null for unsupported types)', async () => { diff --git a/packages/api/src/files/code/extract.ts b/packages/api/src/files/code/extract.ts index 7f6b7e7af4..9f6b1e6adf 100644 --- a/packages/api/src/files/code/extract.ts +++ b/packages/api/src/files/code/extract.ts @@ -114,16 +114,37 @@ const extractDocument = async ( } }; +/** + * Minimal valid HTML document substituted when a producer's output + * exceeds `MAX_TEXT_CACHE_BYTES`. Byte-truncating the producer's HTML + * would land mid-tag (e.g. `
con\n…[truncated]`) and the + * Sandpack iframe would render the malformed markup unpredictably — + * see review finding #2 on PR #12934. The banner stays under the cap by + * construction. + */ +const OVERSIZED_HTML_BANNER = ` +Preview + +Preview exceeds the size limit. Download the file to view the full contents.`; + /** * Render an office-format buffer as a sanitized HTML preview document. Used * for docx, xlsx/xls/ods, csv, and pptx — produces interactive HTML the * frontend feeds into the Sandpack `static` template via `index.html`. * - * Soft-fails: returns `null` if the file isn't an office type, rendering - * timed out, or the parser threw. Caller can fall through to the existing - * text-extraction path on null. This is intentional graceful degradation — - * a malformed `.docx` whose HTML rendering fails should still get a plain- - * text preview if `parseDocument` can salvage one. + * **HTML-or-null contract** (security-critical, do NOT relax). Returns + * `null` if the file isn't an office type, rendering timed out, or the + * parser threw. Callers MUST NOT fall back to plain-text extraction on + * null — the client routes office types into `index.html` and would + * inject any text-shaped fallback as executable HTML, creating an XSS + * vector. The pre-fix versions of this function permitted text fallback + * and that path was a real vulnerability; see Codex P1 review on + * PR #12934 (commit b06f08a) for the original bug and remediation. + * + * On oversized output (>`MAX_TEXT_CACHE_BYTES`), returns a small + * "preview too large" banner document instead of byte-truncating the + * producer's HTML — slicing mid-tag would ship malformed markup to the + * iframe. */ const renderOfficeHtml = async ( buffer: Buffer, @@ -139,7 +160,10 @@ const renderOfficeHtml = async ( if (html == null) { return null; } - return truncate(html); + if (Buffer.byteLength(html, 'utf-8') > MAX_TEXT_CACHE_BYTES) { + return OVERSIZED_HTML_BANNER; + } + return html; } catch (error) { logger.debug( `[renderOfficeHtml] Failed to render "${name}" (${mimeType}): ${(error as Error).message}`, diff --git a/packages/api/src/files/documents/html.ts b/packages/api/src/files/documents/html.ts index f8d8c2a7c0..21a25b4f5a 100644 --- a/packages/api/src/files/documents/html.ts +++ b/packages/api/src/files/documents/html.ts @@ -1,7 +1,3 @@ -import * as os from 'os'; -import * as path from 'path'; -import * as fs from 'fs/promises'; -import { randomUUID } from 'crypto'; import yauzl from 'yauzl'; import { excelMimeTypes, megabyte } from 'librechat-data-provider'; import { assertSafeZipSize } from './zipSafety'; @@ -31,24 +27,6 @@ async function getSanitizer(): Promise { return sanitizeHtmlModule; } -/** - * Strip color-bearing inline styles so the dark/light wrapper around the - * sandboxed iframe controls foreground/background. Mammoth and SheetJS both - * emit hardcoded `color: black` / `background-color: white` declarations that - * make the preview unreadable in dark mode otherwise. - */ -const COLOR_PROPERTY_PATTERN = /(?:^|;)\s*(?:color|background|background-color)\s*:[^;]*/gi; -function stripColorStyles(style: string | undefined): string | undefined { - if (!style) { - return style; - } - const cleaned = style - .replace(COLOR_PROPERTY_PATTERN, '') - .replace(/^\s*;+\s*/, '') - .trim(); - return cleaned.length > 0 ? cleaned : undefined; -} - /** * Sanitize HTML produced by mammoth / SheetJS / our own pptx renderer for * embedding in the Sandpack `static` iframe. Allows the structural and @@ -139,7 +117,6 @@ export async function sanitizeOfficeHtml(html: string): Promise { attribs: { ...attribs, rel: 'noopener noreferrer', target: '_blank' }, }), }, - exclusiveFilter: () => false, /* sanitize-html runs `allowedAttributes` BEFORE per-attribute filtering, so * `style` only survives where we explicitly allow it (td/th/col/colgroup). * For those we still want to drop color declarations — defense against the @@ -432,10 +409,6 @@ export async function csvToHtml(buffer: Buffer): Promise { * formats; the default sheet name for CSV is `Sheet1` which we relabel * below for a friendlier tab. */ const workbook = XLSX.read(text, { type: 'string', raw: true }); - if (workbook.SheetNames.length > 0) { - const original = workbook.SheetNames[0]; - workbook.Sheets[original]['!_lc_csv_label'] = 'CSV'; - } const sheets = await renderWorkbookSheets(workbook, XLSX); // Single sheet for CSV — relabel to "CSV" for clarity, no tab strip emitted. const sanitized = await Promise.all( @@ -460,27 +433,19 @@ interface PptxSlide { * counts real decompressed bytes mid-inflate so a falsified central-directory * `uncompressedSize` cannot bypass the cap. Returns slides in slide-number * order; ignores everything else in the archive. + * + * Uses `yauzl.fromBuffer` (no disk I/O) — the safety pre-flight in + * `assertSafeZipSize` already proved the buffer is well-formed enough + * to walk, and fromBuffer keeps the hot path memory-only. */ -async function extractPptxSlideXml( - buffer: Buffer, -): Promise> { - const tempPath = path.join(os.tmpdir(), `pptx-${randomUUID()}`); - await fs.writeFile(tempPath, buffer); - try { - return await readSlidesFromZip(tempPath); - } finally { - fs.unlink(tempPath).catch(() => {}); - } -} - -function readSlidesFromZip(filePath: string): Promise> { +function extractPptxSlideXml(buffer: Buffer): Promise> { return new Promise((resolve, reject) => { - yauzl.open(filePath, { lazyEntries: true }, (err, zipfile) => { + yauzl.fromBuffer(buffer, { lazyEntries: true }, (err, zipfile) => { if (err) { return reject(err); } if (!zipfile) { - return reject(new Error('Failed to open PPTX file')); + return reject(new Error('Failed to open PPTX buffer')); } let settled = false; @@ -490,7 +455,15 @@ function readSlidesFromZip(filePath: string): Promise