Skip to content

fix(guardrails): buffer stdin before parsing in cli-flag-verify hook - #757

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/446-cli-flag-verify-stdin-buffer
Jul 20, 2026
Merged

fix(guardrails): buffer stdin before parsing in cli-flag-verify hook#757
kyle-sexton merged 1 commit into
mainfrom
fix/446-cli-flag-verify-stdin-buffer

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #446

hook::read_file_path ran jq directly against the inherited fd0 with no read bound in cli-flag-verify.sh — the last remaining fd0-direct reader after the fleet-wide hook::buffer_stdin migration, same Windows Win32-pipe late-EOF stall class. Fix per the issue's decided shape: buffer first, pipe the payload into the parser; empty/timed-out stdin skips this advisory hook (plain || exit 0 collapse matching its class and its siblings). Guardrails 0.9.0 → 0.9.1 + CHANGELOG.

Verification: cli-flag-verify.test.sh PASS=37 FAIL=0 (35 baseline + 2 new empty-stdin skip-contract assertions); shellcheck clean. The new test asserts the skip contract, not the stall itself — a here-string//dev/null harness cannot reproduce the Win32 late-EOF stall; behavior-preservation evidence is the suite green before/after with every case now routed through the buffered read.

Related

🤖 Generated with Claude Code

hook::read_file_path ran jq directly against the inherited fd0 with no
read bound — the last remaining fd0-direct reader after the fleet-wide
hook::buffer_stdin migration, and the same Windows Win32-pipe late-EOF
stall class that migration eliminated. Buffer first, then pipe the
payload into the parser; empty or timed-out stdin skips this advisory
hook (plain exit-0 collapse, matching its class). Adds an empty-stdin
skip-contract test case.

Closes #446

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


Security Review

  • Fetch PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

No exploitable security vulnerabilities found in this PR.

The change touches four files. The security-relevant portion is the two-line replacement in cli-flag-verify.sh (lines 53–54):

# Before:
FILE=$(hook::read_file_path) || exit 0

# After:
INPUT=$(hook::buffer_stdin) || exit 0
FILE=$(printf '%s' "$INPUT" | hook::read_file_path) || exit 0

