From 3f6708e487cd96af28a9588e658dcc8e8d707a45 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 12:21:51 +0000 Subject: [PATCH] fix(review-risk): a recursive rm or rmSync keeps the security class whatever its arguments hold (GST-942) The rmSync rule stopped at the first ), so path.join(...) as the first argument or a ) in a comment hid recursive: true, and async fs.rm had no rule. Both now read up to 500 characters of the line, in linear time. Repin lib/review-risk.ts under no-loss replay GST-942 (dispatch superset: 170 of 170 old matches kept, 19 added, no tier change over 100 main commits). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-942-rm-recursive.md | 3 +++ docs/review-policy.md | 2 +- lib/review-risk.ts | 2 +- test/review-packets-removed.test.ts | 3 ++- test/review-risk-redos.test.ts | 15 ++++++++++++--- 5 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 changelog.d/gst-942-rm-recursive.md diff --git a/changelog.d/gst-942-rm-recursive.md b/changelog.d/gst-942-rm-recursive.md new file mode 100644 index 000000000..f6227e042 --- /dev/null +++ b/changelog.d/gst-942-rm-recursive.md @@ -0,0 +1,3 @@ +### Fixed + +- **A recursive delete keeps the security class however its arguments are written (GST-942).** `fs.rmSync(path.join(dir, "x"), { recursive: true })`, a `)` inside a comment before `recursive: true`, and the async `fs.rm`/`fs.promises.rm` all slipped past the rule, because it stopped reading at the first `)`. The review depth could then drop to light on real recursive-delete code. The rule now reads up to 500 characters of the line after `rmSync(` or `.rm(`, still in linear time. diff --git a/docs/review-policy.md b/docs/review-policy.md index db10c3753..6aba673b9 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -228,7 +228,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 counts as security when `recursive: true` follows within 500 characters on the same line, whatever the arguments hold (a `path.join(...)` first argument or a `)` in a comment included, GST-942), and on its own when more than 500 characters come before the first `)` after it. **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..b9a98d68c 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -55,7 +55,7 @@ export const SECURITY_LINE = [ /\b(?:includes|startsWith|endsWith|indexOf)\s*\(\s*['"`]\.\.[/\\]{0,2}['"`]|[!=]==?\s*['"`]\.\.['"`]|['"`]\.\.['"`]\s*[!=]==?|\\\.\\\./, /(? = { 'review/specialists/targeted.md': { sha: '7f92a0d0a8319c3c0e076fb3b1a4689c45120b2be2c14dfc7d4cb03aafa9d6a5', replay: 'GST-257' }, 'scripts/resolvers/fix-check.ts': { sha: '712bf5b0c439e96bac32f19cbe25394dabb4c94aa66600100bfde9b100e15a08', replay: '#3638' }, 'lib/review-depth.ts': { sha: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' }, - 'lib/review-risk.ts': { sha: '126eff0f1de6982e03dc24d8017c67e6807c91dfa518b18d2db3aa910aac911f', replay: 'GST-911' }, + 'lib/review-risk.ts': { sha: '262ae8a14cda820c3ea0e508045229c2e36993c0fdfd3a4d51a8834a915de94f', replay: 'GST-942' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-risk-redos.test.ts b/test/review-risk-redos.test.ts index 7d40d9df3..df759c50e 100644 --- a/test/review-risk-redos.test.ts +++ b/test/review-risk-redos.test.ts @@ -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('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', ' '), @@ -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,15 @@ 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 })']; + expect(recursive.filter(l => !security(l))).toEqual([]); + expect(['fs.rm(path.join(dir, "x"), { force: true });', 'const rm = (a) => a; rm(x, { recursive: true });', 'farm(x, { recursive: true });'].filter(security)).toEqual([]); + }); }); describe('svgDrawing in linear time', () => {