SVG Sandboxing by KManolov3 · Pull Request #567 · scratchfoundation/scratch-editor

KManolov3 · 2026-05-11T15:35:48Z

Resolves

https://scratchfoundation.atlassian.net/browse/UEPR-231

Proposed Changes

Load SVGs into a sandboxed iframe for measurement vs directly into the DOM.
Introduce a new function for removing malicious content from SVGs - canonicalizeSvgText
TODO: Route all svg loads through canonicalizeSvgText - at the point of loadVector_

Reason for Changes

Currently we attempt to sanitize SVGs, but the approach is piecemeal. The biggest security issue in the current state is that we load SVGs directly into the DOM, which is an inherently unsafe operation.

Test Coverage

Added tests for sandboxing, canonicalization and measuring SVGs in a sandboxed environment.

github-actions · 2026-05-11T15:43:05Z

Test Results

8 files ± 0 1 146 suites +32 12m 54s ⏱️ -43s
2 960 tests +115 2 948 ✅ +111 8 💤 ±0 4 ❌ +4
6 618 runs +147 6 571 ✅ +139 39 💤 ±0 8 ❌ +8

For more details on these failures, see this check.

Results for commit a069b29. ± Comparison against base commit 825af6e.

This pull request removes 1 and adds 116 tests. Note that renamed tests count towards both.

test/visual-fidelity.js > visual fidelity: sanitized cat costume matches committed snapshot ‑ sanitized cat costume vs snapshot: 0 differing pixels (tolerance 0)

test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ internal mask reference preserved
test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ no animate
test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ no foreignObject
test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ no script
test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ no set
test/canonicalize-svg.js > canonicalize: handles blocked-elements.svg fixture ‑ safe rect preserved
test/canonicalize-svg.js > canonicalize: handles css-links.svg fixture ‑ internal mask reference preserved
test/canonicalize-svg.js > canonicalize: handles css-links.svg fixture ‑ no external URLs
test/canonicalize-svg.js > canonicalize: handles css-links.svg fixture ‑ no style elements
test/canonicalize-svg.js > canonicalize: handles external-hrefs.svg fixture ‑ data: URI preserved
…

♻️ This comment has been updated with latest results.

rubiidev18alt · 2026-05-12T21:13:10Z

Please keep the cool css theming via svg 🙏

adzhindzhi · 2026-05-13T10:00:52Z

    updateSVGSkin (skinId, svgData, rotationCenter) {
        if (this._allSkins[skinId] instanceof SVGSkin) {
-            this._allSkins[skinId].setSVG(svgData, rotationCenter);
-            return;
+            return this._allSkins[skinId].setSVG(svgData, rotationCenter);
        }

        const newSkin = new SVGSkin(skinId, this);
-        newSkin.setSVG(svgData, rotationCenter);
        this._reskin(skinId, newSkin);
+        return newSkin.setSVG(svgData, rotationCenter);
    }


Now that createSVGSkin and updateSvgSkin return a promise, if anyone tries to call getSkinSize without having awaited the create/update call, the dimensions would not be set yet, right? I think one such instance is the updateSvg method in virtual-machine.js. We should probably await the update there as well (and anywhere else where it might make sense). Also, looking at the implementation notes, the original plan was to pre-parse the SVG with DOMParser to extract viewBox synchronously (keeping _size available immediately for getSkinSize callers), is there a reason that changed?

adzhindzhi · 2026-05-13T10:26:41Z

