fix: preserve SVG <use> elements and href attributes in sanitization#8520
fix: preserve SVG <use> elements and href attributes in sanitization#8520mscolnick merged 4 commits intomarimo-team:mainfrom
Conversation
Closes marimo-team#8316 DOMPurify was stripping <use> elements and their href/xlink:href attributes during sanitization, breaking SVGs that define shapes in <defs> and reference them via <use> (common in Matplotlib SVG output). Add "use" to ADD_TAGS and "href"/"xlink:href" to ADD_ATTR in the DOMPurify config so these elements survive sanitization.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
Updates the frontend HTML/SVG sanitization settings to preserve SVG <use> elements (and their reference attributes) so SVGs that rely on <defs> + <use>—such as common Matplotlib SVG output—render correctly when displayed as image/svg+xml.
Changes:
- Allowlist the SVG
<use>tag in the DOMPurify configuration. - Allowlist
hrefandxlink:hrefattributes to preserve<use>references. - Update and extend sanitizer tests to assert
<use>preservation and cover a<defs>+<use>pattern.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/plugins/core/sanitize.ts | Updates DOMPurify config to preserve SVG <use> and href/xlink:href attributes. |
| frontend/src/plugins/core/test/sanitize.test.ts | Updates tests to expect <use> preservation and adds a <defs> + <use> regression case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Allow SVG <use> elements and their href attributes, which are needed | ||
| // for SVGs that reference <defs> (e.g., Matplotlib SVG output). | ||
| ADD_TAGS: ["use"], | ||
| ADD_ATTR: ["href", "xlink:href"], |
There was a problem hiding this comment.
Now that <use> and href/xlink:href are explicitly allowlisted, we should add a regression test that sanitizeHtml still strips dangerous URI schemes for these attributes on <use> specifically (e.g., href="javascript:..." and xlink:href="javascript:..."). The existing SVG URI tests cover <a>, but not <use>, which is the newly-enabled surface area.
| const html = | ||
| '<svg width="60" height="60"><circle cx="30" cy="30" r="30" fill="orange"></circle><defs><circle id="myCircle" cx="0" cy="0" r="10" fill="green"></circle></defs><use href="#myCircle" x="20" y="20"></use></svg>'; | ||
| expect(sanitizeHtml(html)).toMatchInlineSnapshot( | ||
| `"<svg width="60" height="60"><circle cx="30" cy="30" r="30" fill="orange"></circle><defs><circle id="myCircle" cx="0" cy="0" r="10" fill="green"></circle></defs><use href="#myCircle" x="20" y="20"></use></svg>"`, | ||
| ); |
There was a problem hiding this comment.
The SVG string in this test is a single very long line, which makes it difficult to read and update. Consider formatting it as a multiline template literal (or otherwise wrapping it) so future diffs are more reviewable and the intent is clearer.
| const html = | |
| '<svg width="60" height="60"><circle cx="30" cy="30" r="30" fill="orange"></circle><defs><circle id="myCircle" cx="0" cy="0" r="10" fill="green"></circle></defs><use href="#myCircle" x="20" y="20"></use></svg>'; | |
| expect(sanitizeHtml(html)).toMatchInlineSnapshot( | |
| `"<svg width="60" height="60"><circle cx="30" cy="30" r="30" fill="orange"></circle><defs><circle id="myCircle" cx="0" cy="0" r="10" fill="green"></circle></defs><use href="#myCircle" x="20" y="20"></use></svg>"`, | |
| ); | |
| const html = ` | |
| <svg width="60" height="60"> | |
| <circle cx="30" cy="30" r="30" fill="orange"></circle> | |
| <defs> | |
| <circle id="myCircle" cx="0" cy="0" r="10" fill="green"></circle> | |
| </defs> | |
| <use href="#myCircle" x="20" y="20"></use> | |
| </svg> | |
| `; | |
| expect(sanitizeHtml(html)).toMatchInlineSnapshot(` | |
| "<svg width=\"60\" height=\"60\"> | |
| <circle cx=\"30\" cy=\"30\" r=\"30\" fill=\"orange\"></circle> | |
| <defs> | |
| <circle id=\"myCircle\" cx=\"0\" cy=\"0\" r=\"10\" fill=\"green\"></circle> | |
| </defs> | |
| <use href=\"#myCircle\" x=\"20\" y=\"20\"></use> | |
| </svg>" | |
| `); |
…lements Address review feedback: - Add tests verifying javascript: href and xlink:href are stripped from <use> - Reformat defs+use SVG test string as array join for readability
2b71611 to
94b135f
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `"<svg><use></use></svg>"`, | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The new <use> coverage verifies javascript: is stripped, but it doesn’t exercise non-javascript: external references (e.g. https://...#id or data:image/svg+xml,...). Adding a test that asserts external/non-fragment href/xlink:href values are removed (if that’s the intended security posture) would help prevent regressions and clarify the expected behavior for <use>.
| test("removes external href from SVG use element", () => { | |
| const html = | |
| '<svg><use href="https://example.com/sprite.svg#icon"></use></svg>'; | |
| expect(sanitizeHtml(html)).toMatchInlineSnapshot( | |
| `"<svg><use></use></svg>"`, | |
| ); | |
| }); | |
| test("removes external xlink:href from SVG use element", () => { | |
| const html = | |
| '<svg><use xlink:href="https://example.com/sprite.svg#icon"></use></svg>'; | |
| expect(sanitizeHtml(html)).toMatchInlineSnapshot( | |
| `"<svg><use></use></svg>"`, | |
| ); | |
| }); | |
| test("removes data URI href from SVG use element", () => { | |
| const html = | |
| '<svg><use href="data:image/svg+xml,%3Csvg%3E%3C/svg%3E"></use></svg>'; | |
| expect(sanitizeHtml(html)).toMatchInlineSnapshot( | |
| `"<svg><use></use></svg>"`, | |
| ); | |
| }); |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.20.3-dev94 |
Summary
Fixes #8316.
SVGs containing
<defs>and<use>elements (common in Matplotlib SVG output) were not rendering correctly when displayed viaimage/svg+xmlMIME type. DOMPurify was stripping<use>elements and theirhref/xlink:hrefattributes during sanitization.Changes
Added
ADD_TAGS: ["use"]andADD_ATTR: ["href", "xlink:href"]to the DOMPurify config insanitize.ts. This preserves SVG<use>elements and their reference attributes while keeping all other sanitization intact.javascript:protocol inhrefattributes is still blocked by DOMPurify at the protocol level, so this does not introduce XSS risk.Test Plan
Updated
sanitize.test.ts:<use>removal to asserting preservation<defs>+<use>SVG pattern (matching the issue reproduction case)