From d8227c100dff7faca2ed61427c953ae2d9cc8bce Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 04:22:06 +0000 Subject: [PATCH] fix(review): the SVG drawing check refuses CSS escapes and reads zero-padded character references; the security class reads a file's changed SVG lines joined (GST-911 review pass 1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 3 ++- lib/review-depth.ts | 9 ++++++--- lib/review-risk.ts | 13 +++++++------ test/review-packets-removed.test.ts | 4 ++-- test/review-planner-misreads.test.ts | 14 +++++++++++--- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index e3662dc64..be7396557 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -183,7 +183,8 @@ directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`, `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named consumer each keep the path code, and YAML and TOML are always code. An SVG counts as data only when its whole text at the head is a drawing (GST-911): SVG drawing -elements and RDF metadata, CSS with no import and no `url()` outside the file, every +elements and RDF metadata, CSS with no import, no `url()` outside the file and no +backslash escape, every `href` a same-file `#` reference or an inline raster image, no event handler and no entity declaration, read after character references are decoded; anything else, a script or `foreignObject` included, keeps it code. More than diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 78af654ac..5044c3b0a 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -81,8 +81,11 @@ const SVG_ELEMENTS = new Set([ ]); /** Markup a drawing may hold besides elements: comments, CDATA, the XML declaration, and a DOCTYPE with no internal subset. */ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg\b[^>[]*>)/i; -/** What a drawing never holds: a named entity beyond the five XML ones, a CSS import, a url() to another document, a CSS expression or binding. */ -const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; +/** + * What a drawing never holds: a named entity beyond the five XML ones, a backslash (a CSS escape such as @\69mport), a CSS + * import, a url() to another document, a CSS expression or binding. + */ +const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\(\s*(?!['"]?\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; /** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+/=\s]*$)/i; @@ -93,7 +96,7 @@ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+ */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); - if (SVG_REFUSED.some(r => r.test(t)) || t.split('\n').some(svgActive)) return false; + if (SVG_REFUSED.some(r => r.test(t)) || svgActive(t)) return false; for (let at = t.indexOf('<'); at !== -1; at = t.indexOf('<', at + 1)) { if (SVG_MARKUP.test(t.slice(at, at + 512))) continue; const tag = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)/.exec(t.slice(at, at + 256)); diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 522c11d62..2c21875a6 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -42,14 +42,15 @@ const SECURITY_LINE = [ /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/, ]; export const SVG_PATH = /\.svg$/i; -/** An SVG line that can run code, read decoded and again with whitespace removed (java script: is javascript:). */ +/** SVG text that can run code, read decoded and again with whitespace removed (java script: is javascript:); a file's changed lines are read joined, so a split attribute still counts. */ export const SVG_ACTIVE = [/<(?:[\w.-]+:)?(?:script|foreignObject)\b/i, /(?:^|[\s"'/])on[a-z]+\s*=/i, /(?:java|vb)script:|data:text\/html/i]; -/** XML character references decoded (j and j are j); an out-of-range one decodes to nothing. */ -export const decodeCharRefs = (s: string) => s.replace(/&#(x[0-9a-f]{1,8}|\d{1,10});?/gi, (_, n: string) => { - const c = n[0] === 'x' || n[0] === 'X' ? parseInt(n.slice(1), 16) : Number(n); +/** XML character references decoded, leading zeros included (j, j and j are j); an out-of-range one decodes to nothing. */ +export const decodeCharRefs = (s: string) => s.replace(/&#(?:[xX]0*([0-9a-fA-F]+)|0*(\d+));?/g, (_, hex: string | undefined, dec: string | undefined) => { + const digits = hex ?? dec!; + const c = digits.length > 8 ? Infinity : hex !== undefined ? parseInt(hex, 16) : Number(dec); return c <= 0x10ffff ? String.fromCodePoint(c) : ''; }); -export const svgActive = (line: string) => { const d = decodeCharRefs(line); const squeezed = d.replace(/[\s\0-\x1f]+/g, ''); return SVG_ACTIVE.some(r => r.test(d) || r.test(squeezed)); }; +export const svgActive = (text: string) => { const d = decodeCharRefs(text); const squeezed = d.replace(/[\s\0-\x1f]+/g, ''); return SVG_ACTIVE.some(r => r.test(d) || r.test(squeezed)); }; const DATA_PATH_TOKENS = ['pii', 'gdpr', 'ccpa', 'privacy', 'personal', 'account', 'accounts', 'customer', 'customers', 'cookie', 'cookies', 'persistence', 'storage', 'database']; const DATA_LINE = [ /\bDELETE\s+FROM\b/, /\b(?:drop\s+(?:table|column|database|schema)|truncate\s+table)\b/i, /\bdelete\s+from\s+[\w."`]+\s*(?:where\b|;|$|["'`)])/i, @@ -84,11 +85,11 @@ export function matchRisk(i: RiskInput): Map { if (t.some(x => MONEY_PATH_TOKENS.includes(x))) hit('money', 'money path: ' + p); if (t.some(x => SECURITY_PATH_TOKENS.includes(x))) hit('security', 'security path: ' + p); if (t.some(x => DATA_PATH_TOKENS.includes(x))) hit('data', 'data path: ' + p); + if (SVG_PATH.test(p) && [i.added?.[p], i.removed?.[p]].some(l => l && svgActive(l.join('\n')))) hit('security', 'active SVG lines: ' + p); for (const line of [...(i.added?.[p] ?? []), ...(i.removed?.[p] ?? [])]) { if (!why.has('money') && MONEY_LINE.some(r => r.test(line))) hit('money', 'money line: ' + p); if (!why.has('secret') && SECRET_LINE.some(r => r.test(line))) hit('secret', 'secret line: ' + p); if (!why.has('security') && SECURITY_LINE.some(r => r.test(line))) hit('security', 'security line: ' + p); - if (!why.has('security') && SVG_PATH.test(p) && svgActive(line)) hit('security', 'active SVG line: ' + p); if (!why.has('data') && DATA_LINE.some(r => r.test(line))) hit('data', 'data line: ' + p); } } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 77ab96def..d84691081 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -79,8 +79,8 @@ const REVIEWER_BRIEFS: Record = { 'review/specialists/testing.md': { sha: 'd6cc2bef7be562387d0c0a22fefe646dff9c295f1d34d8453a6cb496584150e4', replay: '#3638' }, 'review/specialists/targeted.md': { sha: '7f92a0d0a8319c3c0e076fb3b1a4689c45120b2be2c14dfc7d4cb03aafa9d6a5', replay: 'GST-257' }, 'scripts/resolvers/fix-check.ts': { sha: '712bf5b0c439e96bac32f19cbe25394dabb4c94aa66600100bfde9b100e15a08', replay: '#3638' }, - 'lib/review-depth.ts': { sha: '7352b9e6e4e9f44c6e94f56cbb908d9a16b2ff536548770470af6169ffabccf7', replay: 'GST-911' }, - 'lib/review-risk.ts': { sha: 'c1b1baec718798df9e7629d641a864dc2125455f51339535f7c95057e0d83fcd', replay: 'GST-911' }, + 'lib/review-depth.ts': { sha: 'd3b269015a8d97b4cf5654c0dacc5bea7562b96e3c1c3c6fc9442ef4bb336637', replay: 'GST-911' }, + 'lib/review-risk.ts': { sha: 'df618e8aef3d641e8bf2073d8dca170740434acbb69c04555c4166df047f566c', replay: 'GST-911' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index f85f066ae..2edcf5e72 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -105,8 +105,8 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se }, 60_000); // Value: protects=an SVG that can execute script stays code and gets the security class (full depth), however it spells the script; - // fails_when=a script element, a namespaced script, an event handler, a javascript: URL (plain or entity-encoded) or a - // foreignObject plans as docs or loses the security class; seam=collectCommit + depthOfCommitInput + // fails_when=a script element, a namespaced script, an event handler, a javascript: URL (plain, entity-encoded or zero-padded) or a + // foreignObject plans as docs or loses the security class, or a handler or URL split across lines loses it; seam=collectCommit + depthOfCommitInput test.each([ ['a script element', ' '], ['a namespaced script', ' window.x = 42'], @@ -114,13 +114,17 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['a javascript: URL', ' x'], ['an entity-encoded javascript: URL', ' x'], ['a foreignObject', '
x
'], + ['a zero-padded javascript: URL', ' x'], + ['an event handler split across lines', ' '], + ['a javascript: URL split across lines', ' x'], ])('%s stays code with the security class', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) }))).toMatchObject({ class: 'code', depth: 'full', risk: ['security'] }); }, 60_000); // Value: protects=only an SVG made of drawing elements is data: anything that loads, links or declares outside that set stays code; - // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs; seam=collectCommit + depthOfCommitInput + // fails_when=an external image or stylesheet, an entity declaration, an animation or a CSS url() to another document reads as docs, + // including through a CSS escape or a zero-padded character reference (review pass 1 findings); seam=collectCommit + depthOfCommitInput test.each([ ['an external image', ' '], ['a CSS import', ' '], @@ -128,6 +132,10 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['an animation', ' '], ['a stylesheet processing instruction', ''], ['an entity declaration', ' ]>'], + ['a CSS-escaped import', ' '], + ['a CSS-escaped url()', ' '], + ['a zero-padded character reference import', ' '], + ['a zero-padded hex character reference url()', ' '], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code');