SVG Sandboxing#567
Conversation
Test Results 10 files 1 167 suites 14m 28s ⏱️ Results for commit d026072. ♻️ This comment has been updated with latest results. |
|
Please keep the cool css theming via svg 🙏 |
| if (REMOVE_ELEMENTS.has(node.name)) { | ||
| parentNode.children = parentNode.children.filter( | ||
| child => child !== node | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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.
| if (val && !isInternalRef(val.replace(/\s/g, ''))) { | ||
| delete node.attributes[attr]; | ||
| } |
There was a problem hiding this comment.
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.
| // ── Element / attribute classification ───────────────────────────────────── | ||
|
|
||
| /** Elements removed entirely (children discarded). */ | ||
| const REMOVE_ELEMENTS = new Set([ |
There was a problem hiding this comment.
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)
… into feature/svgs-iframing
| paperSandboxPromise = import( | ||
| /* webpackChunkName: "paper-source" */ | ||
| '@scratch/paper/dist/paper-full.min.js?source' | ||
| ).then(module => { |
There was a problem hiding this comment.
It's important to note that loading this lazily means we'd need to update scratch-www's webpack configuration to copy the chunk from the package's dist folder in node_modules into the js folder it serves chunks from. It would also mean any other consumer of scratch-editor would have to do the same.
4 participants
Resolves
https://scratchfoundation.atlassian.net/browse/UEPR-231
Proposed Changes
canonicalizeSvgTextcanonicalizeSvgText- at the point ofloadVector_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.