open/voyage
1
0
Fork 0
Code Issues Releases 5 Activity

fix(verification): the criteria runner screens with an ALLOWLIST, not a denylist

Browse source 
A denylist in front of /bin/sh is whack-a-mole. Measured 2026-09-18, end to
end through both screens: 5 of 11 named evasions ran with real effect - a
`command` prefix reached git, an escaped `rm` inside a shell fence deleted a
directory, `find -delete` deleted a file, `>|` and `tee` wrote outside the
working tree, a python one-liner deleted the whole tree - and 19 of 28 got
past the refusal list on its own. Every quoting, aliasing and indirection form
of the shell is another mole.

So the screen is now an ALLOWLIST. A criterion runs only when its first word
is a known test runner (npm test, npm run <script package.json declares>,
node --test, vitest, jest, pytest, python -m pytest, uv run pytest,
cargo test, go test, make test, bash <script under tests/>, a read-only git
subcommand) AND the command carries no shell operator and no newline.
Everything else is NOT RUN with the reason said out loud: never run, and never
reported as a failure either - an absent measurement is not a finding. That
also closes the smaller hole in the same file: a bare word a sentence merely
names (`whoami`, `login`, `package.json`) is no longer executed, because it is
not a runner.

REFUSED_BY_POLICY is gone with the list that produced it; a command outside
the allowlist is `unrunnable`, which in plan mode still fells the run and in
brief mode is reported to the reviewer as an absent measurement.

What the allowlist deliberately does NOT do, said in the file and in the
reviewer's rubric: it is not a sandbox. `npm test`, `npm run <script>` and
`make test` run whatever the repo's own package.json/Makefile says they run,
including a script that pushes - that is the repo's responsibility. And it
rejects honest commands too: an env prefix, a project's own binary, anything
piped. A check that needs one of those is declared through
`bash tests/<script>.sh`, the documented way in.

Red first: 6 of the new tests fail against the previous runner (measured with
an always-allow shim so the module still loads), including the end-to-end one
where the canary directory was deleted and files were written outside the
tree. The fixtures move from `true`/`false` to two allowlisted shell fixtures,
because `false` is no longer a runner - the fail case must still be a real
non-zero exit, not an unrun criterion.

Suite 1148 (1146/0/2).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-09-18 02:51:40 +02:00
commit 02243c6365
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
12 changed files with 350 additions and 255 deletions
Download patch file Download diff file Expand all files Collapse all files

214
lib/verification/criteria-runner.mjs
View file