+        /**
+         * Tear down the iframe and reject any in-flight calls.
+         * After `destroy()`, the next `send()` lazily creates a fresh iframe.
+         */
+        destroy () {


Since destroy() is currently never called and the iframe would stay alive/in memory indefinitely, I wonder if it would make sense for the sandbox to manage its own lifecycle and destroy itself once it becomes idle. For example, maybe the sandbox could start/reset an inactivity timeout whenever a message is sent, and destroy the iframe if no new messages arrive within some window (e.g. 10 seconds). That would still keep the sandbox alive during initial project load or bulk SVG imports, while avoiding a permanently alive iframe. I'm mostly thinking out loud here, but it seems like something along these lines could make sense and would avoid coupling the sandbox to the project loading lifecycle?

I'm not sure if there are any concerns performance-wise, but I think it should be relatively cheap, considering that we'd still reuse the iframe during the peak project loading?

adzhindzhi · 2026-05-13T13:54:48Z

+                if (REMOVE_ELEMENTS.has(node.name)) {
+                    parentNode.children = parentNode.children.filter(
+                        child => child !== node
+                    );
+                    return;
+                }


Would the current logic remove tags like <svg:script> or <svg:style> (or other namespaced tags), which I believe are also valid SVG elements? It seems like they would currently survive the canonicalization step, meaning their CSS rules (including external url() references) could persist.

This should be fine for now since sanitizeSvgText is still used, and as far as I can tell DOMPurify strips those tags (and leaves the content as a text node, which is still not ideal). But if we're planning to deprecate that at some point, or if we want canonicalize-svg to handle sanitization more comprehensively, it might be worth addressing here as well.

adzhindzhi · 2026-05-13T14:08:29Z

+                        if (val && !isInternalRef(val.replace(/\s/g, ''))) {
+                            delete node.attributes[attr];
+                        }


One thing I noticed is that, as far as I can tell, the current canonicalization allows data: URIs on all elements, whereas the old sanitization logic only allowed them on image tags. I'm not sure whether this could actually be abused in practice, especially considering the current pipeline, but it still might be worth mentioning.

adzhindzhi · 2026-05-14T10:12:48Z

Why did this snapshot change, are there any changes in the current sanitization logic that lead to it being changed? It doesn't seem so. Could this be a MacOS vs. Linux difference on how the snapshots are generated, and the cause of the failing tests?

This test was failing for me even before the current changes. It could be an OS difference, now that you mentioned it - will take a look.

kbangelov · 2026-05-14T13:22:13Z

+// ── Element / attribute classification ─────────────────────────────────────
+
+/** Elements removed entirely (children discarded). */
+const REMOVE_ELEMENTS = new Set([


I approve of the work that has been done so far on the PR!

The main concern is to not be overly restrictive or not restrictive enough when it comes to removing elements and attributes here. You seem to have taken care of all cases that I could think of. The most important thing here has to be to double-check with a handful of examples right before deploying the code that we catch all of the SVGs that have been known to cause trouble so far and also not restrict the friendly ones (which, by looking at the current code, shouldn't be a thing).

Minor nitpick: consider the <discard> element, which might remove some other elements in the SVG (doesn't seem like the end of the world but might consider it while we're at it)

KManolov3 added 5 commits May 10, 2026 17:41

feat: implement sandboxing to be used for measuring SVGs; add tests

7e67bb4

feat: add canonicalize-svg method

116301e

feat: implement measure svg script to be used in sandboxed environment

114ce54

feat: use the sandboxing mechanism for svg measurement

4eeb5d7

feat: update usage of consumers of loadSvgString

f99b4f8

KManolov3 requested review from adzhindzhi and cwillisf May 11, 2026 15:35

KManolov3 requested a review from a team as a code owner May 11, 2026 15:35

KManolov3 marked this pull request as draft May 11, 2026 15:37

KManolov3 added 2 commits May 12, 2026 17:19

feat: update setSvg to be awaitable; update usages

1438d2a

fix: lint error

28e5b2a

feat: add tests for the updated usages of svg measurement

a069b29

adzhindzhi reviewed May 13, 2026

View reviewed changes

adzhindzhi reviewed May 14, 2026

View reviewed changes

kbangelov reviewed May 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SVG Sandboxing#567

SVG Sandboxing#567
KManolov3 wants to merge 8 commits into
developfrom
feature/svgs-iframing

KManolov3 commented May 11, 2026

Uh oh!

github-actions Bot commented May 11, 2026 •

edited

Loading

Uh oh!

rubiidev18alt commented May 12, 2026

Uh oh!

adzhindzhi May 13, 2026

Uh oh!

adzhindzhi May 13, 2026

Uh oh!

adzhindzhi May 13, 2026

Uh oh!

adzhindzhi May 13, 2026

Uh oh!

adzhindzhi May 14, 2026

Uh oh!

KManolov3 May 14, 2026

Uh oh!

kbangelov May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

KManolov3 commented May 11, 2026

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Uh oh!

github-actions Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

rubiidev18alt commented May 12, 2026

Uh oh!

adzhindzhi May 13, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi May 13, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi May 13, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi May 13, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi May 14, 2026

Choose a reason for hiding this comment

Uh oh!

KManolov3 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

kbangelov May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented May 11, 2026 •

edited

Loading