diff --git a/CHANGELOG.md b/CHANGELOG.md index b532d94ec..e5b016d95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [1.94.12.0] - 2026-11-03 + +### Fixed + +- **Six markup tricks no longer pass a remote-loading or scripted SVG off as a chart (GST-940).** `</style>` or `<!--` inside a `` let CSS that loads a remote image read as a plain drawing, and a non-ASCII namespace prefix such as `` hid a script. Any of these could get the SVG a light review. The style walker now reads the ``, comment that only an HTML parser ends early, quoted `>` or Unicode space can hide CSS. A non-ASCII character in an element or attribute name, such as a `gé:` prefix, keeps the SVG code too (GST-940); anything else, a script or `foreignObject` included, keeps it code. More than 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed` and fails the depth closed. The scan first lists the files that contain a candidate's diff --git a/lib/review-depth.ts b/lib/review-depth.ts index e8751cc8e..c337ebd98 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -93,7 +93,16 @@ const SVG_MARKUP = /^<(?:!--|!\[CDATA\[|\?xml\s|!DOCTYPE\s+svg(?:\s+PUBLIC\s+([" export const SVG_REFUSED = [/&(?!(?:amp|lt|gt|quot|apos);)/, /\\/, /@import/i, /url\((?!\s*#|['"]\s*#)/i, /expression\s*\(/i, /-moz-binding|\bbehavior\s*:/i]; /** A drawing's CSS, a style element's text or a style attribute's value, holds declarations only: no @ rule and no function but url(#id). */ const SVG_STYLE_OPEN = /<(?:[A-Za-z_][\w.-]*:)?style\b/gi; -const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style\s*>/i; +const SVG_STYLE_CLOSE = /^<\/(?:[A-Za-z_][\w.-]*:)?style[ \t\n\r]*>/i; +/** + * A style element's opening tag, read as quoted name="value" pairs separated by the ASCII whitespace XML and HTML share: any + * other shape (a value holding < or >, an unquoted or valueless attribute, />, another space) refuses, so it cannot hide a closing tag. + */ +const SVG_STYLE_TAG = /^<(?:[A-Za-z_][\w.-]*:)?style(?:[ \t\n\r]+[\w.:-]+[ \t\n\r]*=[ \t\n\r]*(?:"[^"<>]*"|'[^'<>]*'))*[ \t\n\r]*>/i; +/** An element name, ended by ASCII whitespace, / or >: a non-ASCII character in the name (a prefix such as gé:) refuses rather than reading as its ASCII start. */ +const SVG_TAG_NAME = /^<\/?(?:[A-Za-z_][\w.-]*:)?([A-Za-z_][\w.-]*)(?=[ \t\n\r/>]|$)/; +/** Every name= in the text, non-ASCII letters included so a prefix such as xé:href is read whole and refused. */ +const SVG_ATTR = /(?=]+)))/g; const SVG_CSS_REF = /url\(\s*(['"]?)#[\w.:-]*\1\s*\)/gi; const SVG_CSS_REFUSED = /[@(]/; /** An href or src value a drawing may hold: a same-document reference, or a raster image inline. */ @@ -102,10 +111,12 @@ const SVG_HREF = /^\s*(?:#|data:image\/(?:png|jpe?g|gif|webp);base64,[A-Za-z0-9+ /** * Whether an SVG's whole text is a drawing (GST-911): every element in SVG_ELEMENTS, no event handler, every href or src a * same-document reference or an inline raster image, CSS of declarations only, and nothing SVG_REFUSED names, read after character references are - * decoded. Every name= in the text is read, so a stray quote cannot hide a later attribute inside its value; an unquoted value - * stops at the next =, which keeps the scan linear. A style element's CSS is walked with indexOf to its closing tag, dropping - * comments and reading CDATA as CSS, so neither can hide a closing tag; any other markup inside it, or a style element that never - * closes, refuses. Anything else, an SVG that can run code included, stays code. + * decoded. Every name= in the text is read (SVG_ATTR), so a stray quote cannot hide a later attribute inside its value, and a name + * holding a non-ASCII character refuses, like an element name (SVG_TAG_NAME); an unquoted value + * stops at the next =, which keeps the scan linear. A style element's opening tag is read quote by quote (SVG_STYLE_TAG), and its CSS is walked with indexOf to its closing tag over the raw + * text, so a character reference cannot fake a closing tag or a comment, and any & in it refuses (GST-940). Comments and CDATA are + * read as CSS, so neither can hide a closing tag and an HTML-only comment end ( or --!>) exposes nothing unread; any other + * markup inside it, or a style element that never closes, refuses. Anything else, an SVG that can run code included, stays code. */ export function svgDrawing(text: string): boolean { const t = decodeCharRefs(text); @@ -113,36 +124,36 @@ export function svgDrawing(text: string): boolean { const cssDrawing = (css: string) => !SVG_CSS_REFUSED.test(css.replace(SVG_CSS_REF, '')); for (let at = 0; ;) { SVG_STYLE_OPEN.lastIndex = at; - const open = SVG_STYLE_OPEN.exec(t); + const open = SVG_STYLE_OPEN.exec(text); if (!open) break; - let i = t.indexOf('>', open.index) + 1; - if (!i) return false; + const tag = SVG_STYLE_TAG.exec(text.slice(open.index, open.index + 4096)); + if (!tag) return false; + let i = open.index + tag[0].length; let css = ''; for (;;) { - const lt = t.indexOf('<', i); + const lt = text.indexOf('<', i); if (lt === -1) return false; - css += t.slice(i, lt); - if (t.startsWith('', lt + 4) + 3; - if (i === 2) return false; - } else if (t.startsWith('', lt + 9); + css += text.slice(i, lt); + const [start, close] = text.startsWith(''] : text.startsWith(''] : [-1, '']; + if (start !== -1) { + const end = text.indexOf(close, start); if (end === -1) return false; - css += t.slice(lt + 9, end); + css += ' ' + text.slice(start, end) + ' '; i = end + 3; - } else if (SVG_STYLE_CLOSE.test(t.slice(lt, lt + 256))) { + } else if (SVG_STYLE_CLOSE.test(text.slice(lt, lt + 256))) { at = lt + 1; break; } else return false; } - if (!cssDrawing(css)) return false; + if (css.includes('&') || !cssDrawing(css)) 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)); + const tag = SVG_TAG_NAME.exec(t.slice(at, at + 256)); if (!tag || !SVG_ELEMENTS.has(tag[1]!.toLowerCase())) return false; } - for (const m of t.matchAll(/(?=]+)))/g)) { + for (const m of t.matchAll(SVG_ATTR)) { + if (/[^\x00-\x7f]/.test(m[1]!)) return false; const local = m[1]!.slice(m[1]!.lastIndexOf(':') + 1).toLowerCase(); const value = m[2] ?? m[3] ?? m[4] ?? ''; if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(value)) || (local === 'style' && !cssDrawing(value))) return false; @@ -153,7 +164,7 @@ export function svgDrawing(text: string): boolean { /** A hash of the constants and patterns that decide a depth, not of this file's bytes: a comment edit keeps it, a threshold edit changes it. */ export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ RISK_RULES, SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, BATCH_SCOPED, SCOPED_RANGE_LINES, ROLE_EFFORT, SVG_ELEMENTS: [...SVG_ELEMENTS], - patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF], SVG_STYLE_OPEN: [SVG_STYLE_OPEN], SVG_STYLE_CLOSE: [SVG_STYLE_CLOSE], SVG_CSS_REF: [SVG_CSS_REF], SVG_CSS_REFUSED: [SVG_CSS_REFUSED] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), + patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG, SVG_MARKUP: [SVG_MARKUP], SVG_REFUSED, SVG_HREF: [SVG_HREF], SVG_STYLE_OPEN: [SVG_STYLE_OPEN], SVG_STYLE_CLOSE: [SVG_STYLE_CLOSE], SVG_STYLE_TAG: [SVG_STYLE_TAG], SVG_TAG_NAME: [SVG_TAG_NAME], SVG_ATTR: [SVG_ATTR], SVG_CSS_REF: [SVG_CSS_REF], SVG_CSS_REFUSED: [SVG_CSS_REFUSED] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); /** diff --git a/package.json b/package.json index b7c574bf4..e8716b2f6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.94.11", + "version": "1.94.12", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 8275412cc..c1cf4fc10 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -74,6 +74,7 @@ const NO_LOSS_REPLAYS = { 'GST-967': "GST-967 dispatch-superset no-loss check (test/review-depth.test.ts, 'GST-967 dispatch-superset no-loss check'): over 1,440 planReview cases the branch plans exactly what main planned at 1022f4f except the 71 gate-class stops before Step 11, which add only Step 11's adversarial, outside and targeted voices, so no reviewer main dispatches is dropped or re-efforted; the brief bodies lib/review-brief.ts cuts from the ship sections hash to main's; only the lead's repeat-decision and Step 11 instructions changed; reviewer text is unchanged", 'GST-508+GST-464+GST-967': "wave review-53cfb5bfe011 merge of main 68410828f into #3852: ship/sections/review-army.md and adversarial.md are main's GST-967 bytes plus exactly the GST-508+GST-464 lines (the merged file minus main equals the wave minus fc292e22f, line for line), so GST-967's dispatch-superset no-loss check and GST-508's no-loss replay both hold; merged-head checks in GStack Minder drive handoffs/GSTA-202/merge-6841-checks.md", 'GST-508+R51+GST-911+GST-967': "wave review-53cfb5bfe011 merge of main 68410828f into #3852: lib/review-depth.ts is main's GST-967 bytes plus exactly the GST-508+R51+GST-911 lines (the merged file minus main equals the wave minus fc292e22f, line for line); GST-967's dispatch-superset no-loss check (test/review-depth.test.ts) passes on the merged file, and the GST-508+R51+GST-911 checks are rerun at the merged head (GStack Minder drive handoffs/GSTA-202/merge-6841-checks.md)", + 'GST-940': "GST-940 verdict no-loss check: svgDrawing reads a style element's opening tag as quoted pairs with ASCII whitespace, walks its body over the raw text, refuses any & in it, reads comments as CSS and refuses a non-ASCII element or attribute name; on 62,805 unique SVG files (21,658 with a style element) it gives the same verdict as main at 4bbad2422 on every file, and the planner over the last 100 first-parent main commits to 4bbad2422 changes no commit's depth, class or risk classes; the only verdicts it changes are the GST-940 cases in test/review-planner-misreads.test.ts; rerun on the merge with main at 8f08cb76a (GST-967 merged), with the same results; rerun on the merge with main at 5f705f367 (#3852 and #3864 merged), with the same results", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; const REVIEWER_BRIEFS: Record = { @@ -95,7 +96,7 @@ 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: 'd76a9afcfadfe0a95b15428c04e6f7a88c6b0fa75591201c45f023fec2bb90db', replay: 'GST-508+R51+GST-911+GST-967' }, + 'lib/review-depth.ts': { sha: '25050a7a61f81df649d1df0d0237dbf961671356395317c6a527741f5bb56877', replay: 'GST-940' }, 'lib/review-risk.ts': { sha: '08464b78a723ee463ca3a28d0abbc4eb8f4f512192061c703743707b66b5a311', replay: 'GST-942' }, }; diff --git a/test/review-planner-misreads.test.ts b/test/review-planner-misreads.test.ts index 0c34036a0..2bbaea298 100644 --- a/test/review-planner-misreads.test.ts +++ b/test/review-planner-misreads.test.ts @@ -125,7 +125,9 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se // 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 external DTD, 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) or behind a stray quote that - // pairs with a later one (review pass 2 finding), or any CSS beyond declarations and url(#id) (Step 11 finding: image-set()); + // pairs with a later one (review pass 2 finding), or any CSS beyond declarations and url(#id) (Step 11 finding: image-set()), + // including CSS behind a character-reference or comment, an HTML-only comment end, a quoted > on the style tag, a + // non-ASCII space before > in , or a tag or attribute under a non-ASCII namespace prefix (GST-940); // seam=collectCommit + depthOfCommitInput test.each([ ['an external image', ' '], @@ -153,11 +155,29 @@ describe('GST-911 misread 1: a chart SVG is data, an SVG that can run code is se ['a style attribute cross-fade()', ' '], ['a style element whose hides in a comment', ' -->image-set("https://x.test/p.png" 1x)}'], ['a style element whose hides in CDATA', ' */ svg{fill:image-set("https://x.test/p.png" 1x)} ]]>'], + ['a character-reference in a style element', ' '], + ['a hex character-reference in a style element', ' '], + ['a character-reference comment in a style element', ' '], + ['CSS after an HTML empty comment in a style element', ' '], + ['CSS after an HTML --!> comment end in a style element', ' '], + ['a inside a quoted style attribute', ' '], + ['an escaped ampersand in a style element', ' '], + ['a no-break space before > in ', ' '], + ['a vertical tab before > in ', ' '], + ['a script under a non-ASCII prefix', ' fetch(1)'], + ['a style under a non-ASCII prefix', ' rect{fill:image-set("https://x.test/p.png" 1x)}'], + ['an external href under a non-ASCII prefix', ' '], ])('%s keeps the SVG code', (_name, line) => { const fx = fixture(); expect(plan(fx, fx.commit({ 'results/chart.svg': chart(5, line) })).class).toBe('code'); }, 60_000); + // Value: protects=the GST-940 style walker still reads a plain chart style as a drawing: a comment, CDATA and a quoted attribute + // without markup; fails_when=the walker over-refuses them, so chart SVGs lose the light review; seam=svgDrawing + test('a style comment, CDATA and quoted attribute keep a chart a drawing (GST-940)', () => { + expect(svgDrawing(chart(5, ' '))).toBe(true); + }); + // Value: protects=a passive edit to an SVG that already carries a script is still code, because the whole file decides, not the // changed lines; and an SVG code names stays code like any consumed data file; fails_when=either plans as docs; seam=collectCommit test('an edit inside a scripted SVG, and an SVG code names, stay code', () => {