diff --git a/changelog.d/gst-942-rm-recursive.md b/changelog.d/gst-942-rm-recursive.md index 7ef600d51..401c1bbc1 100644 --- a/changelog.d/gst-942-rm-recursive.md +++ b/changelog.d/gst-942-rm-recursive.md @@ -1,3 +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`, `fs.rm`/`fs.promises.rm`/an imported `rm`, and a 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. +- **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`, `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. diff --git a/docs/review-policy.md b/docs/review-policy.md index 878fbac7f..0d7a91ade 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(` 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 the call across lines, which is read over the file's changed lines joined (GST-942); 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, +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 7b6d18ce1..1f2132522 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -59,7 +59,7 @@ export const SECURITY_LINE = [ /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/, ]; /** A recursive delete split across lines (a formatter puts `recursive: true` on its own line), read over a file's changed lines joined. */ -export const SECURITY_JOINED = [/\brm(?:Sync)?\s*\([\s\S]{0,500}?recursive\s*:\s*true/]; +export const SECURITY_JOINED = [/\brm(?:Sync)?\s*\((?:[\s\S]{0,500}?recursive\s*:\s*true|[^)]{501})/]; export const SVG_PATH = /\.svg$/i; /** 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]; diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 5e5eb6927..20c72893a 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -84,7 +84,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: '166dd7b1148a272b345dd9d2ceccd14348cc59c52ef9c88109436f6f61cf6b69', replay: 'GST-911-redos' }, - 'lib/review-risk.ts': { sha: 'd8845fdb7b4cffe73d400b367c1fdf7df03ca155f2e579ed5c9833d4e9d09094', replay: 'GST-942' }, + 'lib/review-risk.ts': { sha: '08464b78a723ee463ca3a28d0abbc4eb8f4f512192061c703743707b66b5a311', 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 df9562d5a..4b9982fcb 100644 --- a/test/review-risk-redos.test.ts +++ b/test/review-risk-redos.test.ts @@ -87,6 +87,7 @@ describe('the rm rule matches what R51 matched', () => { 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); }); });