Skip to content

feat(conventions): codify hook-config-delivery channel matrix with CI gate - #1249

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/hook-config-delivery-convention
Jul 24, 2026
Merged

feat(conventions): codify hook-config-delivery channel matrix with CI gate#1249
kyle-sexton merged 3 commits into
mainfrom
feat/hook-config-delivery-convention

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Codifies the userConfig→hook channel decision matrix as a new hook-* family owner doc,
docs/conventions/hook-config-delivery/ (README + CHANGELOG, contract_version 1.0), registered in
the PLUGIN-PHILOSOPHY convention registry. It composes with config-cascade (which owns
consumer-tracked file layering; this owns the harness-prompted userConfig path), characterizes
channels A–F — including the direct-settings-read channel (F) that disk-hygiene 0.9.0 shipped in
#1242 — and version-pins every upstream fact to CC 2.1.218 with explicit recheck triggers
(docs re-fetched 2026-07-24; behavioral facts from the 2026-07-23 fresh-session probe).

Enforces the matrix's "never bare argv" rule with a new userconfig-argv-gate CI lane:
scripts/check-hook-userconfig-argv.sh fails on any ${user_config.*} token in a plugin hook
config — the default hooks/hooks.json, manifest-pointed hook files (string or array), and inline
manifest hooks objects. MCP/LSP configs are out of scope (substitution there is sanctioned). A
stale-guarded allowlist (scripts/hook-userconfig-argv-allowlist.txt, currently comment-only) is
reserved for a ratified channel D adoption once the G-required probe passes. This pins the exact
regression #1242 fixed: an unset-but-defaulted argv token silently drops the whole hook entry
(upstream default unimplemented — anthropics/claude-code#46477, closed not-planned).

Test plan

  • bash scripts/check-hook-userconfig-argv.test.sh — 13/13 scenarios green (bare token in default /
    manifest-pointed / array / inline configs fail with file:line; clean, MCP, unreferenced-sibling,
    non-hooks-manifest cases stay quiet; allowlist honored; stale allowlist entries fail; comments
    inert; unparsable manifest skipped). CRLF-tolerant on Windows (jq emits \r under Git Bash).
  • bash scripts/check-hook-userconfig-argv.sh — real tree passes (0.9.0 already removed the last
    bare token).
  • shellcheck --rcfile=.shellcheckrc clean on both scripts; actionlint + YAML parse clean on
    ci.yml; markdownlint-cli2 clean on the new/edited docs.
  • CI job runs its self-test first (broken-detector-cannot-mask pattern) and is wired into the
    ci-status needs aggregate.

Related

No related issue: the delivery-channel program is tracked outside this tracker; this PR closes
nothing. Context: supersedes the draft matrix in #1182 (which stays open, demoted to the
adoption/tracking pointer), builds on #1242 (disk-hygiene 0.9.0, closed #1019).

🤖 Generated with Claude Code

… gate

