fix(claude-ops): load hook-utils.sh from script-relative root, remove FLEET_STATE_HOOK_UTILS source footgun#907
Conversation
fleet-state.sh sourced hook-utils.sh from a caller-supplied FLEET_STATE_HOOK_UTILS path, guarded only by an existence check. Any process able to set that env var (a project settings.json env block, an inherited shell, another hook) could redirect `source` at an arbitrary file executed with the script's ambient permissions. hook-utils.sh is a fixed sibling shipped with the plugin, so resolve it unconditionally from the script-relative plugin root and drop the override entirely: once the path is script-relative, an override could only ever equal that trusted default (both test call sites already resolved to it), so the seam was dead. CLAUDE_PLUGIN_ROOT is no longer consulted for this sibling file; it is still used for marketplace self-resolution. No behavior change in production. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 35s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
SummaryThis is a clean, minimal security fix. The root cause, chosen remedy, and trade-off analysis are all sound. No blocking issues; one suggestion below. Security fix —
|
|
Claude finished @kyle-sexton's task in 2m 24s —— View job Security Review — PR #907
No security issues found in the changes. Fix assessmentThe vulnerability was an arbitrary-code-execution surface: HOOK_UTILS="${FLEET_STATE_HOOK_UTILS:-${CLAUDE_PLUGIN_ROOT:-$PLUGIN_ROOT_DEFAULT}/hooks/hook-utils.sh}"Any process that could set The fix at fleet-state.sh:67 resolves this correctly: HOOK_UTILS="$PLUGIN_ROOT_DEFAULT/hooks/hook-utils.sh"
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
PLUGIN_ROOT_DEFAULT="$(cd "$SCRIPT_DIR/../../.." && pwd)"This is the canonical pattern for env-independent path resolution in bash. The fix is root-cause correct: there is no attacker-reachable Remaining env overrides (pre-existing, not changed in this PR)The four remaining
|
Lock in the security fix with a regression case: a caller-supplied FLEET_STATE_HOOK_UTILS pointing at a decoy script must not be sourced. A future refactor that re-introduces an env-based override would re-source the decoy and surface its marker, failing this case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 44s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This synchronize push adds two commits on top of the earlier state reviewed previously. Summary of what changed:
Security fix —
|
|
Claude finished @kyle-sexton's task in 1m 27s —— View job Security Review — PR #907
No security issues found in these changes. Primary fix — arbitrary-file
|
|
Claude finished @kyle-sexton's task in 2m 52s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
Security fix —
|
|
Claude finished @kyle-sexton's task in 2m 8s —— View job Security Review — PR #907
No security issues found in these changes. Primary fix — arbitrary-file
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eff04eb69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The script-relative root computation used plain `cd`/`pwd`/`dirname`. Bash imports environment-exported shell functions (BASH_FUNC_cd%% and friends) before a script runs, so an inherited hostile environment could shadow `cd`/`pwd` and make PLUGIN_ROOT_DEFAULT resolve to an attacker tree — redirecting the `source` of hook-utils.sh at an arbitrary file even with CLAUDE_PLUGIN_ROOT set to the real plugin root. Resolve the root with `builtin cd`/`builtin pwd` and `command dirname` so exported functions of those names cannot hijack the computation. Add a regression case delivering `BASH_FUNC_cd%%` through the child environment and asserting the decoy hook-utils.sh is never sourced; verified it fails on the pre-fix resolution and passes after.
|
Claude finished @kyle-sexton's task in 2m 18s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This review covers the third commit ( Root-resolution hardening —
|
|
Claude finished @kyle-sexton's task in 1m 37s —— View job Security Review — PR #907
No security issues remain in the current HEAD ( The three commits in this PR form a complete, layered fix for the arbitrary-file Codex P1 —
|
|
Classification of the automated review feedback on head
Fresh review/security-review jobs will run against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e84d9900a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The hook-utils resolution used `command dirname` to derive the script's
own directory. `command` suppresses shell-function lookup but still
resolves the `dirname` binary through PATH, so an inherited PATH prefixed
with an attacker-controlled directory could supply a hostile `dirname`
that returns an arbitrary path, redirecting the subsequent `source` at a
decoy hook-utils.sh — arbitrary code execution on the resolution path.
Derive the directory with `${BASH_SOURCE[0]%/*}` parameter expansion
instead. Combined with the existing `builtin cd`/`builtin pwd`, the
resolution now consults nothing external and nothing shadowable, so the
plugin-shipped hook-utils.sh is always sourced from the real script
location regardless of a hostile PATH or exported shell functions.
Add a regression case that plants a hostile `dirname` at the front of
PATH and asserts the decoy is never sourced (verified non-vacuous: it is
sourced under the prior `command dirname` resolution).
|
Claude finished @kyle-sexton's task in 2m 38s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This review covers commit Codex P1 — PATH-based
|
| Channel | Defence |
|---|---|
Exported shell function BASH_FUNC_cd%% |
builtin cd bypasses function lookup |
Exported shell function BASH_FUNC_pwd%% |
builtin pwd bypasses function lookup |
Hostile dirname binary in PATH |
${BASH_SOURCE[0]%/*} — no PATH consultation at all |
The comment at L58–66 names all three vectors and the precise defence for each. Future maintainers have the context they need not to inadvertently reintroduce an external call here.
Regression test — fleet-state.test.sh L617–651
Well-constructed and non-vacuous. Key points verified:
The delivery channel is correct. run_state expands "$@" before the bash "$SCRIPT" invocation in its env call. Passing "PATH=$attack_bin_dir" as the second positional argument makes env see it as a NAME=VALUE assignment and set PATH to exactly $attack_bin_dir in the child environment — the genuine attack channel.
Non-vacuousness is correctly claimed. With the old command dirname code, the hostile dirname at $attack_bin_dir/dirname would be found by the PATH lookup, return $case_dir/evilroot/skills/plugins/scripts, and make PLUGIN_ROOT_DEFAULT resolve to the attacker tree. The source of $case_dir/evilroot/hooks/hook-utils.sh happens before the jq check, so the PWNED marker appears in captured output even though jq is missing and the script then exits 2. With the new parameter-expansion code, no external dirname is consulted, so the real hook-utils.sh loads and the PWNED marker never appears.
The bash copy and Windows DLL loop are correct. When env encounters PATH=$attack_bin_dir before the bash command name, it replaces PATH before the PATH lookup for bash itself — so the bash binary must be in $attack_bin_dir. The test copies it there. The shopt -s nullglob DLL loop is a no-op on Linux and defensive for potential Windows CI use; no cost.
The hostile dirname script is anatomically correct. It ignores its argument (printf "%%s\n" "$decoy_scripts" — %s hardcoded, argument discarded) and returns the decoy path unconditionally. This is precisely the attack Codex described.
One minor note: unlike the cd-shadow test at L608–615, this new case does not have an assert_exit 0 guard before the PWNED check. It wouldn't be vacuous without it (see above — the hostile dirname fires before jq is checked), but the symmetry is worth noting. Adding rc=$?; assert_exit "hostile PATH dirname ignored: runs to completion (exit 0)" 0 "$rc" after the out=$(...) line would make the pattern uniform and give a clear signal if the script crashes for a different reason. Not a blocking issue; a minor hygiene point.
Remaining surface — source function shadowing (pre-existing, unchanged)
fleet-state.sh L84 still calls source "$HOOK_UTILS" without builtin. An exported BASH_FUNC_source%% could shadow it. This is a meaningfully higher-bar attack — the hostile function must discard the correctly-resolved trusted path and source a separate file, affecting all source calls in the invocation context — and was noted as out-of-scope in the prior review. Calling it out again for completeness; builtin source "$HOOK_UTILS" would close it at zero cost if the team wants to be comprehensive. Add builtin →
Overall
All three env-reachable attack channels in the root-resolution block are now closed. The fix is idiomatically correct Bash (parameter expansion is the canonical way to strip a path component without external tools), the comment is precise, and the regression test is non-vacuous and exercises the genuine delivery channel. The single non-blocking hygiene note (assert_exit symmetry) is cosmetic.
No blocking issues. The do-not-merge / #853 dependency remains a process concern only.
|
Claude finished @kyle-sexton's task in 3m 25s —— View job Security Review — PR #907
Two findings. The primary fix (env-var override removal, IMPORTANT —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8d71de8ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ion shadowing The `source "$HOOK_UTILS"` call could be hijacked by an inherited environment exporting a shell function named `source` (BASH_FUNC_source%%): bash imports the function before the script runs, so `source` invokes the attacker's function instead of the builtin. The function receives the correctly resolved real path but can ignore it and run arbitrary code, defeating the trusted-path resolution entirely. Use `builtin source` so the exported function is bypassed and the resolved hook-utils.sh is always sourced by the real builtin. This completes the function-shadowing defense already applied to cd/pwd on the resolution path — the source line is the actual execution point. Add a regression case exporting BASH_FUNC_source%% that asserts the decoy never runs (verified non-vacuous: it runs under a plain `source`).
|
Claude finished @kyle-sexton's task in 5m 14s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This review covers the latest commit
|
| Call site | Guard | Defeated by |
|---|---|---|
SCRIPT_DIR=$(builtin cd ... && builtin pwd) |
BASH_FUNC_cd%% / BASH_FUNC_pwd%% |
BASH_FUNC_builtin%% returning attacker tree from pwd |
builtin source "$HOOK_UTILS" |
BASH_FUNC_source%% |
BASH_FUNC_builtin%% ignoring the resolved path and sourcing decoy |
Fix — one line before the first builtin call (L72):
# Clear any inherited shell function named `builtin`; BASH_FUNC_builtin%% in
# the environment shadows the builtin keyword itself and would defeat every
# subsequent `builtin X` guard in this script.
unset -f builtin 2>/dev/null || trueThis is the correct fence for this vector. The only way to defeat it is to also export BASH_FUNC_unset%%, making it a two-variable attack — and unset approaches special-built-in territory (listed as a POSIX special built-in, and more scrutinized than builtin). The practical depth of in-script defense ends here; beyond this, the defense must be at the invocation layer (stripping BASH_FUNC_* from the env before launching the script). Acknowledging that limit in a comment alongside the unset -f is appropriate.
Regression test to add (mirrors the existing four security cases):
CASE_NUM=$((CASE_NUM + 1))
case_dir=$(new_case_dir)
write "$case_dir/known_marketplaces.json" '{"market1": {"source": {"source": "github", "repo": "example/market1"}, "installLocation": "z", "lastUpdated": "2026-01-01T00:00:00Z"}}'
write "$case_dir/catalog/market1.json" '{"plugins": []}'
ARGS=(--marketplace market1)
# An inherited function named `builtin` would shadow the builtin keyword and
# receive every `builtin cd / pwd / source` call — defeating all prior guards.
out=$(run_state "$case_dir" 'BASH_FUNC_builtin%%=() { echo "PWNED-BUILTIN-SHADOW"; }')
rc=$?
assert_exit "exported builtin shadow ignored: runs to completion (exit 0)" 0 "$rc"
case "$out" in
*PWNED-BUILTIN-SHADOW*) fail "exported builtin shadow ignored: hijacked builtin must NOT intercept source" "decoy marker present in output" ;;
*) pass "exported builtin shadow ignored: builtin function cleared at script start" ;;
esacAdd unset -f builtin and test →
Minor — two of the four security tests missing assert_exit 0 guard
The PATH dirname test (L647–651) and the source-shadow test (L669–673) lack the rc=$?; assert_exit ... 0 "$rc" guard present in the first two security cases. The PATH dirname case is the more notable gap: $attack_bin_dir contains only bash (and DLLs), so jq is absent and the script always exits 2 — the decoy IS sourced before the jq check (at L82–91, before L93), so the PWNED marker would appear in the attack-success path and the test is not fully vacuous, but the exit-2 prevents confirming the script completed normally. Worth addressing for consistency if assert_exit 0 is added to the builtin test above.
All prior commits — no new findings
The chain from commit 4188519 through 4505929 is correct: env-var override removed, builtin cd/builtin pwd guards added, ${BASH_SOURCE[0]%/*} eliminates PATH dependency, builtin source closes the source-shadow channel. Each has a non-vacuous regression test. No new injection surfaces are introduced.
The do-not-merge / #853 dependency remains a process concern only.
|
Claude finished @kyle-sexton's task in 4m 34s —— View job Security Review — PR #907
One security finding. The four previously reviewed vectors (env-var override, IMPORTANT —
|
|
Re: the awareness note that
Fixed in
This closes the last shadowing gap on the resolution+source path: |
… honestly The prior comments claimed the builtin/parameter-expansion resolution made hook-utils.sh sourced from the real location "regardless of a hostile environment." That overclaims: an exported `BASH_FUNC_builtin%%` shadows `builtin` itself, and `BASH_ENV` executes before the script's first line — neither is stoppable in-script. Both sit at the same environment-trust boundary as the PATH-resolved jq/git/tr the script already relies on. Narrow the comments to the property that actually holds: resolution uses no external process and the builtin prefixes cover the common single-function cd/pwd/source shadow, with the broader environment-control vectors named as the out-of-scope boundary they are. No behavior change.
|
Claude finished @kyle-sexton's task in 2m 53s —— View job Claude finished @kyle-sexton's task —— PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This review covers the final commit Comment hardening —
|
|
Claude finished @kyle-sexton's task in 3m 27s —— View job Security Review — PR #907
One SUGGESTION. No CRITICAL or IMPORTANT findings in HEAD ( The primary vulnerability (arbitrary-file Closed attack surfaces (confirmed correct)
SUGGESTION —
|
|
🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate. |
Resolve claude-ops version/changelog collision from concurrent landings: restack the branch's 0.17.5 security bump to 0.18.2 (next patch above main's landed 0.18.1) and reorder CHANGELOG so the fleet-state hook-utils.sh security entry sits newest-on-top above main's 0.18.1 and 0.18.0 entries.
|
Claude finished @kyle-sexton's task in 2m 38s —— View job PR Review — fix(claude-ops): load hook-utils.sh from script-relative root
This review covers the full current HEAD, which is the merge commit ( Security fix chain — summary across all commitsThe four-layer defence is correct and complete for the stated threat model:
The NOTABLE — CHANGELOG 0.18.2 entry describes a superseded implementationThe entry reads:
But the actual implementation ( script_src="${BASH_SOURCE[0]}"
case "$script_src" in
*/*) script_src_dir="${script_src%/*}" ;;
*) script_src_dir="." ;;
esac
SCRIPT_DIR="$(builtin cd "$script_src_dir" && builtin pwd)"The MINOR — two of the four security tests missing
|
|
Claude finished @kyle-sexton's task in 2m 47s —— View job Security Review — PR #907
One SUGGESTION. No CRITICAL or IMPORTANT findings in HEAD ( The four attack surfaces documented across the commit chain are correctly closed and mechanically tested. No new injection surfaces are introduced. Closed attack surfaces (all confirmed correct)
SUGGESTION — CHANGELOG entry describes
|
|
Classification of the two automated review summaries on head
Why no fix is pushed on this pass:
For the record, both summaries independently agree the Blockers this pass: (a) |
Resolve the claude-ops version/CHANGELOG collision: main landed claude-ops 0.18.2 (shared hook-utils.sh git-option parser sync, #740) while this branch also targeted 0.18.2 for its fleet-state FLEET_STATE_HOOK_UTILS source hardening. Restack this branch's Security entry to 0.18.3 (plugin.json + CHANGELOG), keeping main's 0.18.2 entry intact. fleet-state.test.sh passes (27/27) on the merged tree.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
Summary
fleet-state.shsourcedhook-utils.shfrom a caller-suppliedFLEET_STATE_HOOK_UTILSpath, guarded only by an existence check(
[[ -f "$HOOK_UTILS" ]]). Any process able to set that env var — a project.claude/settings.jsonenv block, an inherited shell environment, another hookthat exports it — could redirect
sourceat an arbitrary file, executed withthe script's ambient permissions. The guard checked only that the path existed,
never that it was the trusted default.
This removes the override entirely and loads
hook-utils.shunconditionallyfrom the script-relative plugin root.
Fix
hook-utils.shis a fixed sibling shipped inside the plugin, always at<plugin-root>/hooks/hook-utils.shrelative to the script. The fix resolves itfrom the script's own location (
PLUGIN_ROOT_DEFAULT, derived fromBASH_SOURCE), which no environment variable can influence:HOOK_UTILS="$PLUGIN_ROOT_DEFAULT/hooks/hook-utils.sh"CLAUDE_PLUGIN_ROOTis no longer consulted for this sibling file (it equals thescript-relative root in production, and tests set it to a fake value to
exercise marketplace self-resolution). It remains in use for marketplace
self-resolution further down the script — that concern legitimately needs the
install-metadata env var; locating a shipped sibling file does not.
Options considered
The issue proposed two remediations; I document both and why Option A wins:
exists in this repo's convention; the test harness sets the individual
FLEET_STATE_*override vars directly. Adding one would be a new abstractioninvented solely to keep an override alive.
sourcing. Once the trusted baseline is the script-relative default, an
override is honored only when it canonically equals that default. Both
existing test call sites already resolved to exactly that file, and an
attacker path is rejected — so both branches source the identical file and the
override cannot even inject a mock. The seam is provably dead code.
only ever equal the trusted default, the override carries no capability worth
keeping. Removing it is the root-cause fix: it fully closes the arbitrary-file
sourcewith no new abstraction and no dead seam. The three now-redundantFLEET_STATE_HOOK_UTILS=lines in the test harness (which already pointed atthe real file) and the doc-comment entry were removed with it.
No behavior change in production:
PLUGIN_ROOT_DEFAULTandCLAUDE_PLUGIN_ROOTresolve to the same directory there, so the same real
hook-utils.shloads.Verification
Existing test suite (
fleet-state.test.sh) — full pass on the committed code:(Later re-runs on this Windows host intermittently exceed the command timeout on
the
git initand binary-copy cases from AV/disk contention; that isenvironmental, not a test failure — the suite completes green on the exact
committed tree.)
Exploit path closed — malicious override in a non-test context:
The repro sets
FLEET_STATE_HOOK_UTILSto a script that echoes a marker, thenruns
fleet-state.sh --marketplace nopewith no test env. Before: the attackerfile is sourced. After: the override is ignored and the trusted sibling loads.
Closes #797
Related
fix/254-fleet-state-review-gaps) — the security-review pass thatsurfaced this as a pre-existing, out-of-scope finding and deferred it here.
docs/820-shell-helper-dedup-or-document) — open draft (do-not-merge)that also bumps
plugins/claude-ops/.claude-plugin/plugin.json+CHANGELOG.md(no code overlap with this PR). This PR is provisionally parkedbehind docs: document shell test-helper duplication and exit-code divergence as deliberate #853's unresolved cross-plugin version/CHANGELOG bump: a merge-drive
must re-diff the version claim before either PR flips ready. Carrying
do-not-mergeuntil then.Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com