fix(guardrails): stop a UNC file path reaching telemetry whole

[kyle-sexton]

No linked issue

Summary

#3463 hoisted a hand-copied repo-relative path computation into hook::repo_relative_path and certified completeness with grep -rl '_file_lm=$(cygpath -lm "$FILE"' plugins/. That grep is scoped to the cygpath spelling. Two further copies survived in guardrails in a different spelling (CLAUDE_PROJECT_DIR plus ${FILE//\\//} folding, no cygpath), and one of them still leaks.

secret-pattern-detection.sh's redaction case was /* | [A-Za-z]:*). It tested two of the three absolute spellings and never UNC. This guard deliberately scans on when no project dir is set, and on that path the copy did no separator folding either, so a file_path of the \\server\share\file shape matched neither arm and the whole share path, server name included, landed in the envelope's data.file.

Found by independent adversarial verification of #3463, which is also where the scope was established: this is pre-existing, not a regression from that PR, and it is the exact bug class that PR exists to eliminate, left behind while its summary read as if the class were closed.

Fix

Both hooks now call hook::repo_relative_path, which carries the UNC arm.

secret-pattern-detection.sh pairs it with hook::repo_root on the file's own directory when no project dir is set, so an ordinary in-repo file is reported relative to its own checkout instead of collapsing to a bare basename. That is the second divergence the verifier measured, fixed in the same change.

hardcoded-path-check.sh carried the same duplicated block but its scope guard exits before the computation whenever the project dir is unset, so the leaking shape was never reachable there and its emitted data.file is unchanged. The copy is removed so a third divergent one cannot reappear.

Both computations move inside emit_tel, behind the start-stamp and hook::telemetry_enabled gates, so a run with no telemetry sink wired does not pay for the new command substitution at all (.claude/rules/hook-budget.md).

Neither site feeds a tool. Read rather than grepped: in both hooks file_rel has exactly one consumer, jq -n --arg file. secret-pattern-detection.sh's stderr block and secrets::scan_text "$CONTENT" use $FILE/$CONTENT; hardcoded-path-check.sh's hpp::scan_text "$CONTENT" "$SCAN_ROOT" "$FILE" takes neither. So neither needs the FILE_REL_DEGRADED branch that three of #3463's twelve sites required, and there is no findings-loss regression to prevent. The bare-assignment idiom matches the telemetry-only siblings; both scripts run set -uo pipefail with no -e.

A regression this PR introduced, then fixed

The first revision dropped a trailing-separator trim the hand-rolled copies carried (_root="${_root%/}", in both). The helper strips "$root/", so a root already ending in a separator forms the prefix /repo//, matches nothing, and degrades every in-project file to its basename. Caught by Codex on review, reproduced, and fixed in 07c22e41:

hook::repo_relative_path /repo/src/config.env "/repo/"  -> config.env
hook::repo_relative_path /repo/src/config.env "/repo"   -> src/config.env

A trailing slash is a supported spelling: secret-pattern-detection.test.sh:95 already passed CLAUDE_PROJECT_DIR="/repo/". That case asserted only the exit code, so it exercised the shape without pinning the value, which is why it stayed green through the regression. Both suites now assert the value.

Trimmed at the two call sites rather than in the helper, deliberately. Every other caller passes $REPO_ROOT from hook::repo_root, which is git rev-parse --show-toplevel and never returns a trailing separator; these two are the only ones handing the helper a caller-supplied CLAUDE_PROJECT_DIR. Normalizing inside lib/hook-utils.sh is defensible on its own merits but would cascade a version bump and CHANGELOG entry to all seventeen carriers under the hook-utils-sync lane, a much larger diff than the defect warrants. Recording the trade rather than leaving it implicit.

Verification

Measured before any change, by running the real hooks and reading data.file out of the envelope:

Shape	pre-fix data.file	post-fix
\\srv\share\secrets.env, no project dir	\\srv\share\secrets.env (leaks, share host and all)	secrets.env
ordinary in-repo file, no project dir	config.env	plugins/…/config.env
ordinary in-repo file, project dir set	plugins/…/config.env	unchanged
symlinked repo, physical spelling, no project dir	config.env	src/config.env
symlinked repo, project dir = symlink	sub/config.env	unchanged

Fail-then-pass, hooks reverted to origin/main with the new tests kept, reproduced independently of the authoring run:

PASS=57 FAIL=7
FAIL: UNC/no-project: data.file is exactly the basename: expected 'secrets.env', got '\\srv\share\secrets.env'
FAIL: UNC/no-project: data.file keeps no backslash: unexpected '\' in output
FAIL: UNC/no-project: data.file drops the share host: unexpected 'srv' in output
FAIL: in-repo/no-project: data.file is repo-relative: 'src/config.env' not in output: config.env
FAIL: symlinked repo, physical spelling: repo-relative: 'src/config.env' not in output: config.env
FAIL: path helper: uses hook::repo_relative_path
FAIL: path helper: no hand-rolled prefix strip

And for the trailing-slash regression, against the pre-trim hooks with those cases in place:

secret-pattern-detection  PASS=64 FAIL=1  expected 'src/config.env', got 'config.env'
hardcoded-path-check      PASS=99 FAIL=1  expected 'src/run.sh',     got 'run.sh'

Against the current head: secret-pattern-detection 65/0, hardcoded-path-check 100/0.

Two disclosures rather than a clean claim:

• The UNC assertion was first written as a containment check, which passed pre-fix because the leaked path contains the basename. It is now an equality check and discriminates on its own; its failure prints the leaked path, which is the clearest evidence of the defect. assert_eq was added to guardrails-test-helpers.sh in the shape claude-ops-test-helpers.sh already uses.
• One assertion in hardcoded-path-check.test.sh passes both ways and is labelled in-file as a regression pin, not a fail-then-pass case. It exists because that suite asserted nothing about data.file before and the computation just moved into emit_tel, so without it a relocation mistake would pass every other case there.

The symlinked-root fixture #3463 used does not discriminate for these two hooks, and is not shipped as a standalone case: when the strip fails, a POSIX-absolute path matches the old case arm too, so both sides land on the same basename. The entire behavioral delta is the missing UNC arm plus the missing root resolution.

Gates, all exit 0: check-changelog-parity.sh --check and --check-bump origin/main, validate-plugins.sh, sync-hook-utils.sh --check ("All 17 plugin copies match"), shellcheck -x, check-shell-portability.sh --paths ("No unexcused GNU-only constructs"), check-purged-em-dashes.sh, typos, editorconfig-checker, markdownlint-cli2. affected-tests.sh --run: all 17 selected suites passed, zero FAIL and zero NOT RUN: lines. Selection widened from 6 to 17 because touching the shared guardrails-test-helpers.sh fans out to every suite sourcing it.

Not verified, stated plainly: everything was measured on Linux, so the cygpath branch of hook::repo_relative_path is exercised by reading rather than running, and the real UNC-mount trigger is reproduced by driving the code path with a UNC-shaped string rather than on a Windows share. The old copies folded \ to / before stripping and the helper does not; on a shell with no cygpath and a genuine backslash-separated path below the root, the helper degrades where the copy stripped. That shape is not constructible as a real Linux path, and #3463's twelve sites accepted the same trade, so the helper's behavior was not forked for these two.

Related

Follow-up to #3463, which hoisted the helper and whose completeness grep missed these two copies. Refs #1133 (the original absolute-path leak) and ADR 0019. Refs .claude/rules/hook-budget.md.

---

Generated by Claude Code

[claude]

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

---

Claude finished the code review

• Gather context (PR body, diff, custom instructions)
• Run /review:code-review per custom instructions
• Post findings as inline comments (none needed — see below)
• Post summary comment

Scope

This is the code-review lane; a claude-security-review.yml workflow exists in this repo, so security findings are out of scope here (owned by that lane). No tool execution permissions are available in this lane, so verification below is static (reading code/tests), not running the suites — the PR body's own PASS/FAIL numbers are author-claimed and not independently re-run by me.

What I checked

• hardcoded-path-check.sh (diff): confirmed the scope guard at L104 ([[ -n "${CLAUDE_PROJECT_DIR:-}" ]] || exit 0) guarantees PROJECT_ROOT is non-empty by the time emit_tel runs, so the claim "the leaking shape was never reachable there" holds.
• secret-pattern-detection.sh (diff): this hook's scope guard (L109-L114) deliberately falls through rather than exiting when CLAUDE_PROJECT_DIR is unset, so emit_tel's root="${CLAUDE_PROJECT_DIR:-}"; [[ -n "$root" ]] || root="$(hook::repo_root "$(dirname "$FILE")")" fallback is real and reachable. Traced hook::repo_root/hook::repo_relative_path in hook-utils.sh (unchanged by this PR — the UNC arm was already there from refactor(hooks): hoist the repo-relative path helper into lib/hook-utils.sh #3463): for a UNC file_path, dirname on Linux returns ., so the resolved root is irrelevant noise — the helper's case on \\\\* catches it regardless and degrades to the basename. Verified this holds for the new fixtures (UNC, in-repo no-project, symlinked checkout) by reading the redaction logic against each input.
• file_rel has one consumer: confirmed by grep in both hooks — only jq -n --arg file "$file_rel". Neither hpp::scan_text nor secrets::scan_text takes it, so the FILE_REL_DEGRADED branch genuinely isn't needed here, matching the PR's claim.
• Test wiring: HOOK_SRC, AWS_TOKEN, LINUX_HOME, assert_absent all pre-exist and are reused correctly; the new assert_eq in guardrails-test-helpers.sh is straightforward and the PR's own callout (containment vs. equality) is accurate — a \\srv\share\secrets.env leak does contain secrets.env, so assert_contains would have been a false-negative-prone test.
• hardcoded-path-check.test.sh's new regression-pin case runs against $TEST_TMPDIR, which is git init-ed at L17 of that file, so the hook's is-inside-work-tree scope check is satisfiable there — the fixture is valid.
• Version bump (0.29.21 → 0.29.22) and CHANGELOG entry match a patch-level bug fix; no other copies of the removed hand-rolled _fwd#/_root//\\ pattern remain outside this diff (the two unrelated hits in hardcoded-path-patterns.sh are a different, pre-existing normalization for a different purpose).

Findings

None that clear the high-signal bar. The fix is narrowly scoped, the root-cause explanation in the PR body matches what the code does, and the new tests exercise the actual gap (UNC with no project dir) rather than a proxy for it.

[claude]

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

---

Security review

• Gather context (PR diff, changed files)
• Apply skip gate
• Review changed files for security-relevant issues
• Adversarial validation of any candidates
• Post final findings

No security issues found.

This PR is itself a security fix, not a source of new risk. It closes an information-disclosure gap: secret-pattern-detection.sh's hand-rolled telemetry-path redaction tested only the POSIX-absolute and drive-letter spellings, never UNC (\\server\share\file), so on the no-project-dir scan path a UNC path leaked whole — including the internal server/share name — into the telemetry envelope's data.file. Both hooks now delegate to the shared hook::repo_relative_path helper (added in #3463), which redacts all three absolute spellings down to a basename when a path can't be made repo-relative.

Checked specifically for regressions this kind of refactor could introduce:

• Data flow to tools: confirmed by reading (not grepping) that file_rel in both hooks feeds only jq -n --arg file for the telemetry payload — never secrets::scan_text/hpp::scan_text, which take $FILE/$CONTENT directly. So a degraded (basename-only) file_rel can't cause a scanner to read/write the wrong file.
• New hook::repo_root "$(dirname "$FILE")" call (secret-pattern-detection.sh#L166): $FILE/dirname are properly quoted and passed as git -C <dir> arguments (not shell-interpolated), matching the same pattern already used unmodified in cli-flag-verify.sh, skill-reference-verify.sh, and stale-path-verify.sh — no injection surface, no new pattern introduced.
• hardcoded-path-check.sh: the scope guard above emit_tel (hardcoded-path-check.sh#L157) guarantees PROJECT_ROOT is set before emit_tel can run, so the leaking shape this PR fixes was never reachable there; removing its duplicate copy is a cleanup, not a behavior change (confirmed against the PR's own before/after table).
• hook::repo_relative_path / hook::repo_root themselves are unchanged (git diff origin/main...HEAD -- plugins/guardrails/hooks/hook-utils.sh is empty) — out of scope, and already reviewed under refactor(hooks): hoist the repo-relative path helper into lib/hook-utils.sh #3463.
• Output only reaches jq --arg, which JSON-encodes the value, so no log/JSON injection via crafted filenames.

No CRITICAL/IMPORTANT/SUGGESTION findings.

[github-actions]

Last security-reviewed head: 07c22e41d5a1cd6dc881e79b760a49e2ecb9f598. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab6f01760f

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

[claude]

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

---

Security review

• Gather context (PR body, diff, custom instructions)
• Invoke /review:security-review and follow its instructions
• Review changed files for security-relevant issues
• Post final findings

No security issues found.

This PR closes an information-disclosure gap (a UNC path leaking whole, share host included, into the telemetry envelope's data.file) and does not introduce new risk. I reviewed the full diff against origin/main at HEAD 07c22e41d5a1cd6dc881e79b760a49e2ecb9f598, which includes one commit beyond what was reviewed previously.

New since the last review on this PR: 07c22e4 trims a trailing separator off the caller-supplied root before it reaches hook::repo_relative_path, in both hooks:

• secret-pattern-detection.sh#L173 — root="${root%/}"
• hardcoded-path-check.sh#L210 — hook::repo_relative_path "$FILE" "${PROJECT_ROOT%/}"

This is a correctness fix for the Codex P2 finding on the prior commit (a CLAUDE_PROJECT_DIR ending in / formed the unmatched prefix /repo// in hook::repo_relative_path, collapsing in-project files to a bare basename), not a security regression — the failure mode it fixed was over-redaction (losing path context), not a leak. Both trims use plain bash parameter expansion (${var%/}) on values already handled elsewhere in these hooks; no new injection surface. Confirmed against hook::repo_relative_path's own stripping logic in hook-utils.sh (unchanged by this PR): it strips exactly "$root"/, so an untrimmed trailing slash is exactly the failure mode described, and the fix addresses it without altering the UNC/absolute-path redaction arms this PR added.

Re-checked the points from the prior review and they still hold against this head:

• file_rel in both hooks still feeds only jq -n --arg file (JSON-encoded telemetry), never a scanner (secrets::scan_text/hpp::scan_text take $FILE/$CONTENT directly) — a degraded or mistrimmed file_rel can't misdirect a read/write.
• hardcoded-path-check.sh's scope guard still guarantees PROJECT_ROOT is set before emit_tel runs.
• hook::repo_relative_path / hook::repo_root themselves remain unchanged by this PR.

No CRITICAL/IMPORTANT/SUGGESTION findings.

[claude]

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

---

Code review

• Gather context (PR body, diff, custom instructions)
• Run /review:code-review per custom instructions
• Post findings as inline comments (none needed — see below)
• Post summary comment

Scope

CODE-REVIEW lane; a claude-security-review workflow exists in this repo, so security findings stay out of scope here. Reviewed the full diff between origin/main and HEAD (07c22e4, 7 files) by reading the changed hooks, their tests, hook-utils.sh's hook::repo_relative_path/hook::repo_root (unchanged by this PR), the CHANGELOG, and the version bump.

Codex's P2 finding is already fixed on this head

Codex flagged that a CLAUDE_PROJECT_DIR/PROJECT_ROOT ending in / would make hook::repo_relative_path's "${file#"$root"/}" strip fail (prefix becomes /repo//), degrading every in-project path to a bare basename — a real regression versus the removed hand-rolled copies, which trimmed the trailing slash first.

That review was against ab6f017. The current head's commit
07c22e4 already closes it: both call sites now trim before calling the helper —
secret-pattern-detection.sh#L166-L173 (root="${root%/}" before the helper call, covering both the CLAUDE_PROJECT_DIR and the hook::repo_root fallback paths) and
hardcoded-path-check.sh#L210 ("${PROJECT_ROOT%/}" inline). Both fixes are pinned by new tests added in the same commit — a trailing-slash CLAUDE_PROJECT_DIR case in each *.test.sh asserting data.file stays repo-relative rather than collapsing to a basename (hardcoded-path-check.test.sh#L541-L551, secret-pattern-detection.test.sh#L268-L279), and the CHANGELOG entry documents it as a second fix. No inline comment needed since the code at this SHA already reflects the suggested change.

What else I checked

• hardcoded-path-check.sh: the scope guard at L104 ([[ -n "${CLAUDE_PROJECT_DIR:-}" ]] || exit 0) guarantees PROJECT_ROOT is non-empty by the time emit_tel runs, so removing its duplicate hand-rolled block is a pure cleanup — confirmed against the PR's own before/after table, and the UNC-leak shape was never reachable there.
• secret-pattern-detection.sh: this hook's scope guard deliberately falls through (rather than exiting) when CLAUDE_PROJECT_DIR is unset, so the hook::repo_root "$(dirname "$FILE")" fallback is real and reachable. For a UNC file_path, dirname on Linux returns ., so the resolved root is irrelevant noise — hook::repo_relative_path's \\\\* case arm catches it regardless and degrades to the basename, matching the new UNC test's assertions.
• file_rel has exactly one consumer in both hooks (jq -n --arg file) — confirmed by reading, not grepping. Neither hpp::scan_text nor secrets::scan_text takes it, so a degraded file_rel can't misdirect either scanner.
• hook::repo_relative_path/hook::repo_root (hook-utils.sh) are unchanged by this PR (git diff origin/main...HEAD on that file is empty) — out of scope, already the subject of refactor(hooks): hoist the repo-relative path helper into lib/hook-utils.sh #3463.
• Version bump (0.29.21 → 0.29.22) and CHANGELOG entries match a patch-level fix and now cover both the UNC leak and the trailing-slash regression.
• assert_eq in guardrails-test-helpers.sh follows the file's existing style for the other assert_* helpers (no blank-line separation between adjacent functions is the established convention there, not a gap introduced by this diff).

Findings

None that clear the high-signal bar on this head. The one substantive issue raised in review (Codex's trailing-separator finding) is already fixed and test-covered as of 07c22e4.