Adds the hook-config-delivery owner doc (docs/conventions/) — the channel
decision matrix for delivering a plugin userConfig value into hook decision
logic, version-pinned to CC 2.1.218 (docs re-fetched 2026-07-24; fresh-session
probe 2026-07-23) with recheck triggers. Registers it in the PLUGIN-PHILOSOPHY
convention registry. Channels A-F characterized, including the shipped
direct-settings-read channel (F) from disk-hygiene 0.9.0 (#1242).

Enforces the "never bare argv" rule with a new userconfig-argv-gate CI lane:
scripts/check-hook-userconfig-argv.sh flags any ${user_config.*} token in a
plugin hook config (default hooks/hooks.json, manifest-pointed files, inline
manifest hooks objects; MCP/LSP configs out of scope), with a stale-guarded
allowlist reserved for a proven channel D. Self-test-first job wired into the
ci-status aggregate. Pins the exact regression #1242 fixed.

Supersedes the draft matrix in #1182, which becomes the adoption pointer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 45s —— View job


PR Review — feat(conventions): hook-config-delivery channel matrix

  • Gather context and read changed files
  • Review CI workflow changes (.github/workflows/ci.yml)
  • Review documentation (docs/conventions/hook-config-delivery/README.md, CHANGELOG.md)
  • Review shell scripts (check-hook-userconfig-argv.sh, .test.sh)
  • Review allowlist file
  • Review docs/PLUGIN-PHILOSOPHY.md change
  • Synthesize findings and post final review

Overall

Solid PR. The documentation is precise and unusually honest about what is doc-stated vs. probe-proven vs. inferred. The gate script is defensively written. The test suite is comprehensive and follows the broken-detector-cannot-mask pattern. No blocking issues — a few minor observations below.


docs/conventions/hook-config-delivery/README.md

Strengths:

  • Version-pinning on CC 2.1.218 with explicit recheck triggers is exactly the right pattern for behavioral facts that live outside docs.
  • The boundary section cleanly separates concerns from config-cascade.
  • The meta-rule ("do not enshrine the outage as law") is important and easy to omit — glad it's here.
  • Cross-referencing hook-precision, hook-observability, and hook-telemetry — all three exist and the links are live.

Minor:

  • Line 62–65 — channel F's "locating the user file only from the tamper-resistant ${CLAUDE_PLUGIN_ROOT} cache anchor" description is accurate but could briefly note why CLAUDE_PLUGIN_ROOT is tamper-resistant for readers who haven't internalized that the cache anchor is written by the harness, not by the repo. The disk-hygiene exemplar link picks this up, but the matrix row itself doesn't carry enough context to evaluate the claim in isolation.

scripts/check-hook-userconfig-argv.sh

Strengths:

  • set -euo pipefail on line 27. Clean early-exit on missing jq (lines 31–34).
  • CRLF tolerance is handled consistently in three distinct ways matching three distinct code paths: tr -d '\r' on jq pipeline output (lines 45, 82), ${rel%$'\r'} on per-element reads (line 89), and tr -d '\r' on the allowlist (line 45). Correct.
  • The flagged_or_allowed associative array (line 49) correctly handles the stale-guard/allowlist interaction — an allowlisted file with multiple flagged lines marks the key once, and the stale guard checks file presence not line count.
  • The scan_file function uses grep -nF (fixed-string, no regex interpretation) on the literal token, which prevents false matches on regex-special characters in the token.

One correctness note — stale allowlist guard has an off-by-one scope:
Lines 104–113 guard against allowlist entries that don't appear in flagged_or_allowed. But flagged_or_allowed only tracks files that were scanned and contain the token. A file that was scanned but is clean does not appear in flagged_or_allowed. So an allowlist entry pointing to a clean file correctly fails the stale guard (tested in scenario 10). A file that was never scanned (e.g., not under plugins/*/) also fails the stale guard correctly (scenario 11). This is the intended behavior — the comment on line 103 ("a scanned hook config that still carries the token") is accurate. Just confirming the implementation matches the stated intent.

Minor path-traversal edge case (non-blocking):
Lines 85 and 89 join $plugin and $rel without constraining rel to remain within the plugin directory. A manifest specifying "hooks": "../../other-dir/config.json" would cause the script to scan other-dir/config.json at repo root. In practice this cannot happen — plugin.json is checked-in code — but a [[ "$rel" != ..* ]] guard would make the script self-documenting about its trust model. Not blocking.

[Informational] Line 94 — the inline-object case reports "inline hooks object" as the location rather than a line number. This is a consequence of using jq -c (compact single-line serialization) to avoid multi-line parsing complexity. It's the right tradeoff and is consistent with the error message format. The test in scenario 6 asserts this string explicitly.


scripts/check-hook-userconfig-argv.test.sh

Strengths:

  • All 13 scenarios are independent (each gets its own mktemp -d fixture and rm -rf cleanup). No test can pollute another.
  • set -uo pipefail without -e is correct here — the test explicitly checks exit codes, so -e would abort on expected failures.
  • run_check uses a subshell ( ) so cd doesn't affect the parent — prevents accidental PWD drift between tests.

Minor gap in coverage:
There is no explicit test for a manifest that exists but has no hooks key (or "hooks": null). The main script handles this via the * branch of the case (line 98), which silently does nothing. This path is correct, but its test coverage is implicit (the manifest-free scenario in test 1 covers the "no manifest at all" case, which is distinct). A test with {"name":"alpha"} (no hooks key) would close the gap. Not blocking — the * branch has one line and can be read directly.

No test for a plugin with both a hooks/hooks.json and a manifest-pointed path that both carry the token. The current test suite verifies each path in isolation. The script would report both (and increment errors twice), but this composite case isn't tested. Low concern.


.github/workflows/ci.yml

  • Checkout SHA matches every other job in the file (9c091bb…). Consistent.
  • persist-credentials: false is set. Good.
  • timeout-minutes: 15 is consistent with adjacent jobs.
  • Self-test first (check-hook-userconfig-argv.test.sh before check-hook-userconfig-argv.sh) correctly implements the broken-detector-cannot-mask pattern.
  • userconfig-argv-gate added to the ci-status needs aggregate on line 713. ✓

docs/PLUGIN-PHILOSOPHY.md

Single-line registry entry is correct: matches the existing table format and links to the right path. The relative link conventions/hook-config-delivery/README.md resolves from docs/PLUGIN-PHILOSOPHY.md.


Summary

No blocking issues. The two items most worth a follow-up are:

  1. The missing null-hooks-key manifest test case (easy to add, closes the * branch gap).
  2. The path-traversal guard note (informational; trust model is fine for a CI script over trusted code).

The stale-allowlist mechanism and the probe-over-docs discipline for behavioral facts are both good patterns worth keeping.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 45s —— View job


Security Review — PR #1249

Tasks

  • Load tools
  • Fetch PR diff
  • Read changed files (ci.yml, gate script, test script, allowlist, docs)
  • Analyze for security issues
  • Post findings

Findings

SUGGESTION · confidence: HIGH — Unvalidated path traversal in scan_file (check-hook-userconfig-argv.sh)

Severity: SUGGESTION
Confidence: HIGH (the path is followed; impact is real but narrow)

The script resolves hook config paths read directly from a plugin's plugin.json without verifying the result stays inside the plugin directory.

Relevant excerpt — string hook path (line 84–85):

rel="$(jq -r '.hooks' "$manifest" | tr -d '\r')"
scan_file "$plugin/${rel#./}"

${rel#./} strips a leading ./ but does not prevent ../../-style escapes. A manifest containing {"hooks": "../../../../etc/hostname"} produces the path plugins/myplugin/../../../../etc/hostname, which resolves outside the repository root. The array-path branch (lines 88–91) carries the same issue.

Impact in practice: scan_file only calls [[ -f "$file" ]] and grep -nF "$TOKEN" "$file" (fixed-string, read-only). The TOKEN ${user_config. will not appear in any OS or CI-runner file, so no diagnostic output leaks and no code executes. The trigger is pull_request (not pull_request_target) so fork PRs run without secrets. The real-world risk is very low, but the invariant "this gate only reads plugin hook configs" is silently broken for any PR that includes a crafted manifest.

Suggested fix: After constructing the candidate path, canonicalize it and assert it still starts with the plugin directory:

# After: rel="${rel%$'\r'}" (or equivalent)
local candidate
candidate="$(realpath -m "$plugin/$rel")"
local plugin_abs
plugin_abs="$(realpath -m "$plugin")"
[[ "$candidate" == "$plugin_abs"/* ]] || {
  echo "check-hook-userconfig-argv: skipping out-of-tree path in $manifest: $rel" >&2
  return 0
}
scan_file "$candidate"

Fix this →


Everything else — no findings

Area Assessment
Workflow trigger pull_request only — not pull_request_target / workflow_run; fork PRs are sandboxed
Workflow permissions Global permissions: contents: read; new job adds no wider scope
Action pins actions/checkout pinned to full commit SHA with inline tag comment; persist-credentials: false set
${{ github context }} in run: New job has no env: or inline interpolation; no script-injection surface
Secrets No secrets referenced anywhere in the new job
grep/jq usage Fixed-string (-F) grep throughout; jq output only piped to grep or captured — no eval or shell expansion of untrusted data
Allowlist guard grep -Fxq (whole-line, fixed-string) — no injection via allowlist content
flag() echo No -e; no command expansion; line-number extracted by parameter expansion (${line%%:*}), not eval
Test script Uses mktemp -d; all fixture content is hardcoded strings; run_check runs in a subshell
Documentation/convention docs No executable content; no secrets

No CRITICAL or IMPORTANT findings.

…w test gaps

Review follow-ups on #1249: scan_manifest_path rejects absolute and ..-segment
hooks paths from plugin.json with a visible skip (portable string check, no
realpath dependency), so a crafted manifest cannot point the gate outside the
tree it claims to scan. Adds three test scenarios (no-hooks-key manifest,
out-of-tree path skipped visibly, composite default+manifest-pointed both
flagged) — 16 total. README channel F now states why ${CLAUDE_PLUGIN_ROOT} is
tamper-resistant inline.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Review + security findings triaged; fixes in 8d0d0aa.

Finding Source Classification Action
Path traversal: manifest hooks path can escape the plugin dir security-review (SUGGESTION) + code review (minor) VALID — ${rel#./} does not stop ../ or absolute paths Fixed: new scan_manifest_path rejects absolute and ..-segment paths with a visible skipping out-of-tree hooks path notice (portable string check, no realpath dependency — macOS/Git Bash safe). Covered by a new test
Missing test: manifest with no hooks key code review (minor) VALID Added — {"name":"alpha"} passes via the case * branch
Missing test: composite (default hooks/hooks.json + manifest-pointed file both carrying the token) code review (low) VALID Added — asserts both files flagged
Matrix row F doesn't say why ${CLAUDE_PLUGIN_ROOT} is tamper-resistant code review (minor) VALID Added inline: harness-substituted install-cache path under the user's config dir; no repo file can redirect it
Stale-allowlist scope note code review (informational) NO CHANGE — reviewer confirmed implementation matches stated intent
Inline-object case reports no line number code review (informational) NO CHANGE — acknowledged tradeoff, test-pinned

Test suite now 16/16; shellcheck (repo rcfile) and markdownlint clean.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 27s —— View job


Re-review — feat(conventions): hook-config-delivery channel matrix (post-8d0d0aa)

Tasks

  • Load tools
  • Read fixed files at HEAD (8d0d0aa)
  • Verify path-traversal fix in check-hook-userconfig-argv.sh
  • Verify new test coverage (null hooks key, out-of-tree path, composite case)
  • Verify README.md channel F explanation
  • Synthesize and post findings

Verdict — all previous findings correctly resolved

This is the follow-up pass after the triage table in kyle-sexton's comment. Each finding is verified below.


1. Path traversal → scan_manifest_path (security finding + code-review minor) ✓

The new function (lines 62–75) guards all three escape vectors with portable string checks:

if [[ "$rel" == /* || "$rel" =~ ^[A-Za-z]: || "/$rel/" == *"/../"* ]]; then
    echo "...skipping out-of-tree hooks path..." >&2
    return 0
fi
  • "$rel" == /* — rejects absolute Unix paths.
  • "$rel" =~ ^[A-Za-z]: — rejects Windows drive-letter paths.
  • "/$rel/" == *"/../"* — rejects any path containing a .. segment, including .. alone, ../../x, a/../b.

The Bash [[ ]] glob match on the right side correctly treats the quoted "/../" as a fixed literal flanked by wildcard * — no false negatives for the intended cases. The guard is conservative (it also rejects a/b/../c.json even though that resolves within the plugin tree), which is appropriate for a CI gate over checked-in plugin manifests. No realpath dependency; macOS / Git Bash safe as stated.

Boundary checked: The jq … select(type == "string") filter on the array branch (line 106) prevents scan_manifest_path from ever receiving a JSON-null element. The string branch never produces a null rel because the case "$hooks_type" dispatches on jq -r '.hooks | type' first; a JSON-null hooks value hits *) ;; and never reaches scan_manifest_path. No leakage path.


2. Missing test: manifest with no hooks key ✓

Added at lines 214–223{"name":"alpha"} with a clean hooks/hooks.json. jq -r '.hooks | type' returns "null", hits the *) branch, silently passes. Test closes the gap correctly.


3. Missing test: out-of-tree path ✓

Added at lines 226–238 — manifest with "hooks":"../../outside.json", a live outside.json carrying the token. Test asserts exit 0 and that the visible skipping out-of-tree hooks path notice appears in stderr. Verifies the skip notice and the non-scan of the out-of-tree file in one scenario.

Note (informational): The array-path traversal case ("hooks":["../../x.json"]) is not tested explicitly — it shares the same scan_manifest_path code path, so coverage is implicit. Not a gap that needs a dedicated test given the guard is one function, but worth knowing.


4. Missing composite test ✓

Added at lines 240–255 — plugin with both hooks/hooks.json and a manifest-pointed config/extra-hooks.json carrying the token. Test asserts both USERCONFIG ARGV lines appear in stderr. ✓


5. Channel F tamper-resistance explanation ✓

Lines 62–69 of README.md now carry the inline rationale directly in the matrix row:

tamper-resistant because the harness substitutes it from the plugin's install cache under the user's own config dir, a path no repo file can redirect

The original finding was that the row made a claim without the supporting reasoning visible locally. The addition is precise and self-contained — a reader evaluating channel F from the matrix doesn't need to follow the exemplar link to understand the trust property.


Summary

16/16 tests pass. All five findings from the previous review cycle are correctly closed. No blocking issues; no new concerns introduced by the fixes. The PR is ready to merge.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 53s —— View job


Security Review — PR #1249 (re-review after 8d0d0aa)

Tasks

  • Load tools
  • Fetch PR diff (post-fix HEAD 8d0d0aa)
  • Read changed files (ci.yml, gate script, test script, allowlist, docs)
  • Analyze for security issues
  • Post findings

Previous finding — resolved

The path-traversal SUGGESTION from the first security review has been addressed in 8d0d0aa. The new scan_manifest_path function at lines 67–75 applies a three-part portable guard before passing any manifest-sourced path to scan_file:

if [[ "$rel" == /* || "$rel" =~ ^[A-Za-z]: || "/$rel/" == *"/../"* ]]; then
  • "$rel" == /* — rejects Unix absolute paths ✓
  • "$rel" =~ ^[A-Za-z]: — rejects Windows absolute paths (C:\…, D:/…) ✓
  • "/$rel/" == *"/../"* — rejects any path containing a .. segment, regardless of position or prefix (e.g. ../x, ./a/../../etc, foo/../bar) ✓

Traced against the boundary cases relevant to the prior finding: ../other.json"/../other.json/" matches *"/../"*; ./subdir/../../etc"/./subdir/../../etc/" contains /../ at the embedded /../. Guard is comprehensive without a realpath dependency (macOS / Git Bash portable). The new test scenario 14 exercises the ../../ case end-to-end.


New scan — no findings

Surface Assessment
Workflow trigger pull_request only — not pull_request_target / workflow_run; fork PRs are sandboxed, no secrets available
Workflow permissions Global permissions: contents: read; new userconfig-argv-gate job adds no wider scope
Action pin actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 — full SHA, tag comment, consistent with adjacent jobs
persist-credentials false
Script injection via github context New job has no env: block and no ${{ }} interpolation in run: steps — no injection surface
Secrets No secrets referenced anywhere in the new job
grep usage -nF throughout — fixed-string, no regex; $TOKEN is a single-quoted literal, never shell-expanded
jq output handling Captured to variables or piped to grep -qF; no eval, no shell expansion of untrusted content
Allowlist guard grep -Fxq — fixed-string, whole-line; allowlist content cannot inject commands
flag() echo No -e; line number extracted by parameter expansion (${line%%:*}), not eval; no ANSI escape injection risk
Test fixture isolation Each of 16 scenarios uses mktemp -d and rm -rf; all fixture content is hardcoded strings; run_check uses a subshell
Empty-plugins edge case for plugin in plugins/*/ with no matching dirs leaves plugin unexpanded; [[ -f "plugins/*/.claude-plugin/plugin.json" ]] is false — no harm
hooks_type injection Only used in a case statement; value is always a jq type keyword or the literal "invalid"

No CRITICAL or IMPORTANT findings. No new SUGGESTION-level items.


@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d0d0aa2a5

ℹ️ 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".

Comment thread scripts/check-hook-userconfig-argv.sh
@kyle-sexton
kyle-sexton enabled auto-merge (squash) July 24, 2026 15:24
Codex P2 on #1249: a hook config can spell the token with a JSON unicode
escape (backslash-u005f for the underscore) that the loader decodes before
substitution, evading a raw-text grep. scan_file now adds a second pass —
jq re-serializes the JSON, resolving escapes — and flags a file whose decoded
content carries the token when the raw pass saw nothing. Raw pass keeps
file:line precision; unparsable files keep the raw pass only. New test builds
the escaped fixture via printf; 17 scenarios total.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 2s —— View job


Re-review — feat(conventions): hook-config-delivery matrix (post-f5fef73)

Tasks

  • Load tools
  • Read gate script and test file at HEAD (f5fef73)
  • Verify decoded-pass implementation for JSON-escaped token bypass
  • Verify new test scenario (13 — JSON-escaped token)
  • Post findings

Codex finding — resolved ✓

The fix at lines 82–93 correctly closes the \u-escape bypass. The updated scan_file now runs two passes:

hits="$(grep -nF "$TOKEN" "$file" || true)"
if [[ -n "$hits" ]]; then
    # raw pass — precise file:line reporting
    while IFS= read -r line; do flag "$file" "${line%%:*}"; done <<<"$hits"
elif jq -c . "$file" 2>/dev/null | grep -qF "$TOKEN"; then
    # decoded pass — catches \u-encoded spellings the loader would resolve
    flag "$file" "escaped token in decoded JSON"
fi

Why this is correct:

jq -c . fully decodes all JSON Unicode escapes before re-serializing. A raw JSON value of "${user_config.toggle}" (where _ = _) becomes "${user_config.toggle}" in jq's compact output, which the grep -qF then matches. The TOKEN (${user_config.) contains no characters that jq would re-escape on output ($, {, . are all safe JSON string characters), so the grep is reliable.

elif structure — intentional tradeoff, not a bug:

The decoded pass only runs when the raw pass returns zero hits. This means a file carrying both raw-text and escaped spellings would report only the raw hits. For the gate's goal (fail the PR), this is correct — the file is already flagged. The comment at line 77–81 documents this tradeoff explicitly ("raw pass is kept for file:line precision"). No issue.

Allowlist interaction:

An allowlisted file that carries only an escaped token goes through the decoded pass → flagallowed() returns true → flagged_or_allowed[$file]=1 set, errors not incremented. The stale-allowlist guard sees the file as active and doesn't report it stale. Correct behavior in all allowlist × raw/escaped combinations. ✓

Error message format:

The decoded-pass branch reports "escaped token in decoded JSON" as the location rather than a line number — expected, since jq -c . compacts to one line. Consistent with the acknowledged tradeoff for the inline-object case from the prior review cycle.


Test scenario 13 — scripts/check-hook-userconfig-argv.test.sh lines 214–230

ESCAPED_HOOK="$(printf '{"hooks":[...,"${user\\u005fconfig.some_toggle}",...]}'])"

Tracing the quoting: the single-quoted printf format string contains \\u005f; printf interprets \\ as a single \, so the written file content is ${user_…} — a valid JSON Unicode escape for _. The raw grep does not match (no underscore in the literal bytes). jq -c . decodes __, decoded output contains ${user_config. → grep matches. Test asserts non-zero exit and stderr containing USERCONFIG ARGV: …:escaped token in decoded JSON. Quoting and assertion are accurate.


Nothing new to flag

Surface Assessment
jq -c . re-serialization correctness $, {, . are not JSON-escapable; decoded token reproduced faithfully in output ✓
Multi-escape vectors (con…) jq decodes all Unicode escapes; combined decoding yields the full token ✓
Unparsable file fallback 2>/dev/null suppresses jq errors; raw-only pass used; consistent with prior *) handling ✓
Allowlist × escaped-only file flagged_or_allowed set by decoded pass; stale guard satisfied ✓
17/17 test count All scenarios present in the file; no orphaned tests ✓

No blocking issues. No new concerns. The PR is ready to merge.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 26s —— View job


Security Review — PR #1249 (post-f5fef73)

Tasks

  • Load tools
  • Fetch PR diff at HEAD (f5fef73)
  • Read changed files
  • Analyze for security issues
  • Post findings

Previous findings — status

Finding Prior verdict Current state
Path traversal in manifest hook path resolution SUGGESTION (initial review) Resolved in 8d0d0aa; scan_manifest_path guard intact at HEAD
JSON-escaped token bypass (Codex review) VALID Fixed in f5fef73; reviewed below

f5fef73 — decoded-pass analysis

The commit adds a second scan pass to scan_file (lines 90–92):

elif jq -c . "$file" 2>/dev/null | grep -qF "$TOKEN"; then
  flag "$file" "escaped token in decoded JSON"
fi

Injection surface: $file is passed as a filename argument to jq, never as part of the filter string. No shell expansion occurs; jq reads the file as input data. $TOKEN is a single-quoted literal in the outer script and is never reassigned. grep -qF uses fixed-string matching and returns only an exit code — no file content is echoed or further processed. No injection surface exists in this code path.

Scope of decoding: jq -c . re-serializes the entire JSON object compactly, resolving all \uXXXX escape sequences in string values before grep sees the output. This covers every single-level Unicode encoding of any character in the token ($$, {{, __, etc.). Double-encoded sequences (\\u005f) remain as literal backslash strings after jq processing and would not match the token — but the Claude Code harness also does not decode second-level escapes after JSON parsing, so double-encoding cannot produce a runtime bypass either. The fix is comprehensive.

Interaction with set -euo pipefail: The elif branch is a conditional expression; set -e does not abort on non-zero exit from a pipeline used as an if/elif condition. If jq fails (malformed JSON), 2>/dev/null suppresses the error, the pipeline exits non-zero, the elif is false, and the script continues with raw-only coverage — matching the documented fallback behavior. ✓

Interaction with scan_manifest_path: The path-traversal guard in scan_manifest_path runs before scan_file is called for manifest-pointed files. The decoded pass inherits this guard — it only runs on files that have already passed the traversal check. ✓

Inline object case (not via scan_file): The object) branch at line 116 uses jq -c '.hooks', which also compactly serializes the hooks value and decodes \uXXXX escapes. This branch was already bypass-resistant before f5fef73; the new commit correctly identifies the gap as being in the file-scan path only.

Test coverage for new path: Scenario 17 (lines 214–230) builds the _ fixture via printf (so the escape survives shell quoting), confirms the raw pass finds nothing, and asserts the decoded-pass flag appears in the output. The test is correctly scoped and independently isolated.


Full surface scan — no findings

Surface Assessment
Workflow trigger pull_request only — not pull_request_target / workflow_run; fork PRs sandboxed, no secrets available
Workflow permissions Global permissions: contents: read; userconfig-argv-gate job adds no wider scope
Action pin actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 — full SHA + tag comment, consistent with all adjacent jobs
persist-credentials false
Script injection via ${{ }} context New CI job has no env: block and no ${{ }} interpolation in run: steps — zero injection surface
Secrets No secrets referenced anywhere in new job
grep usage throughout -nF / -qF / -Fxq throughout — fixed-string only; $TOKEN never shell-expanded
jq output handling All jq output is piped to grep -qF (exit-code only) or captured to a variable for CRLF-stripping — no eval, no shell expansion of file content
Decoded-pass jq -c . Read-only; filename argument only; stderr suppressed; output consumed by grep -qF (no echo)
Allowlist guard grep -Fxq — whole-line, fixed-string; allowlist content cannot inject commands
flag() echo No -e; $where is either ${line%%:*} (numeric line number from grep -n) or a hardcoded literal string; no ANSI injection
Stale allowlist echo $entry from checked-in allowlist file; echo without -e
Test fixture isolation All 17 scenarios use mktemp -d + rm -rf; run_check subshell; all fixture content hardcoded strings

No CRITICAL, IMPORTANT, or SUGGESTION findings.

@kyle-sexton
kyle-sexton merged commit d39ea66 into main Jul 24, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the feat/hook-config-delivery-convention branch July 24, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

disk-hygiene: kill switch (disk_hygiene_enabled) cannot reach a skill-frontmatter guard hook — audit-only degrades to prompt-gated

1 participant