Skip to content

fix(guardrails): restore path-detection prefilter fail-open fix (0.9.8) - #946

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/944-restore-path-detection-prefilter
Jul 22, 2026
Merged

fix(guardrails): restore path-detection prefilter fail-open fix (0.9.8)#946
kyle-sexton merged 2 commits into
mainfrom
fix/944-restore-path-detection-prefilter

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

The hardcoded-path-check guard's cheap scan_text pre-filter had narrowed on main: it gated only on Users|/home/|repos, while the detailed drive-letter bodies (HPP_WIN_REPO_BODY / HPP_ESCAPED_WIN_REPO_BODY) accept the broadened repos|Repos|projects|Projects|dev|Dev roots shipped in 0.9.7. Content whose only machine path used a widened root (e.g. C:\Projects\…, C:\Dev\…) therefore early-returned 0 before the detailed scan ever ran — a fail-open in a security gate. This re-widens the pre-filter gate to a strict superset of every root token the detailed bodies accept, adds a Projects-root regression test, folds in the stale block_no_verify_enabled description sync (it still read "lefthook disables" only, now the full configurable default set), and bumps the plugin to 0.9.8.

All 40 hardcoded-path-check tests pass; shellcheck clean.

Closes #944

Related

HPP_WIN_REPO_BODY / HPP_ESCAPED_WIN_REPO_BODY now accept
repos|Repos|projects|Projects|dev|Dev, but the cheap pre-filter gate in
hpp::scan_text still only tripped on Users|/home/|repos. Content whose only
machine path was e.g. C:\Projects\acme\... or C:\Dev\acme\... early-returned
0 before the detailed scan ran — the guardrail under-blocked the very roots
this change adds (fail-open in a security gate).

Extend the gate alternation to list every root token the detailed bodies
accept, keeping it a strict superset. Add a regression test covering a
generic Windows checkout root under a widened name (Projects), which exits 2
with the fix and would silently pass without it.
…c sync

Records the restored `hardcoded-path-check` pre-filter fail-open fix under a
new [0.9.8] CHANGELOG entry (the gate had narrowed to `Users|/home/|repos` on
main while the detailed bodies accept the widened Projects/Dev roots, so those
machine paths early-returned before the detailed scan — fail-open in a
security gate).

Also broadens the `block_no_verify_enabled` userConfig description, which
still enumerated "lefthook disables" only, to the actual configurable default
set (lefthook, husky, pre-commit, simple-git-hooks) that shipped in 0.9.7.
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


PR Review: fix(guardrails): restore path-detection prefilter fail-open fix (0.9.8)

  • Gather context (changed files, diff)
  • Review hardcoded-path-patterns.sh changes
  • Review test coverage in hardcoded-path-check.test.sh
  • Review plugin.json version bump
  • Review CHANGELOG.md entry
  • Verify correctness of the prefilter fix
  • Post final review

Summary

The bug and fix are both correctly diagnosed. The cheap scan_text pre-filter gate had drifted out of sync with the detailed patterns it gates, producing a real fail-open for any content whose only machine-path used a widened checkout root (Projects, Dev, Repos). The fix is minimal, correct, and well-tested.


Findings

No blocking issues. One observation and one minor note follow.

Observation — dev/Dev performance tail (pre-existing, not introduced here)

hardcoded-path-patterns.sh L76

The tokens dev and Dev are extremely common in programming content (npm run dev, devDependencies, variable names), so adding them to the pre-filter means the pre-filter will trip on a large fraction of clean content and force a full scan. This is not a correctness issue — the comment correctly notes that "a looser gate only costs a wasted full scan" — but it weakens the stated "~99% of calls early-return" performance argument. This was already accepted when 0.9.7 widened the detailed bodies; this PR is just making the gate consistent with that decision. No action required, but it's worth noting if the pre-filter performance claim is revisited.

Minor — Projects regression test only; dev/Dev/Repos roots not covered

hardcoded-path-check.test.sh L85–88

