Merge pull request #717 from dealerweb/fix/data-uri-allowlist-media-tags

fix: apply the data: URI allowlist to media tags too

components/email/email-viewer.tsx

@@ -5,7 +5,7 @@ import DOMPurify from "dompurify";
 import { Email, ContactCard, Mailbox } from "@/lib/jmap/types";
 import { emailExportFilename, attachmentDownloadFilename, attachmentsBundleFilename, DEFAULT_EMAIL_TEMPLATE, DEFAULT_ATTACHMENT_TEMPLATE } from "@/lib/download-filename";
 import { EML_IMPORT_ACCEPT, expandImportableEmails } from "@/lib/eml-import";
-import { EMAIL_IFRAME_SANITIZE_CONFIG, applyNewTabToAnchor, blockExternalResourcesOnNode, collapseBlockedImageContainers, escapeHtml, plainTextToSafeHtml, sanitizeEmailHtml, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization";
+import { EMAIL_IFRAME_SANITIZE_CONFIG, applyNewTabToAnchor, blockExternalResourcesOnNode, collapseBlockedImageContainers, escapeHtml, plainTextToSafeHtml, restrictDataUriResourcesOnNode, sanitizeEmailHtml, sanitizeEmailHtmlForIframe, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization";
 import { hasMeaningfulHtmlBody } from "@/lib/signature-utils";
 import { collapsePlainTextQuotes, setupQuoteCollapse } from "@/lib/quote-collapse";
 import { withBasePath } from "@/lib/browser-navigation";
@@ -1694,6 +1694,9 @@ export function EmailViewer({
           // http(s) links open in a new tab; other schemes keep their default.
           applyNewTabToAnchor(node);
 
+          // Re-apply the data:-URI allowlist DOMPurify skips on media tags.
+          restrictDataUriResourcesOnNode(node);
+
           // No dark mode color transforms - emails render true-to-life in iframe
         });
 
@@ -1786,7 +1789,7 @@ export function EmailViewer({
           return cidBlobUrls[cidRef] || 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
         }
       );
-      const cleanHtml = DOMPurify.sanitize(htmlWithCidUrls, EMAIL_IFRAME_SANITIZE_CONFIG);
+      const cleanHtml = sanitizeEmailHtmlForIframe(htmlWithCidUrls);
       return { html: cleanHtml, isHtml: true, hasStyleTag: /<style[\s>]/i.test(pluginRenderedHtml), externalBlocked: false };
     }
     if (pluginRenderedText) {
@@ -1794,7 +1797,7 @@ export function EmailViewer({
     }
     // TNEF (winmail.dat) extracted content
     if (tnefHtml) {
-      const cleanHtml = DOMPurify.sanitize(tnefHtml, EMAIL_IFRAME_SANITIZE_CONFIG);
+      const cleanHtml = sanitizeEmailHtmlForIframe(tnefHtml);
       return { html: cleanHtml, isHtml: true, hasStyleTag: /<style[\s>]/i.test(tnefHtml), externalBlocked: false };
     }
     if (tnefText) {
@@ -1802,7 +1805,7 @@ export function EmailViewer({
     }
     // Embedded message/rfc822 unwrapped content
     if (embeddedEmailHtml) {
-      const cleanHtml = DOMPurify.sanitize(embeddedEmailHtml, EMAIL_IFRAME_SANITIZE_CONFIG);
+      const cleanHtml = sanitizeEmailHtmlForIframe(embeddedEmailHtml);
       return { html: cleanHtml, isHtml: true, hasStyleTag: /<style[\s>]/i.test(embeddedEmailHtml), externalBlocked: false };
     }
     if (embeddedEmailText) {

components/email/thread-conversation-view.tsx

@@ -3,7 +3,7 @@
 import { useState, useEffect, useMemo, useRef, useCallback } from "react";
 import DOMPurify from "dompurify";
 import { Email, ThreadGroup } from "@/lib/jmap/types";
-import { EMAIL_SANITIZE_CONFIG, collapseBlockedImageContainers, plainTextToSafeHtml, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization";
+import { EMAIL_SANITIZE_CONFIG, collapseBlockedImageContainers, plainTextToSafeHtml, restrictDataUriResourcesOnNode, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization";
 import { hasMeaningfulHtmlBody } from "@/lib/signature-utils";
 import { collapsePlainTextQuotes, setupQuoteCollapse } from "@/lib/quote-collapse";
 import { transformInlineStyles, transformColorForDarkMode, transformBgColorForDarkMode } from "@/lib/color-transform";
@@ -364,6 +364,9 @@ function EmailCard({
         DOMPurify.addHook('afterSanitizeAttributes', (node) => {
           const htmlNode = node as HTMLElement;
 
+          // Re-apply the data:-URI allowlist DOMPurify skips on media tags.
+          restrictDataUriResourcesOnNode(node);
+
           if (!allowExternal) {
             if (node.tagName === 'IMG') {
               const src = node.getAttribute('src');

lib/__tests__/email-sanitization.test.ts

@@ -20,6 +20,8 @@ import {
   stripExternalCssUrls,
   stripExternalStyleSheetCss,
   blockExternalResourcesOnNode,
+  restrictDataUriResourcesOnNode,
+  sanitizeEmailHtmlForIframe,
   TRANSPARENT_BLOCKED_PIXEL,
 } from '../email-sanitization';
 
@@ -786,4 +788,101 @@ describe('email-sanitization', () => {
       expect(sanitizePluginBodyHtml('   ')).toBe('');
     });
   });
+
+  // ALLOWED_URI_REGEXP excludes image/svg+xml on purpose (DOMPurify cannot see
+  // inside a data: URI), but its media-tag carve-out short-circuits that check
+  // before the regexp runs - so every data: URI reached <img> and friends.
+  describe('restrictDataUriResourcesOnNode', () => {
+    const blocked = [
+      ['svg base64', 'data:image/svg+xml;base64,PHN2Zy8+'],
+      ['svg inline', 'data:image/svg+xml,<svg onload=alert(1)></svg>'],
+      ['text/html', 'data:text/html;base64,PHNjcmlwdD4='],
+      ['javascript', 'data:application/javascript,alert(1)'],
+      ['svg, leading space', ' data:image/svg+xml,x'],
+      ['svg, control chars in scheme', 'da\tta:image/svg+xml,x'],
+    ] as const;
+
+    for (const [label, uri] of blocked) {
+      it(`drops ${label} from an img src`, () => {
+        expect(sanitizeEmailHtmlForIframe(`<img src="${uri}">`)).not.toContain('data:');
+        expect(sanitizeEmailHtml(`<img src="${uri}">`)).not.toContain('data:');
+      });
+    }
+
+    it('covers the other media tags DOMPurify waves through', () => {
+      for (const tag of ['video', 'audio', 'source', 'track']) {
+        const html = `<${tag} src="data:image/svg+xml,x"></${tag}>`;
+        expect(sanitizeEmailHtmlForIframe(html)).not.toContain('data:');
+      }
+      // <image> is rewritten to <img> by the parser, href included.
+      expect(sanitizeEmailHtmlForIframe('<image href="data:image/svg+xml,x">')).not.toContain('data:');
+    });
+
+    it('keeps inline raster images and every other allowed scheme', () => {
+      expect(sanitizeEmailHtmlForIframe('<img src="data:image/png;base64,iVBORw0KGgo=">')).toContain('data:image/png');
+      expect(sanitizeEmailHtmlForIframe('<img src="data:image/gif;base64,R0lGODlh">')).toContain('data:image/gif');
+      expect(sanitizeEmailHtmlForIframe('<img src="data:image/jpeg;base64,/9j/4AAQ">')).toContain('data:image/jpeg');
+      expect(sanitizeEmailHtmlForIframe('<img src="cid:part1">')).toContain('cid:part1');
+      expect(sanitizeEmailHtmlForIframe('<img src="https://example.com/a.png">')).toContain('https://example.com/a.png');
+      expect(sanitizeEmailHtmlForIframe('<img src="blob:https://example.com/x">')).toContain('blob:');
+    });
+
+    it('leaves non-media elements to ALLOWED_URI_REGEXP', () => {
+      const node = parseHtmlSafely('<a href="data:image/svg+xml,x">x</a>').querySelector('a')!;
+      restrictDataUriResourcesOnNode(node);
+      expect(node.getAttribute('href')).toBe('data:image/svg+xml,x');
+    });
+
+    // The blocked-external placeholder shares this hook pass, so it must not be
+    // a data: URI the restriction would strip right back out.
+    it('preserves the blocked-image placeholder', () => {
+      const node = parseHtmlSafely('<img src="https://tracker.example/p.gif">').querySelector('img')!;
+      blockExternalResourcesOnNode(node);
+      restrictDataUriResourcesOnNode(node);
+      expect(node.getAttribute('src')).toBe(TRANSPARENT_BLOCKED_PIXEL);
+    });
+
+    // DOMPurify URI-tests srcset as one string, so an allowed first candidate
+    // would wave an SVG second candidate through (#717 review finding). The
+    // guard re-checks every data: occurrence and drops the whole attribute.
+    it('drops a srcset that smuggles an svg candidate behind a raster one', () => {
+      const html = '<img srcset="data:image/gif;base64,R0lGODlh 1x, data:image/svg+xml,<svg onload=alert(1)></svg> 2x">';
+      expect(sanitizeEmailHtmlForIframe(html)).not.toContain('svg');
+      expect(sanitizeEmailHtml(html)).not.toContain('svg');
+    });
+
+    it('drops a <picture><source srcset> with a disallowed data: candidate', () => {
+      const html = '<picture><source srcset="data:image/png;base64,AAAA 1x, data:image/svg+xml,<svg/> 2x"><img src="data:image/png;base64,AAAA"></picture>';
+      const clean = sanitizeEmailHtmlForIframe(html);
+      expect(clean).not.toContain('svg');
+      expect(clean).toContain('data:image/png;base64,AAAA');
+    });
+
+    it('keeps an all-raster srcset and non-data candidates', () => {
+      const raster = '<img srcset="data:image/png;base64,AAAA 1x, data:image/gif;base64,R0lGODlh 2x">';
+      expect(sanitizeEmailHtmlForIframe(raster)).toContain('srcset');
+      const node = parseHtmlSafely('<img srcset="https://example.com/a.png 1x, https://example.com/b.png 2x">').querySelector('img')!;
+      restrictDataUriResourcesOnNode(node);
+      expect(node.getAttribute('srcset')).toContain('example.com/a.png');
+    });
+
+    // The plain (non-base64) data: form ends its metadata with a comma; the
+    // srcset verdict must match the src verdict for the same URI.
+    it('keeps the comma-form raster data: URI a src would allow', () => {
+      const node = parseHtmlSafely('<img srcset="data:image/gif,R0lGODlh 1x">').querySelector('img')!;
+      restrictDataUriResourcesOnNode(node);
+      expect(node.getAttribute('srcset')).toContain('data:image/gif,R0lGODlh');
+    });
+
+    // The external blocker stashes the whole srcset before this guard runs in
+    // the shared hook pass - the stash must not keep the disallowed candidate.
+    it('scrubs the data-blocked-srcset stash the external blocker leaves', () => {
+      const node = parseHtmlSafely('<img srcset="https://tracker.example/a.png 1x, data:image/svg+xml,<svg/> 2x">').querySelector('img')!;
+      blockExternalResourcesOnNode(node);
+      expect(node.getAttribute('data-blocked-srcset')).toContain('svg');
+      restrictDataUriResourcesOnNode(node);
+      expect(node.hasAttribute('data-blocked-srcset')).toBe(false);
+      expect(node.hasAttribute('srcset')).toBe(false);
+    });
+  });
 });

lib/email-sanitization.ts

@@ -34,13 +34,27 @@ export const EMAIL_SANITIZE_CONFIG = {
   ],
 };
 
+/**
+ * Sanitize with `restrictDataUriResourcesOnNode` applied - the config alone
+ * cannot express it, because DOMPurify's data:-URI carve-out for media tags
+ * short-circuits ALLOWED_URI_REGEXP and is not removable via config.
+ */
+function sanitizeWithDataUriGuard(html: string, config: typeof EMAIL_SANITIZE_CONFIG): string {
+  DOMPurify.addHook('afterSanitizeAttributes', restrictDataUriResourcesOnNode);
+  try {
+    return DOMPurify.sanitize(html, config);
+  } finally {
+    DOMPurify.removeAllHooks();
+  }
+}
+
 /**
  * Sanitize email HTML content
  * @param html - Raw HTML content from email
  * @returns Sanitized HTML safe for rendering
  */
 export function sanitizeEmailHtml(html: string): string {
-  return DOMPurify.sanitize(html, EMAIL_SANITIZE_CONFIG);
+  return sanitizeWithDataUriGuard(html, EMAIL_SANITIZE_CONFIG);
 }
 
 /**
@@ -60,7 +74,7 @@ export const EMAIL_IFRAME_SANITIZE_CONFIG = {
  * Preserves <style> tags so the email's own CSS is applied.
  */
 export function sanitizeEmailHtmlForIframe(html: string): string {
-  return DOMPurify.sanitize(html, EMAIL_IFRAME_SANITIZE_CONFIG);
+  return sanitizeWithDataUriGuard(html, EMAIL_IFRAME_SANITIZE_CONFIG);
 }
 
 /**
@@ -221,12 +235,85 @@ export function sanitizePlainTextRenderedHtml(html: string): string {
 }
 
 /**
- * 1x1 transparent SVG used to replace a blocked external <img> so the layout
+ * 1x1 transparent GIF used to replace a blocked external <img> so the layout
  * doesn't reflow to a broken-image icon. The real URL is stashed in
  * `data-blocked-src` for restore.
+ *
+ * Deliberately a raster GIF, not an SVG: `restrictDataUriResourcesOnNode`
+ * removes every non-raster data: URI from media elements, and it runs in the
+ * same afterSanitizeAttributes pass as the external-content blocking above.
+ * An SVG placeholder would depend on which of the two happens to run first.
  */
 export const TRANSPARENT_BLOCKED_PIXEL =
-  'data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMSIgaGVpZ2h0PSIxIiB2aWV3Qm94PSIwIDAgMSAxIiBmaWxsPSJub25lIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPgo8cmVjdCB3aWR0aD0iMSIgaGVpZ2h0PSIxIiBmaWxsPSJ0cmFuc3BhcmVudCIvPgo8L3N2Zz4=';
+  'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
+
+/**
+ * Media elements where DOMPurify waves any `data:` URI through. Its attribute
+ * check short-circuits on `DATA_URI_TAGS` *before* ALLOWED_URI_REGEXP is
+ * consulted, and the set can only be extended via config (ADD_DATA_URI_TAGS),
+ * never trimmed - so the regexp's careful SVG exclusion is unreachable for
+ * these tags and has to be re-applied by hand. `<image>` is absent on purpose:
+ * the HTML parser rewrites it to `<img>`.
+ */
+const DATA_URI_BYPASS_TAGS = new Set(['IMG', 'VIDEO', 'AUDIO', 'SOURCE', 'TRACK']);
+
+/** Raster image types that stay allowed as a data: URI (mirrors ALLOWED_URI_REGEXP). */
+const RASTER_IMAGE_DATA_URI =
+  /^data:image\/(?:png|jpe?g|gif|webp|bmp|avif|x-icon|vnd\.microsoft\.icon)[;,]/i;
+
+/**
+ * True if a `srcset` value carries a `data:` URI that isn't an allowed inline
+ * raster image. srcset candidates are comma-separated, but a data: URI itself
+ * contains commas, so the value can't be split into candidates reliably.
+ * Instead every `data:` occurrence is checked where it stands: its MIME type
+ * must be on the raster allowlist. The token is captured through the first
+ * comma so the plain form (`data:image/gif,...`) keeps the `[;,]` terminator
+ * the raster regexp requires - same verdict as the src path for the same URI.
+ * One disallowed hit condemns the whole attribute. No whitespace
+ * pre-normalization here: whitespace splits srcset tokens before the URL
+ * parser ever runs, so unlike in `src` it cannot hide a scheme inside a
+ * candidate.
+ */
+function srcsetHasDisallowedDataUri(srcset: string): boolean {
+  const dataUris = srcset.match(/data:[^,\s]*,?/gi);
+  if (!dataUris) return false;
+  return dataUris.some((uri) => !RASTER_IMAGE_DATA_URI.test(uri));
+}
+
+/**
+ * Drop `data:` URIs that aren't inline raster images from media elements.
+ * DOMPurify cannot inspect the bytes inside a data: URI, so an SVG payload can
+ * carry <script>/<foreignObject> that the surrounding sanitizer never sees -
+ * the same reason ALLOWED_URI_REGEXP excludes image/svg+xml for every other
+ * element. Browsers render SVG-in-<img> in secure static mode, so this closes
+ * the gap between what the sanitizer promises and what it enforces rather than
+ * a live script vector.
+ */
+export function restrictDataUriResourcesOnNode(node: Element): void {
+  if (!DATA_URI_BYPASS_TAGS.has(node.tagName)) return;
+  for (const attr of ['src', 'href', 'xlink:href']) {
+    const value = node.getAttribute(attr);
+    if (!value) continue;
+    // Mirror the URL parser (and isExternalResourceUrl): C0 controls and
+    // spaces are ignored, so they can't be used to hide the scheme.
+    // eslint-disable-next-line no-control-regex
+    const normalized = value.replace(/[\u0000-\u0020]+/g, '');
+    if (/^data:/i.test(normalized) && !RASTER_IMAGE_DATA_URI.test(normalized)) {
+      node.removeAttribute(attr);
+    }
+  }
+  // DOMPurify URI-tests srcset as one string, so an allowed first candidate
+  // waves every later one through - re-check per candidate. The external
+  // blocker's data-blocked-srcset stash gets the same treatment: it shares
+  // this hook pass with no ordering guarantee, and a stashed disallowed
+  // candidate must not outlive the live attribute.
+  for (const attr of ['srcset', 'data-blocked-srcset']) {
+    const value = node.getAttribute(attr);
+    if (value && srcsetHasDisallowedDataUri(value)) {
+      node.removeAttribute(attr);
+    }
+  }
+}
 
 /**
  * True if a resource URL would trigger an external (network) fetch once the