🧽 chore: Address follow-up review findings on PR #12934

Wraps up the 10-finding follow-up review. Two MAJOR + four MINOR + two NIT addressed; one NIT skipped after verifying it was a misread of the package.json structure. MAJOR - #1: Rewrite `renderOfficeHtml` JSDoc to document the HTML-or-null contract explicitly. The pre-fix doc described a text-fallback path that was the original XSS vector (commit b06f08a). A future maintainer trusting the stale doc could reintroduce the fallback. - #2: Replace byte-truncation of office HTML with a small "preview too large" banner document. Cutting at a UTF-8 boundary lands mid-tag (`<table><tr><td>con\n…[truncated]`) and ships malformed markup to the iframe — unpredictable rendering, occasional broken layouts on DOCX with embedded images / wide spreadsheets. MINOR - #4: Wrap `readSlidesFromZip`'s `zipfile.close()` in try/catch so a close-time exception (mid-flight stream) doesn't replace the original error. Mirrors the defensive pattern in zipSafety.ts. - #5: Refactor PPTX extraction to use `yauzl.fromBuffer` directly, eliminating the temp-file write/unlink the safety pre-flight already proved unnecessary. Removes 4 unused imports (os, path, fs/promises, randomUUID). - #6: Extract `isPreviewOnlyArtifact(type)` to `client/src/utils/ artifacts.ts` so the membership check is unit-testable without mounting the full Artifacts component (Recoil + Sandpack + media query). 15 new test cases covering positive types, negative types, null/undefined, and unknown strings. NIT - #3: Remove dead `stripColorStyles` / `COLOR_PROPERTY_PATTERN` — unused (sanitizer's `allowedStyles` config handles color implicitly). - #7: Remove dead `!_lc_csv_label` worksheet property write. - #9: Remove no-op `exclusiveFilter: () => false` sanitize-html config. - #10: Type-narrow `PREVIEW_ONLY_ARTIFACT_TYPES` to `ReadonlySet<ToolArtifactType>` so the membership table is compile-time checked against the enum. SKIPPED - #8: Reviewer flagged `sanitize-html` as duplicated in devDeps and dependencies. The package has no `dependencies` section — only `devDependencies` and `peerDependencies`. Existing convention (mammoth, xlsx, yauzl, pdfjs-dist) is to appear in BOTH. Removing the devDep entry would break local test runs. Tests: packages/api 4406/4406, client artifacts 128/128.
2026-10-22 01:53:33 +00:00 · 2026-05-04 10:49:25 +09:00 · 2026-05-04 10:49:25 +09:00 · afa3c54990
commit afa3c54990
parent 1feedceb88
6 changed files with 130 additions and 65 deletions
--- 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<string>([
-  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],
--- 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);
+    },
+  );
+});
--- 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<ToolArtifactType> = 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
--- 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 `<table><tr><td>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(/^<!DOCTYPE html>/);
+      expect(text!).toContain('</html>');
+      // 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 = '<!DOCTYPE html><html><body><p>hello</p></body></html>';
+      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 () => {
--- 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. `<table><tr><td>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 = `<!DOCTYPE html>
+<html lang="en"><head><meta charset="UTF-8"><title>Preview</title>
+<style>body{font-family:-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,sans-serif;color:#6b7280;padding:16px;font-size:14px;line-height:1.5}@media(prefers-color-scheme:dark){body{color:#9ca3af;background:#1a1a2e}}</style>
+</head><body>Preview exceeds the size limit. Download the file to view the full contents.</body></html>`;
+
 /**
 * 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}`,
--- 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<typeof import('sanitize-html')> {
  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<string> {
        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<string> {
   * 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<Array<{ number: number; xml: string }>> {
-  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<Array<{ number: number; xml: string }>> {
+function extractPptxSlideXml(buffer: Buffer): Promise<Array<{ number: number; xml: string }>> {
  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<Array<{ number: number; xm
          return;
        }
        settled = true;
-        zipfile.close();
+        try {
+          zipfile.close();
+        } catch {
+          /* zipfile.close() is best-effort — yauzl will throw if a
+           * stream is mid-flight. We've already settled the outer
+           * promise, so swallow this so the original error (if any)
+           * isn't replaced by a close-time exception. Mirrors
+           * `assertSafeZipSize`'s defensive pattern in zipSafety.ts. */
+        }
        if (error) {
          reject(error);
        } else {