The new test covers D:\Projects\acme\src and proves the general mechanism works (pre-filter doesn't drop Projects-root paths). Since all root tokens go through the same code path, one representative test is adequate for the fix. Testing dev, Dev, and Repos as well would give slightly stronger coverage, but is not required to merge.


Correctness Verification

The superset invariant holds exactly:

Detailed pattern tokens Pre-filter covers?
repos ✓ (was already present)
Repos ✓ (added in this PR)
projects ✓ (added in this PR)
Projects ✓ (added in this PR)
dev ✓ (added in this PR)
Dev ✓ (added in this PR)
Users ✓ (already covered Windows + macOS user paths)
/home/ ✓ (already covered Linux user paths)

The updated comment at L62–64 correctly encodes the maintenance rule ("a widened body must widen this list too"), which is the right way to document this invariant for future contributors.

The block_no_verify_enabled description update accurately reflects the shipped behavior (configurable set of hook managers, not just lefthook).

Version bump 0.9.7 → 0.9.8 and CHANGELOG entry are both appropriate for a bug fix that closes a fail-open in a security gate.

Verdict: LGTM. The fix is correct, minimal, and the regression test directly exercises the fail-open path.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


Security Review — PR #946

  • Read changed files
  • Analyze security implications of regex/prefilter changes
  • Check for injection surfaces or unsafe patterns
  • Review test changes for security relevance
  • Post findings

No security vulnerabilities found. Here is the full analysis of the four security-relevant surfaces touched by this PR.


1. Prefilter gate widening — SAFE

Line 76, hardcoded-path-patterns.sh

The gate changed from:

'Users|/home/|repos'

to:

'Users|/home/|repos|Repos|projects|Projects|dev|Dev'

The gate is if ! grep -qE ... ; then return 0; fi — content must match for the full scan to run; a miss causes an early-return-clean. Widening the alternation forces more content through to the detailed scan, which strictly reduces false negatives. The detailed patterns (HPP_WIN_REPO_BODY, HPP_ESCAPED_WIN_REPO_BODY) both require one of repos|Repos|projects|Projects|dev|Dev as a mandatory literal, so the prefilter is now an exact superset — no matching path can escape through the gate. The fix correctly closes the fail-open.

2. Here-string injection surface — SAFE

The prefilter invokes grep -qE '...' <<<"$content". The <<< here-string passes $content as data on stdin, not as a shell argument; shell metacharacters inside $content (backticks, $(...), &&, etc.) are not evaluated. No command injection is possible here.

The project-root branch uses grep -qFi "$gate_root", where -F means fixed string — pattern injection through the path segment is not possible. (The risk of gate_root starting with - and being misread as a flag is a pre-existing condition not touched by this PR, and the gate position means a worst-case outcome is a skipped optimization, not a bypass.)

3. dev/Dev token — SUGGESTION (performance, not security)

Adding dev and Dev to the prefilter alternation is correct for closing the fail-open. However, dev is an extremely common substring in programming content (developer, device, devops, advisory, etc.), so the prefilter will now fire on nearly all content in this domain, eliminating most of its optimization value. This is a performance observation, not a security concern — a wider gate cannot introduce a bypass — and it is the correct tradeoff given the alternative was the silent fail-open.

4. Description change and version bump — NO SECURITY IMPACT

The block_no_verify_enabled description update and the 0.9.7→0.9.8 version bump are metadata-only changes with no effect on runtime behavior or security posture.


Summary: The fix is directionally correct for a security gate — narrowing that gate was the vulnerability, widening it is the fix. No injection surfaces, authorization gaps, or unsafe patterns are introduced.

@kyle-sexton
kyle-sexton merged commit 9316a61 into main Jul 22, 2026
26 of 28 checks passed
@kyle-sexton
kyle-sexton deleted the fix/944-restore-path-detection-prefilter branch July 22, 2026 04:44
kyle-sexton added a commit that referenced this pull request Jul 22, 2026
main released guardrails 0.9.8 (#946 hardcoded-path prefilter fail-open
restore + block-no-verify description) while this branch also carried
0.9.8 for the #740 config-env work; branch entry restacked to 0.9.9,
main's 0.9.8 section preserved intact below it. No hook/lib/test code
overlap with #740 — the #946 change is in hardcoded-path-check.
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(path-detection): fail-open regression LIVE on main — scan_text prefilter drops C:\Projects\ / C:\Dev\ machine paths

1 participant