fix(hooks): the write guard covers Edit and NotebookEdit and fails closed
hooks.json wired pre-write-executor.mjs to the Write tool only, so Edit and NotebookEdit reached .git/hooks/, .env, shell configs and the other protected paths unguarded. The matcher is now Write|Edit|NotebookEdit. The guard reads the target path per tool: file_path for Write and Edit (hooks reference, PreToolUse tool_input tables), notebook_path for NotebookEdit (the tool's input schema). When it cannot read a path (unparseable input, unknown tool, missing field) it now blocks instead of allowing. New tests resolve hooks.json with Claude Code's matcher semantics and run the hooks that would fire per tool: 9 failed on the old code, all pass now. The two fail-open pins in path-guard.test.mjs are flipped to fail closed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
02f320fbe4
commit
2c89a6b580
6 changed files with 201 additions and 15 deletions
|
|
@ -347,7 +347,7 @@ Use `--fg` to run every step sequentially in the current session instead, even w
|
||||||
|
|
||||||
The executor implements defense-in-depth security across three layers:
|
The executor implements defense-in-depth security across three layers:
|
||||||
|
|
||||||
1. **Plugin hooks** — `pre-bash-executor.mjs` blocks 13 categories of destructive commands (rm -rf /, chmod 777, pipe-to-shell, eval injection, disk wipe, shutdown, fork bombs, cron persistence, process killing, history destruction) with bash evasion normalization. `pre-write-executor.mjs` blocks writes to `.git/hooks/`, `.claude/settings.json`, shell configs, `.ssh/`, `.aws/`, and `.env` files
|
1. **Plugin hooks** — `pre-bash-executor.mjs` blocks 13 categories of destructive commands (rm -rf /, chmod 777, pipe-to-shell, eval injection, disk wipe, shutdown, fork bombs, cron persistence, process killing, history destruction) with bash evasion normalization. `pre-write-executor.mjs` blocks writes (Write, Edit, NotebookEdit; fails closed without a path) to `.git/hooks/`, `.claude/settings.json`, shell configs, `.ssh/`, `.aws/`, and `.env` files
|
||||||
2. **Prompt-level denylist** — Security rules embedded in the executor command and session spec template that hold even if a hook does not fire
|
2. **Prompt-level denylist** — Security rules embedded in the executor command and session spec template that hold even if a hook does not fire
|
||||||
3. **Pre-execution plan scan** — Phase 2.4 scans all `Verify:` and `Checkpoint:` commands against the denylist before execution begins, catching dangerous commands before they reach the executor
|
3. **Pre-execution plan scan** — Phase 2.4 scans all `Verify:` and `Checkpoint:` commands against the denylist before execution begins, catching dangerous commands before they reach the executor
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -32,7 +32,7 @@ The plugin's security posture covers:
|
||||||
| Hook | Trigger | Purpose |
|
| Hook | Trigger | Purpose |
|
||||||
|------|---------|---------|
|
|------|---------|---------|
|
||||||
| `pre-bash-executor.mjs` | `PreToolUse` for Bash | BLOCKs known-dangerous shell patterns; WARNs on suspicious ones; fails open on parse errors |
|
| `pre-bash-executor.mjs` | `PreToolUse` for Bash | BLOCKs known-dangerous shell patterns; WARNs on suspicious ones; fails open on parse errors |
|
||||||
| `pre-write-executor.mjs` | `PreToolUse` for Write | BLOCKs writes to `.git/hooks/`, `~/.ssh/`, `.env`, and other sensitive paths |
|
| `pre-write-executor.mjs` | `PreToolUse` for Write, Edit, NotebookEdit | BLOCKs writes to `.git/hooks/`, `~/.ssh/`, `.env`, and other sensitive paths; fails closed when it cannot read the target path |
|
||||||
| `pre-compact-flush.mjs` | `PreCompact` | Flushes `progress.json` from git history before compaction (P0 drift fix); read-only beyond `progress.json` |
|
| `pre-compact-flush.mjs` | `PreCompact` | Flushes `progress.json` from git history before compaction (P0 drift fix); read-only beyond `progress.json` |
|
||||||
| `session-title.mjs` *(planned, F9)* | `UserPromptSubmit` | Sets session title `voyage:<command>:<slug>` for telling parallel sessions apart |
|
| `session-title.mjs` *(planned, F9)* | `UserPromptSubmit` | Sets session title `voyage:<command>:<slug>` for telling parallel sessions apart |
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -12,7 +12,7 @@
|
||||||
]
|
]
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
"matcher": "Write",
|
"matcher": "Write|Edit|NotebookEdit",
|
||||||
"hooks": [
|
"hooks": [
|
||||||
{
|
{
|
||||||
"type": "command",
|
"type": "command",
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,6 @@
|
||||||
#!/usr/bin/env node
|
#!/usr/bin/env node
|
||||||
// Hook: pre-write-executor.mjs
|
// Hook: pre-write-executor.mjs
|
||||||
// Event: PreToolUse (Write)
|
// Event: PreToolUse (Write, Edit, NotebookEdit)
|
||||||
// Purpose: Block writes to security-sensitive paths. Wired universally (every
|
// Purpose: Block writes to security-sensitive paths. Wired universally (every
|
||||||
// Voyage session, not just execution) by deliberate decision — protecting
|
// Voyage session, not just execution) by deliberate decision — protecting
|
||||||
// ~/.ssh, ~/.aws, .git/hooks, .env, shell configs benefits every session.
|
// ~/.ssh, ~/.aws, .git/hooks, .env, shell configs benefits every session.
|
||||||
|
|
@ -9,8 +9,10 @@
|
||||||
//
|
//
|
||||||
// Protocol:
|
// Protocol:
|
||||||
// - Read JSON from stdin: { tool_name, tool_input }
|
// - Read JSON from stdin: { tool_name, tool_input }
|
||||||
// - tool_input.file_path — the target path for Write tool
|
// - Target path: tool_input.file_path (Write, Edit) or
|
||||||
|
// tool_input.notebook_path (NotebookEdit)
|
||||||
// - BLOCK (exit 2): writes to security infrastructure, shell configs, secrets
|
// - BLOCK (exit 2): writes to security infrastructure, shell configs, secrets
|
||||||
|
// - BLOCK (exit 2): no readable target path (fail closed)
|
||||||
// - Allow (exit 0): everything else
|
// - Allow (exit 0): everything else
|
||||||
|
|
||||||
import { readFileSync } from 'node:fs';
|
import { readFileSync } from 'node:fs';
|
||||||
|
|
@ -94,6 +96,24 @@ const BLOCK_RULES = [
|
||||||
},
|
},
|
||||||
];
|
];
|
||||||
|
|
||||||
|
// Which tool_input field carries the target path, per tool. Write and Edit:
|
||||||
|
// hooks reference, PreToolUse tool_input tables. NotebookEdit: the tool's own
|
||||||
|
// input schema. hooks.json must wire exactly these tools.
|
||||||
|
const PATH_FIELD = {
|
||||||
|
Write: 'file_path',
|
||||||
|
Edit: 'file_path',
|
||||||
|
NotebookEdit: 'notebook_path',
|
||||||
|
};
|
||||||
|
|
||||||
|
function blockNoPath(reason) {
|
||||||
|
process.stderr.write(
|
||||||
|
`[voyage] BLOCKED: no target path (${reason})\n` +
|
||||||
|
' The write guard could not read the path this call writes to, so it ' +
|
||||||
|
'cannot be checked. Blocked (fail closed).\n'
|
||||||
|
);
|
||||||
|
process.exit(2);
|
||||||
|
}
|
||||||
|
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
// Main
|
// Main
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
@ -102,14 +122,18 @@ try {
|
||||||
const raw = readFileSync(0, 'utf-8');
|
const raw = readFileSync(0, 'utf-8');
|
||||||
input = JSON.parse(raw);
|
input = JSON.parse(raw);
|
||||||
} catch {
|
} catch {
|
||||||
// Cannot parse stdin — fail open.
|
blockNoPath('unreadable hook input');
|
||||||
process.exit(0);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
const filePath = input?.tool_input?.file_path;
|
const field = PATH_FIELD[input?.tool_name];
|
||||||
|
if (!field) {
|
||||||
|
blockNoPath(`unknown tool ${JSON.stringify(input?.tool_name)}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
const filePath = input?.tool_input?.[field];
|
||||||
|
|
||||||
if (!filePath || typeof filePath !== 'string') {
|
if (!filePath || typeof filePath !== 'string') {
|
||||||
process.exit(0);
|
blockNoPath(`${input.tool_name} without tool_input.${field}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
const resolved = resolve(filePath);
|
const resolved = resolve(filePath);
|
||||||
|
|
|
||||||
|
|
@ -164,14 +164,15 @@ test('pre-write-executor BLOCKS .env.template writes (current over-block behavio
|
||||||
});
|
});
|
||||||
|
|
||||||
// -----------------------------------------------------------------------
|
// -----------------------------------------------------------------------
|
||||||
// FAIL OPEN — malformed input must not crash the hook chain
|
// FAIL CLOSED — a write the guard cannot read a path from is blocked
|
||||||
// -----------------------------------------------------------------------
|
// -----------------------------------------------------------------------
|
||||||
test('pre-write-executor fails open on missing file_path', async () => {
|
test('pre-write-executor fails closed on missing file_path', async () => {
|
||||||
const { code } = await runHook(PRE_WRITE, { tool_name: 'Write', tool_input: {} });
|
const { code, stderr } = await runHook(PRE_WRITE, { tool_name: 'Write', tool_input: {} });
|
||||||
assert.strictEqual(code, 0, 'missing file_path should fail open (exit 0)');
|
assert.strictEqual(code, 2, 'missing file_path must fail closed (exit 2)');
|
||||||
|
assert.match(stderr, /no target path/);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('pre-write-executor fails open on malformed JSON', async () => {
|
test('pre-write-executor fails closed on malformed JSON', async () => {
|
||||||
const { code } = await runHook(PRE_WRITE, 'not-json');
|
const { code } = await runHook(PRE_WRITE, 'not-json');
|
||||||
assert.strictEqual(code, 0, 'malformed JSON should fail open (exit 0)');
|
assert.strictEqual(code, 2, 'malformed JSON must fail closed (exit 2)');
|
||||||
});
|
});
|
||||||
|
|
|
||||||
161
tests/hooks/write-guard-coverage.test.mjs
Normal file
161
tests/hooks/write-guard-coverage.test.mjs
Normal file
|
|
@ -0,0 +1,161 @@
|
||||||
|
// tests/hooks/write-guard-coverage.test.mjs
|
||||||
|
// Pins WHICH tools the write guard (pre-write-executor.mjs) fires on, not just
|
||||||
|
// what it blocks. path-guard.test.mjs runs the script directly, so it stayed
|
||||||
|
// green while hooks.json wired the guard to `Write` alone and `Edit` /
|
||||||
|
// `NotebookEdit` reached .git/hooks/, .env and shell configs unguarded.
|
||||||
|
//
|
||||||
|
// The dispatch tests below resolve hooks.json the way Claude Code does
|
||||||
|
// (matcher semantics: code.claude.com/docs/en/hooks § Matcher patterns) and run
|
||||||
|
// every PreToolUse hook that would fire for the tool, so a matcher gap fails
|
||||||
|
// here even when the script itself is correct.
|
||||||
|
//
|
||||||
|
// Field names: Write and Edit carry `file_path` (hooks reference, PreToolUse
|
||||||
|
// tool_input tables); NotebookEdit carries `notebook_path` (the tool's own
|
||||||
|
// input schema; the hooks reference has no NotebookEdit table).
|
||||||
|
|
||||||
|
import { test } from 'node:test';
|
||||||
|
import { strict as assert } from 'node:assert';
|
||||||
|
import { readFileSync } from 'node:fs';
|
||||||
|
import { dirname, join } from 'node:path';
|
||||||
|
import { fileURLToPath } from 'node:url';
|
||||||
|
import { runHook } from '../helpers/hook-helper.mjs';
|
||||||
|
|
||||||
|
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||||
|
const ROOT = join(HERE, '..', '..');
|
||||||
|
const HOOKS_JSON = join(ROOT, 'hooks', 'hooks.json');
|
||||||
|
const PRE_WRITE = join(ROOT, 'hooks', 'scripts', 'pre-write-executor.mjs');
|
||||||
|
|
||||||
|
const WRITE_TOOLS = ['Write', 'Edit', 'NotebookEdit'];
|
||||||
|
|
||||||
|
// Claude Code matcher semantics: "*", "" or omitted match all; a value of only
|
||||||
|
// letters, digits, _, -, spaces, `,` and `|` is a list of exact names; anything
|
||||||
|
// else is an unanchored JavaScript regular expression.
|
||||||
|
function matcherFires(matcher, toolName) {
|
||||||
|
if (matcher === undefined || matcher === '' || matcher === '*') return true;
|
||||||
|
if (/^[A-Za-z0-9_\- ,|]+$/.test(matcher)) {
|
||||||
|
return matcher.split(/[|,]/).map((s) => s.trim()).includes(toolName);
|
||||||
|
}
|
||||||
|
return new RegExp(matcher).test(toolName);
|
||||||
|
}
|
||||||
|
|
||||||
|
function preToolUseScriptsFor(toolName) {
|
||||||
|
const cfg = JSON.parse(readFileSync(HOOKS_JSON, 'utf8'));
|
||||||
|
const scripts = [];
|
||||||
|
for (const entry of cfg.hooks.PreToolUse || []) {
|
||||||
|
if (!matcherFires(entry.matcher, toolName)) continue;
|
||||||
|
for (const h of entry.hooks || []) {
|
||||||
|
const arg = (h.args || []).find((a) => a.endsWith('.mjs'));
|
||||||
|
if (arg) scripts.push(arg.replace('${CLAUDE_PLUGIN_ROOT}', ROOT));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return scripts;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Runs every PreToolUse hook Claude Code would run for this call; the call is
|
||||||
|
// blocked when any of them exits 2.
|
||||||
|
async function dispatch(tool_name, tool_input) {
|
||||||
|
const results = [];
|
||||||
|
for (const script of preToolUseScriptsFor(tool_name)) {
|
||||||
|
results.push(await runHook(script, { hook_event_name: 'PreToolUse', tool_name, tool_input }));
|
||||||
|
}
|
||||||
|
return { blocked: results.some((r) => r.code === 2), results };
|
||||||
|
}
|
||||||
|
|
||||||
|
function editInput(file_path) {
|
||||||
|
return { file_path, old_string: 'a', new_string: 'b', replace_all: false };
|
||||||
|
}
|
||||||
|
|
||||||
|
function notebookInput(notebook_path) {
|
||||||
|
return { notebook_path, cell_id: 'c1', new_source: 'x', edit_mode: 'replace' };
|
||||||
|
}
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// (a) The matcher covers every file-writing tool, and nothing else
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
test('hooks.json wires pre-write-executor to Write, Edit and NotebookEdit', () => {
|
||||||
|
for (const tool of WRITE_TOOLS) {
|
||||||
|
const scripts = preToolUseScriptsFor(tool);
|
||||||
|
assert.ok(
|
||||||
|
scripts.some((s) => s.endsWith('pre-write-executor.mjs')),
|
||||||
|
`pre-write-executor must fire on ${tool}; fires: ${JSON.stringify(scripts)}`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('hooks.json does not wire pre-write-executor to read-only tools', () => {
|
||||||
|
// The guard fails closed without a path, so firing on Read would block
|
||||||
|
// reads of .env and friends. Coverage must be exact, not "*".
|
||||||
|
for (const tool of ['Read', 'Glob', 'Grep', 'Bash']) {
|
||||||
|
const scripts = preToolUseScriptsFor(tool);
|
||||||
|
assert.ok(
|
||||||
|
!scripts.some((s) => s.endsWith('pre-write-executor.mjs')),
|
||||||
|
`pre-write-executor must not fire on ${tool}`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// (b) An Edit against a protected path is stopped
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
test('dispatch BLOCKS Edit to .git/hooks/', async () => {
|
||||||
|
const { blocked } = await dispatch('Edit', editInput('/some/repo/.git/hooks/pre-commit'));
|
||||||
|
assert.ok(blocked, 'Edit to .git/hooks/ must be blocked');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch BLOCKS Edit to .env', async () => {
|
||||||
|
const { blocked } = await dispatch('Edit', editInput('/some/repo/.env'));
|
||||||
|
assert.ok(blocked, 'Edit to .env must be blocked');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch BLOCKS Write to .git/hooks/ (unchanged)', async () => {
|
||||||
|
const { blocked } = await dispatch('Write', { file_path: '/some/repo/.git/hooks/pre-commit', content: 'x' });
|
||||||
|
assert.ok(blocked);
|
||||||
|
});
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// (c) A NotebookEdit is stopped via notebook_path
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
test('pre-write-executor BLOCKS NotebookEdit via notebook_path', async () => {
|
||||||
|
const { code, stderr } = await runHook(PRE_WRITE, {
|
||||||
|
tool_name: 'NotebookEdit',
|
||||||
|
tool_input: notebookInput('/some/repo/.claude/hooks/evil.ipynb'),
|
||||||
|
});
|
||||||
|
assert.strictEqual(code, 2, 'notebook_path must be read and checked');
|
||||||
|
assert.match(stderr, /Claude hooks/);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch BLOCKS NotebookEdit to .git/hooks/', async () => {
|
||||||
|
const { blocked } = await dispatch('NotebookEdit', notebookInput('/some/repo/.git/hooks/nb.ipynb'));
|
||||||
|
assert.ok(blocked, 'NotebookEdit to .git/hooks/ must be blocked');
|
||||||
|
});
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// (d) Ordinary, legitimate edits pass (the other direction)
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
test('dispatch ALLOWS a legitimate Edit', async () => {
|
||||||
|
const { blocked, results } = await dispatch('Edit', editInput('/some/repo/lib/util/foo.mjs'));
|
||||||
|
assert.ok(!blocked, `legitimate Edit must pass: ${JSON.stringify(results)}`);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('dispatch ALLOWS a legitimate NotebookEdit', async () => {
|
||||||
|
const { blocked, results } = await dispatch('NotebookEdit', notebookInput('/some/repo/notebooks/analysis.ipynb'));
|
||||||
|
assert.ok(!blocked, `legitimate NotebookEdit must pass: ${JSON.stringify(results)}`);
|
||||||
|
});
|
||||||
|
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
// Fail closed: no path found means no write
|
||||||
|
// -----------------------------------------------------------------------
|
||||||
|
test('dispatch BLOCKS an Edit with no path (fail closed)', async () => {
|
||||||
|
const { blocked } = await dispatch('Edit', { old_string: 'a', new_string: 'b' });
|
||||||
|
assert.ok(blocked, 'a write call with no readable path must be blocked');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('pre-write-executor BLOCKS NotebookEdit carrying file_path but no notebook_path', async () => {
|
||||||
|
// NotebookEdit's path field is notebook_path; any other shape is unknown and
|
||||||
|
// must not be waved through.
|
||||||
|
const { code } = await runHook(PRE_WRITE, {
|
||||||
|
tool_name: 'NotebookEdit',
|
||||||
|
tool_input: { file_path: '/some/repo/notebooks/a.ipynb', new_source: 'x' },
|
||||||
|
});
|
||||||
|
assert.strictEqual(code, 2);
|
||||||
|
});
|
||||||
Loading…
Add table
Add a link
Reference in a new issue