timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

pretooluse-merge-consent.sh resolves its verdict classifier from $CLAUDE_PROJECT_DIR — an env var as a security input #858

New Issue
Closed
opened 2026-08-27 22:01:23 +02:00 by timothy · 2 comments
timothy commented 2026-08-27 22:01:23 +02:00
Owner
Copy Link
Copy Source

Found by cold review while working #787, and labelled there as pre-existing rather than folded in.

.claude/hooks/pretooluse-merge-consent.sh resolves the H10 verdict classifier as:

verdict_script="${CLAUDE_PROJECT_DIR:-.}/scripts/check-review-verdict.sh"

$CLAUDE_PROJECT_DIR is an environment variable, and the value it selects decides whether a merge is
granted. A wrong value pointing at a tree that happens to contain an executable
scripts/check-review-verdict.sh classifies THIS PR's comments with ANOTHER checkout's code. A
missing path only asks, so the failure is quiet exactly where it is worst.

#787 hit the same question for its own arm and resolved it the other way: the guard-scope freshness
checker and its snapshot both bind to $repo_root, derived from the hook's own ${BASH_SOURCE[0]},
with the reasoning written down beside it. The two resolutions now disagree inside one file, which is
the state most likely to be "tidied" toward the weaker one.

Not urgent, and say why: $CLAUDE_PROJECT_DIR is set by the harness to the project directory, so
today both resolve to the same tree. This is about removing an unsound input, not about a live
defect.

Scope decision (2026-08-30, during implementation)

The original first box read "verdict_script (and any sibling using the same pattern)". Working it
established that the parenthesis covers a genuinely different class, so it is split out to #891
rather than carried as a box this PR would leave unticked:

  • verdict_script selects a predicate whose content changes a decision → fixed here.
  • ETV_HOOK_FIRE_LIB selects the telemetry logger → not fixed here. A wrong log destination is
    not a wrong verdict, and the line is byte-identical across all 13 tracked hooks, so editing this
    one copy would manufacture the one-of-many-copies divergence that extracting
    scripts/lib/branch-rule-classifier.jq exists to prevent. Changing it is a cross-hook decision with
    its own population and its own review — that is #891.

The boundary itself ("does the resolved file's CONTENT change a decision") is recorded in the new
process.hook-resolves-inputs-from-repo-root, so the split is a stated rule rather than a
convenient stopping point.

Also worth recording: the obvious security framing does not survive contact, and the record says so.
$CLAUDE_PROJECT_DIR already names the hook binary in .claude/settings.json, so a hostile value has
chosen which hook runs and the gate is moot long before either inner path is read. The defensible
threat is worktree version skew — routine here — where a sibling checkout's copy of the H10
grammar classifies this PR.

Done-when

  • verdict_script resolves from $repo_root rather than $CLAUDE_PROJECT_DIR
  • The existing merge-consent tests still exercise the arms they name — several set
    CLAUDE_PROJECT_DIR deliberately, so check each still reaches its arm rather than passing on a
    changed path
  • A test pins the new behaviour, with a NEGATIVE CONTROL — an allow is also what an inert decoy
    produces, so the decoy must be shown to flip the decision when it genuinely is $repo_root
  • The reasoning is recorded once, beside both resolutions, rather than in two places that can drift
  • The ETV_HOOK_FIRE_LIB sibling is either fixed or explicitly scoped out with its reason recorded
    and a follow-up filed
  • Adversarial review passed
Found by cold review while working #787, and labelled there as pre-existing rather than folded in. `.claude/hooks/pretooluse-merge-consent.sh` resolves the H10 verdict classifier as: ```sh verdict_script="${CLAUDE_PROJECT_DIR:-.}/scripts/check-review-verdict.sh" ``` `$CLAUDE_PROJECT_DIR` is an environment variable, and the value it selects decides whether a merge is granted. A wrong value pointing at a tree that happens to contain an executable `scripts/check-review-verdict.sh` classifies THIS PR's comments with ANOTHER checkout's code. A *missing* path only asks, so the failure is quiet exactly where it is worst. #787 hit the same question for its own arm and resolved it the other way: the guard-scope freshness checker and its snapshot both bind to `$repo_root`, derived from the hook's own `${BASH_SOURCE[0]}`, with the reasoning written down beside it. The two resolutions now disagree inside one file, which is the state most likely to be "tidied" toward the weaker one. **Not urgent, and say why:** `$CLAUDE_PROJECT_DIR` is set by the harness to the project directory, so today both resolve to the same tree. This is about removing an unsound *input*, not about a live defect. ## Scope decision (2026-08-30, during implementation) The original first box read "`verdict_script` **(and any sibling using the same pattern)**". Working it established that the parenthesis covers a genuinely different class, so it is **split out to #891** rather than carried as a box this PR would leave unticked: - `verdict_script` selects a **predicate whose content changes a decision** → fixed here. - `ETV_HOOK_FIRE_LIB` selects the **telemetry logger** → not fixed here. A wrong log destination is not a wrong verdict, and the line is byte-identical across **all 13 tracked hooks**, so editing this one copy would manufacture the one-of-many-copies divergence that extracting `scripts/lib/branch-rule-classifier.jq` exists to prevent. Changing it is a cross-hook decision with its own population and its own review — that is #891. The boundary itself ("does the resolved file's CONTENT change a decision") is recorded in the new `process.hook-resolves-inputs-from-repo-root`, so the split is a stated rule rather than a convenient stopping point. Also worth recording: the obvious *security* framing does not survive contact, and the record says so. `$CLAUDE_PROJECT_DIR` already names the hook binary in `.claude/settings.json`, so a hostile value has chosen which hook runs and the gate is moot long before either inner path is read. The defensible threat is **worktree version skew** — routine here — where a sibling checkout's copy of the H10 grammar classifies this PR. ## Done-when - [x] `verdict_script` resolves from `$repo_root` rather than `$CLAUDE_PROJECT_DIR` - [x] The existing merge-consent tests still exercise the arms they name — several set `CLAUDE_PROJECT_DIR` deliberately, so check each still reaches its arm rather than passing on a changed path - [x] A test pins the new behaviour, with a NEGATIVE CONTROL — an `allow` is also what an inert decoy produces, so the decoy must be shown to flip the decision when it genuinely is `$repo_root` - [x] The reasoning is recorded once, beside both resolutions, rather than in two places that can drift - [x] The `ETV_HOOK_FIRE_LIB` sibling is either fixed or explicitly scoped out with its reason recorded and a follow-up filed - [x] Adversarial review passed
timothy added the priority: mediumsecurity labels 2026-08-27 22:01:38 +02:00
timothy commented 2026-08-30 09:52:03 +02:00
Author
Owner
Copy Link
Copy Source

Claiming as a bundle with #859 — Claude Code session, worktree ~/orca/workspaces/ersatztv/main-3, branch fix/858-859-merge-consent-hook-residuals off origin/main @ 58681b3a7.

Bundle rationale (Bundles axis (c), shared subject): #858 and #859 were both split out of #787's cold review one second apart, and both land in .claude/hooks/pretooluse-merge-consent.sh / scripts/lib/branch-rule-classifier.jq and the same merge-consent test suite. Working them separately would put two sessions in one file.

Parallel-session note: three sessions started simultaneously this morning and all three initially claimed #887. I yielded #887 to the earliest claim and posted a 3-way split there (#887 → session 22984, #855 → main-2, #858+#859 → me), so this claim is de-conflicted rather than a race.

Pre-claim checks clear: no remote branch matching *858*/*859*/*787*, no open PR referencing either, no prior comments on either issue, git fetch origin main → 58681b3a7.

Claiming as a **bundle with #859** — Claude Code session, worktree `~/orca/workspaces/ersatztv/main-3`, branch `fix/858-859-merge-consent-hook-residuals` off `origin/main` @ `58681b3a7`. Bundle rationale (`Bundles` axis (c), shared subject): #858 and #859 were both split out of #787's cold review one second apart, and both land in `.claude/hooks/pretooluse-merge-consent.sh` / `scripts/lib/branch-rule-classifier.jq` and the same merge-consent test suite. Working them separately would put two sessions in one file. Parallel-session note: three sessions started simultaneously this morning and all three initially claimed #887. I yielded #887 to the earliest claim and posted a 3-way split there (#887 → session 22984, #855 → `main-2`, #858+#859 → me), so this claim is de-conflicted rather than a race. Pre-claim checks clear: no remote branch matching `*858*`/`*859*`/`*787*`, no open PR referencing either, no prior comments on either issue, `git fetch origin main` → `58681b3a7`.
timothy added the in-progress label 2026-08-30 09:52:11 +02:00
timothy commented 2026-08-30 13:35:14 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #897 (squash cf5f42edf). verdict_script now resolves from $repo_root. But the issue's parenthetical — "and any sibling using the same pattern" — turned out to name the more serious half, and that is what this record is mostly about.

Root cause: two resolutions of one question inside one file. #787 bound the guard-scope checker and its snapshot to $repo_root and wrote the reasoning beside them; the H10 verdict classifier, added at #629, kept ${CLAUDE_PROJECT_DIR:-.}. The file's own thesis is that two answers to one question is the state most likely to be tidied toward the weaker one, and it was carrying exactly that.

The finding the issue did not anticipate. The first draft of the fix exempted ETV_HOOK_FIRE_LIB as "telemetry, not a predicate — a wrong log destination is not a wrong verdict". Cold review refuted that by execution: the line is . -SOURCED, so it is code, running before stdin is read and before decide exists. Measured:

$ echo '{"tool_input":{"method":"merge",...}}' | CLAUDE_PROJECT_DIR=<decoy> bash .claude/hooks/pretooluse-merge-consent.sh
{"hookSpecificOutput":{...,"permissionDecision":"allow","permissionDecisionReason":"DECOY LOGGER GRANTED THIS"}}

The entire H6/H10 gate bypassed before it ran. Hardening verdict_script while leaving that would have been decorative, so this hook's copy is self-located too.

Reachability, stated honestly — because the obvious security framing does not survive contact. $CLAUDE_PROJECT_DIR also names the hook binary in .claude/settings.json, so for PreToolUse hooks the two roots agree by construction and this is consistency rather than repair. It does not hold for the prepush family: husky launches them as ./.claude/hooks/…, a relative path independent of the variable. Measured — a decoy tree's code executed inside prepush-donewhen.sh. That is the reachable case, and it is ordinary worktree usage, not an attack.

Scope decision: the ETV_HOOK_FIRE_LIB line is byte-identical across all 13 tracked hooks. One copy is fixed (the file whose gate this PR hardens); the other 12 are #891 (priority: high + security), left whole because diverging byte-identical copies is the defect that extracting scripts/lib/branch-rule-classifier.jq exists to end. scripts/hook-fire-log.sh's own USAGE recipe — what a new hook copies — was corrected too, so the sweep does not race new instances. The original first Done-when box was split rather than ticked on a technicality; that split is recorded in the issue body above.

Decisions/conventions changed: new record process.hook-resolves-inputs-from-repo-root. Its rule is not "an env var is a security input" (refutable, and it got refuted) but: classify a path by how it is CONSUMED, never by what it is called — a sourced path is code whatever its purpose, and the threat is launcher divergence, not an attacker. Both inline sites now cite the record instead of arguing it twice.

Reusable knowledge:

  • A first draft of this record granted a telemetry exemption that does not exist. The lesson generalises past this variable: a path's stated purpose grants it no protection.
  • A binding test needs a negative control. "The env var was ignored" and "the decoy was inert" produce the same passing outcome. Both env-var tests re-run the same decoy as genuine $repo_root and assert the decision flips.
  • test_hook_fire_log.py::test_the_stripper_removes_EXACTLY_the_preamble_and_nothing_else permits only its own recognised lines inside the instrumentation preamble — explanatory comments go above it, not beside the assignment.

Verification: 1377 passed / 2 skipped; 11 declared mutants, 11 detected; CI green first run. Four cold rounds (1 cross-family Codex, 3 worktree-isolated Claude) + a bounded prose check. Round 4 confirmed this issue's Done-when box 2 explicitly: every pre-existing test that sets CLAUDE_PROJECT_DIR also passes hook_path=copied_hook, so each still reaches the arm it names after the resolution change.

Deferred: #891 (the remaining 12 hooks + hook-fire-log.sh:460's internal resolution). #895 unrelated to this issue's subject.

Docs updated: new decision record + regenerated catalog; docs/remote-state-inventory.md. No skill or vault impact — this is fork code, not homelab operations.

