fix: apply the data: URI allowlist to media tags too - #717
Open
dealerweb wants to merge 1 commit into
Open
Conversation
ALLOWED_URI_REGEXP excludes image/svg+xml on purpose - DOMPurify cannot inspect the bytes inside a data: URI, so an SVG payload can carry <script>/<foreignObject> the surrounding sanitizer never sees. That exclusion never took effect on <img>, <video>, <audio>, <source> and <track>: DOMPurify's attribute check short-circuits on DATA_URI_TAGS before ALLOWED_URI_REGEXP is consulted, and that set can only be extended via ADD_DATA_URI_TAGS, never trimmed. Any data: URI reached those tags, including image/svg+xml and text/html; <a href> behaved as documented. Not a live script vector - browsers render SVG-in-<img> in secure static mode - but the sanitizer promised something it did not enforce, and the print window renders sanitized bodies without the iframe sandbox and CSP that otherwise back it up. restrictDataUriResourcesOnNode re-applies the raster allowlist on those tags, wired in through the existing afterSanitizeAttributes hooks. The blocked-external-image placeholder becomes a 1x1 GIF: it is set in the same hook pass, so an SVG placeholder would depend on hook order. The three viewer call sites that hand-rolled DOMPurify.sanitize(html, EMAIL_IFRAME_SANITIZE_CONFIG) now use sanitizeEmailHtmlForIframe, which is the same thing and picks up the guard automatically.
dealerweb
force-pushed
the
fix/data-uri-allowlist-media-tags
branch
from
July 31, 2026 11:47
f653165 to
f4b21c8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ALLOWED_URI_REGEXPexcludesimage/svg+xmlon purpose, with the reason spelled out in the config: DOMPurify cannot inspect the bytes inside adata:URI, so an SVG payload can carry<script>/<foreignObject>the surrounding sanitizer never sees. That exclusion never took effect on<img>,<video>,<audio>,<source>and<track>- anydata:URI reached them, includingimage/svg+xmlandtext/html, while<a href>behaved exactly as documented. Not a live script vector (browsers render SVG-in-<img>in secure static mode), but the sanitizer promised something it did not enforce, andhandlePrintwrites sanitized bodies into awindow.opendocument with neither the iframe sandbox nor the strict CSP that back up the normal viewer.Root cause
DOMPurify's attribute check short-circuits on
DATA_URI_TAGSbeforeALLOWED_URI_REGEXPis consulted:DATA_URI_TAGSdefaults toaudio, video, img, source, image, trackand resolves via_resolveSetOption(cfg, 'ADD_DATA_URI_TAGS', DEFAULT_DATA_URI_TAGS, { base: DEFAULT_DATA_URI_TAGS })- it can only be extended, never trimmed. The config cannot express the restriction, so a hook is the only way to re-apply it.Changes
restrictDataUriResourcesOnNodedropsdata:URIs that are not inline raster images from the affected tags, checkingsrc/href/xlink:hrefand normalising C0 controls first (the same treatmentisExternalResourceUrlalready applies).<image>needs no case of its own - the HTML parser rewrites it to<img>.afterSanitizeAttributeshooks in the viewer and the thread view, next toapplyNewTabToAnchor;sanitizeEmailHtml/sanitizeEmailHtmlForIframeapply it through theaddHook/try/finallyidiom already used by the signature sanitizers.TRANSPARENT_BLOCKED_PIXELbecomes a 1x1 GIF instead of a 1x1 SVG. It is assigned inside the same hook pass, so an SVG placeholder would be stripped or kept depending on hook order; a raster placeholder removes that coupling. The constant is compared by identity in the existing tests, so nothing else changes.DOMPurify.sanitize(html, EMAIL_IFRAME_SANITIZE_CONFIG)(plugin-rendered, TNEF, embeddedmessage/rfc822) now callsanitizeEmailHtmlForIframe, which is the same call and picks up the guard automatically.lib/__tests__/email-sanitization.test.ts, eight of which fail against the previous code: every blocked type (svg base64, svg inline,text/html,application/javascript, leading space, control chars in the scheme) through both email sanitizers; the other affected tags (video,audio,source,track,<image href>); the sources that must survive (data:image/png|gif|jpeg,cid:,https:,blob:);<a href>staying withALLOWED_URI_REGEXP; and the blocked-image placeholder survivingblockExternalResourcesOnNodeplus the new restriction in the same pass.Related issues
None - found while reviewing Bulwark against the OWA half-click XSS (CVE-2026-42897 / TA488).
Type of change
Checklist
npm run typecheck && npm run lintand there are no errorsnpm run build)locales/) if my changes affect user-facing textScreenshots / demo
No UI changes - sanitizer logic only, nothing visible to show. No user-facing strings changed either, so
locales/is untouched.Notes for reviewers
npm run typecheckclean,npm run lint0 errors (8 pre-existing warnings, all in untouched calendar files),npx vitest run2341/2341,npm run buildsucceeds,npm run test:translations48/48.npm run test:integrationwas not run - no Docker stack on this machine - so the Playwright pass over the render path is still owed if you want it.FORBID_ATTR(onbeforetoggle,onanimationstart,onpointerover, ...) at the sanitizers; all were neutralised, since DOMPurify's allowlist is what does the work there. Thedata:gap was the one thing that came back.sanitizeWithDataUriGuardis extracted rather than inlined twice, unlike the signature pair - there the two hooks genuinely differ, here they are identical and the shared helper keeps them in sync. Happy to inline it if you prefer.sanitizePlainTextRenderedHtml,sanitizeI18nHtmland the signature sanitizers are deliberately untouched: the first two use strictALLOWED_TAGSlists with no media elements, and signatures already restrictimg srcthroughrestrictSignatureImages.jmap-client-resilience's keep-alive tests fail intermittently (fake timers withshouldAdvanceTime, which the file itself flags). Reproduced on unmodifiedmain, roughly one run in three.