diff --git a/docs/review-policy.md b/docs/review-policy.md index e4381290e..3b5fbd8a1 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -208,16 +208,16 @@ beside a cost or charge word, so a CLI's usage text and a plain proxy stay out (GST-850, from gbrain-evals #289, whose metering proxy and billing clients read `required=none`). **security** is a trust-boundary path (trust, redact, sanitize, neutralize, consent, escape) or line (`shell: true`, `exec(` and `execSync(`, `eval`, `new Function(`, -raw HTML sinks, a shell run through argv, recursive deletes), outbound URL -handling (`fetch`, `http(s).request`, axios, got, undici, `requests` or `urlopen` -on anything but one string literal, `new URL` from input, redirect toggles, proxy -settings and host allowlists), archive extraction and path traversal (`tar.x`, -adm-zip, yauzl, extract-zip, `extractall`, `tar -x`, `unzip`, `path.join` or -`resolve` on request or entry input, a `'..'` check added or removed), and Node -exec through any `child_process` spelling: a named import of `exec`, `execSync` -or `spawn` (on one line, split over lines when the whole statement is in the -diff, or an added `exec` binding line inside one), a default or namespace -import, a whole-module or destructured `require` or dynamic `import`. **data** is user data: a path naming pii, gdpr, ccpa, privacy, +raw HTML sinks, a shell run through argv, recursive deletes), any call to an +outbound HTTP client (`fetch`, `http(s).get` or `.request`, axios, got, undici, +`requests`, `httpx`, urllib, with no exemption for a literal URL), `new URL` from +input, redirect toggles, proxy settings and host allowlists, archive extraction +and path traversal (`tar.x`, adm-zip, yauzl, extract-zip, `extractall`, `tar -x`, +`unzip`, `path.join` or `resolve` on request or entry input, a `'..'` check added +or removed), and any changed line that names the `child_process` or +`node:child_process` module, in any import or require spelling. These rules are +coarse on purpose: they parse no import clause or call arguments, and every +quantifier is bounded, so they run in linear time on any line. **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 @@ -230,11 +230,6 @@ version, the same pattern as three rounds of R51. When a diff publishes numbers, the batched specialist's diff keeps the write-up's text files instead of leaving docs out. -A write-up with one clean chart and no code is one reader (the outside voice on -its numbers). When a diff publishes numbers, the batched specialist's diff keeps -the write-up's text files instead of leaving docs out. `ALLOWLIST_RULES` hashes -the lists and patterns and `DEPTH_RULES` includes it. - **Batched checklists (the lean ensemble, GSTA-167 part 2).** One specialist subagent reads Testing and Security on every code or gate diff, plus State-machines, Data-migration, API-contract and Design when scope asks (a diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 514cb5d25..e4f22b195 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -5,7 +5,7 @@ * * 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; outbound URL handling; archive - * extraction and path traversal; Node exec through any import spelling); data is user data (a path naming personal or + * extraction and path traversal; any line naming the child_process module); 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, and a CLI's usage text never names money. */ @@ -40,50 +40,29 @@ export const SECRET_LINE = [ ]; const SECURITY_PATH_TOKENS = ['trust', 'redact', 'redaction', 'redactor', 'sanitize', 'sanitizer', 'neutralize', 'neutralizer', 'consent', 'escape']; -/** An argument that is one plain string literal: outbound calls and URL building on anything else are input-driven. */ -const NOT_LITERAL = String.raw`\s*\(\s*(?![rbu]?['"][^'"\\]*['"]\s*[,)])`; -const CHILD_PROCESS = String.raw`['"](?:node:)?child_process['"]`; -const SECURITY_LINE = [ +/** Coarse, linear-time rules (GST-850 B+): any changed line naming the child_process module, any outbound HTTP client call; quantifiers bounded. */ +export const SECURITY_LINE = [ /\bshell\s*:\s*true\b/, /\bshell\s*=\s*True\b/, /(?]))/, /\b(?:allowed|trusted|blocked|denied)[_-]?(?:hosts?|domains?|origins?|urls?)|\b(?:hosts?|domains?|origins?|urls?)[_-]?(?:allow|deny|block)[_-]?list/i, /\btar\.(?:x|extract)\s*\(|\.(?:extractall|extractAllTo|extractEntryTo)\s*\(|\b(?:tar|tf|zf|zip|archive)\.extract\s*\(|\bshutil\.unpack_archive\s*\(|\b(?:adm-zip|AdmZip|yauzl|extract-zip|unzipper|node-stream-zip)\b/, /\btar\s+(?:-[A-Za-z]*x|x[A-Za-z]*\s|--extract)|\bunzip\s+[-\w"'$]/, /\bpath\.(?:join|resolve)\s*\([^)]{0,300}?(?:\b(?:req|request|params|query|argv|headers?|filename|fileName|entryName|member|upload|untrusted|userInput)\b|\bentry\.(?:path|name|fileName)\b)/, /\b(?:includes|startsWith|endsWith|indexOf)\s*\(\s*['"`]\.\.[/\\]{0,2}['"`]|[!=]==?\s*['"`]\.\.['"`]|['"`]\.\.['"`]\s*[!=]==?|\\\.\\\./, - /^\s*(?:exec|execSync)(?:\s+as\s+[\w$]+|\s*:\s*[\w$]+)?\s*,?\s*$/, /(? t.some(x => words.includes(x)) || MONEY_PAIRS.some(([w, beside]) => t.includes(w) && t.some(x => beside.includes(x))); @@ -91,7 +70,7 @@ const namesMoney = (t: readonly string[], words: readonly string[]) => 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, SECURITY_TEXT: [IMPORT_OPEN, FROM_CHILD_PROCESS, EXEC_NAME], 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 }).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); @@ -116,9 +95,6 @@ export function matchRisk(i: RiskInput): Map { if (namesMoney(t, MONEY_PATH_TOKENS)) hit('money', 'money path: ' + p); if (t.some(x => SECURITY_PATH_TOKENS.includes(x))) hit('security', 'security path: ' + p); if (t.some(x => DATA_PATH_TOKENS.includes(x))) hit('data', 'data path: ' + p); - for (const side of [i.added?.[p], i.removed?.[p]]) { - if (!why.has('security') && side?.length && execImport(side.join('\n'))) hit('security', 'security import: ' + p); - } for (const line of [...(i.added?.[p] ?? []), ...(i.removed?.[p] ?? [])]) { if (!why.has('money') && (MONEY_LINE.some(r => r.test(line)) || namesMoney(tokens(line), MONEY_LINE_TOKENS))) hit('money', 'money line: ' + p); if (!why.has('secret') && SECRET_LINE.some(r => r.test(line))) hit('secret', 'secret line: ' + p); diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 7d29c7982..c8ab8f706 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -80,7 +80,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: '839205a409014fb2cca6988a01aa04c98c495d8d24bd94895a86bf7794627338', replay: 'GST-850' }, - 'lib/review-risk.ts': { sha: 'e0e1986620bf9962f99b9915bba9dd4d818cf997c2a41407b0ac69344e5e8663', replay: 'GST-850' }, + 'lib/review-risk.ts': { sha: '18f0330ddfda0032eb93516b0eb941fa4244e0a68b028e19c436c74609093b03', replay: 'GST-850' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-risk-linear.test.ts b/test/review-risk-linear.test.ts index c346de43a..0eb665942 100644 --- a/test/review-risk-linear.test.ts +++ b/test/review-risk-linear.test.ts @@ -1,13 +1,13 @@ /** - * GST-850: the new security and money patterns (lib/review-risk.ts) finish in linear time on hostile input, and every one of - * them is in RISK_RULES, which DEPTH_RULES folds in (docs/review-policy.md#risk-classes). + * GST-850: every risk pattern (lib/review-risk.ts) finishes in linear time on hostile input, and every new one is in + * RISK_RULES, which DEPTH_RULES folds in (docs/review-policy.md#risk-classes). */ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { matchRisk } from '../lib/review-risk'; +import { DATA_LINE, MONEY_LINE, SECRET_LINE, SECURITY_LINE, matchRisk } from '../lib/review-risk'; const ROOT = path.join(import.meta.dir, '..'); const N = 10_000; @@ -16,16 +16,19 @@ const scan = (lines: string[]) => matchRisk({ codePaths: ['src/a.ts'], added: { describe('linear time on hostile input', () => { // Value: protects=gstack-review-plan and the queue verdict never hang on a diff (the R51 resource regex did at about 50 - // backslashes; round 1 of GST-850 found a quadratic split-import scan); fails_when=a risk pattern or the split-import - // scan backtracks on long input; seam=matchRisk - test('a 512 KB comment of unclosed imports and a long path.join chain finish under 200 ms', () => { - expect(fast(() => scan(Array.from({ length: 65_536 }, () => 'import {')))).toBeLessThan(200); - expect(fast(() => scan(['path.join('.repeat(50_000)]))).toBeLessThan(200); + // backslashes; GST-850 rounds 1 and 2 found quadratic import and exec-binding scans); fails_when=any risk pattern + // backtracks on a long or minified line; seam=every pattern in SECURITY_LINE, DATA_LINE, SECRET_LINE and MONEY_LINE, and matchRisk + const L = 200_000; + const fill = (s: string) => s.repeat(Math.ceil(L / s.length)); + const hostile = [' ', '(', "'", '"', 'a', '{', '\\', '..', 'import a, {', 'fetch(', 'path.join(', 'rmSync(', ' exec', 'new URL("', 'child_process', 'http.get(', 'requests.', + 'tar -', 'unzip ', 'DELETE FROM ', 'delete from a ', 'process.env.', 'x_usd', 'aUsd'].map(fill).concat([' exec' + ' '.repeat(L) + '// c', 'import a, {' + 'b,'.repeat(L / 2)]); + test.each(Object.entries({ SECURITY_LINE, DATA_LINE, SECRET_LINE, MONEY_LINE }))('every %s pattern finishes under 100 ms on each 200k-character hostile line', (_n, patterns) => { + const slow = patterns.flatMap(r => hostile.map((s, k) => [r.source.slice(0, 40), k, fast(() => r.test(s))] as const)).filter(([, , ms]) => ms >= 100); + expect(slow).toEqual([]); }); - test('the risk patterns finish under 100 ms on 10k-character lines', () => { - for (const body of ['\\'.repeat(N), 'fetch(' + "'".repeat(N), 'import {' + ' exec'.repeat(N / 5), 'path.join(' + 'a'.repeat(N), 'import ' + 'a '.repeat(N / 2) + 'from', 'require(' + ' '.repeat(N), - ' '.repeat(N) + '} = require(', "'..".repeat(N / 3), 'x'.repeat(N) + 'Usage', 'tar ' + '-'.repeat(N), 'import {'.repeat(N / 8), 'new URL(' + '"'.repeat(N)]) - expect(fast(() => scan([body]))).toBeLessThan(100); + test('a 512 KB comment of unclosed imports and a minified import line run through matchRisk in under 1 s', () => { + expect(fast(() => scan(Array.from({ length: 65_536 }, () => 'import {')))).toBeLessThan(1000); + expect(fast(() => scan(['import a, {'.repeat(32_000)]))).toBeLessThan(1000); }); }); diff --git a/test/review-risk-routing.test.ts b/test/review-risk-routing.test.ts index 06175dcf5..66f3d8fa9 100644 --- a/test/review-risk-routing.test.ts +++ b/test/review-risk-routing.test.ts @@ -253,14 +253,14 @@ describe('GST-850: security reaches outbound URLs, archives, path traversal and "import { exec } from 'node:child_process';", 'import { execSync as run } from "child_process";', "import { spawn, spawnSync } from 'node:child_process';", "import cp from 'node:child_process';", "import * as cp from 'child_process';", "import cp, { spawnSync } from 'child_process';", "const { exec } = require('child_process');", "const cp = require('node:child_process');", "const cp = await import('node:child_process');", - 'exec(cmd, cb);', 'cp.exec(cmd);', - 'const res = await fetch(url);', "await fetch('https://' + host + '/v1');", 'https.get(target, cb);', 'await axios.get(req.query.u);', 'const body = await got(url).text();', + 'exec(cmd, cb);', 'cp.exec(cmd);', "import { spawnSync } from 'node:child_process';", "import type { ExecOptions } from 'node:child_process';", + "const r = await fetch('https://api.github.com/repos');", 'r = requests.request("GET", user_url)', "http.get('http://x.example/', { hostname: req.query.t }, cb);", 'const res = await fetch(url);', "await fetch('https://' + host + '/v1');", 'https.get(target, cb);', 'await axios.get(req.query.u);', 'const body = await got(url).text();', 'r = requests.get(url, timeout=5)', 'urllib.request.urlopen(target)', 'const next = new URL(req.query.next, base);', "import { request } from 'undici';", "const res = await fetch('https://x.example', { redirect: 'follow' });", 'maxRedirects: 10,', 'r = httpx.get("https://x.example", follow_redirects=True)', 'const p = process.env.HTTPS_PROXY;', 'const agent = new HttpsProxyAgent(p);', "const ALLOWED_HOSTS = ['api.example.com'];", 'hostAllowlist.push(h);', 'await tar.x({ file, cwd: dest });', 'new AdmZip(buf).extractAllTo(dir, true);', "import yauzl from 'yauzl';", "import extract from 'extract-zip';", 'zipfile.ZipFile(p).extractall(d)', 'tarfile.open(p).extractall(d)', 'tar -xzf "$TGZ" -C "$DIR"', 'unzip -o "$ZIP" -d "$DIR"', 'fs.writeFileSync(path.join(root, entry.path), data);', 'const dest = path.resolve(out, req.params.name);', - "if (rel.startsWith('..')) throw new Error('escape');", "if (parts.includes('..')) return;", ' exec as run,', ' exec: run,', + "if (rel.startsWith('..')) throw new Error('escape');", "if (parts.includes('..')) return;", ])('%s is security', line => { expect(risk(['src/net.ts'], { 'src/net.ts': [line] })).toEqual(['security']); }); @@ -268,11 +268,10 @@ describe('GST-850: security reaches outbound URLs, archives, path traversal and const split = ['import {', ' spawnSync,', ' exec,', "} from 'node:child_process';"]; expect(risk(['src/run.ts'], { 'src/run.ts': split })).toEqual(['security']); expect([...matchRisk({ codePaths: ['src/run.ts'], added: {}, removed: { 'src/run.ts': split }, scopeAuth: false }).keys()]).toEqual(['security']); - expect(risk(['src/run.ts'], { 'src/run.ts': ['import {', ' spawnSync,', "} from 'node:child_process';"] })).toEqual([]); + expect(risk(['src/run.ts'], { 'src/run.ts': ['import {', ' type ExecOptions,', ' exec as run,', "} from 'node:child_process';"] })).toEqual(['security']); }); - test('literal URLs, spawnSync imports, tarball creation and parent-dir joins are not security', () => { - for (const line of ["const r = await fetch('https://api.github.com/repos');", "const u = new URL('https://example.com');", 'const here = new URL(import.meta.url);', - "import { spawnSync } from 'node:child_process';", "import type { ExecOptions } from 'node:child_process';", 'tar -czf out.tgz dir', "const root = path.join(import.meta.dir, '..');", + test('a literal new URL, tarball creation, parent-dir joins and look-alike names are not security', () => { + for (const line of ["const u = new URL('https://example.com');", 'const here = new URL(import.meta.url);', 'tar -czf out.tgz dir', "const root = path.join(import.meta.dir, '..');", 'const m = re.exec(line);', "ENV.fetch('HOME')", 'git fetch origin main']) expect(risk(['src/run.ts'], { 'src/run.ts': [line] })).toEqual([]); });