@ -17,10 +17,11 @@
// command, a screen that is unavailable — is NEVER reported as passed.
//
// Screening, two layers: every command is first screened against this file's
// own refusal list (writes — git push, recursive delete, pipe-to-shell,
// redirection outside the working tree), then through the plugin's own
// PreToolUse denylist (hooks/scripts/pre-bash-executor.mjs) — catastrophe —
// before it reaches a shell.
// own ALLOWLIST of test runners (npm test, node --test, pytest, `bash
// tests/<script>.sh`, a read-only git subcommand, …) — anything else is NOT
// RUN, never a failure — and what survives is then screened through the
// plugin's own PreToolUse denylist (hooks/scripts/pre-bash-executor.mjs) —
// catastrophe — before it reaches a shell.
// Commands spawned from here do not pass through the Bash tool, so that hook
// would not otherwise fire; the hook is invoked over its documented stdin
// protocol rather than copied, so there is exactly one denylist.
@ -30,12 +31,11 @@
// node lib/verification/criteria-runner.mjs --brief <brief.md> [--json|--evidence] [--cwd D] [--timeout MS]
// --evidence emits the markdown block /trekreview hands to the conformance
// reviewer; --json emits the whole report. They are two shapes of one run.
// Exit: 0 = ok, 1 = a criterion failed / was refused / blocked / could not run, 2 = usage
// Exit: 0 = ok, 1 = a criterion failed / was blocked / could not run, 2 = usage
// or read error (deliberately distinct from "a criterion failed").
import { readFileSync } from 'node:fs';
import { resolve, dirname, join, sep } from 'node:path';
import { homedir } from 'node:os';
import { resolve, dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import { spawnSync } from 'node:child_process';
@ -213,112 +213,105 @@ export function screenCommand(command, opts = {}) {
};
}
// --- refusing ---------------------------------------------------------------
// --- the allowlist ----------------------------------------------------------
// The executor denylist screens CATASTROPHE (rm -rf /, fork bombs, mkfs). It
// was never meant to screen an artifact that arrives from outside the repo,
// and /trekreview now runs the commands a BRIEF declares. Measured 2026-09-18:
// `git push origin main` and `rm -rf <path>` both ran; only the pipe-to-shell
// was stopped. This list screens WRITES, and it is pinned by
// tests/lib/criteria-runner.test.mjs so it cannot quietly shrink.
// and /trekreview runs the commands a BRIEF declares. The first screen written
// for that was a denylist of writes; measured 2026-09-18, end to end through
// both layers, 5 of 11 named evasions still ran with real effect — `command`
// in front of the command name, an escaped `\rm` inside a shell fence,
// `find -delete`, `>|`, `tee`, a python one-liner — and 19 of 28 got past the
// refusal list on its own. A denylist in front of a shell is whack-a-mole:
// every quoting, aliasing and indirection form of /bin/sh is another mole.
//
// A refusal is its own outcome, REFUSED_BY_POLICY: the command never reaches a
// shell, and a refused criterion is never ok in either mode.
// So the screen is an ALLOWLIST. A criterion runs only when the command is a
// known TEST RUNNER invocation and carries no shell operator at all. Anything
// else is NOT RUN, with the reason said out loud: it never reaches a shell,
// and — this is the other half — it is never reported as a failure either. An
// absent measurement is not a finding.
//
// What the allowlist deliberately does NOT do:
// - It is not a sandbox. `npm test`, `npm run <script>` and `make test` run
// whatever the repo's own package.json/Makefile says they run, including a
// script that pushes. That is the repo's responsibility, not this file's.
// - It has no opinion about arguments beyond the shape rules below.
// - It rejects plenty of honest commands: an env prefix (`CI=1 npm test`), a
// project's own binary (`./build/app --check`), anything piped. A check
// that needs one of those is declared through a runner instead —
// `bash tests/<script>.sh` is the documented way in.
// Segments of a command that each start at command position.
const SEPARATORS = /(?:\|\||&&|[;|&\n])/;
const ENV_PREFIX = /^[A-Za-z_][A-Za-z0-9_]*=\S*$/;
// Anything that would let one command become two, or redirect a stream.
const SHELL_OPERATORS = /[;&|<>]|\$\(|`/;
// git's own options before the subcommand; these take a separate value.
const GIT_VALUE_OPTS = new Set(['-C', '-c', '--git-dir', '--work-tree', '--namespace', '--exec-path']);
// git subcommands that write somewhere this process does not own.
const WRITING_GIT_SUBCOMMANDS = new Set(['push']);
// Read-only git subcommands: they report, they do not write a repository.
const READONLY_GIT = new Set(['status', 'log', 'diff', 'show', 'ls-files']);
const RECURSIVE_FLAG = /^(?:-[A-Za-z]*r[A-Za-z]*|--recursive)$/;
const PIPE_TO_SHELL =
/\b(?:curl|wget|fetch)\b[^|]*\|\s*(?:sudo\s+)?(?:[\w./-]*\/)?(?:ba|z|k|da|a)?sh\b/;
const deny = (reason) => ({ allowed: false, reason });
const permit = () => ({ allowed: true, reason: '' });
// `>` / `>>` and their target. `2>&1` and `>&2` duplicate a descriptor rather
// than open a file, so `&` is excluded.
const REDIRECT = /(?:^|[^>&\w])\d?>>?\s*(?!&)(?:"([^"]*)"|'([^']*)'|([^\s;|&<>]+))/g;
const WRITABLE_DEVICES = new Set(['/dev/null', '/dev/stdout', '/dev/stderr', '/dev/tty']);
const basenameOf = (token) => token.split('/').pop();
function segments(command) {
return String(command).split(SEPARATORS).map((seg) => seg.trim()).filter(Boolean);
// `bash <path>` is the way a repo declares a check the allowlist has no name
// for. The path must live under tests/, be relative, and not climb out of it.
function isTestScript(path) {
if (path === undefined) return false;
if (!path.startsWith('tests/')) return false;
return !path.split('/').includes('..');
}
function tokensOf(segment) {
const t = segment.split(/\s+/).filter(Boolean);
while (t.length > 0 && ENV_PREFIX.test(t[0])) t.shift();
return t;
}
function gitSubcommand(tokens) {
let i = 1;
while (i < tokens.length) {
const t = tokens[i];
if (!t.startsWith('-')) return t;
if (GIT_VALUE_OPTS.has(t)) { i += 2; continue; }
i += 1;
function npmScripts(cwd) {
try {
const pkg = JSON.parse(readFileSync(join(cwd, 'package.json'), 'utf8'));
return new Set(Object.keys(pkg.scripts ?? {}));
} catch {
return new Set();
}
return null;
}
function redirectTargets(command) {
const out = [];
for (const m of String(command).matchAll(REDIRECT)) out.push(m[1] ?? m[2] ?? m[3]);
return out.filter(Boolean);
}
// One entry per runner: given the command's tokens (and the working tree the
// criteria run in), either allow it or say why not.
const RUNNERS = {
npm: (t, cwd) => {
if (t[1] === 'test') return permit();
if (t[1] !== 'run') return deny(`npm, but not \`npm test\` or \`npm run\` (\`npm ${t[1] ?? ''}\`)`);
if (t[2] === undefined) return deny('npm run names no script');
return npmScripts(cwd).has(t[2])
? permit()
: deny(`npm run names a script package.json does not declare (${t[2]})`);
},
node: (t) => (t[1] === '--test' ? permit() : deny('node, but not `node --test` — an arbitrary script is not a test runner')),
vitest: () => permit(),
jest: () => permit(),
pytest: () => permit(),
python: (t) => (t[1] === '-m' && t[2] === 'pytest' ? permit() : deny('python, but not `python -m pytest`')),
python3: (t) => (t[1] === '-m' && t[2] === 'pytest' ? permit() : deny('python3, but not `python3 -m pytest`')),
uv: (t) => (t[1] === 'run' && t[2] === 'pytest' ? permit() : deny('uv, but not `uv run pytest`')),
cargo: (t) => (t[1] === 'test' ? permit() : deny('cargo, but not `cargo test`')),
go: (t) => (t[1] === 'test' ? permit() : deny('go, but not `go test`')),
make: (t) => (t[1] === 'test' ? permit() : deny('make, but not `make test`')),
bash: (t) => (isTestScript(t[1]) ? permit() : deny('bash, but not a script under tests/')),
git: (t) =>
READONLY_GIT.has(t[1])
? permit()
: deny(`git, but not a read-only subcommand first (${[...READONLY_GIT].join('|')})`),
};
/**
* Whether a redirect target lands inside `cwd`. A target that cannot be
* resolved statically — one carrying an unexpanded `$VAR` — is treated as
* outside: the runner cannot know where it points, and guessing is how a
* screen stops screening.
* Whether a command is one the runner may execute. Returns `{allowed, reason}`.
* `cwd` is the working tree the criteria run in — `npm run` is checked against
* the package.json found there.
*/
function writesOutside(target, cwd) {
if (WRITABLE_DEVICES.has(target)) return false;
if (target.includes('$') || target.includes('`')) return true;
const expanded = target.startsWith('~') ? join(homedir(), target.slice(1)) : target;
const abs = resolve(cwd, expanded);
return abs !== cwd && !abs.startsWith(cwd.endsWith(sep) ? cwd : cwd + sep);
}
/**
* Screen one command against the refusal list. Returns `{refused, rule}`.
* `cwd` is the working tree the criteria run in; writes outside it are refused.
*/
export function refuseCommand(command, opts = {}) {
export function allowedCommand(command, opts = {}) {
const cwd = resolve(opts.cwd ?? process.cwd());
const text = String(command ?? '');
const refuse = (rule) => ({ refused: true, rule });
if (text.trim() === '') return deny('the command is empty');
if (/\n/.test(text)) return deny('a newline: one criterion declares one command');
const operator = text.match(SHELL_OPERATORS);
if (operator) return deny(`a shell operator (${operator[0]}) — one criterion declares one command, unpiped`);
if (PIPE_TO_SHELL.test(text)) return refuse('a download piped straight into a shell');
for (const segment of segments(text)) {
const tokens = tokensOf(segment);
if (tokens.length === 0) continue;
const name = basenameOf(tokens[0]);
if (name === 'git') {
const sub = gitSubcommand(tokens);
if (sub && WRITING_GIT_SUBCOMMANDS.has(sub)) {
return refuse(`a remote-writing git subcommand (git ${sub})`);
}
}
if (name === 'rm' && tokens.slice(1).some((t) => RECURSIVE_FLAG.test(t))) {
return refuse('a recursive delete (rm -r)');
}
}
for (const target of redirectTargets(text)) {
if (writesOutside(target, cwd)) {
return refuse(`a write outside the working tree (${target})`);
}
}
return { refused: false, rule: '' };
const tokens = text.trim().split(/\s+/);
const runner = RUNNERS[tokens[0]];
if (!runner) return deny(`\`${tokens[0]}\` is not a known test runner`);
return runner(tokens, cwd);
}
// --- running ----------------------------------------------------------------
@ -337,14 +330,14 @@ function cap(text, max) {
* Run each criterion that has a command. Returns one result per criterion:
* passed the command exited 0
* failed the command exited non-zero (or was killed by the timeout)
* refused the refusal list denied it — it never reached a shell
* blocked the executor denylist denied it — it never reached a shell
* unrunnable there is no command to run (placeholder or prose-only criterion)
* unrunnable there is nothing to run: no command (placeholder or prose-only
* criterion), or a command outside the allowlist
*/
export function runCriteria(criteria, opts = {}) {
const exec = opts.exec ?? defaultExec;
const screen = opts.screen ?? screenCommand;
const refuse = opts.refuse ?? refuseCommand;
const allow = opts.allow ?? allowedCommand;
const cwd = opts.cwd ?? process.cwd();
const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS;
const maxOutput = opts.maxOutput ?? DEFAULT_MAX_OUTPUT;
@ -355,9 +348,14 @@ export function runCriteria(criteria, opts = {}) {
if (!c.command) {
return { ...base, status: 'unrunnable', exitCode: null, output: `not runnable: ${c.reason || 'no command'}` };
}
const refusal = refuse(c.command, { cwd });
if (refusal.refused) {
return { ...base, status: 'refused', exitCode: null, output: `REFUSED_BY_POLICY: ${refusal.rule}` };
const permission = allow(c.command, { cwd });
if (!permission.allowed) {
return {
...base,
status: 'unrunnable',
exitCode: null,
output: `not runnable: outside the allowlist (${permission.reason})`,
};
}
const verdict = screen(c.command, { hookPath });
if (!verdict.allowed) {
@ -388,13 +386,11 @@ export function summarize(results, opts = {}) {
passed: count('passed'),
failed: count('failed'),
blocked: count('blocked'),
refused: count('refused'),
unrunnable: count('unrunnable'),
};
summary.ok =
summary.failed === 0 &&
summary.blocked === 0 &&
summary.refused === 0 &&
(!requireCommand || (summary.unrunnable === 0 && summary.total > 0));
return summary;
}
@ -455,7 +451,7 @@ export function runSuccessCriteriaChecks(briefPath, opts = {}) {
// --- rendering --------------------------------------------------------------
const MARK = { passed: 'PASS', failed: 'FAILED', blocked: 'BLOCKED', refused: 'REFUSED', unrunnable: 'NOT RUN' };
const MARK = { passed: 'PASS', failed: 'FAILED', blocked: 'BLOCKED', unrunnable: 'NOT RUN' };
export function render(rep) {
const out = [];
@ -463,7 +459,7 @@ export function render(rep) {
out.push(`criteria-runner: ${s.ok ? 'OK' : 'NOT OK'} — ${rep.source} ${rep.heading}`);
if (rep.error) out.push(` error: ${rep.error.code} — ${rep.error.message}`);
out.push(
` ${s.passed} passed · ${s.failed} failed · ${s.blocked} blocked · ${s.refused} refused · ${s.unrunnable} not run (of ${s.total})`
` ${s.passed} passed · ${s.failed} failed · ${s.blocked} blocked · ${s.unrunnable} not run (of ${s.total})`
);
for (const r of rep.results) {
const code = r.exitCode === null ? '' : ` (exit ${r.exitCode})`;
@ -505,17 +501,17 @@ export function formatCriteriaEvidence(rep) {
const s = rep.summary;
out.push('');
out.push(
`${s.passed} passed · ${s.failed} failed · ${s.blocked} blocked · ${s.refused} refused · ${s.unrunnable} not run (of ${s.total}).`
`${s.passed} passed · ${s.failed} failed · ${s.blocked} blocked · ${s.unrunnable} not run (of ${s.total}).`
);
}
out.push('');
out.push(
'**NOT RUN is not a pass.** You cannot run these commands yourself — never infer, assume or ' +
'reconstruct a result for a criterion that has none. A criterion whose result is FAILED or ' +
'BLOCKED is `Broken`; a criterion marked REFUSED was refused by policy before it reached a ' +
'shell, so like NOT RUN it is the ABSENCE of a measurement and never on its own a finding; ' +
'a criterion with no result is judged on delivered code alone, and you say so in the ' +
'Evidence column.'
'BLOCKED is `Broken`; a criterion marked NOT RUN declared no command, or a command outside ' +
'the allowlist of test runners, so it never reached a shell — that is the ABSENCE of a ' +
'measurement and never on its own a finding; a criterion with no result is judged on ' +
'delivered code alone, and you say so in the Evidence column.'
);
return out.join('\n');
}