From 6a9b227dda86b67c00852dbda3c1a11247b8a2d9 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 9 Aug 2026 16:50:32 -0700 Subject: [PATCH] fix(site-builder): repair dead-code data:image clause in HTML block URI regex Review follow-up on the Task 24 sanitiser widening (approved, no bypass found). Two Important findings to close: 1. ALLOWED_URI_REGEXP's data:image/...;base64, arm sat inside the group that appends a trailing `:` to every alternative, so it required a second colon no real data URI has -- the clause could never match. Confirmed dead before the fix (poster/cite/href all stripped a valid base64 PNG data URI) and working after (all three now survive), while javascript:/data:text/html stay blocked. Pulled the arm out into its own top-level alternative. 2. Corrected an inaccurate comment/report claim that every allowed attribute value goes through this regex -- `src` on img/video/audio/source/image/track is additionally covered by DOMPurify's own DATA_URI_TAGS allow-list, which is mimetype-blind and bypasses the regex entirely (acceptable: none of those tags execute src as a document; iframe is correctly excluded from that list). Adds two regression tests: the regex fix actually working, and the DATA_URI_TAGS bypass pinned so a future DOMPurify change surfaces as a failing test rather than a surprise. Fixture byte figures unchanged (15,815 -> 14,899; fixture has no data:image URIs). Co-Authored-By: Claude Opus 5 (1M context) --- .../basic/HtmlBlock.security.test.ts | 39 +++++++++++++++++++ craft/src/components/basic/HtmlBlock.tsx | 32 ++++++++++++--- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/craft/src/components/basic/HtmlBlock.security.test.ts b/craft/src/components/basic/HtmlBlock.security.test.ts index ddfcca7..079fd51 100644 --- a/craft/src/components/basic/HtmlBlock.security.test.ts +++ b/craft/src/components/basic/HtmlBlock.security.test.ts @@ -257,4 +257,43 @@ describe('purifyHtml -- Task 24: security properties on newly-allowed elements', ); expect(out).not.toContain('data:text/html'); }); + + test('review fix: data:image/*;base64, URIs now actually survive on poster/cite/href (dead-code regex bug)', () => { + // ALLOWED_URI_REGEXP used to put the data:image arm inside the group + // that gets a trailing `:` appended to every alternative, requiring a + // second colon after the one already in "base64," -- which no real + // data URI has, so the clause could never match anything. Confirm the + // fixed regex actually allows a real base64 image data URI through on + // ordinary URI-checked attributes (not just the DATA_URI_TAGS-covered + // src ones tested below). + const b64 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; + const out = purifyHtml( + `` + + `
q
` + + `img`, + ); + expect(out).toContain(`poster="data:image/png;base64,${b64}"`); + expect(out).toContain(`cite="data:image/png;base64,${b64}"`); + expect(out).toContain(`href="data:image/png;base64,${b64}"`); + }); + + test('documented reality: data: on img/video/audio/source src is mimetype-blind (DOMPurify DATA_URI_TAGS bypasses ALLOWED_URI_REGEXP)', () => { + // This is NOT gated by ALLOWED_URI_REGEXP at all -- DOMPurify has its + // own internal DATA_URI_TAGS allow-list (img, video, audio, source, + // image, track) that accepts ANY data: URI on the `src` attribute of + // those tags regardless of declared mimetype, before our regex is ever + // consulted. Acceptable because none of those tags execute their src + // as a document/script context in mainstream browsers -- the sink + // doesn't execute. Pinned here so a future DOMPurify version change to + // DATA_URI_TAGS shows up as a failing test, not a surprise in + // production. `); + expect(iframeOut).not.toContain('data:'); + }); }); diff --git a/craft/src/components/basic/HtmlBlock.tsx b/craft/src/components/basic/HtmlBlock.tsx index 98bfd2d..efad45c 100644 --- a/craft/src/components/basic/HtmlBlock.tsx +++ b/craft/src/components/basic/HtmlBlock.tsx @@ -41,11 +41,18 @@ const PURIFY_CONFIG = { // on*= handlers specifically to prove they still get neutralized/ // dropped by staying outside the allow-list. 'details','summary', - // Media (Task 24). All URL-bearing attributes on these (src, poster, - // srcset...) go through the same ALLOWED_URI_REGEXP gate as everything - // else -- see _isValidAttribute in dompurify, which URI-checks every - // allowed attribute value except a small fixed "inert" list (alt, - // class, id, style, title, ...) that never includes src/poster/srcset. + // Media (Task 24). URL-bearing attributes on these (poster, srcset, + // action, cite...) go through the ALLOWED_URI_REGEXP gate like + // everything else -- see _isValidAttribute in dompurify, which + // URI-checks every allowed attribute value except a small fixed + // "inert" list (alt, class, id, style, title, ...) that never includes + // src/poster/srcset. The one exception: `src` itself on img/video/ + // audio/source/image/track is additionally covered by DOMPurify's own + // `DATA_URI_TAGS` allow-list, which accepts any data: URI on those + // tag/attribute pairs regardless of mimetype, bypassing this regex -- + // see the ALLOWED_URI_REGEXP comment below and + // HtmlBlock.security.test.ts. Not a gap in the four non-negotiables: + // none of those tags execute their src as a document. 'picture','source','video','audio','track','canvas', // Forms (Task 24). Site owner's explicit decision: allow the full // ordinary form surface. No on*= survives (FORBID_ATTR below), and @@ -98,7 +105,20 @@ const PURIFY_CONFIG = { 'stop-color','stop-opacity','gradientunits','gradienttransform', 'preserveaspectratio', ], - ALLOWED_URI_REGEXP: /^(?:(?:https?|mailto|tel|data:image\/[a-z]+;base64,):|[^a-z]|[a-z+.-]+(?:[^a-z+.\-:]|$))/i, + // Review fix (Task 24 follow-up): the data:image arm used to sit inside + // the group that gets a trailing `:` appended for every alternative + // (`(?:https?|mailto|tel|data:image\/[a-z]+;base64,):`), so it required + // a SECOND colon after the one already in "base64,figure" -- no real + // data URI has that, so the clause could never match. It is now its own + // top-level alternative. NOTE: this regex is not the only thing gating + // data: URIs -- DOMPurify has its own internal `DATA_URI_TAGS` allow-list + // (img/video/audio/source/image/track) that accepts ANY data: URI on + // those tag/attribute pairs regardless of declared mimetype, bypassing + // this regex entirely. See HtmlBlock.security.test.ts for a regression + // test documenting that (acceptable: none of those tags execute their + // src as a document in mainstream browsers, and