mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-10 06:58:59 +02:00
fix(hooks): fail-closed freeze + shared extractor + careful HIGH tier
Freeze boundary hook had four verified bugs: the grep-first JSON extractor truncated at escaped quotes and failed OPEN on unparseable payloads; the deny JSON was printf-interpolated so a quote- or newline-bearing path silently no-oped the block; the freeze path read stripped INTERNAL spaces (a boundary like ~/My Project could never match); and the path resolver skipped the final component, letting an in-boundary symlink write through to an out-of-boundary target. Fixes, structurally: one shared sourced helper (careful/bin/hook-extract.sh) now owns JSON extraction and JSON-encoded decision envelopes for BOTH hooks -- the two-copy drift is how freeze kept a broken extractor after careful's was fixed. Freeze is now deny-tier fail-closed (unparseable payload denies, parsed-but-no-file_path still allows), trims only leading/trailing whitespace, and resolves symlinks through the final path component. Careful gains a HIGH tier (hard deny, simple commands only): recursive delete of /, ~, or $HOME, and force-push to the repo's default branch. Compound commands always fall through to the MEDIUM ask; --force-with-lease is never HIGH. Documented as a best-effort advisory hard-stop, not a policy boundary. Plus additive-only project patterns (~/.gstack/careful-patterns.txt + per-project file): config can only ADD warn rules, never suppress a baseline family. test/hook-scripts.test.ts: 89 tests incl. malformed-payload deny, parseable deny JSON for hostile paths, space-bearing boundaries, symlink escape, HIGH tier splits, additive invariant, invalid-regex resilience. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Fable 5
Claude Fable 5
parent
2be6c06ba8
commit
455c805125
+265
-15
@@ -8,11 +8,12 @@ const ROOT = path.resolve(import.meta.dir, '..');
|
||||
const CAREFUL_SCRIPT = path.join(ROOT, 'careful', 'bin', 'check-careful.sh');
|
||||
const FREEZE_SCRIPT = path.join(ROOT, 'freeze', 'bin', 'check-freeze.sh');
|
||||
|
||||
function runHook(scriptPath: string, input: object, env?: Record<string, string>): { exitCode: number; output: any; raw: string } {
|
||||
function runHook(scriptPath: string, input: object, env?: Record<string, string>, cwd?: string): { exitCode: number; output: any; raw: string } {
|
||||
const result = spawnSync('bash', [scriptPath], {
|
||||
input: JSON.stringify(input),
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
env: { ...process.env, ...env },
|
||||
cwd,
|
||||
timeout: 5000,
|
||||
});
|
||||
const raw = result.stdout.toString().trim();
|
||||
@@ -23,6 +24,25 @@ function runHook(scriptPath: string, input: object, env?: Record<string, string>
|
||||
return { exitCode: result.status ?? 1, output, raw };
|
||||
}
|
||||
|
||||
// Scratch git repo with a resolvable origin default branch — the HIGH-tier
|
||||
// force-push check reads `git symbolic-ref refs/remotes/origin/HEAD` from the
|
||||
// hook's cwd, and Conductor worktrees don't reliably carry that ref.
|
||||
function withGitRepo(defaultBranch: string, currentBranch: string, fn: (repoDir: string) => void) {
|
||||
const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-git-'));
|
||||
try {
|
||||
const git = (args: string[]) =>
|
||||
spawnSync('git', ['-c', 'user.email=t@test', '-c', 'user.name=t', ...args], { cwd: repoDir, timeout: 5000 });
|
||||
git(['init', '-q', '-b', defaultBranch]);
|
||||
git(['commit', '--allow-empty', '-q', '-m', 'init']);
|
||||
// A symbolic ref may dangle; the hook only reads its NAME.
|
||||
git(['symbolic-ref', 'refs/remotes/origin/HEAD', `refs/remotes/origin/${defaultBranch}`]);
|
||||
if (currentBranch !== defaultBranch) git(['checkout', '-q', '-b', currentBranch]);
|
||||
fn(repoDir);
|
||||
} finally {
|
||||
fs.rmSync(repoDir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
function runHookRaw(scriptPath: string, rawInput: string, env?: Record<string, string>): { exitCode: number; output: any; raw: string } {
|
||||
const result = spawnSync('bash', [scriptPath], {
|
||||
input: rawInput,
|
||||
@@ -161,12 +181,13 @@ describe('check-careful.sh', () => {
|
||||
|
||||
// Capital -R is the documented recursive flag on BSD rm (macOS) and accepted
|
||||
// by GNU rm. Both greps previously required a lowercase r, so `rm -R /`
|
||||
// silently allowed.
|
||||
test('rm -R / warns (capital -R recursive)', () => {
|
||||
// silently allowed. A bare recursive delete of / is now HIGH-tier: denied,
|
||||
// not asked.
|
||||
test('rm -R / denies (HIGH tier: recursive delete of root)', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -R /'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete');
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH');
|
||||
});
|
||||
|
||||
test('rm -fR /home/user warns (capital R in flag cluster)', () => {
|
||||
@@ -326,18 +347,26 @@ describe('check-careful.sh', () => {
|
||||
// --- Git destructive commands ---
|
||||
|
||||
describe('git destructive commands', () => {
|
||||
test('git push --force warns with force-push', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
// Force-push to a NON-default branch is MEDIUM (ask). Force-push to the
|
||||
// default branch is HIGH (deny) — covered in the HIGH tier describe. The
|
||||
// fixture repo pins the default branch so the split is deterministic
|
||||
// regardless of the host repo's origin/HEAD.
|
||||
test('git push --force warns with force-push (non-default target)', () => {
|
||||
withGitRepo('trunk', 'trunk', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
});
|
||||
});
|
||||
|
||||
test('git push -f warns', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin main'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
test('git push -f warns (non-default target)', () => {
|
||||
withGitRepo('trunk', 'trunk', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin main'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
});
|
||||
});
|
||||
|
||||
test('git reset --hard warns with uncommitted', () => {
|
||||
@@ -443,6 +472,132 @@ describe('check-careful.sh', () => {
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete');
|
||||
});
|
||||
});
|
||||
|
||||
// --- HIGH tier (hard deny) ---
|
||||
// A tiny set of catastrophic SIMPLE commands is denied outright while
|
||||
// /careful is active. Best-effort advisory hard-stop, not a policy boundary:
|
||||
// compound commands always fall through to the MEDIUM ask.
|
||||
|
||||
describe('HIGH tier (hard deny)', () => {
|
||||
test.each(['rm -rf /', 'rm -rf ~', 'rm -rf $HOME', 'sudo rm -rf /', 'rm -Rf ~/'])(
|
||||
'denies catastrophic recursive delete: %s',
|
||||
(command) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH');
|
||||
},
|
||||
);
|
||||
|
||||
test('rm -rf ~/subdir stays MEDIUM ask (not the whole home dir)', () => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf ~/subdir'));
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
});
|
||||
|
||||
test('git push --force origin <default branch> denies', () => {
|
||||
withGitRepo('main', 'feature', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force origin main'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('default branch');
|
||||
});
|
||||
});
|
||||
|
||||
test('bare git push --force while ON the default branch denies', () => {
|
||||
withGitRepo('main', 'main', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('HIGH');
|
||||
});
|
||||
});
|
||||
|
||||
test('bare git push --force on a feature branch asks (MEDIUM)', () => {
|
||||
withGitRepo('main', 'feature', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
});
|
||||
});
|
||||
|
||||
test('git push -f origin feature asks (MEDIUM — not the default branch)', () => {
|
||||
withGitRepo('main', 'main', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push -f origin feature'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('force-push');
|
||||
});
|
||||
});
|
||||
|
||||
test('compound force-push falls through to ask, never deny (cannot resolve cwd)', () => {
|
||||
withGitRepo('main', 'main', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('cd elsewhere && git push --force origin main'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
});
|
||||
});
|
||||
|
||||
test('--force-with-lease is never HIGH (the safe force variant)', () => {
|
||||
withGitRepo('main', 'main', (repoDir) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('git push --force-with-lease origin main'), undefined, repoDir);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).not.toBe('deny');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// --- Additive project patterns ---
|
||||
// Config can only ADD warn rules. The files are consulted after the baseline
|
||||
// families, so no file content can suppress a baseline match.
|
||||
|
||||
describe('additive project patterns', () => {
|
||||
function withPatternFile(content: string, fn: (gstackHome: string) => void) {
|
||||
const gstackHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-pat-'));
|
||||
fs.writeFileSync(path.join(gstackHome, 'careful-patterns.txt'), content);
|
||||
try {
|
||||
fn(gstackHome);
|
||||
} finally {
|
||||
fs.rmSync(gstackHome, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
test('a project pattern adds an ask rule', () => {
|
||||
withPatternFile('# infra safety\nterraform\\s+destroy\n', (gstackHome) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('terraform destroy -auto-approve'), { GSTACK_HOME: gstackHome });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('Project rule');
|
||||
});
|
||||
});
|
||||
|
||||
test('a garbage pattern file cannot suppress a baseline match (additive invariant)', () => {
|
||||
withPatternFile('# override: allow everything\nallow-everything\nignore baseline\n', (gstackHome) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf /var/data'), { GSTACK_HOME: gstackHome });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete');
|
||||
});
|
||||
});
|
||||
|
||||
test('an invalid regex line is skipped without breaking the hook', () => {
|
||||
withPatternFile('([unclosed\nterraform\\s+destroy\n', (gstackHome) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('terraform destroy'), { GSTACK_HOME: gstackHome });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('Project rule');
|
||||
});
|
||||
});
|
||||
|
||||
test('safe commands still allow with a pattern file present', () => {
|
||||
withPatternFile('terraform\\s+destroy\n', (gstackHome) => {
|
||||
const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('ls -la'), { GSTACK_HOME: gstackHome });
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ============================================================
|
||||
@@ -550,5 +705,100 @@ describe('check-freeze.sh', () => {
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
test('malformed JSON payload DENIES (fail closed — freeze is a deny-tier hook)', () => {
|
||||
withFreezeDir('/Users/dev/project/src/', (stateDir) => {
|
||||
const { exitCode, output } = runHookRaw(
|
||||
FREEZE_SCRIPT,
|
||||
'not json at all {{{{',
|
||||
{ CLAUDE_PLUGIN_DATA: stateDir },
|
||||
);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('fail closed');
|
||||
});
|
||||
});
|
||||
|
||||
test('a quote-bearing path outside the boundary emits PARSEABLE deny JSON', () => {
|
||||
// The old printf-interpolated deny emitted malformed JSON for paths
|
||||
// containing quotes — Claude Code silently ignored the whole decision,
|
||||
// so the deny no-oped exactly when the path was hostile.
|
||||
withFreezeDir('/Users/dev/project/src/', (stateDir) => {
|
||||
const { exitCode, output, raw } = runHook(
|
||||
FREEZE_SCRIPT,
|
||||
freezeInput('/tmp/evil"quoted/x.ts'),
|
||||
{ CLAUDE_PLUGIN_DATA: stateDir },
|
||||
);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(() => JSON.parse(raw)).not.toThrow();
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
});
|
||||
});
|
||||
|
||||
test('a newline-bearing path outside the boundary emits PARSEABLE deny JSON', () => {
|
||||
withFreezeDir('/Users/dev/project/src/', (stateDir) => {
|
||||
const { exitCode, output, raw } = runHook(
|
||||
FREEZE_SCRIPT,
|
||||
freezeInput('/tmp/evil\npath.ts'),
|
||||
{ CLAUDE_PLUGIN_DATA: stateDir },
|
||||
);
|
||||
expect(exitCode).toBe(0);
|
||||
expect(() => JSON.parse(raw)).not.toThrow();
|
||||
expect(output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('space-bearing freeze boundary', () => {
|
||||
// The old `tr -d '[:space:]'` stripped INTERNAL spaces from the freeze
|
||||
// path, so a boundary like ".../My Project/src" never matched anything.
|
||||
test('a boundary containing spaces allows edits inside it', () => {
|
||||
const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-space-'));
|
||||
const boundary = path.join(base, 'My Project', 'src');
|
||||
fs.mkdirSync(boundary, { recursive: true });
|
||||
try {
|
||||
withFreezeDir(boundary + '/', (stateDir) => {
|
||||
const inside = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'index.ts')), { CLAUDE_PLUGIN_DATA: stateDir });
|
||||
expect(inside.exitCode).toBe(0);
|
||||
expect(inside.output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
|
||||
const outside = runHook(FREEZE_SCRIPT, freezeInput(path.join(base, 'elsewhere.ts')), { CLAUDE_PLUGIN_DATA: stateDir });
|
||||
expect(outside.exitCode).toBe(0);
|
||||
expect(outside.output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
});
|
||||
} finally {
|
||||
fs.rmSync(base, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('symlink boundary escape', () => {
|
||||
// The old resolver followed the parent directory but NOT the final path
|
||||
// component, so an in-boundary symlink pointing outside the boundary was
|
||||
// allowed while the write landed outside.
|
||||
test('an in-boundary symlink to an outside target denies', () => {
|
||||
const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-freeze-link-'));
|
||||
const boundary = path.join(base, 'boundary');
|
||||
const outside = path.join(base, 'outside');
|
||||
fs.mkdirSync(boundary, { recursive: true });
|
||||
fs.mkdirSync(outside, { recursive: true });
|
||||
fs.writeFileSync(path.join(outside, 'secret.txt'), 'x');
|
||||
fs.symlinkSync(path.join(outside, 'secret.txt'), path.join(boundary, 'link.txt'));
|
||||
try {
|
||||
withFreezeDir(boundary + '/', (stateDir) => {
|
||||
const viaLink = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'link.txt')), { CLAUDE_PLUGIN_DATA: stateDir });
|
||||
expect(viaLink.exitCode).toBe(0);
|
||||
expect(viaLink.output.hookSpecificOutput?.permissionDecision).toBe('deny');
|
||||
|
||||
// A real in-boundary file is unaffected.
|
||||
fs.writeFileSync(path.join(boundary, 'real.txt'), 'y');
|
||||
const real = runHook(FREEZE_SCRIPT, freezeInput(path.join(boundary, 'real.txt')), { CLAUDE_PLUGIN_DATA: stateDir });
|
||||
expect(real.exitCode).toBe(0);
|
||||
expect(real.output.hookSpecificOutput?.permissionDecision).toBeUndefined();
|
||||
});
|
||||
} finally {
|
||||
fs.rmSync(base, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user