From a12a50fc45f0cd2c8697e016b2dc94f9f82c7bae Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 22 Sep 2026 19:42:50 -0400 Subject: [PATCH] fix(media-use): close three sanitizer bypasses, add fetch timeout Independent review of the maintainer pass found the SVG sanitizer (packages/core/src/figma/sanitizeSvg.ts, reused by media-use's own port) was itself bypassable: an unquoted href/xlink:href survived, inline style="..." attributes were never checked, and a href bound through a renamed XML namespace prefix (xmlns:x=".../1999/xlink", x:href=...) slipped past the literal "xlink:href" match. Rewrote sanitizeSvg as a tag/attribute tokenizer instead of a set of literal-spelling regexes: parse each tag's attributes in any quoting form, resolve the local name after stripping any namespace prefix, then drop by local name (any on* handler, any *href whose value is not a #fragment or data:image/ embed, any style attribute containing url(, @import, expression( or javascript:). Ported the fix into media-use's svg-sanitize.mjs identically. Legitimate url(#id) references on non-style attributes (clip-path, fill) are untouched. Also adds a 10s AbortSignal.timeout to freezeUrl's fetch, matching every other fetchMedia call in this package - it had a size cap but no deadline. New cases for all three bypasses in both sanitizeSvg.test.ts and the parity corpus, each reproducibly failing against the prior head. --- packages/cli/src/media-use/lib/freeze.mjs | 2 +- .../cli/src/media-use/lib/svg-sanitize.mjs | 46 ++++++++-- packages/core/src/figma/sanitizeSvg.test.ts | 38 ++++++++ packages/core/src/figma/sanitizeSvg.ts | 89 ++++++++++++------- scripts/check-svg-sanitize-parity.test.mjs | 16 ++++ 5 files changed, 147 insertions(+), 44 deletions(-) diff --git a/packages/cli/src/media-use/lib/freeze.mjs b/packages/cli/src/media-use/lib/freeze.mjs index b8ca20e50f..9d3c7dbff0 100644 --- a/packages/cli/src/media-use/lib/freeze.mjs +++ b/packages/cli/src/media-use/lib/freeze.mjs @@ -22,7 +22,7 @@ function writeFrozen(destPath, buffer) { export async function freezeUrl(url, destPath) { const where = String(url).slice(0, 80); - const res = await fetchMedia(url); + const res = await fetchMedia(url, { signal: AbortSignal.timeout(10_000) }); if (!res.ok) throw new Error(`freeze failed: HTTP ${res.status} for ${where}`); // Fail fast on an advertised oversize body before reading a single byte. diff --git a/packages/cli/src/media-use/lib/svg-sanitize.mjs b/packages/cli/src/media-use/lib/svg-sanitize.mjs index 21b435e087..6fae3a7daf 100644 --- a/packages/cli/src/media-use/lib/svg-sanitize.mjs +++ b/packages/cli/src/media-use/lib/svg-sanitize.mjs @@ -10,11 +10,47 @@ function replaceStable(input, pattern, replacement) { return out; } +const TAG_RE = /<([a-zA-Z][\w:-]*)((?:[^"'>]|"[^"]*"|'[^']*')*)>/g; +const ATTR_RE = /([a-zA-Z_][\w:.-]*)\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s"'=<>`]*))/g; + +function localName(name) { + const i = name.lastIndexOf(":"); + return (i === -1 ? name : name.slice(i + 1)).toLowerCase(); +} + function isAllowedHref(value) { const v = value.trim().toLowerCase(); return v.startsWith("#") || v.startsWith("data:image/"); } +function isDangerousStyle(value) { + const v = value.toLowerCase(); + return ( + v.includes("url(") || + v.includes("@import") || + v.includes("expression(") || + v.includes("javascript:") + ); +} + +function isDangerousAttr(name, value) { + const local = localName(name); + if (local.startsWith("on")) return true; + if (local === "href") return !isAllowedHref(value); + if (local === "style") return isDangerousStyle(value); + return false; +} + +function sanitizeAttributes(svg) { + return svg.replace(TAG_RE, (tag, tagName, attrs) => { + const kept = attrs.replace(ATTR_RE, (match, name, dq, sq, uq) => { + const value = dq ?? sq ?? uq ?? ""; + return isDangerousAttr(name, value) ? "" : match; + }); + return `<${tagName}${kept}>`; + }); +} + export function sanitizeSvg(svg) { let out = svg; out = replaceStable(out, /]*>/gi, ""); @@ -23,14 +59,6 @@ export function sanitizeSvg(svg) { out = replaceStable(out, /]*>/gi, ""); out = replaceStable(out, /]*\/>/gi, ""); out = out.replace(/<\/(?:script|style|foreignObject)\b[^>]*>/gi, ""); - out = replaceStable(out, /\son[a-z]+\s*=\s*"[^"]*"/gi, ""); - out = replaceStable(out, /\son[a-z]+\s*=\s*'[^']*'/gi, ""); - out = replaceStable(out, /\son[a-z]+\s*=\s*[^\s>'"]+/gi, ""); - out = out.replace(/\s(href|xlink:href)\s*=\s*"([^"]*)"/gi, (m, _attr, value) => - isAllowedHref(value) ? m : "", - ); - out = out.replace(/\s(href|xlink:href)\s*=\s*'([^']*)'/gi, (m, _attr, value) => - isAllowedHref(value) ? m : "", - ); + out = sanitizeAttributes(out); return out; } diff --git a/packages/core/src/figma/sanitizeSvg.test.ts b/packages/core/src/figma/sanitizeSvg.test.ts index ee3c98f7d0..f046b79b45 100644 --- a/packages/core/src/figma/sanitizeSvg.test.ts +++ b/packages/core/src/figma/sanitizeSvg.test.ts @@ -78,6 +78,44 @@ describe("sanitizeSvg hardening", () => { expect(clean).not.toContain("blob:"); expect(clean).not.toContain("//evil"); }); + // Reported against b7ace99b4: quoted-only regexes let three adversarial + // shapes survive — unquoted href/xlink:href, style="" attributes, and any + // namespace prefix bound to href other than the literal string "xlink:". + it("drops an unquoted javascript: href", () => { + const clean = sanitizeSvg(``); + expect(clean).not.toContain("javascript:"); + }); + it("drops an unquoted external use href", () => { + const clean = sanitizeSvg(``); + expect(clean).not.toContain("evil.example"); + }); + it("drops style attributes carrying url(, @import, expression( or javascript:", () => { + const cases = [ + ``, + ``, + ``, + ``, + ]; + for (const dirty of cases) { + const clean = sanitizeSvg(dirty); + expect(clean, dirty).not.toContain("style="); + expect(clean, dirty).not.toContain("evil"); + } + }); + it("keeps a benign style attribute with no dangerous function", () => { + const clean = sanitizeSvg(``); + expect(clean).toContain('style="fill:#123456"'); + }); + it("does not touch url(#id) references on non-style attributes (clip-path, fill)", () => { + const clean = ``; + expect(sanitizeSvg(clean)).toBe(clean); + }); + it("drops href bound through a renamed xlink namespace prefix", () => { + const dirty = ``; + const clean = sanitizeSvg(dirty); + expect(clean).not.toContain("evil.example"); + expect(clean).toContain('xmlns:x="http://www.w3.org/1999/xlink"'); + }); }); describe("sanitizeSvg close-tag variants (js/bad-tag-filter)", () => { diff --git a/packages/core/src/figma/sanitizeSvg.ts b/packages/core/src/figma/sanitizeSvg.ts index 0a51edc1f7..dd9fc4885b 100644 --- a/packages/core/src/figma/sanitizeSvg.ts +++ b/packages/core/src/figma/sanitizeSvg.ts @@ -1,18 +1,6 @@ -/** - * Sanitize a figma-exported SVG before it touches disk (design spec §5). - * - * Threat model: figma-SHAPED exports, hardened against the cheap adversarial - * variants (nesting, unquoted attrs, scheme smuggling). This is a lexical - * pass, not a general-purpose HTML sanitizer — content from arbitrary - * untrusted sources should go through a real sanitizer (DOMPurify) instead. - * Strips: - * -