diff --git a/src/hook.mjs b/src/hook.mjs index 68d4eb9..3e3d24e 100644 --- a/src/hook.mjs +++ b/src/hook.mjs @@ -14,6 +14,7 @@ import { resolvePolicy } from './policy-resolver.mjs'; import { engineForPolicy, resolveStagedPolicy } from './scan-target.mjs'; import { scanEntries } from './scan-session.mjs'; import { + GIT_POLICY_INPUT_COMMANDS, GIT_POLICY_TRANSITION_COMMANDS, GIT_REF_MUTATION_COMMANDS, SAFE_NON_COMMIT_GIT, @@ -271,6 +272,17 @@ function hookPreToolUse(input) { 'aimhooman cannot determine the policy that will apply after dynamic execution; run the Git commit separately.' ); } + // Some ref-mutation verbs have read-only listing forms that move no + // ref and so cannot bypass the reference-transaction guard. Reading + // a repository (`git branch | grep`, `git remote -v | grep origin`, + // `git stash list | head`) is everyday work; refusing it behind a + // pipeline forced developers out of their normal workflow. A real + // mutation still carries a mutating flag or positional and stays + // subject to every check below. This sits above the transition veto + // because a command that reads cannot be made unsafe by whatever ran + // before it; it stays below the checks above, which are about not + // being able to see the repository at all. + if (gitReadOnlyRefCommand(gitCommand.verb, gitCommand.args || [])) continue; if (gitCommand.policyTransitionRisk) { return emitDecision( 'deny', @@ -279,14 +291,6 @@ function hookPreToolUse(input) { } targetRepo = openRepo(gitCommand.cwd); const targetProfile = enforcementPolicy(targetRepo).profile; - // Some ref-mutation verbs have read-only listing forms that move no - // ref and so cannot bypass the reference-transaction guard. Reading - // a repository (`git branch | grep`, `git remote -v | grep origin`, - // `git stash list | head`) is everyday work; refusing it behind a - // pipeline forced developers out of their normal workflow. A real - // mutation still carries a mutating flag or positional and stays - // subject to every check below. - if (gitReadOnlyRefCommand(gitCommand.verb, gitCommand.args || [])) continue; // An unresolved subcommand/alias may itself move a ref. When its // hook path or execution context is altered, there is no safe // content snapshot to fall back to, so treat it like a direct ref @@ -791,9 +795,12 @@ function resolveGitAliases(parsed) { } : resolved; commands.push(effective); if (effective.verb === 'add') aliasAddPaths.push(...effective.addPaths); + // The same narrowing the parser applies. Without it every commit-like + // candidate is re-stamped here after parseGit has run, and the veto + // below fires again on lines where nothing moved a policy input. aliasPolicyTransitionRisk ||= effective.verb === 'unknown' - || GIT_POLICY_TRANSITION_COMMANDS.has(effective.verb) - || GIT_REF_MUTATION_COMMANDS.has(effective.verb); + || (GIT_POLICY_INPUT_COMMANDS.has(effective.verb) + && !gitReadOnlyRefCommand(effective.verb, effective.args || [])); aliasIndexMutationRisk ||= effective.verb === 'unknown' || effective.indexMutationRisk || gitIndexMutationRisk(effective.verb, effective.args); diff --git a/src/shell-parse.mjs b/src/shell-parse.mjs index a342390..b4dce40 100644 --- a/src/shell-parse.mjs +++ b/src/shell-parse.mjs @@ -419,7 +419,7 @@ export function parseGit(cmd, initialCwd = process.cwd()) { targetUncertain: commandTargetUncertain, pathDialectUncertain: commandPathDialectUncertain, }); - policyTransitionRisk = true; + policyTransitionRisk ||= raisesPolicyTransitionRisk(verb, verbArgs); } else if (verb === 'add') { const paths = toks.slice(i + 1).filter((t) => !t.startsWith('-')); @@ -464,7 +464,7 @@ export function parseGit(cmd, initialCwd = process.cwd()) { pathDialectUncertain: commandPathDialectUncertain, classification: 'unknown', }); - policyTransitionRisk ||= GIT_POLICY_TRANSITION_COMMANDS.has(verb); + policyTransitionRisk ||= raisesPolicyTransitionRisk(verb, verbArgs); } else if (GIT_POLICY_TRANSITION_COMMANDS.has(verb) || GIT_REF_MUTATION_COMMANDS.has(verb)) { commands.push({ @@ -481,7 +481,7 @@ export function parseGit(cmd, initialCwd = process.cwd()) { pathDialectUncertain: commandPathDialectUncertain, classification: 'mutation', }); - policyTransitionRisk = true; + policyTransitionRisk ||= raisesPolicyTransitionRisk(verb, verbArgs); } } const indirectCommits = [ @@ -666,6 +666,26 @@ export const GIT_POLICY_TRANSITION_COMMANDS = new Set([ 'update-ref', ]); +// Verbs whose effect a later command's policy resolution reads: they move HEAD, +// the index, or the worktree that `.aimhooman.json` is resolved from. Only these +// raise the transition flag that refuses a later Git command on the same line. +// +// Deliberately a third set rather than a narrowing of the two around it. Those +// two also decide whether a verb becomes a modelled candidate at all, so +// dropping `push` from ref mutation would take the installed-guard check for +// `git push` with it. `commit` is absent on purpose: the state it moves is state +// this tool just supervised through pre-commit, commit-msg and the ref guard, +// unlike `symbolic-ref` or `stash pop`, which move it unwatched. +export const GIT_POLICY_INPUT_COMMANDS = new Set([ + 'am', 'branch', 'checkout', 'cherry-pick', 'merge', 'pull', 'read-tree', 'rebase', + 'replace', 'reset', 'restore', 'revert', 'rm', 'stash', 'switch', 'symbolic-ref', + 'update-index', 'update-ref', +]); + +function raisesPolicyTransitionRisk(verb, args = []) { + return GIT_POLICY_INPUT_COMMANDS.has(verb) && !gitReadOnlyRefCommand(verb, args); +} + // These verbs can create a commit or move a branch/reference. Their final // safety boundary is the managed reference-transaction hook, so PreToolUse // must not let config/environment/prefix indirection disable that boundary. @@ -755,7 +775,7 @@ const GIT_TAG_MUTATING_FLAGS = new Set([ const GIT_BRANCH_READONLY_FLAGS = new Set([ '-a', '--all', '-r', '--remotes', '-l', '--list', '-v', '--verbose', '-vv', - '-q', '--quiet', '--no-color', '--color', + '-q', '--quiet', '--no-color', '--color', '--show-current', '--sort', '--format', '--contains', '--no-contains', '--merged', '--no-merged', '--points-at', ]); diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index fee923f..ab087ca 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -3144,3 +3144,75 @@ test('an unwritable .git/info produces the same verdict as a writable one', asyn } } }); + +// One running flag was raised after every commit and every ref-affecting verb, +// then stamped onto every later command on the same line. So the second Git +// command was refused whatever it was, and told to "run the Git commit +// separately" even on lines with no commit in them. What actually matters is +// whether the earlier command moved something a later policy resolution reads: +// HEAD, the index, or the worktree. +test('a second Git command is allowed when the first moved no policy input', async () => { + const dir = makeHookRepo('clean', 'aim-hook-second-command-'); + try { + for (const command of [ + 'git commit -m x && git push', + 'git fetch && git rebase origin/main', + 'git remote -v && git push', + 'git tag -l && git push --tags', + 'git stash list && git commit -m x', + 'git branch --show-current && git commit -m x', + ]) { + assert.equal( + await invokePreToolUse(dir, { tool_name: 'Bash', tool_input: { command } }), + null, + command, + ); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +// The other half of the same decision. These four move HEAD or the index +// outside any guarded path, so a commit after them is resolved against state +// the tool never saw. They stay denied. +test('a second Git command is denied when the first moved a policy input', async () => { + const dir = makeHookRepo('clean', 'aim-hook-policy-input-'); + try { + for (const command of [ + 'git symbolic-ref HEAD refs/heads/other && git commit -m x', + 'git update-ref refs/heads/main HEAD && git commit -m x', + 'git rm --cached .aimhooman.json && git commit -m x', + 'git stash pop && git commit -m x', + 'git checkout other-branch && git commit -m x', + ]) { + const out = await invokePreToolUse(dir, { tool_name: 'Bash', tool_input: { command } }); + assert.equal(out?.permissionDecision, 'deny', command); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +// The listing carve-out was only ever exercised on clean, which is why a change +// that turned it off under strict could ship green. +test('read-only Git ref-command listings are allowed under strict too', async () => { + const dir = makeHookRepo('strict', 'aim-hook-readonly-strict-'); + try { + for (const command of [ + 'git branch | grep feat', + 'git branch -a | head', + 'git remote -v | grep origin', + 'git stash list | head', + 'git notes list | head', + ]) { + assert.equal( + await invokePreToolUse(dir, { tool_name: 'Bash', tool_input: { command } }), + null, + command, + ); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } +});