diff --git a/CHANGELOG.md b/CHANGELOG.md index b82f404c5..ef9570ae7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [1.94.2.0] - 2026-11-03 + +### Fixed + +- **More recursive deletes keep the security class (GST-942).** `fs.rmSync(path.join(dir, "x"), { recursive: true })`, a `)` inside a comment before `recursive: true`, `fs.rm`/`fs.promises.rm`/an imported `rm`, and a new call a formatter split across lines all slipped past the rule, because it stopped reading at the first `)` and read one line at a time. The review depth could then drop to light on real recursive-delete code. The rule now reads up to 500 characters after `rmSync(` or `rm(` across the file's changed lines, still in linear time. An edit to one line of an existing split call is still read on its own. + ## [1.94.1.0] - 2026-11-03 ### Fixed diff --git a/VERSION b/VERSION index 39bbd02ef..03e5fa81d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.94.1.0 +1.94.2.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index 43ae74b32..c066b3562 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.94.1.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.94.2.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/docs/review-policy.md b/docs/review-policy.md index 89e4681d0..7f5b229c6 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -252,7 +252,7 @@ line that can run code (a script or `foreignObject` element, an event handler, a rules are coarse on purpose and parse no import clause or call arguments; `test/review-risk-linear.test.ts` holds each of them under 100 ms on 200,000-character hostile lines, its anchor followed by a long run of the -characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` call with more than 500 characters before the first `)` after it counts as security on its own. **data** is user data: a path naming pii, gdpr, ccpa, privacy, +characters it repeats included. `test/review-risk-redos.test.ts` (GST-898) holds every risk pattern, R51's `rm` and `rmSync` rules included, under 100 ms on 100,000-character hostile lines. An `rmSync(` or `rm(` call (`fs.rm`, `fs.promises.rm` or an imported `rm`) counts as security when `recursive: true` follows within 500 characters, whatever the arguments hold (a `path.join(...)` first argument or a `)` in a comment included) and also when a formatter split a new call across lines, which is read over the file's changed lines joined (GST-942; an edit to one line of an existing split call is read on its own); a call with more than 500 characters before the first `)` after it counts on its own. **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 diff --git a/lib/review-risk.ts b/lib/review-risk.ts index b6c5b92f9..1f2132522 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -55,9 +55,11 @@ export const SECURITY_LINE = [ /\b(?:includes|startsWith|endsWith|indexOf)\s*\(\s*['"`]\.\.[/\\]{0,2}['"`]|[!=]==?\s*['"`]\.\.['"`]|['"`]\.\.['"`]\s*[!=]==?|\\\.\\\./, /(? t.some(x 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, MONEY_LINE_TOKENS, MONEY_PAIRS, - 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)]), + patterns: Object.entries({ MONEY_LINE, SECRET_LINE, SECRET_PATH: [SECRET_PATH], SECURITY_LINE, SECURITY_JOINED, 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); @@ -113,6 +115,7 @@ export function matchRisk(i: RiskInput): Map { if (!why.has('security') && SECURITY_LINE.some(r => r.test(line))) hit('security', 'security line: ' + p); if (!why.has('data') && DATA_LINE.some(r => r.test(line))) hit('data', 'data line: ' + p); } + if (!why.has('security') && [i.added?.[p], i.removed?.[p]].some(l => l && SECURITY_JOINED.some(r => r.test(l.join('\n'))))) hit('security', 'security lines: ' + p); } return new Map(RISK_CLASSES.filter(c => why.has(c)).map(c => [c, why.get(c)!])); } diff --git a/package.json b/package.json index fe23e332c..6ed555daa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.94.1", + "version": "1.94.2", "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 89eaf5aa5..8275412cc 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -66,6 +66,7 @@ const NO_LOSS_REPLAYS = { 'GST-508+R51': "wave review-53cfb5bfe011 merge of #3744 into main: lib/review-depth.ts keeps both admitted changes, the GST-508 no-loss replay of review_exclude_paths (Arm V 33 of 41 blockers vs R 33 of 41, p_NI 0.0279; GStack Minder drive handoffs/GSTA-202/noloss-results.md) and the R51 risk-tier re-score with GST-850's dispatch-superset no-loss check: excluded paths sit beside the added and removed line maps, and the exclusion filter keeps the publishes-docs rule", 'GST-508+GST-464': "wave review-53cfb5bfe011 merge of #3744 into main: ship/sections/review-army.md and adversarial.md carry the GST-508 no-loss-replayed exclude pathspec placeholders inside the brief bodies (handoffs/GSTA-202/noloss-results.md) and the GST-464 lead-instruction edits outside the cut spans, which its brief-identity no-loss check admitted", 'GST-986': "GST-986 brief-identity no-loss check (the brief-body test below): every reviewer brief lib/review-brief.ts cuts from ship/sections/adversarial.md is byte-identical to the bodies at the GST-257 pin; only the lead's Step 11 instruction naming the targeted record's path changed, outside the cut spans", + 'GST-942': "GST-942 dispatch-superset no-loss check: the rmSync rule no longer stops at the first ), a matching rm( rule is added, and both read a file's changed lines joined for a delete split across lines, so a recursive delete raises security on more diffs and never on fewer; over the last 100 first-parent main commits to 3d99c2249 (62,531 changed lines) the line rules match all 170 lines the old rule matched plus 19 more, and no commit changes depth, class or risk class; reviewer text is unchanged", 'GST-911-redos': "GST-911 ReDoS fix verdict-identity no-loss check (wave review-e7b551ebd865): SVG_REFUSED's url() entry, rewritten to run in linear time, refuses exactly the strings #3838's pattern refused (test/review-risk-redos.test.ts: 60,000 seeded strings and the hand-picked edges, and every SVG blob in the repository's history, with zero differences); SVG_MARKUP now refuses a DOCTYPE naming any document but W3C's SVG DTD (test/review-planner-misreads.test.ts), which only keeps more SVGs code and so drops no reviewer; GST-911's no-loss replay holds unchanged; 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", 'GST-508+R51+GST-911': "wave review-53cfb5bfe011 merge of main 3d99c2249 (#3848) into #3852: lib/review-depth.ts keeps GST-508's review_exclude_paths (no-loss replay, GStack Minder drive handoffs/GSTA-202/noloss-results.md), R51 with GST-850's dispatch-superset no-loss check, and GST-911's SVG drawings with GST-911-redos (no-loss replay: 0 of 100 main commits changed, reports/GSTA-249-911-decisions.tsv; test/review-planner-misreads.test.ts, test/review-risk-redos.test.ts); lib/review-risk.ts is main's GST-911 bytes. Merged-head checks (handoffs/GSTA-202/merge-3d99c22-checks.md): Arm P on unit 3593 differs only in the exclude pathspec and the disclosure line, the same brief diff as before the merge, and GST-911 panel unit f5cb6ec1e plans, dispatches and renders byte-identical briefs under 3d99c2249 and the merged head", @@ -95,7 +96,7 @@ const REVIEWER_BRIEFS: Record = { '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-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-508+R51+GST-911' }, + 'lib/review-risk.ts': { sha: '08464b78a723ee463ca3a28d0abbc4eb8f4f512192061c703743707b66b5a311', replay: 'GST-942' }, }; /** diff --git a/test/review-risk-redos.test.ts b/test/review-risk-redos.test.ts index 7d40d9df3..4b9982fcb 100644 --- a/test/review-risk-redos.test.ts +++ b/test/review-risk-redos.test.ts @@ -5,7 +5,7 @@ */ import { describe, expect, test } from 'bun:test'; import { SVG_REFUSED, svgDrawing } from '../lib/review-depth'; -import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_LINE, matchRisk } from '../lib/review-risk'; +import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECRET_PATH, SECURITY_JOINED, SECURITY_LINE, matchRisk } from '../lib/review-risk'; const L = 100_000; const ms = (f: () => unknown) => { const t = performance.now(); f(); return performance.now() - t; }; @@ -18,7 +18,7 @@ describe('linear time on hostile input', () => { // or on a line of repeated anchors; seam=every pattern in SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE and SECRET_PATH const hostile = [ run('rm -', 'r'), run('rm -', 'f'), run('rm -r', 'r'), run('rm -f', 'a'), run('rm\t-', 'r'), run('rm ', ' '), run('rm -', 'r', '1'), run('rm -', 'rf', '_'), run('', 'rm -r '), - run('', 'rmSync('), run('rmSync(', ' '), run('rmSync(', 'recursive '), run('rmSync(', 'recursive: '), + run('', 'rmSync('), run('rmSync(', ' '), run('rmSync(', 'recursive '), run('rmSync(', 'recursive: '), run('', '.rm('), run('.rm(', ' '), run('.rm(', 'recursive: '), run('', 'rm('), run('rm(', ' '), run('rmSync(', ' \n'), run('shell', ' '), run('shell:', ' '), run('shell=', ' '), run('', '; '), run(';', ' '), run('; eval', ' '), run('eval', ' '), run('', ';eval '), run('execSync', ' '), run('child_process.exec', ' '), run('os.system', ' '), run('new', ' '), run('new Function', ' '), run('spawn(', ' '), run('Bun.spawn(', ' '), run('Bun.spawn([', ' '), run('innerHTML', ' '), @@ -27,7 +27,7 @@ describe('linear time on hostile input', () => { run('', 'a_'), run('', 'aU'), run('a', 'A'), run('spend', '_'), run('new Anthropic', ' '), run('.messages.create', ' '), run('$', 'A_'), run('${', 'A_'), run('', '$A_'), run('process.env.', 'A_'), run('process.env[', ' '), run('os.environ[', ' '), run('os.getenv(', ' '), run('ENV[', ' '), ]; - test.each(Object.entries({ SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE, SECRET_PATH: [SECRET_PATH] }))('every %s pattern finishes under 100 ms on each 100k-character hostile line', (_n, patterns) => { + test.each(Object.entries({ SECURITY_LINE, SECURITY_JOINED, DATA_LINE, SECRET_LINE, MONEY_LINE, SECRET_PATH: [SECRET_PATH] }))('every %s pattern finishes under 100 ms on each 100k-character hostile line', (_n, patterns) => { const slow = patterns.flatMap(r => hostile.map(([label, s]) => [r.source.slice(0, 50), label, Math.round(ms(() => r.test(s)))] as const)).filter(([, , t]) => t >= 100); expect(slow).toEqual([]); }, 120_000); @@ -57,13 +57,13 @@ describe('the rm rule matches what R51 matched', () => { expect(lines.filter(s => r51.test(s)).length).toBeGreaterThan(1000); expect(edges.filter(s => rm.test(s))).toEqual(['rm -rf /', 'rm -fr x', 'rm -rvf x', 'rm -rf-x', 'rm -rrf', 'rm -ffr', 'rm -rfr', 'rm\t\t-rf', 'sudo rm -rf "$DIR"']); }); - test('the rmSync rule matches what R51 matched on 60,000 seeded short lines, and padding past its window still raises security', () => { + test('the rmSync rule matches every line R51 matched on 60,000 seeded short lines, and padding past its window still raises security', () => { x = 898; const r51Sync = /\brmSync\s*\([^)]*recursive\s*:\s*true/; const rmSync = SECURITY_LINE.find(r => r.source.startsWith(String.raw`\brmSync`))!; const parts = ['rmSync(', 'rmSync (', 'xrmSync(', 'recursive', ':', ' ', 'true', ')', 'a', 'recursive: true', 'rmSync', '(', '\t']; const lines = Array.from({ length: 60_000 }, () => Array.from({ length: next(9) + 1 }, () => parts[next(parts.length)]).join('')); - expect(lines.filter(s => rmSync.test(s) !== r51Sync.test(s))).toEqual([]); + expect(lines.filter(s => r51Sync.test(s) && !rmSync.test(s))).toEqual([]); expect(lines.filter(s => r51Sync.test(s)).length).toBeGreaterThan(1000); expect(security('fs.rmSync(dir, { recursive: true, force: true });')).toBe(true); expect(security('fs.rmSync(' + 'a'.repeat(480) + ', { recursive: true })')).toBe(true); @@ -71,6 +71,25 @@ describe('the rm rule matches what R51 matched', () => { expect(security('fs.rmSync(dir, /*' + 'x'.repeat(5_000_000) + '*/ { recursive: true });')).toBe(true); expect(security('fs.rmSync(dir, { force: true });')).toBe(false); }); + test('a recursive rm or rmSync whose first argument is a call, or whose arguments hold a ), raises security (GST-942)', () => { + // Value: protects=a recursive delete written the common way keeps the security class; fails_when=the rule stops at the first + // ) inside the first argument or a comment, or skips the async fs.rm; seam=matchRisk on one added line + const recursive = ['fs.rmSync(path.join(dir, "x"), { recursive: true, force: true });', 'fs.rmSync(dir, /* ) */ { recursive: true });', + 'fs.rmSync(path.resolve(root, name), { recursive: true });', 'await fs.promises.rm(path.join(dir, "x"), { recursive: true });', + 'await fs.rm(dir, { recursive: true })', "await rm(dir, { recursive: true, force: true }); // import { rm } from 'node:fs/promises'", + 'fs.rm(' + 'a'.repeat(600) + ', { force: true })']; + expect(recursive.filter(l => !security(l))).toEqual([]); + expect(['fs.rm(path.join(dir, "x"), { force: true });', 'farm(x, { recursive: true });', 'confirm(x, { recursive: true });'].filter(security)).toEqual([]); + }); + test('a recursive delete a formatter split across lines raises security, read over the joined lines (GST-942 review)', () => { + // Value: protects=a Prettier-formatted rmSync or fs.promises.rm keeps the security class; fails_when=the delete rules read + // only one line at a time; seam=matchRisk over a file's added lines + const split = (lines: string[]) => matchRisk({ codePaths: ['src/clean.ts'], added: { 'src/clean.ts': lines }, removed: {}, scopeAuth: false }).has('security'); + expect(split([' fs.rmSync(path.join(root, "build-output-cache"), {', ' recursive: true,', ' force: true,', ' });'])).toBe(true); + expect(split([' await fs.promises.rm(', ' path.join(root, "cache"),', ' { recursive: true },', ' );'])).toBe(true); + expect(split([' fs.rmSync(', ' dir,', ' /*' + 'x'.repeat(510) + '*/', ' { recursive: true },', ' );'])).toBe(true); + expect(split([' fs.rmSync(path.join(root, "file"), {', ' force: true,', ' });'])).toBe(false); + }); }); describe('svgDrawing in linear time', () => {