## Closing record **Outcome:** Shipped in PR #897 (squash `cf5f42edf`). `verdict_script` now resolves from `$repo_root`. **But the issue's parenthetical — "and any sibling using the same pattern" — turned out to name the more serious half**, and that is what this record is mostly about. **Root cause:** two resolutions of one question inside one file. #787 bound the guard-scope checker and its snapshot to `$repo_root` and wrote the reasoning beside them; the H10 verdict classifier, added at #629, kept `${CLAUDE_PROJECT_DIR:-.}`. The file's own thesis is that two answers to one question is the state most likely to be tidied toward the weaker one, and it was carrying exactly that. **The finding the issue did not anticipate.** The first draft of the fix exempted `ETV_HOOK_FIRE_LIB` as "telemetry, not a predicate — a wrong log destination is not a wrong verdict". Cold review refuted that **by execution**: the line is `. `-SOURCED, so it is code, running before stdin is read and before `decide` exists. Measured: ``` $ echo '{"tool_input":{"method":"merge",...}}' | CLAUDE_PROJECT_DIR=<decoy> bash .claude/hooks/pretooluse-merge-consent.sh {"hookSpecificOutput":{...,"permissionDecision":"allow","permissionDecisionReason":"DECOY LOGGER GRANTED THIS"}} ``` The entire H6/H10 gate bypassed before it ran. Hardening `verdict_script` while leaving that would have been decorative, so this hook's copy is self-located too. **Reachability, stated honestly** — because the obvious security framing does not survive contact. `$CLAUDE_PROJECT_DIR` also names the hook binary in `.claude/settings.json`, so for **PreToolUse hooks the two roots agree by construction** and this is consistency rather than repair. It does **not** hold for the prepush family: husky launches them as `./.claude/hooks/…`, a relative path independent of the variable. Measured — a decoy tree's code executed inside `prepush-donewhen.sh`. That is the reachable case, and it is ordinary worktree usage, not an attack. **Scope decision:** the `ETV_HOOK_FIRE_LIB` line is byte-identical across all 13 tracked hooks. One copy is fixed (the file whose gate this PR hardens); the other 12 are **#891** (`priority: high` + `security`), left whole because diverging byte-identical copies is the defect that extracting `scripts/lib/branch-rule-classifier.jq` exists to end. `scripts/hook-fire-log.sh`'s own USAGE recipe — what a new hook copies — was corrected too, so the sweep does not race new instances. The original first Done-when box was split rather than ticked on a technicality; that split is recorded in the issue body above. **Decisions/conventions changed:** new record **`process.hook-resolves-inputs-from-repo-root`**. Its rule is *not* "an env var is a security input" (refutable, and it got refuted) but: **classify a path by how it is CONSUMED, never by what it is called** — a sourced path is code whatever its purpose, and the threat is launcher divergence, not an attacker. Both inline sites now cite the record instead of arguing it twice. **Reusable knowledge:** - **A first draft of this record granted a telemetry exemption that does not exist.** The lesson generalises past this variable: a path's stated purpose grants it no protection. - **A binding test needs a negative control.** "The env var was ignored" and "the decoy was inert" produce the *same* passing outcome. Both env-var tests re-run the same decoy as genuine `$repo_root` and assert the decision flips. - `test_hook_fire_log.py::test_the_stripper_removes_EXACTLY_the_preamble_and_nothing_else` permits only its own recognised lines inside the instrumentation preamble — explanatory comments go **above** it, not beside the assignment. **Verification:** 1377 passed / 2 skipped; 11 declared mutants, 11 detected; CI green first run. Four cold rounds (1 cross-family Codex, 3 worktree-isolated Claude) + a bounded prose check. Round 4 confirmed this issue's Done-when box 2 explicitly: every pre-existing test that sets `CLAUDE_PROJECT_DIR` also passes `hook_path=copied_hook`, so each still reaches the arm it names after the resolution change. **Deferred:** #891 (the remaining 12 hooks + `hook-fire-log.sh:460`'s internal resolution). #895 unrelated to this issue's subject. **Docs updated:** new decision record + regenerated catalog; `docs/remote-state-inventory.md`. No skill or vault impact — this is fork code, not homelab operations.
timothy removed the in-progress label 2026-08-30 13:35:21 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels priority: medium security
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#858