diff --git a/package-lock.json b/package-lock.json index ebf4731a7da..27cf9bbc8c5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13069,7 +13069,6 @@ }, "node_modules/boolbase": { "version": "1.0.0", - "dev": true, "license": "ISC" }, "node_modules/bottleneck": { @@ -14971,7 +14970,6 @@ }, "node_modules/css-select": { "version": "5.2.2", - "dev": true, "license": "BSD-2-Clause", "dependencies": { "boolbase": "^1.0.0", @@ -15006,7 +15004,6 @@ }, "node_modules/css-what": { "version": "6.2.2", - "dev": true, "license": "BSD-2-Clause", "engines": { "node": ">= 6" @@ -15030,6 +15027,39 @@ "node": ">=4" } }, + "node_modules/csso": { + "version": "5.0.5", + "resolved": "https://registry.npmjs.org/csso/-/csso-5.0.5.tgz", + "integrity": "sha512-0LrrStPOdJj+SPCCrGhzryycLjwcgUSHBtxNA8aIDxf0GLsRh1cKYhB00Gd1lDOS4yGH69+SNn13+TWbVHETFQ==", + "license": "MIT", + "dependencies": { + "css-tree": "~2.2.0" + }, + "engines": { + "node": "^10 || ^12.20.0 || ^14.13.0 || >=15.0.0", + "npm": ">=7.0.0" + } + }, + "node_modules/csso/node_modules/css-tree": { + "version": "2.2.1", + "resolved": "https://registry.npmjs.org/css-tree/-/css-tree-2.2.1.tgz", + "integrity": "sha512-OA0mILzGc1kCOCSJerOeqDxDQ4HOh+G8NbOJFOTgOCzpw7fCBubk0fEyxp8AgOL/jvLgYA/uV0cMbe43ElF1JA==", + "license": "MIT", + "dependencies": { + "mdn-data": "2.0.28", + "source-map-js": "^1.0.1" + }, + "engines": { + "node": "^10 || ^12.20.0 || ^14.13.0 || >=15.0.0", + "npm": ">=7.0.0" + } + }, + "node_modules/csso/node_modules/mdn-data": { + "version": "2.0.28", + "resolved": "https://registry.npmjs.org/mdn-data/-/mdn-data-2.0.28.tgz", + "integrity": "sha512-aylIc7Z9y4yzHYAJNuESG3hfhC+0Ibp/MAMiaOZgNv4pmEdFyfZhhhny4MNiAfWdBQ1RQ2mfDWmM1x8SvGyp8g==", + "license": "CC0-1.0" + }, "node_modules/cssom": { "version": "0.3.8", "dev": true, @@ -15495,7 +15525,6 @@ }, "node_modules/dom-serializer": { "version": "2.0.0", - "dev": true, "license": "MIT", "dependencies": { "domelementtype": "^2.3.0", @@ -15511,7 +15540,6 @@ }, "node_modules/domelementtype": { "version": "2.3.0", - "dev": true, "funding": [ { "type": "github", @@ -15530,7 +15558,6 @@ }, "node_modules/domhandler": { "version": "5.0.3", - "dev": true, "license": "BSD-2-Clause", "dependencies": { "domelementtype": "^2.3.0" @@ -15551,7 +15578,6 @@ }, "node_modules/domutils": { "version": "3.2.2", - "dev": true, "license": "BSD-2-Clause", "dependencies": { "dom-serializer": "^2.0.0", @@ -15713,7 +15739,6 @@ }, "node_modules/entities": { "version": "4.5.0", - "dev": true, "license": "BSD-2-Clause", "engines": { "node": ">=0.12" @@ -28754,7 +28779,6 @@ }, "node_modules/nth-check": { "version": "2.1.1", - "dev": true, "license": "BSD-2-Clause", "dependencies": { "boolbase": "^1.0.0" @@ -32532,9 +32556,13 @@ "license": "MIT" }, "node_modules/sax": { - "version": "1.4.3", - "dev": true, - "license": "BlueOak-1.0.0" + "version": "1.6.0", + "resolved": "https://registry.npmjs.org/sax/-/sax-1.6.0.tgz", + "integrity": "sha512-6R3J5M4AcbtLUdZmRv2SygeVaM7IhrLXu9BmnOGmmACak8fiUtOsYNWUS4uK7upbmHIBbLBeFeI//477BKLBzA==", + "license": "BlueOak-1.0.0", + "engines": { + "node": ">=11.0.0" + } }, "node_modules/saxes": { "version": "3.1.11", @@ -34698,6 +34726,40 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/svgo": { + "version": "4.0.1", + "resolved": "https://registry.npmjs.org/svgo/-/svgo-4.0.1.tgz", + "integrity": "sha512-XDpWUOPC6FEibaLzjfe0ucaV0YrOjYotGJO1WpF0Zd+n6ZGEQUsSugaoLq9QkEZtAfQIxT42UChcssDVPP3+/w==", + "license": "MIT", + "dependencies": { + "commander": "^11.1.0", + "css-select": "^5.1.0", + "css-tree": "^3.0.1", + "css-what": "^6.1.0", + "csso": "^5.0.5", + "picocolors": "^1.1.1", + "sax": "^1.5.0" + }, + "bin": { + "svgo": "bin/svgo.js" + }, + "engines": { + "node": ">=16" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/svgo" + } + }, + "node_modules/svgo/node_modules/commander": { + "version": "11.1.0", + "resolved": "https://registry.npmjs.org/commander/-/commander-11.1.0.tgz", + "integrity": "sha512-yPVavfyCcRhmorC7rWlkHn15b4wDVgVmBA7kV4QVBsF7kv/9TKJAbAXVTxvTnwP8HHKjRCJDClKbciiYS7p0DQ==", + "license": "MIT", + "engines": { + "node": ">=16" + } + }, "node_modules/symbol-tree": { "version": "3.2.4", "license": "MIT" @@ -40687,12 +40749,14 @@ "css-tree": "3.2.1", "fastestsmallesttextencoderdecoder": "1.0.22", "isomorphic-dompurify": "2.36.0", + "svgo": "^4.0.1", "transformation-matrix": "1.15.3", "tslog": "4.10.2" }, "devDependencies": { "@babel/core": "7.29.7", "@babel/preset-env": "7.29.7", + "@playwright/test": "1.59.1", "babel-loader": "9.2.1", "canvas": "3.2.3", "copy-webpack-plugin": "6.4.1", @@ -40713,6 +40777,22 @@ "scratch-render-fonts": "1.0.252" } }, + "packages/scratch-svg-renderer/node_modules/@playwright/test": { + "version": "1.59.1", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.59.1.tgz", + "integrity": "sha512-PG6q63nQg5c9rIi4/Z5lR5IVF7yU5MqmKaPOe0HSc0O2cX1fPi96sUQu5j7eo4gKCkB2AnNGoWt7y4/Xx3Kcqg==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright": "1.59.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=18" + } + }, "packages/scratch-svg-renderer/node_modules/canvas": { "version": "3.2.3", "resolved": "https://registry.npmjs.org/canvas/-/canvas-3.2.3.tgz", @@ -40728,6 +40808,21 @@ "node": "^18.12.0 || >= 20.9.0" } }, + "packages/scratch-svg-renderer/node_modules/fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": "^8.16.0 || ^10.6.0 || >=11.0.0" + } + }, "packages/scratch-svg-renderer/node_modules/mkdirp": { "version": "2.1.6", "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-2.1.6.tgz", @@ -40744,6 +40839,38 @@ "url": "https://github.com/sponsors/isaacs" } }, + "packages/scratch-svg-renderer/node_modules/playwright": { + "version": "1.59.1", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.59.1.tgz", + "integrity": "sha512-C8oWjPR3F81yljW9o5OxcWzfh6avkVwDD2VYdwIGqTkl+OGFISgypqzfu7dOe4QNLL2aqcWBmI3PMtLIK233lw==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright-core": "1.59.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=18" + }, + "optionalDependencies": { + "fsevents": "2.3.2" + } + }, + "packages/scratch-svg-renderer/node_modules/playwright-core": { + "version": "1.59.1", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.59.1.tgz", + "integrity": "sha512-HBV/RJg81z5BiiZ9yPzIiClYV/QMsDCKUyogwH9p3MCP6IYjUFu/MActgYAvK0oWyV9NlwM3GLBjADyWgydVyg==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=18" + } + }, "packages/scratch-svg-renderer/node_modules/rimraf": { "version": "3.0.2", "resolved": "https://registry.npmjs.org/rimraf/-/rimraf-3.0.2.tgz", diff --git a/packages/scratch-paint/src/containers/paper-canvas.jsx b/packages/scratch-paint/src/containers/paper-canvas.jsx index eba8a363dd2..c19f5c16490 100644 --- a/packages/scratch-paint/src/containers/paper-canvas.jsx +++ b/packages/scratch-paint/src/containers/paper-canvas.jsx @@ -7,6 +7,7 @@ import {sanitizeSvg} from '@scratch/scratch-svg-renderer'; import Formats from '../lib/format'; import log from '../log/log'; +import {getPaperSandbox} from '../helper/paper-sandbox'; import {stripInvalidPaperData} from '../helper/strip-invalid-paper-data'; import {performSnapshot} from '../helper/undo'; import {undoSnapshot, clearUndoState} from '../reducers/undo'; @@ -39,6 +40,7 @@ class PaperCanvas extends React.Component { 'onViewResize', 'recalibrateSize' ]); + this._importGeneration = 0; } componentDidMount () { paper.setup(this.canvas); @@ -196,10 +198,11 @@ class PaperCanvas extends React.Component { this.props.updateViewBounds(paper.view.matrix); } importSvg (svg, rotationCenterX, rotationCenterY) { - const paperCanvas = this; + this._importGeneration += 1; + const generation = this._importGeneration; + // Pre-process SVG to prevent parsing errors (discussion from #213) // 1. Remove svg: namespace on elements. - // TODO: remove svg = svg.split(/<\s*svg:/).join('<'); svg = svg.split(/<\/\s*svg:/).join(' appendChild internally, so anything dangerous left - // in the SVG executes against the embedding origin. DOMPurify's SVG - // profile drops '; + const script = createPaperImportScript(sourceWithSpecialChars); + expect(script).toContain(JSON.stringify(sourceWithSpecialChars)); + expect(script).toMatch(/^\(function \(\) \{/); + expect(script).toMatch(/\}\)\(\);$/); + }); +}); diff --git a/packages/scratch-paint/test/unit/sanitize-svg-import.test.js b/packages/scratch-paint/test/unit/sanitize-svg-import.test.js index c37bf813855..b9b0353df50 100644 --- a/packages/scratch-paint/test/unit/sanitize-svg-import.test.js +++ b/packages/scratch-paint/test/unit/sanitize-svg-import.test.js @@ -1,16 +1,16 @@ /* eslint-env jest */ /** * Regression coverage for the SVG sanitization step that paper-canvas.jsx's - * `importSvg` runs before `paper.project.importSVG`. Paper.js's import path - * appends parsed SVG nodes into the document during processing, which fires - * execution paths on ``, event-handler attributes, and - * similar features. `sanitizeSvg.sanitizeSvgText` uses DOMPurify's SVG - * profile, which strips those shapes. + * `importSvg` runs before sending the SVG to the sandboxed iframe. + * Paper.js's import path appends parsed SVG nodes into the document during + * processing, which fires execution paths on ``, event-handler + * attributes, and similar features. The sanitize step (DOMPurify's SVG + * profile) strips those shapes before the SVG reaches the iframe. * * Two layers of coverage: sanitizer-level assertions on hostile and * legitimate inputs, plus an integration assertion that `importSvg` * actually routes its input through the sanitizer before handing it to - * paper. + * the sandbox. */ import paper from '@scratch/paper'; @@ -20,6 +20,21 @@ import PaperCanvasConnected from '../../src/containers/paper-canvas'; const PaperCanvas = PaperCanvasConnected.WrappedComponent; +// Mock the paper-sandbox module to capture what gets sent to the sandbox. +// getPaperSandbox() now returns a Promise. The send mock returns +// a never-resolving promise so the .then() chain doesn't execute (we only +// need to assert on the input sent to the sandbox, not the full import +// pipeline which requires layers, undo, etc.) +jest.mock('../../src/helper/paper-sandbox', () => { + const sendMock = jest.fn(() => new Promise(() => {})); + return { + getPaperSandbox: () => Promise.resolve({send: sendMock}), + __sendMock: sendMock + }; +}); + +const {__sendMock: sandboxSendMock} = require('../../src/helper/paper-sandbox'); + const wrap = body => `${body}`; @@ -80,37 +95,32 @@ describe('sanitizeSvgText strips dangerous SVG shapes', () => { }); }); -describe('PaperCanvas.importSvg routes input through sanitizeSvgText', () => { - let importSpy; +describe('PaperCanvas.importSvg routes sanitized input to the sandbox', () => { let sanitizeSpy; beforeEach(() => { const canvas = document.createElement('canvas'); paper.setup(canvas); - // Mock paper.project.importSVG to a no-op so we don't have to - // satisfy the rest of importSvg's onLoad chain — the assertion is - // about what gets handed to paper, not what paper does with it. - importSpy = jest.spyOn(paper.project, 'importSVG').mockImplementation(() => {}); sanitizeSpy = jest.spyOn(sanitizeSvg, 'sanitizeSvgText'); + sandboxSendMock.mockClear(); }); afterEach(() => { - importSpy.mockRestore(); sanitizeSpy.mockRestore(); while (paper.projects.length > 0) { paper.projects[paper.projects.length - 1].remove(); } }); - test('paper.project.importSVG receives input that has been through sanitizeSvgText', () => { - // importSvg accesses this.props.changeFormat / undoSnapshot only on - // the SVG-import-failed branch, and this.recalibrateSize only when - // the (mocked-out) onLoad fires. A minimal fakeThis with jest.fn() - // stand-ins is enough to call through. - const fakeThis = { - props: {changeFormat: jest.fn(), undoSnapshot: jest.fn()}, - recalibrateSize: jest.fn() - }; + const makeFakeThis = () => ({ + _importGeneration: 0, + props: {changeFormat: jest.fn(), undoSnapshot: jest.fn(), updateViewBounds: jest.fn()}, + recalibrateSize: jest.fn(), + queuedImport: null + }); + + test('sandbox receives input that has been through sanitizeSvgText', async () => { + const fakeThis = makeFakeThis(); const hostile = '' + '' + @@ -121,12 +131,16 @@ describe('PaperCanvas.importSvg routes input through sanitizeSvgText', () => { PaperCanvas.prototype.importSvg.call(fakeThis, hostile, 0, 0); + // getPaperSandbox() returns a resolved Promise, so sandbox.send() + // is called in a microtask. Flush it before asserting. + await Promise.resolve(); + expect(sanitizeSpy).toHaveBeenCalledTimes(1); - expect(importSpy).toHaveBeenCalledTimes(1); - const svgPassedToPaper = importSpy.mock.calls[0][0]; - expect(svgPassedToPaper).not.toMatch(/} rotationCenter Optional: rotation center of the skin. If not supplied, the center of the * skin will be used - * @returns {!int} the ID for the new skin. + * @returns {Promise} A promise that resolves with the skin ID once the skin's + * dimensions are available via getSkinSize. */ createSVGSkin (svgData, rotationCenter) { const skinId = this._nextSkinId++; const newSkin = new SVGSkin(skinId, this); - newSkin.setSVG(svgData, rotationCenter); this._allSkins[skinId] = newSkin; - return skinId; + return newSkin.setSVG(svgData, rotationCenter).then(() => skinId); } /** @@ -398,16 +398,16 @@ class RenderWebGL extends EventEmitter { * @param {!string} svgData - new SVG to use. * @param {?Array} rotationCenter Optional: rotation center of the skin. If not supplied, the center of the * skin will be used + * @returns {Promise} A promise that resolves once the skin's dimensions are updated. */ 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); } /** diff --git a/packages/scratch-render/src/SVGSkin.js b/packages/scratch-render/src/SVGSkin.js index 71b38ffef6c..d3768dede50 100644 --- a/packages/scratch-render/src/SVGSkin.js +++ b/packages/scratch-render/src/SVGSkin.js @@ -54,6 +54,14 @@ class SVGSkin extends Skin { * @type {number} */ this._maxTextureScale = 1; + + /** + * Generation counter for cancelling stale setSVG loads. + * Incremented each time setSVG is called; async callbacks + * compare their captured generation to skip outdated results. + * @type {number} + */ + this._svgGeneration = 0; } /** @@ -192,46 +200,56 @@ class SVGSkin extends Skin { * @fires Skin.event:WasAltered */ setSVG (svgData, rotationCenter) { - const svgTag = loadSvgString(svgData); - const svgText = serializeSvgToString(svgTag, true /* shouldInjectFonts */); this._svgImageLoaded = false; - const {x, y, width, height} = svgTag.viewBox.baseVal; - // While we're setting the size before the image is loaded, this doesn't cause the skin to appear with the wrong - // size for a few frames while the new image is loading, because we don't emit the `WasAltered` event, telling - // drawables using this skin to update, until the image is loaded. - // We need to do this because the VM reads the skin's `size` directly after calling `setSVG`. - // TODO: return a Promise so that the VM can read the skin's `size` after the image is loaded. - this._size[0] = width; - this._size[1] = height; - - // If there is another load already in progress, replace the old onload to effectively cancel the old load - this._svgImage.onload = () => { - if (width === 0 || height === 0) { - super.setEmptyImageData(); - return; - } - - const maxDimension = Math.ceil(Math.max(width, height)); - let testScale = 2; - for (testScale; maxDimension * testScale <= MAX_TEXTURE_DIMENSION; testScale *= 2) { - this._maxTextureScale = testScale; - } - - this.resetMIPs(); - - if (typeof rotationCenter === 'undefined') rotationCenter = this.calculateRotationCenter(); - // Compensate for viewbox offset. - // See https://github.com/LLK/scratch-render/pull/90. - this._rotationCenter[0] = rotationCenter[0] - x; - this._rotationCenter[1] = rotationCenter[1] - y; - - this._svgImageLoaded = true; - - this.emit(Skin.Events.WasAltered); - }; + // Increment generation so that if setSVG is called again before + // the async pipeline finishes, the stale result is discarded. + const generation = ++this._svgGeneration; + + // Full async pipeline: sanitize, normalize, serialize, render. + // Resolves after _size is updated; callers that need correct dimensions + // must await the returned Promise. + return loadSvgString(svgData).then(svgTag => { + // A newer setSVG call supersedes this one. + if (this._svgGeneration !== generation) return; + + const svgText = serializeSvgToString(svgTag, true /* shouldInjectFonts */); + const {x, y, width, height} = svgTag.viewBox.baseVal; + + // Update size from the normalized SVG (normalization may have + // changed dimensions, e.g. for Scratch 2 quirks-mode SVGs). + this._size[0] = width; + this._size[1] = height; + + this._svgImage.onload = () => { + if (this._svgGeneration !== generation) return; + + if (width === 0 || height === 0) { + super.setEmptyImageData(); + return; + } + + const maxDimension = Math.ceil(Math.max(width, height)); + let testScale = 2; + for (testScale; maxDimension * testScale <= MAX_TEXTURE_DIMENSION; testScale *= 2) { + this._maxTextureScale = testScale; + } + + this.resetMIPs(); + + if (typeof rotationCenter === 'undefined') rotationCenter = this.calculateRotationCenter(); + // Compensate for viewbox offset. + // See https://github.com/LLK/scratch-render/pull/90. + this._rotationCenter[0] = rotationCenter[0] - x; + this._rotationCenter[1] = rotationCenter[1] - y; + + this._svgImageLoaded = true; + + this.emit(Skin.Events.WasAltered); + }; - this._svgImage.src = `data:image/svg+xml;utf8,${encodeURIComponent(svgText)}`; + this._svgImage.src = `data:image/svg+xml;utf8,${encodeURIComponent(svgText)}`; + }); } } diff --git a/packages/scratch-render/test/integration/skin-size-tests.js b/packages/scratch-render/test/integration/skin-size-tests.js index 2d7db354150..aea4d1b2115 100644 --- a/packages/scratch-render/test/integration/skin-size-tests.js +++ b/packages/scratch-render/test/integration/skin-size-tests.js @@ -14,8 +14,8 @@ const indexHTML = path.resolve(__dirname, 'index.html'); await test('SVG skin size set properly', async t => { t.plan(1); - const skinSize = await page.evaluate(() => { - const skinID = render.createSVGSkin(``); + const skinSize = await page.evaluate(async () => { + const skinID = await render.createSVGSkin(``); return render.getSkinSize(skinID); }); t.same(skinSize, [50, 100]); diff --git a/packages/scratch-render/test/integration/svg-skin-async-tests.js b/packages/scratch-render/test/integration/svg-skin-async-tests.js new file mode 100644 index 00000000000..0edaf866984 --- /dev/null +++ b/packages/scratch-render/test/integration/svg-skin-async-tests.js @@ -0,0 +1,111 @@ +/* global render, requestAnimationFrame */ +/** + * Integration tests for SVGSkin.setSVG async pipeline and generation counter. + * + * Verifies that: + * - Rapid successive setSVG calls discard stale results (generation counter). + * - The async loadSvgString pipeline correctly updates skin size. + * - Superseded setSVG calls do not corrupt skin state. + * + * Requires built dist bundles: run `npm run build` in scratch-render and + * scratch-svg-renderer before running these tests. + */ +const {chromium} = require('playwright-chromium'); +const test = require('tap').test; +const path = require('path'); + +const indexHTML = path.resolve(__dirname, 'index.html'); + +(async () => { + const browser = await chromium.launch(); + const page = await browser.newPage(); + + await page.goto(`file://${indexHTML}`); + + // Wait for the renderer to be ready. + await page.waitForFunction(() => typeof render !== 'undefined' && render.createSVGSkin); + + await test('createSVGSkin with viewBox SVG resolves with correct size', async t => { + const skinSize = await page.evaluate(async () => { + const svg = '' + + ''; + const skinId = await render.createSVGSkin(svg); + return render.getSkinSize(skinId); + }); + t.same(skinSize, [75, 120]); + }); + + await test('createSVGSkin with viewBox but no width/height resolves with correct size', async t => { + const skinSize = await page.evaluate(async () => { + const svg = '' + + ''; + const skinId = await render.createSVGSkin(svg); + return render.getSkinSize(skinId); + }); + t.same(skinSize, [63, 47]); + }); + + await test('rapid updateSVGSkin calls: only last SVG dimensions persist', async t => { + const result = await page.evaluate(async () => { + const small = '' + + ''; + const medium = '' + + ''; + const large = '' + + ''; + + // Create initial skin + const skinId = await render.createSVGSkin(small); + + // Fire three rapid updates without awaiting the first two. + render.updateSVGSkin(skinId, small); + render.updateSVGSkin(skinId, medium); + const lastPromise = render.updateSVGSkin(skinId, large); + + // Wait for the last one to settle. + await lastPromise; + + // Give a frame for img.onload to fire. + await new Promise(resolve => requestAnimationFrame(resolve)); + + return render.getSkinSize(skinId); + }); + + // Only the last SVG (200x150) should be reflected. + t.same(result, [200, 150]); + }); + + await test('superseded setSVG load is discarded when a newer call wins', async t => { + const result = await page.evaluate(async () => { + // SVG without viewBox forces the async measurement path (slower). + const slowSvg = '' + + ''; + // SVG with viewBox takes the fast path. + const fastSvg = '' + + ''; + + const skinId = await render.createSVGSkin( + '' + ); + + // First: fire the slow measurement path (no viewBox). + render.updateSVGSkin(skinId, slowSvg); + // Immediately supersede with the fast path. + const fastPromise = render.updateSVGSkin(skinId, fastSvg); + + await fastPromise; + // Wait for any stale callbacks to potentially fire. + await new Promise(resolve => setTimeout(resolve, 200)); + + return render.getSkinSize(skinId); + }); + + // The fast SVG (99x77) should win; the slow SVG's stale result should be discarded. + t.same(result, [99, 77]); + }); + + await browser.close(); +})().catch(err => { + console.error(err.message); + process.exit(1); +}); diff --git a/packages/scratch-svg-renderer/eslint.config.mjs b/packages/scratch-svg-renderer/eslint.config.mjs index c816685ea7d..638a7de215a 100644 --- a/packages/scratch-svg-renderer/eslint.config.mjs +++ b/packages/scratch-svg-renderer/eslint.config.mjs @@ -16,11 +16,24 @@ export default eslintConfigScratch.defineConfig( '*.{,c,m}js', // for example, webpack.config.js 'test/**/*.{,c,m}js' ], + ignores: ['test/playwright/**/*.{,c,m}js'], extends: [eslintConfigScratch.legacy.node], languageOptions: { globals: globals.node } }, + { + // Playwright test files run in Node but contain page.evaluate() callbacks + // that reference browser globals (window, document) executed in Chromium. + files: ['test/playwright/**/*.{,c,m}js'], + extends: [eslintConfigScratch.legacy.node], + languageOptions: { + globals: { + ...globals.node, + ...globals.browser + } + } + }, globalIgnores([ 'dist/**/*', 'node_modules/**/*', diff --git a/packages/scratch-svg-renderer/package.json b/packages/scratch-svg-renderer/package.json index 63a8178ba0e..0b1c4b3200e 100644 --- a/packages/scratch-svg-renderer/package.json +++ b/packages/scratch-svg-renderer/package.json @@ -14,10 +14,13 @@ "license": "AGPL-3.0-only", "author": "Massachusetts Institute of Technology", "exports": { - "webpack": "./src/index.js", - "browser": "./dist/web/scratch-svg-renderer.js", - "node": "./dist/node/scratch-svg-renderer.js", - "default": "./src/index.js" + ".": { + "webpack": "./src/index.js", + "browser": "./dist/web/scratch-svg-renderer.js", + "node": "./dist/node/scratch-svg-renderer.js", + "default": "./src/index.js" + }, + "./sandbox": "./src/sandbox/index.js" }, "main": "./dist/node/scratch-svg-renderer.js", "browser": "./dist/web/scratch-svg-renderer.js", @@ -37,6 +40,7 @@ "start": "webpack-dev-server", "test": "npm run test:lint && npm run test:unit", "test:lint": "eslint", + "test:playwright": "playwright test", "test:unit": "tap ./test/*.js", "watch": "webpack --watch" }, @@ -55,12 +59,14 @@ "css-tree": "3.2.1", "fastestsmallesttextencoderdecoder": "1.0.22", "isomorphic-dompurify": "2.36.0", + "svgo": "^4.0.1", "transformation-matrix": "1.15.3", "tslog": "4.10.2" }, "devDependencies": { "@babel/core": "7.29.7", "@babel/preset-env": "7.29.7", + "@playwright/test": "1.59.1", "babel-loader": "9.2.1", "canvas": "3.2.3", "copy-webpack-plugin": "6.4.1", diff --git a/packages/scratch-svg-renderer/playwright.config.js b/packages/scratch-svg-renderer/playwright.config.js new file mode 100644 index 00000000000..525025df08c --- /dev/null +++ b/packages/scratch-svg-renderer/playwright.config.js @@ -0,0 +1,30 @@ +// @ts-check +const path = require('path'); +const {pathToFileURL} = require('url'); +const {defineConfig, devices} = require('@playwright/test'); + +module.exports = defineConfig({ + testDir: './test/playwright', + + fullyParallel: true, + forbidOnly: !!process.env.CI, + retries: process.env.CI ? 1 : 0, + + outputDir: 'test-results/playwright-artifacts', + reporter: [ + ['list'], + ['html', {outputFolder: 'test-results/playwright-html', open: 'never'}] + ], + + use: { + baseURL: `${pathToFileURL(path.resolve(__dirname, 'test/playwright'))}/`, + trace: 'on-first-retry' + }, + + projects: [ + { + name: 'chromium', + use: {...devices['Desktop Chrome']} + } + ] +}); diff --git a/packages/scratch-svg-renderer/src/canonicalize-svg.js b/packages/scratch-svg-renderer/src/canonicalize-svg.js new file mode 100644 index 00000000000..333e8fdf8f9 --- /dev/null +++ b/packages/scratch-svg-renderer/src/canonicalize-svg.js @@ -0,0 +1,228 @@ +/** + * Canonicalize SVG text using SVGO 4 with a security-focused, deny-by-default + * plugin set. Strips dangerous elements, attributes, and external references + * while preserving visual fidelity. + */ + +const {ident} = require('css-tree/utils'); +const { + cssHasExternalUrls, filterCssText, isInternalRef, rawTextHasExternalUrls +} = require('./util/svg-url-helpers'); + +// Cache the SVGO module after first dynamic import. +let _svgoPromise = null; +const getSvgo = () => { + if (!_svgoPromise) { + _svgoPromise = import('svgo/browser'); + } + return _svgoPromise; +}; + +/** + * Strip declarations with external url() references from a CSS declaration + * list string (used for style attributes). Returns the cleaned string, or + * an empty string if everything was removed. + * @param {string} cssText raw CSS declaration list. + * @returns {string} cleaned CSS text. + */ +const stripExternalUrlDeclarations = cssText => { + try { + return filterCssText(cssText, 'declarationList'); + } catch { + // Unparseable CSS — strip entirely rather than risk external loads. + return rawTextHasExternalUrls(ident.decode(cssText)) ? '' : cssText; + } +}; + +// ── Element / attribute classification ───────────────────────────────────── + +/** Elements removed entirely (children discarded). */ +const REMOVE_ELEMENTS = new Set([ + 'script', + 'foreignObject', + 'foreignobject', // case-normalized variant + // SVG animation elements + 'animate', + 'animateMotion', + 'animateTransform', + 'animateColor', + 'set' +]); + +/** Elements whose wrapper is removed but children are preserved. */ +const UNWRAP_ELEMENTS = new Set(['a']); + +/** Attributes that carry a direct URI reference. */ +const URI_ATTRS = new Set(['href', 'xlink:href']); + +/** + * Normalize an attribute name for security checks. + * + * Applies NFKC to collapse full-width and other compatibility equivalents + * (e.g. ｏｎclick → onclick), then strips U+200C (ZWNJ) and U+200D (ZWJ), + * which are valid XML name characters and can be embedded invisibly to break + * naive prefix checks (e.g. on\u200Cclick). All legitimate SVG attribute + * names are pure ASCII so these transformations produce no false positives. + * @param {string} name raw attribute name from the parsed SVG AST. + * @returns {string} normalized attribute name. + */ +const normalizeAttrName = name => + name.normalize('NFKC').replace(/[\u200C\u200D]/g, ''); + +const isEventHandler = name => /^on/i.test(normalizeAttrName(name)); + +// ── SVGO custom plugins ─────────────────────────────────────────────────── + +/** + * Remove dangerous elements (script, foreignObject, animation) and unwrap + * anchor elements (preserve children, drop the wrapper). + */ +const removeDangerousElements = { + name: 'removeDangerousElements', + fn: () => ({ + element: { + enter: (node, parentNode) => { + if (REMOVE_ELEMENTS.has(node.name)) { + parentNode.children = parentNode.children.filter( + child => child !== node + ); + return; + } + if (UNWRAP_ELEMENTS.has(node.name)) { + parentNode.children = parentNode.children.flatMap( + child => (child === node ? node.children : [child]) + ); + } + } + } + }) +}; + +/** + * Remove event-handler attributes (on*) and external href / xlink:href + * references. Strip individual external-url declarations from style + * attributes; remove presentation attributes that reference external URLs. + */ +const removeDangerousAttributes = { + name: 'removeDangerousAttributes', + fn: () => ({ + element: { + enter: node => { + for (const attr of Object.keys(node.attributes)) { + const normalizedAttr = normalizeAttrName(attr); + if (isEventHandler(attr)) { + delete node.attributes[attr]; + continue; + } + + // Direct URI attributes (href, xlink:href) + if (URI_ATTRS.has(normalizedAttr)) { + const val = node.attributes[attr]; + if (val && !isInternalRef(val.replace(/\s/g, ''))) { + delete node.attributes[attr]; + } + continue; + } + + // style attribute — strip only the offending declarations + if (normalizedAttr === 'style') { + const cleaned = stripExternalUrlDeclarations( + node.attributes.style + ); + if (cleaned) { + node.attributes.style = cleaned; + } else { + delete node.attributes.style; + } + continue; + } + + // Presentation attributes that might carry url() + const val = node.attributes[attr]; + if (val && /url\s*\(/i.test(val) && + cssHasExternalUrls(val, 'value')) { + delete node.attributes[attr]; + } + } + } + } + }) +}; + +// ── Plugin pipeline ──────────────────────────────────────────────────────── + +/** + * Deny-by-default plugin list. Order matters: + * 1. Inline styles from '; + const result = await canonicalizeSvgText(input); + t.notMatch(result, /