From 8b034914cc37e621b84d0c03dc824f087bd13ea8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 03:58:06 +0000 Subject: [PATCH] fix(review): a drawing-only SVG is data and a scripted one is security; session data files are not auth; diff-scope and the receipt depth read origin/ by full ref name (GST-911, GST-884) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-diff-scope | 53 ++++++++++++++++++++--- bin/gstack-review-plan | 2 +- changelog.d/gst-911-planner-misreads.md | 8 ++++ docs/review-policy.md | 20 ++++++--- lib/review-collect.ts | 31 +++++++++++--- lib/review-depth.ts | 57 ++++++++++++++++++++++--- lib/review-risk.ts | 15 ++++++- lib/ship-preflight.ts | 2 +- lib/ship-receipt-depth.ts | 7 ++- test/review-packets-removed.test.ts | 5 ++- 10 files changed, 169 insertions(+), 31 deletions(-) create mode 100644 changelog.d/gst-911-planner-misreads.md diff --git a/bin/gstack-diff-scope b/bin/gstack-diff-scope index 3ec81cf09..d7da8ad86 100755 --- a/bin/gstack-diff-scope +++ b/bin/gstack-diff-scope @@ -37,6 +37,30 @@ BASE="${1:-$(_default_base)}" HEAD_REF="" if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi +# The base as a commit (GST-884, mirroring lib/gate-diff/git.ts resolveBase from +# GST-883): a branch name reads refs/remotes/ (origin/main as given), then +# refs/remotes// that shares history with the head (origin first), +# then refs/heads/, all by full ref name, so a stale local main never widens +# the scope and a tag named main or origin/main cannot stand in for the branch. +# A sha, a relative ref (HEAD~1, main^2, @{u}) or a full refs/ name resolves as +# given. Prints nothing when nothing resolves. +_is_branch_name() { + case "$1" in refs/*|*~*|*^*|*@\{*|HEAD|*_HEAD) return 1 ;; esac + [[ ! "$1" =~ ^[0-9a-f]{7,64}$ ]] +} +_commit() { git rev-parse --verify -q "$1^{commit}" 2>/dev/null; } +_resolve_base() { + local ref="$1" head="$2" sha remote + if ! _is_branch_name "$ref"; then _commit "$ref"; return; fi + sha=$(_commit "refs/remotes/$ref") && { echo "$sha"; return; } + for remote in $(git remote 2>/dev/null | grep -x origin; git remote 2>/dev/null | grep -vx origin); do + sha=$(_commit "refs/remotes/$remote/$ref") || continue + git merge-base "$sha" "$head" >/dev/null 2>&1 && { echo "$sha"; return; } + done + sha=$(_commit "refs/heads/$ref") && { echo "$sha"; return; } + _commit "$ref" +} + FRONTEND=false BACKEND=false PROMPTS=false @@ -53,6 +77,19 @@ AUTH=false # the root-anchored globs alone (#3335). TEST_PATH_RE='(^|/)(tests?|spec|__tests__)/|(^|/)tests?\.py$|(^|/)test_[^/]+\.py$|_test\.(go|py|rb|ts|js|exs|dart)$|\.(test|spec)\.[jt]sx?$|_spec\.rb$|Test\.(java|kt)$' +# Data files (lib/review-depth.ts DATA_PATH) outside the config locations its +# DATA_CONFIG and agent-prose rules keep code: .github/, config/, dotfiles, +# *.config.*, .gstack/. Case-insensitive, as DATA_PATH is. +DATA_FILE_RE='\.(json|jsonl|ndjson|csv|tsv|png|jpe?g|gif|webp|bmp|ico|avif)$' +DATA_CONFIG_RE='(^|/)\.github/|(^|/)config/|(^|/)\.[^/]+$|(^|/)[^/]+\.config\.[^/]+$|^\.gstack/' +_session_data() { + local r=1 + shopt -s nocasematch + if [[ "$1" =~ $DATA_FILE_RE ]] && [[ ! "$1" =~ $DATA_CONFIG_RE ]]; then r=0; fi + shopt -u nocasematch + return "$r" +} + _print_flags() { echo "SCOPE_FRONTEND=$FRONTEND" echo "SCOPE_BACKEND=$BACKEND" @@ -68,7 +105,8 @@ _print_flags() { # Base reachability (#2526): a shallow CI checkout or an unfetched ref makes # `git diff` return an empty list ā€” all-false with exit 0, a green that means # "we could not look". Distinguish it before diffing. -if ! git rev-parse --verify -q "${BASE}^{commit}" >/dev/null 2>&1; then +BASE_SHA=$(_resolve_base "$BASE" "${HEAD_REF:-HEAD}" || true) +if [ -z "$BASE_SHA" ]; then _print_flags echo "SCOPE_ERROR=no_base" echo "# base ref '${BASE}' is not resolvable ā€” shallow checkout or missing fetch. Run: git fetch origin ${BASE}" @@ -91,16 +129,16 @@ if [ -n "$HEAD_REF" ] || [ "${2:-}" = "--head" ]; then echo "# --head '${HEAD_REF}' is not a commit in this repository" exit 2 fi - if ! git merge-base "${BASE}" "${HEAD_REF}" >/dev/null 2>&1; then + if ! git merge-base "${BASE_SHA}" "${HEAD_REF}" >/dev/null 2>&1; then _print_flags echo "SCOPE_ERROR=no_merge_base" echo "# '${BASE}' and '${HEAD_REF}' share no merge base" exit 2 fi - _collect < <(git diff -z "${BASE}...${HEAD_REF}" --name-only 2>/dev/null) + _collect < <(git diff -z "${BASE_SHA}...${HEAD_REF}" --name-only 2>/dev/null) else # Committed diff vs base (merge-base form; two-dot fallback when no merge base). -_collect < <(git diff -z "${BASE}...HEAD" --name-only 2>/dev/null || git diff -z "${BASE}" --name-only 2>/dev/null || true) +_collect < <(git diff -z "${BASE_SHA}...HEAD" --name-only 2>/dev/null || git diff -z "${BASE_SHA}" --name-only 2>/dev/null || true) # Working-tree changes (staged + unstaged). `git diff HEAD` fails on a repo # with no commits; tolerated. _collect < <(git diff -z HEAD --name-only 2>/dev/null || true) @@ -190,9 +228,12 @@ for f in ${FILES_LIST[@]+"${FILES_LIST[@]}"}; do *.graphql|*.gql|openapi.*|swagger.*) m_api=true ;; esac - # Auth: authentication, authorization, sessions, permissions + # Auth: authentication, authorization, sessions, permissions. A data file + # named for sessions (recovered-sessions.jsonl, GST-911) holds rows, not + # session handling, so it is not auth; session config still is. case "$f" in - *auth*|*session*|*jwt*|*oauth*|*permission*|*role*) m_auth=true ;; + *auth*|*jwt*|*oauth*|*permission*|*role*) m_auth=true ;; + *session*) _session_data "$f" || m_auth=true ;; esac # Backend: code that isn't a frontend component/view file. Includes ESM/CJS diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 93f6aee5e..56031d8e8 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -149,7 +149,7 @@ function main(): number { const gateScan = { tagged: gateTagged, head }; if (has('--json')) { const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; - process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed }) + '\n'); + process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed, drawings: collected.drawings }) + '\n'); return EXIT.ok; } const out = [`REVIEW_CLASS: ${plan.class} (${plan.reasons.slice(0, 3).join('; ') || 'no signals'})`, reviewDepthLine(plan), `GATE_SCAN: ${gateTagged === null ? 'unavailable' : `tagged=${gateTagged}`} head=${head}`, diff --git a/changelog.d/gst-911-planner-misreads.md b/changelog.d/gst-911-planner-misreads.md new file mode 100644 index 000000000..b3944c884 --- /dev/null +++ b/changelog.d/gst-911-planner-misreads.md @@ -0,0 +1,8 @@ +### Fixed + +- **/ship no longer reviews a results write-up as if it were gate code.** Three planner misreads on gstack-evals#132 (GST-911) pushed a docs PR to the five-reader full panel. A chart SVG made of drawing elements now counts as data, like the same chart as a PNG; an SVG with a script, `foreignObject`, event handler or `javascript:` URL stays code and now gets the security class. A data file named for sessions (`recovered-sessions.jsonl`) no longer sets the auth scope, so it adds no auth reader and no gate class; session code and session config still do. +- **`gstack-diff-scope` and the receipt depth read `origin/` by its full ref name (GST-884).** A local `main` behind `origin/main` no longer adds upstream files to the scope, and a tag named `main` or `origin/main` can no longer stand in for the branch. + +### Tests + +- `test/review-planner-misreads.test.ts`: a 600-point matplotlib-style chart plans docs, light, runtime 0 (before: code, 636 runtime lines); six ways an SVG can run script keep it code with the security class, and six other non-drawing constructs keep it code; session data files leave `SCOPE_AUTH` false while session code and config set it; a stale local `main` and tags named `main` or `origin/main` leave the scope and the receipt base unchanged. diff --git a/docs/review-policy.md b/docs/review-policy.md index 1d4e3612b..e3662dc64 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -181,7 +181,12 @@ follows that file, four files deep. Precedence: agent prose (`.gstack/**`), a te fixture path, a gate path, a config file (`.github/**`, anything under a `config/` 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, TOML and SVG are always code. More than +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 +`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 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed` and fails the depth closed. Only `classifyDiff` applies the rule (`lib/review-collect.ts` finds the consumers, `gstack-ship-preflight`'s `RUNTIME_LINES` reads the same @@ -203,15 +208,18 @@ added and deleted lines; any class makes the depth full (`risk:` in [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code)'s matchers. **security** is a trust-boundary path (trust, redact, sanitize, neutralize, consent, escape) or line (`shell: true`, `execSync(`, `eval`, `new Function(`, -raw HTML sinks, a shell run through argv, recursive deletes). **data** is user data: a path naming pii, gdpr, ccpa, privacy, +raw HTML sinks, a shell run through argv, recursive deletes), or a changed SVG +line that can run code (a script or `foreignObject` element, an event handler, a +`javascript:` URL). **data** is user data: a path naming pii, gdpr, ccpa, privacy, personal, account, customer, cookie, persistence, storage or database, or a line that drops a table or column, deletes rows or bulk-deletes records. Auth scope, migrations and deleted code paths were already gate class. `RISK_RULES` hashes the patterns and `DEPTH_RULES` includes it. -**UI text and SVGs** stay code (light unless a risk class applies). A docs path -for UI copy, locale values and chart SVGs waits for an allowlist classifier: three -rounds of R51 review found bypasses in line-pattern versions of both. +**UI text** stays code (light unless a risk class applies). A docs path for UI +copy and locale values waits for an allowlist classifier: three rounds of R51 +review found bypasses in line-pattern versions. Chart SVGs use the drawing +allowlist above, which reads the whole file rather than changed lines. **Batched checklists (the lean ensemble, GSTA-167 part 2).** One specialist subagent reads Testing and Security on every code or gate diff, plus @@ -330,7 +338,7 @@ and matching is token-bounded: `costume` never reads as cost. |---|---| | money | a path segment naming cost, spend, budget, ledger, billing, price, fee or invoice; or a changed line with an identifier ending in `_usd` or `Usd`, any `spend_cap` spelling, or a call to a paid provider `lib/pricing.ts` prices | | gate | a hunk the gate scan tagged: a relaxed threshold, a skipped test, a loosened check | -| auth | `SCOPE_AUTH` from `gstack-diff-scope`, on a code path with an auth, session, jwt, permission or role token in its name | +| auth | `SCOPE_AUTH` from `gstack-diff-scope`, on a code path with an auth, session, jwt, permission or role token in its name; a data file named for sessions (`recovered-sessions.jsonl`) outside a config location does not set `SCOPE_AUTH` | | secret | a secrets, credentials, keychain or token path, or a changed line that reads a key-named environment variable (`OPENAI_API_KEY`, `GH_TOKEN`) or a credential store | An input the classifier cannot read (the diff, `gstack-diff-scope`, the gate diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7fbfdae12..66fa33910 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -12,7 +12,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { GENERATED_PATH, parseRegistry } from './regen'; import { RESULT_CODES } from './result-codes'; -import { classifyPath, dataCandidate, type ClassInput, type DepthError, type PathLines, type RegistryInfo } from './review-depth'; +import { classifyPath, dataCandidate, svgCandidate, svgDrawing, type ClassInput, type DepthError, type PathLines, type RegistryInfo } from './review-depth'; export interface Collected extends ClassInput { perPath: PathLines[]; @@ -28,6 +28,7 @@ export interface Collected extends ClassInput { mergeBase: string | null; untracked: string[]; consumed: string[] | 'all'; + drawings: string[]; } function git(root: string, args: string[]): { ok: boolean; status: number | null; out: string; err: string } { @@ -106,7 +107,7 @@ export const CONSUMER_SCAN_DEPTH = 4; * a collector error, which fails the depth closed. */ export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[]): string[] | 'all' { - const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot)).sort(); + const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot) || svgCandidate(p, classRoot)).sort(); if (candidates.length > CONSUMER_SCAN_CAP) return 'all'; const base = (p: string) => p.slice(p.lastIndexOf('/') + 1); const named = new Map>(); @@ -146,9 +147,20 @@ export function scanConsumers(root: string, paths: readonly string[], revs: read return candidates.filter(p => consumed.has(p)); } -function result(diff: ReturnType, extra: Pick): Collected { +/** The largest SVG whose text is read for the drawing check; a larger one stays code. */ +export const DRAWING_READ_CAP = 8 * 1024 * 1024; + +/** The changed SVG candidates whose whole text, read by `read` (null when it cannot be read), is a drawing (lib/review-depth.ts svgDrawing). */ +function drawingsAmong(perPath: readonly PathLines[], classRoot: string | readonly string[], read: (p: string) => Buffer | null): string[] { + return perPath.filter(e => !e.binary && svgCandidate(e.path, classRoot)).map(e => e.path).filter(p => { + const buf = read(p); + return !!buf && buf.length <= DRAWING_READ_CAP && !buf.includes(0) && svgDrawing(buf.toString('utf8')); + }).sort(); +} + +function result(diff: ReturnType, extra: Pick & { drawings?: string[] }): Collected { const paths = diff.perPath.map(e => e.path).sort(); - return { ...diff, ...extra, paths, gateTagged: null, diffLines: diff.perPath.reduce((n, e) => n + e.lines, 0) }; + return { ...diff, drawings: [], ...extra, paths, gateTagged: null, diffLines: diff.perPath.reduce((n, e) => n + e.lines, 0) }; } /** The range `baseSha`...`headSha` from git objects alone; the registry is the base commit's, never the head's. */ @@ -165,7 +177,11 @@ export function collectCommit(root: string, baseSha: string, headSha: string): C const tree = git(root, ['ls-tree', '-r', '--name-only', '-z', headSha]); const skillDirs = [...new Set(tree.out.split('\0').flatMap(p => /^([^/]+)\/SKILL\.md(\.tmpl)?$/.exec(p)?.[1] ?? []))].sort(); const consumed = scanConsumers(root, [...diff.perPath.map(e => e.path), ...diff.deleted], [headSha, mergeBase], false, skillDirs, errors); - return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [], consumed }); + const drawings = drawingsAmong(diff.perPath, skillDirs, p => { + const blob = spawnSync('git', ['cat-file', 'blob', `${headSha}:${p}`], { cwd: root, timeout: 60_000, maxBuffer: DRAWING_READ_CAP + 1 }); + return blob.status === 0 && !blob.error ? blob.stdout : null; + }); + return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [], consumed, drawings }); } /** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ @@ -193,5 +209,8 @@ export function collectLive(root: string, base: string): Collected { if (!binary) diff.added[p] = text.split('\n'); } const consumed = scanConsumers(root, [...diff.perPath.map(e => e.path), ...diff.deleted], [mergeBase], true, root, errors); - return result(diff, { root, registry, errors, mergeBase, untracked, consumed }); + const drawings = drawingsAmong(diff.perPath, root, p => { + try { return fs.statSync(path.join(root, p)).size <= DRAWING_READ_CAP ? fs.readFileSync(path.join(root, p)) : null; } catch { return null; } + }); + return result(diff, { root, registry, errors, mergeBase, untracked, consumed, drawings }); } diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 714e44183..78af654ac 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -13,7 +13,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import type { ResultCodeName } from './result-codes'; import { matchesAny } from './ship-policy'; -import { matchRisk, RISK_RULES, type RiskClass } from './review-risk'; +import { decodeCharRefs, matchRisk, RISK_RULES, SVG_PATH, svgActive, type RiskClass } from './review-risk'; export type DiffClass = 'docs' | 'tests' | 'code' | 'gate'; export type PathClass = 'docs' | 'tests' | 'code'; @@ -58,7 +58,7 @@ const GATE_PATH = [ /^lib\/review-(depth|collect|scope|targeted|risk)\.ts$/, /^review\/specialists\/targeted\.md$/, /^bin\/gstack-review-plan$/, /^lib\/ship-queue[^/]*\.ts$/, /^lib\/regen\.ts$/, /^\.gstack\/generated\.json$/, /^ship\/sections\/(review-army|adversarial)\.md\.tmpl$/, /^scripts\/resolvers\/review-army\.ts$/, ]; -/** Data files: JSON, CSV, TSV and raster images. YAML, TOML and SVG are never data here: deploy configs, API specs and executable SVG stay code. */ +/** Data files: JSON, CSV, TSV and raster images. YAML and TOML are never data here: deploy configs and API specs stay code. An SVG is data only as a drawing (svgDrawing). */ export const DATA_PATH = /\.(json|jsonl|ndjson|csv|tsv|png|jpe?g|gif|webp|bmp|ico|avif)$/i; /** Data-shaped files tools read by convention, with no code naming them: always code. */ const DATA_CONFIG = [ @@ -66,10 +66,50 @@ const DATA_CONFIG = [ /(^|\/)(package|package-lock|npm-shrinkwrap|composer|deno|jsr|tsconfig[^/]*|jsconfig[^/]*|manifest|app|vercel|netlify|firebase|renovate|turbo|nx|lerna|biome|angular|nest-cli|lint-staged|components|pyrightconfig|cdk|serverless|appsettings[^/]*)\.json$/i, ]; +/** + * The SVG elements a chart is drawn with, lowercased, under any namespace prefix: shapes, text, paint servers, clip paths, + * filters (feImage loads a document, so it is left out), CSS, and the RDF metadata plotting libraries write. No script, + * foreignObject, link, animation or embedded document. + */ +const SVG_ELEMENTS = new Set([ + 'svg', 'g', 'defs', 'symbol', 'use', 'path', 'rect', 'circle', 'ellipse', 'line', 'polyline', 'polygon', 'text', 'tspan', 'textpath', 'title', 'desc', + 'metadata', 'clippath', 'mask', 'pattern', 'marker', 'lineargradient', 'radialgradient', 'stop', 'style', 'image', 'filter', + 'feblend', 'fecolormatrix', 'fecomponenttransfer', 'fecomposite', 'feconvolvematrix', 'fediffuselighting', 'fedisplacementmap', 'fedistantlight', + 'fedropshadow', 'feflood', 'fefunca', 'fefuncb', 'fefuncg', 'fefuncr', 'fegaussianblur', 'femerge', 'femergenode', 'femorphology', 'feoffset', + 'fepointlight', 'fespecularlighting', 'fespotlight', 'fetile', 'feturbulence', + 'rdf', 'work', 'agent', 'type', 'date', 'format', 'creator', 'description', 'license', +]); +/** 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]; +/** 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; + +/** + * 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, and nothing SVG_REFUSED names, read after character references are + * decoded. Anything else, an SVG that can run code included, stays code. + */ +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; + 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)); + if (!tag || !SVG_ELEMENTS.has(tag[1]!.toLowerCase())) return false; + } + for (const m of t.matchAll(/(?]+))/g)) { + const local = m[1]!.slice(m[1]!.lastIndexOf(':') + 1).toLowerCase(); + if (local.startsWith('on') || ((local === 'href' || local === 'src') && !SVG_HREF.test(m[2] ?? m[3] ?? m[4] ?? ''))) return false; + } + return true; +} + /** 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, - patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH], DATA_PATH: [DATA_PATH], DATA_CONFIG }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), + 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] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), })).digest('hex').slice(0, 16); /** @@ -97,6 +137,11 @@ export function dataCandidate(p: string, root?: string | readonly string[]): boo return DATA_PATH.test(p) && classifyPath(p, root) === 'code' && ![...AGENT_PROSE, ...GATE_PATH, ...DATA_CONFIG].some(r => r.test(p)); } +/** An SVG the data rule may move to docs once the collector reads its whole text as a drawing (ClassInput.drawings); never a data file another file is named through. */ +export function svgCandidate(p: string, root?: string | readonly string[]): boolean { + return SVG_PATH.test(p) && classifyPath(p, root) === 'code' && ![...AGENT_PROSE, ...GATE_PATH, ...DATA_CONFIG].some(r => r.test(p)); +} + /** Whether a range needs the gate scan: any path that is not docs by the path-only rule. The data rule never skips it. */ export function gateScanNeeded(paths: readonly string[], root?: string | readonly string[]): boolean { return paths.some(p => classifyPath(p, root) !== 'docs'); @@ -131,6 +176,8 @@ export interface ClassInput { * (lib/review-collect.ts). A candidate not listed counts as docs; 'all' or no scan keeps every data file code. */ consumed?: readonly string[] | 'all'; + /** The SVG candidates (svgCandidate) whose whole text the collector read as a drawing (svgDrawing); any other SVG stays code. */ + drawings?: readonly string[]; /** Added and deleted lines per path (lib/review-collect.ts): the risk classes read them; without them no line signal applies. */ added?: Record; removed?: Record; @@ -139,7 +186,7 @@ export interface ClassResult { class: DiffClass; reasons: string[]; errors: Dept const sanitize = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').slice(0, 200); const isOutput = (i: ClassInput, p: string) => !!i.registry?.outputs.length && matchesAny(i.registry.outputs, p); -const unconsumedData = (i: ClassInput, p: string) => Array.isArray(i.consumed) && !i.consumed.includes(p) && dataCandidate(p, i.root); +const unconsumedData = (i: ClassInput, p: string) => Array.isArray(i.consumed) && !i.consumed.includes(p) && (dataCandidate(p, i.root) || (!!i.drawings?.includes(p) && svgCandidate(p, i.root))); const pathClass = (i: ClassInput, p: string): PathClass => ((i.stampOnly ?? []).includes(p) || unconsumedData(i, p) ? 'docs' : classifyPath(p, i.root)); /** The diff's class, raised to gate by any gate signal; `errors` lists every input that fails the depth closed; `runtimeLines` counts code-class lines outside the base registry's outputs. */ diff --git a/lib/review-risk.ts b/lib/review-risk.ts index c1ed328a1..522c11d62 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -6,7 +6,8 @@ * money, auth and secret are the targeted pass's matchers (GST-257). security is a trust-boundary path or line * (a redaction, sanitizer, consent or trust module; a shell, eval or raw HTML sink); data is user data (a path * naming personal or stored data, or a line that drops or bulk-deletes it). Path and identifier signals are - * token-bounded: costume never names cost. + * token-bounded: costume never names cost. A changed SVG line that can run code (a script or foreignObject element, an + * event handler, a javascript: URL; character references decoded) is security (GST-911). */ import { createHash } from 'node:crypto'; @@ -40,6 +41,15 @@ const SECURITY_LINE = [ /\brmSync\s*\([^)]*recursive\s*:\s*true/, /\brm\s+-(?:[a-z]*r[a-z]*f|[a-z]*f[a-z]*r)\b/, /\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:). */ +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); + 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)); }; 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, @@ -49,7 +59,7 @@ const DATA_LINE = [ /** A hash of the patterns that decide the risk classes, not of this file's bytes; lib/review-depth.ts folds it into DEPTH_RULES. */ export const RISK_RULES = createHash('sha256').update(JSON.stringify({ RISK_CLASSES, MONEY_PATH_TOKENS, AUTH_PATH_TOKENS, AUTH_PATH_PREFIXES, KEY_NAME, SECURITY_PATH_TOKENS, DATA_PATH_TOKENS, - patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, DATA_LINE }).map(([k, v]) => [k, v.map(r => '/' + r.source + '/' + r.flags)]), + patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, DATA_LINE, SVG_PATH: [SVG_PATH], SVG_ACTIVE }).map(([k, v]) => [k, v.map(r => '/' + r.source + '/' + r.flags)]), })).digest('hex').slice(0, 16); export const tokens = (p: string) => p.replace(/([a-z0-9])([A-Z])/g, '$1 $2').toLowerCase().split(/[^a-z0-9]+/).filter(Boolean); @@ -78,6 +88,7 @@ export function matchRisk(i: RiskInput): Map { 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/lib/ship-preflight.ts b/lib/ship-preflight.ts index 519ee5475..405a4eda8 100644 --- a/lib/ship-preflight.ts +++ b/lib/ship-preflight.ts @@ -117,7 +117,7 @@ function merge(cwd: string, base: string, r: Report): boolean { let runtime = 'unknown'; try { const j = JSON.parse(plan.stdout); - const c = classifyDiff({ paths: j.paths, perPath: j.per_path, stampOnly: j.stamp_only, registry: j.registry, consumed: j.consumed, root: cwd, gateTagged: 0 }); + const c = classifyDiff({ paths: j.paths, perPath: j.per_path, stampOnly: j.stamp_only, registry: j.registry, consumed: j.consumed, drawings: j.drawings, root: cwd, gateTagged: 0 }); runtime = String(c.runtimeLines ?? 'unknown'); } catch { /* unknown */ } r.add(`RUNTIME_LINES: ${runtime} (gstack-ship-budget check --runtime-lines at the Step 9 and 12 boundaries)`); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index ad8933cfb..4452321e9 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -40,6 +40,9 @@ function git(root: string, args: string[]): { ok: boolean; out: string } { return { ok: r.status === 0, out: (r.stdout ?? '').trim() }; } +/** A base as a commit; origin/ reads refs/remotes/origin/ by its full name, so a tag named origin/main cannot stand in for it (GST-884). */ +const baseCommit = (root: string, ref: string) => git(root, ['rev-parse', '--verify', '--quiet', `${/^origin\//.test(ref) ? `refs/remotes/${ref}` : ref}^{commit}`]); + /** gstack-diff-scope on the committed range `base`...`head` (`--head`): the same signals at write, carry and the queue. */ function scopeOfRange(root: string, binDir: string, base: string, head: string): Record { const sr = runScriptSync(path.join(binDir, 'gstack-diff-scope'), [base, '--head', head], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024 }); @@ -58,7 +61,7 @@ function scopeOfRange(root: string, binDir: string, base: string, head: string): */ export function receiptDepthAtHead(repoRoot: string, base: string, binDir: string, head: string): ReceiptDepth { const branch = base.replace(/^origin\//, ''); - const baseSha = git(repoRoot, ['rev-parse', '--verify', '--quiet', `origin/${branch}^{commit}`]); + const baseSha = baseCommit(repoRoot, `origin/${branch}`); const baseRev = baseSha.ok ? baseSha.out : branch; const c = collectCommit(repoRoot, baseRev, head); const scope = scopeOfRange(repoRoot, binDir, baseRev, head); @@ -156,7 +159,7 @@ export interface DepthVerdict { ok: boolean; line: string; message?: string } /** The head's committed range against the base, read once for the depth verdict and the targeted line. */ interface HeadInputs { baseSha: { ok: boolean; out: string }; c: ReturnType; scope: Record } function headInputs(root: string, baseRef: string, head: string): HeadInputs { - const baseSha = git(root, ['rev-parse', '--verify', '--quiet', `${baseRef}^{commit}`]); + const baseSha = baseCommit(root, baseRef); const baseRev = baseSha.ok ? baseSha.out : baseRef; return { baseSha, c: collectCommit(root, baseRev, head), scope: scopeOfRange(root, BIN_DIR, baseRev, head) }; } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 747723390..77ab96def 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -56,6 +56,7 @@ const NO_LOSS_REPLAYS = { 'G1': "GSTA-167 variant G Arm G1: live no-loss replay of both prose voices with part 1's prompts, 2 passes, normalizer v2, paired rule against pooled C' (parts 1 and 3; specialists at medium as in C and C2)", 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored Lāˆ˜G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", + 'GST-911': "GST-911 no-loss replay (GSTA-249/13): the planner over the last 100 first-parent main commits to f5cb6ec1e, origin/main's code against this branch, gate scan fixed at 0, changes no commit's depth, class, risk classes, targeted classes or SCOPE_AUTH; the three misreads (a drawing-only SVG, a session-named data file, a bare base) are proven on fixtures (test/review-planner-misreads.test.ts), where every SVG that can run script stays code with the security class; reviewer text is unchanged", 'R51': "R51 risk-tier routing no-loss re-score (GSTA-167): the saved panel findings (349 records over 24 of the 26 no-loss units) re-scored against the branch's plan lose no blocker whose only finders are roles it drops, and every gate, security, user-data and money fixture keeps main's reviewers plus the full panel (test/review-targeted.test.ts, test/review-risk-routing.test.ts); reviewer text is unchanged", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; @@ -78,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: 'd649569a163c03476c6e0c5ea295f5d02088f96aab85fbc7d558f2ac2b27006a', replay: 'R51' }, - 'lib/review-risk.ts': { sha: 'dbef7475d467e17340b21a2bacb52a16a7ccb1ee66291900ed2d36dfb5cfb7f2', replay: 'R51' }, + 'lib/review-depth.ts': { sha: '7352b9e6e4e9f44c6e94f56cbb908d9a16b2ff536548770470af6169ffabccf7', replay: 'GST-911' }, + 'lib/review-risk.ts': { sha: 'c1b1baec718798df9e7629d641a864dc2125455f51339535f7c95057e0d83fcd', replay: 'GST-911' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => {