COMMITLORE_BIN carried none of the checks its sibling config key already had

[MongLong0214]

Summary

Fixes #71 (security). COMMITLORE_BIN let exec "$COMMITLORE_BIN" run any executable the environment pointed at, while the recorded commitlore.bin config path had carried an extension allowlist since #64/1fd0d53. An env var is reachable from CI configuration, a sourced profile, or a compromised toolchain — none of which a reviewer reads as executable config.

Severity, stated honestly (per the issue): this is not remote code execution and not privilege escalation. Anyone who can write .git/config can already write .git/hooks/commit-msg directly — same permission, same reach. The bug is inconsistency and auditability, not new capability.

Before / after (measured against this branch's build)

commitlore hooks install
COMMITLORE_BIN=/tmp/evil.sh git commit -m x

• Before: evil.sh ran, commit succeeded.
• After: COMMITLORE_BIN fails the *.mjs|*.js allowlist, falls through to the remaining resolution steps. evil.sh never runs; with an invalid trailer present the commit is rejected by real validation.

git config commitlore.bin /tmp/evil.js
git commit -m x

• Before: evil.js ran, commit succeeded — naming a file .js satisfies the existing extension check for free, so that check alone doesn't stop this.
• After: hooks install now also records commitlore.root (this installation's own package root). The stub checks the recorded commitlore.bin's physically-resolved location (cd "$(dirname ...)" && pwd -P, symlinks at the final component rejected via -L) against that root before executing it. /tmp/evil.js is outside the root, so the stub refuses it and falls through; the commit fails closed (cannot find the CLI), and evil.js never runs.

Controls (both re-verified on this branch): a Blast: wide commit is rejected in the same repository (hook is live), a clean commit is accepted (hook still validates normally), and a legitimate COMMITLORE_BIN override pointed at a real .mjs build still works and still validates.

Changes

1. src/hooks/commit-msg.ts — COMMITLORE_BIN now carries the same *.mjs|*.js allowlist the recorded path already had. Additionally, hooks install records commitlore.root, and the recorded-path branch refuses to execute a commitlore.bin whose resolved directory doesn't sit under it (symlink-at-final-component closed via -L).
2. src/commands/hooks.ts — recordBinPath records commitlore.root (realpathSync(PACKAGE_ROOT)) alongside the existing commitlore.bin/commitlore.node, best-effort/non-fatal like the rest of that function.
3. src/commands/doctor.ts / src/core/hook-target.ts — doctor already printed commitlore.bin and an active COMMITLORE_BIN override (since ADR-0012 is accepted but unimplemented: index needs better-sqlite3, so it fails on any fresh install #64/1fd0d53); it now also says when that override will be ignored for failing the extension check, using a shared hasAllowedBinExtension predicate so the shell's case pattern and the TypeScript check can't disagree.

Ruled-out: an install-root containment check for COMMITLORE_BIN itself — its whole reason to exist is aiming the hook at a build outside the install root (a test harness, a monorepo's local bin per the existing docstring). Restricting its location would remove the one thing it's for.

Ruled-out: resolving commitlore.bin with an external realpath/readlink -f binary — neither is guaranteed on every machine a hook runs on (same reasoning #64/1fd0d53 gave for not shelling out to shasum). cd ... && pwd -P is POSIX-specified and covers the same need with no new dependency.

Warn (disclosed residual risk): the containment check doesn't protect a relative, separator-less commitlore.bin (a bare filename), which resolves against the hook's cwd. This sits in the same accepted-risk category as the severity note above.

Test plan

• Baseline confirmed on dev before changes: 1233 passed / 34 files (full suite; test/mcp.test.ts re-run alone too, per known flakiness under load)
• npm run typecheck, npm run build clean; committed dist/ matches a fresh build of src/
• Both attacks reproduced against this branch's build and now fail closed (see above); both controls (hook live + rejects bad trailer, hook live + accepts clean message) still pass
• Symlink-inside-root-pointing-outside variant of attack 1 also verified blocked
• Legitimate COMMITLORE_BIN override (pointed at a real .mjs build) still works
• Full suite after the fix: 1236 passed / 34 files, 1 skipped (three new test/hooks.test.ts cases; test/doctor.test.ts fixtures updated to record commitlore.root, one status assertion changed warn → fail where the hook now genuinely can't resolve a CLI in a PATH-less environment)
• test/dogfood.test.ts passes against this branch's own history
• node dist/commitlore.mjs validate --commit HEAD passes on the commit in this PR

Out of scope (per issue and explicit instructions)

Did not touch MCP path rejection or ANSI stripping, did not change exit-code semantics (bug-issue-65 is in flight separately), not merged to dev.

https://claude.ai/code/session_014adVVJ3eo6FUiqUVqLGfm9

[github-actions]

CommitLore — record lint

Trailers: clean — 1 commit in origin/dev..8a49ddc9f76b4fc65c03fc874a8478ce3ca2f860
Active constraints: 17 limits · 52 ruled-out · 33 warnings — from 41 records over 17 changed paths

Active constraints for the paths this PR touches

Limits (17)

• r-fix70a1 d707fc7 — one encoding layer and explicit lexical forms in the four published languages; semantic paraphrases, nested encodings, and split payloads remain outside coverage
• r-shwt66 5efa206 — git rev-parse --git-path may return a repository-relative path, so resolve it against cwd
• r-merge66 40e7987 — Generated dist files were resolved only by npm run build and npm run bundle
• r-fix760 fb8ba45 — Git remains the authority on trailer recognition; diagnostics must not loosen the parser
• r-refint74 572f573 — validate cannot perform conservation checks because it has no before state
• r-warn75 24c7cc8 — exit-code semantics remain owned by guard's exit 2 means blocked; everywhere else in the same CLI exit 2 means bad usage #65
• r-shallow66 60a8659 — a depth-1 clone can only inspect its reachable commit history
• r-doctor72 996bcde — generated dist artifacts must come from npm run build and npm run bundle, not a hand merge
• r-fix067 a915af0 — PreToolUse hook failures must always exit 0 and never change stdout's hookSpecificOutput contract
• r-fix063 0b8c496 — doctor performs remote probes; an unreachable remote reports could not verify instead of ok
• r-fix053 ecc4b90 — QueryResult.notes remains repository-level availability and is independent from whether one record was mirrored
• r-fix055 43b40f8 — harvest-verify makes no model call, so semantic entailment is outside its contract
• r-fix054 664d4e2 — notes-only metadata must survive folding; a mirror is one record, not two
• r-fix056 55cb8bc — blocked output may retain only validated structural values that cannot carry prose
• r-7a3e91 cf859e4 — better-sqlite3 stays external because it is native — the bundle degrades to --no-index without it, which only works because r-6f2a08 made that load lazy first
• r-4e9c72 a7a7e26 — the index is derived, so nothing about its state can make the tool give a wrong answer -- only a slower one
• r-9a5e17 6d68703 — five workers on one repository share npm test and tsc, so file ownership alone does not prevent one worker from "fixing" another's half-written code -- verification scope had to be split too

Ruled out (52)

• r-83d43117 8a49ddc — an install-root check for COMMITLORE_BIN | its only reason to exist is aiming the hook at a build outside the install root -- a test harness, a monorepo's local bin. Restricting its location would remove the one thing it is for; doctor's existing COMMITLORE_BIN visibility is the right amount of scrutiny for a channel that is supposed to point anywhere.
• r-83d43117 8a49ddc — resolving commitlore.bin with an external realpath/readlink -f binary | neither is guaranteed on every machine a hook runs on, the same reasoning 1fd0d53 gave for not shelling out to shasum. cd ... && pwd -P is a POSIX-specified builtin and covers the same physical-resolution need without a new dependency.
• r-fix70a1 d707fc7 — exhaustive per-language phrase enumeration | unbounded phrase lists cannot provide semantic coverage, so this fix documents a bounded lexical policy and independent corpus
• r-shwt66 5efa206 — checking --git-dir/shallow | linked worktrees keep the shallow marker in the common Git directory
• r-fix760 fb8ba45 — testing commits with more than one parent in dogfood scope | their platform-generated merge messages carry no authored decision, so requiring a record would require one nobody wrote
• r-refint74 572f573 — allowing a note to extend a commit record under the same Record-Id | notes are remote-reachable, so divergent content would inherit a human-approved identity
• r-warn75 24c7cc8 — accepting run-on or indented trailers | Git treats them as prose and accepting them would make records ambiguous
• r-doctor72 996bcde — choosing either parent doctor list | each drops a runtime check required by the other incident
• r-fix067 a915af0 — treat an outside-repository file_path as a legitimate no-record result | only an in-repository query can truthfully establish that no records apply; silence would hide a broken matcher
• r-fix063h f0bb995 — leave hook runtime as a separate contradictory check | a fix-bearing installation check must not report ok or warn when the installed hook demonstrably fails
• r-fix063 0b8c496 — add the explicit refspec only after confirming the remote ref exists | setup would depend on today’s remote state and require another doctor run after the first notes push
• r-fix053 ecc4b90 — add a public mirrored boolean | the per-record sources array already models contributing channels, so a second representation would create drift
• r-fix055 43b40f8 — infer whether a quote supports Verified | deterministic text matching can prove presence, not that a check ran
• r-fix055 43b40f8 — downgrade harvested Verified to reconstructed | it preserves a citation-bearing assertion the verifier cannot substantiate
• r-fix054 664d4e2 — write X-Inherited-From into commit messages too | transport metadata would lengthen every preserved user-facing message merely to restore symmetry
• r-fix054 664d4e2 — stop squash-preserve from writing notes | it discards the mirror instead of fixing the query seam
• r-fix056 55cb8bc — reuse the injection omission list for blocked withholding | it includes prose-bearing Evidence and Expires, so it is not a safety boundary
• r-7b26f1 ec070ec — retrying the read inside the transaction | a write lock held across a subprocess is a worse failure than the one being fixed
• r-1e58d3 1fd0d53 — removing the recorded-path branch entirely | a clone is on no PATH and in no node_modules (ADR-0011), and that branch is the only thing that finds the CLI there
• r-1e58d3 1fd0d53 — hashing the recorded binary at install time | shasum is not guaranteed on every machine a hook runs on, and a check that silently no-ops is worse than the one being replaced
• r-9c74b3 68340e4 — withholding only in --json | a shell agent reads stdout, and the text form is what it reads
• r-4b17f8 7efba5c — retrying the read inside the transaction | a transaction holding a write lock while it shells out to git is a lock held across a subprocess
• r-7a48c3 b85d847 — a CONFLICTED lifecycle state for divergent declarations | see above — it would block work on records that have a correct answer
• r-8d51a6 27f73b0 — filtering blocked matches out of the result | the caller needs to know something matched; withholding is a rendering decision, made once
• r-8d51a6 27f73b0 — reusing exit 1 for "could not check" | 1 already means a broken invocation, and a hook that cannot tell a bad flag from an unreadable repository will treat both as noise
• r-2f7d94 a7673d0 — an allow-list of free-text keys | it is the shape of the original bug, and a new key would be unguarded until someone remembered
• r-2f7d94 a7673d0 — leaving the wording generic ("a trailer") | an operator needs to know which line to edit, and the key was available two frames up
• r-4e29b7 66829bb — folding this into the existing notes field | they are independent axes and can co-occur; one enum would have to enumerate the product
• r-4e29b7 66829bb — throwing on an unreadable repository | context runs from a hook on every edit, and an exception there is a broken editor rather than a refusal
• r-1c47e9 0e9930b — dropping the check | the two installation failures it exists for are real and were both invisible to configuration reads
• r-1c47e9 0e9930b — probing whichever file is newer | "which artifact is this installation" is a fact about the layout, not about timestamps
• r-3d92a8 f85101a — keeping the searches first and fixing the shim | the shim belongs to npm, not to us, and the version-skew problem survives the fix
• r-3d92a8 f85101a — a config-only hook check | it was written, it reported ok, and the hook failed on the next commit
• r-7c05e2 218ea28 — fetching notes automatically when the ref is missing | a query is a read, and silently reaching the network on a read is a surprise that belongs to git fetch
• r-7c05e2 218ea28 — leaving it to doctor | doctor is run by a person once, and the answer that misleads is the one an agent gets on every task
• r-7c05e2 218ea28 — a diagnostic string alone | the field it qualifies is records: [], and prose is not something a consumer can branch on
• r-9b31c7 e8d45fb — keeping the placeholder until author trust was configurable | the placeholder was the permissive direction, so waiting meant shipping the hole
• r-9b31c7 e8d45fb — withholding blocked payloads from the CLI too | a person reading a terminal can disbelieve a sentence; a tool result is retrieved fact
• r-9b31c7 e8d45fb — dropping blocked records from the MCP answer entirely | an agent that silently receives less than there is cannot notice, and cannot audit
• r-6c48b2 aaadedf — matching the whole file at edit time | the file contains everything the agent did not write, and GUARD-CANNOT-BLOCK measured prose surfaces producing false alarms specifically on compliant agents
• r-6c48b2 aaadedf — blocking on a match | the score bands overlap, measured
• r-6c48b2 aaadedf — running T-705 · guard route benchmark — measure the path SPEC §5 assigned to Ruled-out #37 without this pre-check | an arm that fires zero times measures nothing, and 120 runs is an expensive way to learn that
• r-5b9e37 010782c — baking the resolved path into the hook stub | hooks status is a byte comparison against commitMsgStub(), so every hook installed from a different checkout would report outdated forever
• r-5b9e37 010782c — an npx fallback | the existing comment is right — npx --no still queries the registry when the package is absent, putting a network call on every commit and breaking offline commits
• r-2f9c40 07f47ca — wiring guard into the plugin as a blocking hook | true and false positives occupy the same score band on real agent output, so the only precision-safe threshold catches 1 of 5 and every useful threshold blocks four compliant edits in twenty-five
• r-2f9c40 07f47ca — raising RECORD_ID_WEIGHT's threshold instead of gating the signal | the false alarms scored 1.0000, so no threshold below the maximum excludes them and the maximum excludes everything
• r-2f9c40 07f47ca — semantic matching to separate the populations | ADR-0002 keeps the core LLM-free and zero-cost, and B-04 · Optional embedding-search tier #31 registers embeddings as opt-in — nothing measured here justifies moving that into the core
• r-7a3e91 cf859e4 — inlining spec/SPEC.md and the schema into the bundle | SPEC.md would need a codegen step that itself needs a drift guard, and the package-root walk removes the reason to want it
• r-7a3e91 cf859e4 — replacing the tsc output with the bundle | test/cli.test.ts, test/hooks.test.ts and test/mcp.test.ts import dist internals by path
• r-4e9c72 a7a7e26 — fail when the index is missing or stale | it would be the first thing users learn to ignore, and then a real failure is quiet too
• r-9a5e17 6d68703 — let each command edit src/cli.ts | guaranteed conflict, and the conflict surfaces only after every worker has finished
• r-9a5e17 6d68703 — npx fallback in the hook stub | a network call on every commit, and offline commits start failing

Warnings (33)

• r-83d43117 8a49ddc (claim) — the containment check resolves the recorded path's directory physically and separately rejects a symlink at the final component, but does not protect a relative commitlore.bin with no path separator (a bare filename), which resolves against the hook's cwd and could coincide with the install root in a repository that installs commitlore against itself. That residual case sits in the same accepted-risk category as the severity note above: an attacker who can write that config key already has an equivalent, direct route.
• r-fix70a1 d707fc7 (claim) — add malicious and benign fixtures together when extending scanner patterns; false positives can make the defence unusable
• r-merge66 40e7987 (claim) — test/hooks.test.ts must keep both worktree git-path resolution and reference-integrity assertions
• r-fix760 fb8ba45 (claim) — unknown-only final paragraphs are treated as prose only when the source is a multi-parent commit
• r-wt77fix f77ef5d (claim) — git rev-parse --git-path may return a repository-relative path; resolve it against the test repository
• r-refint74 572f573 (claim) — exact commit and note mirrors remain one logical record; only divergent note payloads collide
• r-warn75 24c7cc8 (claim) — query several paths one at a time when rename history matters
• r-shallow66 60a8659 (claim) — shallow history remains advisory; query and guard exit-code semantics are unchanged
• r-doctor72 996bcde (claim) — keep commit-msg health bound to hook-runtime and keep inject-runtime as a separate known-good payload probe
• r-fix067 a915af0 (claim) — the accepted tool set is the union of the plugin and settings matchers; aligning those matchers remains Shallow clone: answers from 1 commit of history without saying history is truncated #66
• r-fix063 0b8c496 (claim) — the wildcard fetches every ref under refs/notes, including notes owned by other tools
• r-fix055 43b40f8 (claim) — Verified remains valid protocol vocabulary for facts recorded from actual command or test execution; only harvest refuses it
• r-7b26f1 ec070ec (claim) — the concurrency test is deterministic rather than sleep-based, so it proves the transaction boundary and not the absence of every race
• r-1e58d3 1fd0d53 (claim) — COMMITLORE_BIN still accepts any executable, deliberately — a harness must be able to aim the hook at a specific build. It is now reported rather than restricted
• r-9c74b3 68340e4 (claim) — context now prints [blocked] beside a record whose payload is gone, which is more visually alarming than the old silent leak. That is the intended direction — a withheld record should be conspicuous — but it changes what a clean repository's output looks like the first time someone commits a Warn: that trips a pattern by accident
• r-1a63f5 2bb4993 (claim) — "CI is green" was said five times today against a red CI, including in the commit that introduced the rule saying to check CI before saying it. The rule is in docs/RELEASE-GATE.md §5 and it was not followed by its own author. This commit is not claiming CI is green; that claim comes after the run reports
• r-4b17f8 7efba5c (claim) — deleteNoteRows opens its own transaction inside the new outer one. better-sqlite3 nests these as savepoints; node:sqlite has neither, so ADR-0012's migration must flatten this rather than assume it works
• r-7a48c3 b85d847 (claim) — these two changes were developed concurrently in one worktree and share a built dist/. Splitting them would leave one commit whose dist/ did not match its src/, so they land together and are described together
• r-5c92e0 73b1285 (claim) — the delegate reported "943 passed" for a suite whose baseline is 1108. It ran while another task was writing to the same worktree and collected a partial set. The real count, verified here on a quiet tree, is 1109 across 31 files — but a delegated test count is now a claim to check, not a result to accept
• r-8d51a6 27f73b0 (claim) — guard stays advisory. Nothing here makes it block, and GUARD-CANNOT-BLOCK still holds — the point is that it no longer lies about what it saw
• r-2f7d94 a7673d0 (claim) — Evidence: and Expires: are now scanned. Both usually hold paths and dates, so a false positive there withholds a legitimate record. No case is known; a legitimate record carrying a path, a URL and a date was checked and passes
• r-4e29b7 66829bb (claim) — historyAvailability spends two git invocations per query. Both are metadata reads, but this is a hot path and nothing measures it yet
• r-1c47e9 0e9930b (claim) — this is the second defect in three days from assuming the development checkout is the deployment. The first was exec node in the run script
• r-3d92a8 f85101a (claim) — hook-runtime executes the hook on every doctor run. The probe message is valid so nothing is written, but it is no longer a read-only command
• r-3d92a8 f85101a (claim) — the check pins PATH to /usr/bin:/bin, which assumes git is there. On a system where it is not, this reports a hook failure that is really a probe failure
• r-7c05e2 218ea28 (claim) — notesAvailability runs git rev-parse and up to two git config reads on every query. Config-only, no network, but it is not free on a hot path
• r-9b31c7 e8d45fb (claim) — the default is now fail-closed on every route — with no --trusted-author, every Warn: grades claim. That is SPEC §7 and it is what inject already did, but a user who saw [directive] yesterday will see [claim] today
• r-6c48b2 aaadedf (claim) — recall here is against four re-proposals. It is a go/no-go signal for whether an arm has anything to measure, not an effect size
• r-5b9e37 010782c (claim) — commitlore.bin and commitlore.node are local config, so they do not survive a fresh clone of a repository whose hook was installed elsewhere — re-run hooks install there, which is what the failure message now says
• r-2f9c40 07f47ca (claim) — --require-content changes precision, not recall; it removes a false-alarm class and catches nothing new
• r-7a3e91 cf859e4 (claim) — hardcoding ../ counts back to the package root is what broke this — new code reads assets through installedPath(), never through import.meta.url
• r-4e9c72 a7a7e26 (claim) — doctor reads the index read-only and never rebuilds it -- a diagnostic that repairs on sight hides how often the repair was needed
• r-9a5e17 6d68703 (claim) — commands are advertised in --help only once they work -- test/cli.test.ts holds the landed and unlanded lists, and moving a name between them belongs in the commit that wires it

git log --follow accepts exactly one pathspec, so renames are not followed for 17 paths; query one path at a time to follow its rename chain

withheld the content of 2 record(s) graded blocked: a Ruled-out, Verified trailers matching an injection pattern is reported, never quoted (SPEC §7)

_{Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR.}