Data flow analysis — stdin → jq → path → shell:

  1. hook::buffer_stdin reads stdin with a bounded read -r -d '' -t "$timeout", strips CRs via tr -d '\r', and stores into a bash variable. Bash variables cannot hold null bytes, so the buffer is implicitly sanitized there. No injection surface.

  2. printf '%s' "$INPUT" pipes the raw JSON payload into hook::read_file_path, which extracts tool_input.file_path with jq -r. jq does not evaluate shell code; it only parses JSON and emits a string. No injection surface.

  3. hook::read_file_path validates the resulting path: it checks [[ -f "$file" ]] (must exist) and, when CLAUDE_PROJECT_DIR is set, enforces a path-segment-anchored containment check (norm_file == norm_project/*). This is pre-existing, unchanged logic.

  4. Downstream uses of $FILEgrep … "$FILE", awk … "$FILE", sed … "$FILE" — are all properly double-quoted (lines 105, 112, 119, 256–294). No word-splitting or glob-expansion risk.

|| exit 0 on hook::buffer_stdin: both rc 1 (empty/no payload) and rc 2 (timeout) cause the hook to exit 0 silently. For an advisory hook this is the correct failure mode — identical to the behavior of flag-commit-pr-skill-bypass and workflow-resilience-check. An attacker who can control stdin cannot do worse than suppress the advisory; they cannot cause execution or escalation.

Pre-existing surfaces not introduced by this PR (out of scope but noted for completeness): $BINS and $SKIP are iterated unquoted (for bin in $BINS), and $bin / $flag are passed positionally to the bundled verifier script. These are unchanged by this PR and carry no new risk.

Test changes (cli-flag-verify.test.sh): the two new assertions exercise </dev/null (empty stdin → exit 0) and verify no output is produced. No new execution paths or trust boundaries introduced.

Version bump (0.9.0 → 0.9.1) and CHANGELOG entry are metadata-only; no security impact.


@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Gather context and read changed files
  • Review cli-flag-verify.sh changes
  • Review cli-flag-verify.test.sh changes
  • Review plugin.json version bump
  • Review CHANGELOG.md entry

Summary

Minimal, focused fix that completes the fleet-wide hook::buffer_stdin migration. cli-flag-verify.sh was the last entry script calling hook::read_file_path directly on the inherited fd0 — the same Windows Win32-pipe late-EOF stall class fixed for all other hooks in [0.8.0]. The change is correct, consistent, and well-tested.


cli-flag-verify.sh — Core Change

Before:

FILE=$(hook::read_file_path) || exit 0

After (lines 53–54):

INPUT=$(hook::buffer_stdin) || exit 0
FILE=$(printf '%s' "$INPUT" | hook::read_file_path) || exit 0

The mechanics are sound:

  • hook::buffer_stdin reads fd0 with a bounded read -r -d '' -t "$timeout", CR-strips, validates with jq, returns 1 (empty) or 2 (timeout) on failure.
  • printf '%s' "$INPUT" | hook::read_file_path pipes the buffered content to jq in hook::read_file_pathjq reads from the pipe rather than the inherited fd0. ✅
  • Both failure codes (1 and 2) collapse to || exit 0 here, which is the correct behaviour for an advisory hook. The blocking hooks correctly discriminate rc=2 for fail-closed; this hook correctly ignores it. ✅
  • printf '%s' (not echo) avoids trailing newline injection and escape interpretation. ✅
  • hook::buffer_stdin already strips \r; hook::read_file_path runs gsub("\r";"") in jq. The double-strip is idempotent and harmless. ✅

The pattern is identical to the advisory sibling hooks flag-commit-pr-skill-bypass (line 63) and workflow-resilience-check (line 31).


cli-flag-verify.test.sh — New Tests

Lines 141–150:

OUT=$(PATH="$FAKE_BIN_DIR:$PATH" CLAUDE_PLUGIN_OPTION_CLI_FLAG_VERIFY_BINS=faketool \
  bash "$HOOK" </dev/null 2>&1); RC=$?
assert_exit "empty stdin → exit 0" 0 "$RC"
assert_silent "empty stdin → no output" "$OUT"
  • Asserts the skip contract (rc=0, no output) for empty stdin. ✅
  • The comment correctly documents the testing limitation (a /dev/null harness cannot reproduce the actual Win32 late-EOF pipe stall; rc=2 from timeout produces the same exit 0 as rc=1 for this advisory hook). ✅
  • The 35-baseline-plus-2 total (PASS=37 FAIL=0) matches the PR description. ✅

plugin.json — Version Bump

0.9.0 → 0.9.1: correct patch-level bump for a bug fix with no API/behaviour change. ✅


CHANGELOG.md

Entry is accurate, references the right prior migration ([0.8.0]), names the sibling hooks for orientation, and explains the rc semantics. Follows Keep a Changelog format. ✅


Overall

No issues found. The change is the minimal diff needed to close the last fd0-direct reader, the approach is consistent with the established pattern, the advisory-hook skip contract is tested, and the version bump + changelog are in order.

Approved.

kyle-sexton added a commit that referenced this pull request Jul 20, 2026
…DMEs (wave 2) (#758)

No linked issue — wave 2 of 3; #537 stays open until all 39 plugins are
done.

Same recipe as wave-1 PR #755 for the next 13 plugins alphabetically;
guardrails is deferred to wave 3 because two in-flight guardrails
version bumps (#757 lane) serialize ahead of it. Per-plugin patch bump +
CHANGELOG entry; discovery and docs-hygiene entries carry dates per
those plugins' own changelog convention.

Verification: repro grep returns 0 hits over the 13 wave-2 READMEs; all
13 manifest versions match their CHANGELOG top entry; base predates #755
so wave-1 plugins intentionally still carry the pointer here (no file
overlap between waves).

## Related

- #537 (wave 2 of 3)
- #755 (wave 1)
- #426 (the two-plugin fix this mirrors)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton merged commit e482583 into main Jul 20, 2026
21 checks passed
@kyle-sexton
kyle-sexton deleted the fix/446-cli-flag-verify-stdin-buffer branch July 20, 2026 20:42
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.

guardrails: cli-flag-verify.sh still reads fd0 unbounded (last Win32-stall stdin site)

1 participant