[UEPR-692] Remove unsafe-eval and unsafe-inline from SVG Sandboxing scripts - #699
adzhindzhi wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/security issues in the new CSP Playwright routing and iframe document generation (path joining for dist assets, plus missing escaping when embedding script URLs/text) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the SVG sandboxing implementation (used by scratch-svg-renderer and scratch-paint) to avoid CSP breakage on hosts that disallow inline scripts / eval by switching the sandboxed iframe from string-built inline scripts to URL-delivered script assets where possible, while preserving a file:// fallback.
Changes:
- Replaces inline/eval-based sandbox execution with a runner script (
runner.js) plus ordered frame scripts loaded via<script src>on http(s) hosts (and embedded text only for file://). - Moves SVG measurement logic into a static frame script (
measure-svg.js) and sends large font CSS via a newinitpayload delivered once per frame. - Updates paint’s Paper.js sandboxing to use
paper-core(avoidsnew Functionusage) and adds/updates Playwright + unit tests to validate CSP behavior and safety.
File summaries
| File | Description |
|---|---|
| packages/scratch-svg-renderer/test/playwright/sandbox.spec.js | Updates sandbox tests for the new “array of script descriptors” API; adds coverage for unsafe-eval removal and message-source guarding. |
| packages/scratch-svg-renderer/test/playwright/measure-svg.spec.js | Updates measurement tests to use a real frame script + init font CSS instead of a generated script string. |
| packages/scratch-svg-renderer/test/playwright/measure-harness.html | Switches Playwright harness to load the built bundle (dist/web) rather than src/ scripts. |
| packages/scratch-svg-renderer/test/playwright/harness.html | Same as above for the general sandbox harness. |
| packages/scratch-svg-renderer/test/playwright/csp-inheritance.spec.js | Adds a served-origin CSP regression test to ensure URL delivery works under strict script-src. |
| packages/scratch-svg-renderer/src/sandbox/runner.js | New in-iframe message loop runner; enforces parent-only messages and removes eval usage. |
| packages/scratch-svg-renderer/src/sandbox/measure-svg.js | New static in-iframe SVG bbox measurement script; consumes fonts via onSandboxInit. |
| packages/scratch-svg-renderer/src/sandbox/measure-svg-script.js | Removes the previous “generate a JS string to eval in the iframe” approach. |
| packages/scratch-svg-renderer/src/sandbox/index.js | Redesigns Sandbox to accept ordered script descriptors (url/text) + adds one-time-per-frame init delivery and URL-vs-inline host selection. |
| packages/scratch-svg-renderer/src/sandbox/iframe-html.js | Replaces fixed iframe template with a document builder that emits CSP + script tags for URL or inline delivery. |
| packages/scratch-svg-renderer/src/load-svg-string.js | Updates measurement sandbox construction to use measure-svg.js asset/source + init font CSS. |
| packages/scratch-svg-renderer/src/index.js | Exposes Sandbox and usesUrlDelivery from the main package export surface. |
| packages/scratch-svg-renderer/eslint.config.mjs | Caps new untranspiled frame scripts to ES2017 via ESLint config. |
| packages/scratch-paint/test/unit/paper-import-script.test.js | Removes tests for the deleted script-string generator approach. |
| packages/scratch-paint/test/unit/paper-core.test.js | Adds tests proving paper-core matches paper-full for required behavior and avoids new Function. |
| packages/scratch-paint/src/helper/paper-sandbox.js | Updates paint’s sandbox setup to load Paper.js in URL or source form and use the new multi-script Sandbox API. |
| packages/scratch-paint/src/helper/paper-import.js | New static frame script for Paper.js SVG import/export inside the sandbox. |
| packages/scratch-paint/src/helper/paper-import-script.js | Removes the previous “generate a JS string to eval in the iframe” approach. |
| packages/scratch-paint/eslint.config.mjs | Caps the new untranspiled frame script to ES2017 via ESLint config. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new implementation breaks public sandbox imports and constructor compatibility, and its untranspiled Paint handler exceeds the declared browser support.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Balanced
| document.body.appendChild(canvas); | ||
| paper.setup(canvas); | ||
|
|
||
| const readViewBox = svg => { |
| const RUNNER_URL = require('./runner.js?resource'); | ||
| const RUNNER_TEXT = require('./runner.js?source'); |
| constructor (scripts, {timeoutMs = DEFAULT_TIMEOUT_MS, idleTimeoutMs = 0, init} = {}) { | ||
| // Copied so a caller mutating the array or a descriptor after | ||
| // construction cannot change a frame built later by the first send(). | ||
| this._scripts = scripts.map(script => ({...script})); |
| 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=="/></svg>'; | ||
|
|
||
| // Shapes with a gradient. | ||
| const SVG_GRADIENT = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><defs>' + |
There was a problem hiding this comment.
probably a nitpick but - can these links become obsolete at some point? are there requests to "http://www.w3.org/2000/svg" for example at all? what my point is that if there are actualy requests to these urls they can be unreachable at any time for a reason, or they can even be shutdown again for a reason so the tests can be unstable? Is adding these resources locally an option?
| // Untranspiled sandbox frame script: capped at the browserslist floor. | ||
| files: ['src/helper/paper-import.js'], | ||
| languageOptions: { | ||
| ecmaVersion: 2017 |
There was a problem hiding this comment.
I guess we intentionally swapped ES5 with ES8? If that is correct then of the copilots comments should be ignored?
| }, SVG_NO_VIEWBOX); | ||
|
|
||
| expect(srcdoc).toContain(`script-src ${ORIGIN}`); | ||
| expect(srcdoc).toContain(`<script src="${ORIGIN}/assets/runner.`); |
There was a problem hiding this comment.
are these both lines necessary?
Resolves
UEPR-692
Proposed Changes
file:path inlined because an opaque-origin frame cannot load local files, and such hosts send no CSP to inherit anywaypaper-coreinstead ofpaper-full- the PaperScript parser inpaper-fullcallsnew Functionat load, which the no 'unsafe-eval' policy stops.paper-coreis sufficient for the sandboxing usage anyway.Reason for Changes
The sandboxed iframes built their scripts as strings and embedded them inline in the frame's
srcdoc. However, asrcdocframe inherits the CSP of the page that created it and cannot relax it. So on any host page with a strictscript-src, those inline scripts never run and both SVG measurement and costume import break.Test Coverage
csp-inheritance.spec.js, which serves the harness under a strict policy and asserts measurement works, the frame really uses URL delivery rather than falling back to inline, and attacker-supplied SVG cannot execute in the frame.paper-core.test.jsto ensure thatpaper-coreis a safe substitute forpaper-full.sandbox.spec.jsandmeasure-svg.spec.js.