diff --git a/docs/integrations.md b/docs/integrations.md index 2c47df6..b50a8f9 100644 --- a/docs/integrations.md +++ b/docs/integrations.md @@ -85,11 +85,11 @@ and shows both gitleaks setups. ## husky -husky sets `core.hooksPath` to `.husky`. `aimhooman init` refuses to install -into or over an external or shared hooks directory by design: `.husky` is -tracked repository content, and a dispatcher written there would stage this -machine's absolute CLI and Node paths for everyone who clones -(`src/githooks.mjs` has the full rule set). Two ways out: +husky 9 sets `core.hooksPath` to `.husky/_` (older versions used `.husky`). +`aimhooman init` refuses to install into or over an external or shared hooks +directory by design: it is worktree content, and a dispatcher written there +would stage this machine's absolute CLI and Node paths for everyone who clones +(`src/githooks.mjs` has the full rule set). Three ways out: - Keep husky and call `aimhooman check --staged` from `.husky/pre-commit`. You get the CLI check only. The agent-tier guard (PreToolUse) asks for @@ -98,6 +98,9 @@ machine's absolute CLI and Node paths for everyone who clones - Drop husky for the guarded hooks and let `aimhooman init` manage them. Existing hooks are not lost: init preserves each one as a chained predecessor, and the dispatcher runs it before its own check. +- Keep the hooks path local: add it to `.git/info/exclude` (for husky 9, + `.husky/_/`) and re-run `aimhooman init`. It then manages that directory and + chains husky's shim, and nothing machine-specific is shared with clones. ## lint-staged diff --git a/src/githooks.mjs b/src/githooks.mjs index 2f3da0d..3154fa2 100644 --- a/src/githooks.mjs +++ b/src/githooks.mjs @@ -73,7 +73,7 @@ function hooksDir(repo) { const where = scope ? `${scope} scope` : 'a non-local scope'; let reason = where; if (trackedByGit) reason = `${where}, tracked by this repository`; - else if (worktreeContent) reason = `${where}, inside the worktree so Git will track it on the next add (add it to .gitignore or .git/info/exclude to manage it locally)`; + else if (worktreeContent) reason = `${where}, inside the worktree where Git can track it (add it to .gitignore or .git/info/exclude to manage it locally)`; else if (localScope && !inside) reason = `${where}, outside this repository`; return { dir, @@ -427,13 +427,28 @@ function installHooksLocked(repo, cliPath, options, dir, warnings) { const current = entry(dest); if (current) { const cur = readFileSync(dest); - if (!ownedHook(cur, name)) { - mkdirSync(chainedDir, { recursive: true }); - const chainedPath = join(chainedDir, name); - const predecessorMode = current.mode & 0o777; - writeHook(chainedPath, cur, { mode: predecessorMode }); - chmodSync(chainedPath, predecessorMode); - chained.push(name); + const chainedPath = join(chainedDir, name); + // A file already naming the backup slot as its predecessor is + // our own damaged output. Chaining it would point the backup at + // itself and the dispatcher would exec itself without bound. + const selfChained = cur.includes(`\nCHAINED=${shq(chainedPath)}\n`); + if (!ownedHook(cur, name) && !selfChained) { + const stored = entry(chainedPath); + if (stored && !readFileSync(chainedPath).equals(cur)) { + // Something replaced the dispatcher after a chained + // install. The stored backup was here first, so it is + // the original worth keeping; the newer file is + // replaced without one, which has to be said out loud. + warnings.push( + `${name} already has a chained backup at ${chainedPath}; kept it and replaced ${dest} without preserving it` + ); + } else { + mkdirSync(chainedDir, { recursive: true }); + const predecessorMode = current.mode & 0o777; + writeHook(chainedPath, cur, { mode: predecessorMode }); + chmodSync(chainedPath, predecessorMode); + chained.push(name); + } } } writeHook(dest, hookScript(name, cmd, cliPath, join(chainedDir, name)), { mode: 0o755 }); @@ -451,8 +466,41 @@ function installHooksLocked(repo, cliPath, options, dir, warnings) { // is recorded as a failure and the remaining hooks are still processed, so a // re-run self-heals (already-processed hooks are skipped via ownedHook). export function uninstallHooks(repo) { - const { dir, shared, warnings } = hooksDir(repo); - if (shared) return { removed: [], restored: [], warnings, failures: [] }; + const { warnings } = hooksDir(repo); + const results = uninstallHookDirs(repo).map((target) => uninstallOneHookDir(repo, target, [])); + return { + removed: results.flatMap((r) => r.removed).sort(), + restored: results.flatMap((r) => r.restored).sort(), + warnings: [...warnings, ...results.flatMap((r) => r.warnings)], + failures: results.flatMap((r) => r.failures), + }; +} + +// The directories a local uninstall must clear. A moved core.hooksPath leaves +// dispatchers behind in .git/hooks, dormant only until the setting is unset, so +// that directory is cleared whatever the hooks path currently says. The global +// directory belongs to `uninstall --global` and is never touched here. +function uninstallHookDirs(repo) { + const { dir, shared } = hooksDir(repo); + const globalCanonical = canonicalPath(globalHooksDir()); + const targets = []; + const seen = new Set(); + for (const candidate of [shared ? null : dir, join(repo.commonDir, 'hooks')]) { + if (!candidate) continue; + let key; + try { + key = canonicalPath(candidate); + } catch { + key = candidate; + } + if (key === globalCanonical || seen.has(key)) continue; + seen.add(key); + targets.push(candidate); + } + return targets; +} + +function uninstallOneHookDir(repo, dir, warnings) { let entered = false; try { return withLock(join(dir, '.aimhooman-hooks.lock'), () => { @@ -513,7 +561,14 @@ function uninstallHooksLocked(repo, dir, warnings) { removed.push(name); continue; } - if (predecessor) { + if (predecessor && readFileSync(chained).includes(`\nCHAINED=${shq(chained)}\n`)) { + // The backup names itself as its own predecessor, so installing + // it would leave a hook that execs itself. It is not the user's + // original either way: drop both and say so. + warnings.push(`${name} chained backup pointed at itself; removed ${chained} instead of restoring it`); + unlinkSync(chained); + unlinkSync(dest); + } else if (predecessor) { const predecessorMode = predecessor.mode & 0o777; atomicWrite(dest, readFileSync(chained), { mode: predecessorMode }); chmodSync(dest, predecessorMode); @@ -542,8 +597,10 @@ function uninstallHooksLocked(repo, dir, warnings) { // comparing paths: two spellings of one directory differ on Windows, and // deciding ownership by string is the bug this change exists to remove. export function remainingDispatchers(repo) { - const { dir, shared } = hooksDir(repo); - if (shared) return []; + return uninstallHookDirs(repo).flatMap((dir) => dispatchersLeftIn(dir)); +} + +function dispatchersLeftIn(dir) { return Object.keys(MANAGED).sort().flatMap((name) => { const path = join(dir, name); try { diff --git a/tests/githooks.test.mjs b/tests/githooks.test.mjs index 0b0c1f6..4ed4d8f 100644 --- a/tests/githooks.test.mjs +++ b/tests/githooks.test.mjs @@ -27,6 +27,7 @@ import { installedHooks, MESSAGE_ANCHOR_ERE, pathCommandReachable, + remainingDispatchers, uninstallGlobalHooks, uninstallHooks, } from '../src/githooks.mjs'; @@ -2523,3 +2524,115 @@ test('install and uninstall refuse to follow hook symlinks', () => { rmSync(base, { recursive: true, force: true }); } }); + +// A hook file that already points at the backup slot is our own damaged output. +// Chaining it makes the backup its own predecessor, so the dispatcher execs +// itself without bound; and copying it over the slot destroys whatever original +// hook was preserved there. +function damagedDispatcher(hooksPath) { + const dispatcher = readFileSync(hooksPath, 'utf8'); + return dispatcher.replace(/^# aimhooman-hook-fingerprint: .*$/m, '# aimhooman-hook-fingerprint: ' + '0'.repeat(64)); +} + +test('a damaged dispatcher is replaced rather than chained onto its own backup', () => { + const base = mkdtempSync(join(tmpdir(), 'aim-hooks-selfchain-')); + try { + isolatedGitConfig(base, () => { + const root = makeRepo(base); + const repo = openRepo(root); + const hooks = git(root, ['rev-parse', '--path-format=absolute', '--git-path', 'hooks']); + const original = '#!/bin/sh\necho original\n'; + writeFileSync(join(hooks, 'pre-commit'), original, { mode: 0o755 }); + installHooks(repo, CLI); + + const backup = join(repo.stateDir, 'chained', 'pre-commit'); + assert.equal(readFileSync(backup, 'utf8'), original); + + writeFileSync(join(hooks, 'pre-commit'), damagedDispatcher(join(hooks, 'pre-commit')), { mode: 0o755 }); + const report = installHooks(repo, CLI); + + assert.equal(report.chained.includes('pre-commit'), false, 'our own damaged output must not be chained'); + assert.equal(readFileSync(backup, 'utf8'), original, "the user's original hook must survive"); + }); + } finally { + rmSync(base, { recursive: true, force: true }); + } +}); + +test('a foreign hook that replaced the dispatcher does not overwrite the stored backup', () => { + const base = mkdtempSync(join(tmpdir(), 'aim-hooks-backup-keep-')); + try { + isolatedGitConfig(base, () => { + const root = makeRepo(base); + const repo = openRepo(root); + const hooks = git(root, ['rev-parse', '--path-format=absolute', '--git-path', 'hooks']); + const original = '#!/bin/sh\necho original\n'; + writeFileSync(join(hooks, 'pre-commit'), original, { mode: 0o755 }); + installHooks(repo, CLI); + + // Another tool installs over the dispatcher. The next init must not + // treat that as the hook worth preserving. + writeFileSync(join(hooks, 'pre-commit'), '#!/bin/sh\necho intruder\n', { mode: 0o755 }); + const report = installHooks(repo, CLI); + + const backup = join(repo.stateDir, 'chained', 'pre-commit'); + assert.equal(readFileSync(backup, 'utf8'), original, 'the first backup is the genuine original'); + assert.ok( + report.warnings.some((w) => w.includes('pre-commit')), + 'skipping a backup has to be reported, not silent', + ); + }); + } finally { + rmSync(base, { recursive: true, force: true }); + } +}); + +test('uninstall does not restore a backup that points at itself', () => { + const base = mkdtempSync(join(tmpdir(), 'aim-hooks-selfbackup-')); + try { + isolatedGitConfig(base, () => { + const root = makeRepo(base); + const repo = openRepo(root); + const hooks = git(root, ['rev-parse', '--path-format=absolute', '--git-path', 'hooks']); + installHooks(repo, CLI); + + // Seed the poisoned state: the backup slot holds a dispatcher whose + // CHAINED= names the slot itself. + const backup = join(repo.stateDir, 'chained', 'pre-commit'); + mkdirSync(dirname(backup), { recursive: true }); + writeFileSync(backup, damagedDispatcher(join(hooks, 'pre-commit')), { mode: 0o755 }); + + uninstallHooks(repo); + + assert.equal(existsSync(join(hooks, 'pre-commit')), false, 'the dispatcher must go'); + assert.equal(existsSync(backup), false, 'and the self-referencing backup with it'); + }); + } finally { + rmSync(base, { recursive: true, force: true }); + } +}); + +// Moving core.hooksPath after install leaves the dispatchers in .git/hooks. +// They are dormant only while the setting points elsewhere: unset it and they +// guard again, which is why uninstall has to clear that directory too rather +// than report success over the top of it. +test('uninstall clears dispatchers left behind when the hooks path moved', () => { + const base = mkdtempSync(join(tmpdir(), 'aim-hooks-moved-')); + try { + isolatedGitConfig(base, () => { + const root = makeRepo(base); + const repo = openRepo(root); + installHooks(repo, CLI); + const local = join(repo.commonDir, 'hooks'); + assert.ok(existsSync(join(local, 'pre-commit')), 'installed into .git/hooks'); + + git(root, ['config', 'core.hooksPath', '.husky/_']); + uninstallHooks(repo); + + assert.equal(existsSync(join(local, 'pre-commit')), false, 'the leftover dispatcher must be removed'); + assert.deepEqual(remainingDispatchers(repo), [], 'and nothing may still be reported as remaining'); + }); + } finally { + rmSync(base, { recursive: true, force: true }); + } +});