chore(security): track the gate kit and close the binary secret-scan gap - #7
Conversation
.gitleaks.toml and lefthook.yml were provisioned onto disk but never committed. They run locally, so nothing surfaced the gap -- but they were unversioned, unreviewed, and would vanish on a fresh clone or git clean. Committed as-is, byte-identical to the standard kit across all repos, so this is an accurate baseline and any later change reads as a real diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded repository-wide secret scanning with shared Gitleaks rules, version verification, staged binary inspection, Lefthook integration, self-tests, and a pinned full-history GitHub Actions workflow. ChangesSecret scanning controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Lefthook
participant gitleaks_guard
participant scan_staged_binaries
participant GitIndex
Developer->>Lefthook: create commit or push
Lefthook->>gitleaks_guard: scan staged text content
gitleaks_guard->>GitIndex: read staged paths
Lefthook->>scan_staged_binaries: scan staged binary content
scan_staged_binaries->>GitIndex: read staged blobs
Lefthook-->>Developer: allow or block operation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
.github/workflows/secret-scan.yml (2)
62-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout and retries to the release download.
curl -sSfLhas no time limit and no retry. A transient GitHub release outage or a stalled connection makes this step hang until the job timeout, and the secret scan never runs. The checksum check below still protects integrity, so this is a reliability change only.♻️ Proposed change
- curl -sSfL -o "$tarball" \ + curl -sSfL --retry 3 --retry-all-errors --connect-timeout 10 --max-time 120 -o "$tarball" \ "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_PINNED_VERSION}/${tarball}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/secret-scan.yml around lines 62 - 63, Update the release download command in the secret-scan workflow to enforce a bounded connection/overall timeout and retry transient failures, while preserving the existing silent, fail-fast, and redirect-following behavior and leaving the checksum verification unchanged.
34-36: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.The job runs no git operation that needs the token after checkout.
actions/checkoutwrites the token into.git/configby default. Disabling persistence removes that credential from the workspace that this job then scans and shares with every later step.🛡️ Proposed change
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 with: fetch-depth: 0 # the history scan needs the whole history + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/secret-scan.yml around lines 34 - 36, Update the actions/checkout step in the secret-scan workflow to set persist-credentials to false alongside fetch-depth, ensuring the checkout token is not retained in .git/config for subsequent steps.Source: Linters/SAST tools
scripts/test-gitleaks-guard.sh (1)
311-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
--configexplicitly on the staged scan.
scan()passes--config "$CONFIG".staged_scan()does not. It relies on gitleaks discovering$STAGE/.gitleaks.tomlfrom the working directory. The failure mode is safe, because a missed discovery makes thecoderabbit-api-keyassertion fail. It is still an unnecessary difference between the two helpers, and the copy at line 364 exists only to serve that discovery.An explicit flag removes both the copy and the assumption:
♻️ Proposed change
- ( cd "$STAGE" && "$GUARD" git --staged --redact --no-banner --log-level error --exit-code 0 \ + ( cd "$STAGE" && "$GUARD" git --staged --config "$CONFIG" \ + --redact --no-banner --log-level error --exit-code 0 \ --report-format json --report-path "$1" >/dev/null 2>&1 ) || rc=$?Keep the copy if you also want to prove that discovery works. If so, state that intent in the comment at line 364.
Also applies to: 364-365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-gitleaks-guard.sh` around lines 311 - 315, Update staged_scan() to pass --config "$CONFIG" explicitly, matching scan(), and remove the now-unnecessary staged .gitleaks.toml copy around the referenced setup lines; retain that copy only if the test intentionally validates configuration discovery and document that intent in its comment..gitleaks.toml (2)
155-155: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAnchor the fixtures exclusion like its neighbours.
(^|/)tests?/fixtures/is the anchored form. The current entry has no path-segment anchor, so it also matches directories whose names only end intest/tests, for exampleunittests/fixtures/orsrc/smoketests/fixtures/. Those paths are then excluded from scanning without anyone intending it. This is the same class of over-match the comment block records for the image entry.♻️ Proposed change
- '''(^|/)tests?/fixtures/''', + '''(^|/)tests?/fixtures/''',Note: the diff above is a no-op placeholder; the actual change is to line 155:
'''(^|/)tests?/fixtures/''',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitleaks.toml at line 155, Update the fixtures exclusion pattern in the gitleaks configuration to use the path-segment anchor `(^|/)tests?/fixtures/`, matching the neighboring exclusions and preventing matches for directory names that merely end in “test” or “tests”.
158-164: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict global allowlist regexes to exact placeholders or specific rules.
The unanchored patterns suppress valid keys containing
sample,dummy, orxxxx. Anchor placeholder patterns to the complete secret or move them to rule-specific allowlists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitleaks.toml around lines 158 - 164, Restrict the global regexes in regexes to exact placeholder values by anchoring the placeholder alternatives, or move them into rule-specific allowlists. Ensure valid secrets containing words such as sample, dummy, or xxxx are not globally suppressed, while preserving detection of complete placeholder values and the existing environment-variable reference patterns.scripts/gitleaks-guard.sh (1)
60-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
sort -Vwith a portable version comparisonThe local macOS hook uses BSD
sort, which does not support-V. Underset -euo pipefail, the assignment exits beforedie()can report the cause. Use portable comparison logic or detect unsupported-Vand calldie(). Also preserve prerelease information;8.30.1-rc1currently passes as8.30.1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gitleaks-guard.sh` around lines 60 - 65, Update the version check around GITLEAKS_MIN_VERSION and found to avoid relying on non-portable sort -V, ensuring unsupported tooling cannot terminate before die() reports the issue. Implement portable comparison logic that correctly handles prerelease identifiers such as 8.30.1-rc1 rather than treating them as the final 8.30.1 release, while preserving the existing upgrade error behavior.scripts/scan-staged-binaries.sh (1)
292-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRoute a
git cat-filefailure into the unknown bucket.Line 292 and Line 306 run
git cat-file blob ":$path"unguarded. If extraction of the staged blob fails,set -eaborts the run. The gate then exits non-zero with a bare git message, and the remaining staged files are never classified. The exit code is still fail-closed, but the report loses both the cause and the coverage.♻️ Proposed change (apply the same pattern at Line 306)
- git cat-file blob ":$path" > "$staged" + if ! git cat-file blob ":$path" > "$staged" 2>/dev/null; then + unknown+=("$path (could not read the staged blob)") + continue + fi run_gitleaks_archive "$path" "$staged"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/scan-staged-binaries.sh` around lines 292 - 293, Guard both staged-blob extraction commands in the scanning flow around run_gitleaks_archive so a failed git cat-file does not trigger set -e termination. Record the extraction failure through the script’s existing unknown classification and reporting path, then continue processing remaining staged files while preserving the fail-closed exit status.scripts/selftest-binary-scan.sh (1)
216-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the archive canary causes.
Case 10 exercises only the
stdincanary. The archive branch inscripts/scan-staged-binaries.shmaps four distinct causes (bad,nopython,buildfail,norule) to four distinct messages, and none of them is asserted. That mapping is the exact cause-conflation the split was made to prevent, so a wrong mapping would pass this suite.A minimal addition: stage an
.xlsxunder the same stub PATH and assert "cannot traverse archives". A second case can strip the canary rule from the copied.gitleaks.tomland assert thenorulemessage.🧪 Proposed additional case
+# 11. Archive branch must name the archive canary, not the stdin one. +new_repo c11 +build_xlsx book.xlsx "token $SECRET" +git add book.xlsx +set +e +STUB_OUT="$(PATH="$WORK/stub:$PATH" "$SUT" 2>&1)"; STUB_RC=$? +set -e +if [ "$STUB_RC" -ne 0 ] && grep -q 'cannot traverse archives' <<<"$STUB_OUT"; then + ok "unsupported gitleaks names the ARCHIVE canary for xlsx" +else bad "archive canary cause not reported (rc=$STUB_RC)" "$STUB_OUT"; fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/selftest-binary-scan.sh` around lines 216 - 239, Extend the self-test after the existing stdin canary case to cover the archive canary in scripts/scan-staged-binaries.sh: stage an .xlsx file while using the stub gitleaks PATH and assert the failure output contains “cannot traverse archives”. Add a second scenario using a copied configuration with the archive canary rule removed, and assert the distinct norule message, preserving nonzero exit checks for both cases.lefthook.yml (1)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInvoke the binary scan the same way as the other scripts.
Line 84 uses
./scripts/gitleaks-guard.sh, and this command usesscripts/scan-staged-binaries.sh. Use one form. Invoking throughbashalso removes the dependency on the executable bit, which is not preserved by every checkout.♻️ Proposed change
secrets-binaries: tags: security - run: scripts/scan-staged-binaries.sh + run: bash ./scripts/scan-staged-binaries.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lefthook.yml` around lines 100 - 102, Update the secrets-binaries hook command to invoke scan-staged-binaries.sh consistently with the other scripts, using the repository-relative ./scripts form and bash so execution does not depend on the file’s executable bit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/secret-scan.yml:
- Around line 101-108: Update the gitleaks result handling around the existing
zero-byte check to first assert that the expected scan-summary line is present
in out; if it is missing, emit a workflow error and fail the step as UNKNOWN.
Only evaluate the scanned-byte value after this presence check, preserving the
existing failure for zero bytes and the final exit with rc for valid summaries.
In `@lefthook.yml`:
- Around line 240-245: Update the selftest-binary-scan hook to invoke
scripts/selftest-binary-scan.sh unconditionally, removing the executable check
so missing or non-executable scripts cause the job to fail rather than silently
pass. Keep the existing security tag and hook structure unchanged.
- Around line 144-152: Quote the {staged_files} expansion in the loop within the
staged-file size check so paths containing spaces remain a single filename;
preserve the existing file validation and oversized-file rejection behavior.
- Around line 19-27: Set pre-commit.parallel to false in the pre-commit
configuration so the format-python command completes and stages formatter
changes before the secrets and secrets-binaries scans execute, preserving the
existing stage_fixed behavior.
In `@scripts/scan-staged-binaries.sh`:
- Around line 98-134: Detect a missing gitleaks executable explicitly in both
canary_stdin_ok and canary_archive_ok, returning a distinct status code while
preserving their existing outcomes for scan failures and configuration issues.
Update the return-code contract comment for canary_archive_ok, map the new
archive status in the caller as archive_canary=nogitleaks, and report “gitleaks
is not installed”; also distinguish canary_stdin_ok’s missing-gitleaks status
from its normal failure in the PDF branch.
---
Nitpick comments:
In @.github/workflows/secret-scan.yml:
- Around line 62-63: Update the release download command in the secret-scan
workflow to enforce a bounded connection/overall timeout and retry transient
failures, while preserving the existing silent, fail-fast, and
redirect-following behavior and leaving the checksum verification unchanged.
- Around line 34-36: Update the actions/checkout step in the secret-scan
workflow to set persist-credentials to false alongside fetch-depth, ensuring the
checkout token is not retained in .git/config for subsequent steps.
In @.gitleaks.toml:
- Line 155: Update the fixtures exclusion pattern in the gitleaks configuration
to use the path-segment anchor `(^|/)tests?/fixtures/`, matching the neighboring
exclusions and preventing matches for directory names that merely end in “test”
or “tests”.
- Around line 158-164: Restrict the global regexes in regexes to exact
placeholder values by anchoring the placeholder alternatives, or move them into
rule-specific allowlists. Ensure valid secrets containing words such as sample,
dummy, or xxxx are not globally suppressed, while preserving detection of
complete placeholder values and the existing environment-variable reference
patterns.
In `@lefthook.yml`:
- Around line 100-102: Update the secrets-binaries hook command to invoke
scan-staged-binaries.sh consistently with the other scripts, using the
repository-relative ./scripts form and bash so execution does not depend on the
file’s executable bit.
In `@scripts/gitleaks-guard.sh`:
- Around line 60-65: Update the version check around GITLEAKS_MIN_VERSION and
found to avoid relying on non-portable sort -V, ensuring unsupported tooling
cannot terminate before die() reports the issue. Implement portable comparison
logic that correctly handles prerelease identifiers such as 8.30.1-rc1 rather
than treating them as the final 8.30.1 release, while preserving the existing
upgrade error behavior.
In `@scripts/scan-staged-binaries.sh`:
- Around line 292-293: Guard both staged-blob extraction commands in the
scanning flow around run_gitleaks_archive so a failed git cat-file does not
trigger set -e termination. Record the extraction failure through the script’s
existing unknown classification and reporting path, then continue processing
remaining staged files while preserving the fail-closed exit status.
In `@scripts/selftest-binary-scan.sh`:
- Around line 216-239: Extend the self-test after the existing stdin canary case
to cover the archive canary in scripts/scan-staged-binaries.sh: stage an .xlsx
file while using the stub gitleaks PATH and assert the failure output contains
“cannot traverse archives”. Add a second scenario using a copied configuration
with the archive canary rule removed, and assert the distinct norule message,
preserving nonzero exit checks for both cases.
In `@scripts/test-gitleaks-guard.sh`:
- Around line 311-315: Update staged_scan() to pass --config "$CONFIG"
explicitly, matching scan(), and remove the now-unnecessary staged
.gitleaks.toml copy around the referenced setup lines; retain that copy only if
the test intentionally validates configuration discovery and document that
intent in its comment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e59f757-fc67-4ec4-a2d7-31afaca74694
📒 Files selected for processing (8)
.github/workflows/secret-scan.yml.gitleaks.tomllefthook.ymlscripts/gitleaks-guard.shscripts/gitleaks-version.envscripts/scan-staged-binaries.shscripts/selftest-binary-scan.shscripts/test-gitleaks-guard.sh
| # A check that passed while scanning zero input is not a pass. gitleaks | ||
| # reports the volume it examined; if that is zero, the green tick means | ||
| # "not inspected", which is a different claim from "clean". | ||
| if printf '%s' "$out" | grep -q 'scanned ~0 bytes'; then | ||
| echo "::error::the scan examined 0 bytes — treat this as UNKNOWN, not clean" | ||
| exit 1 | ||
| fi | ||
| exit "$rc" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The zero-byte check goes inert if the gitleaks summary wording changes.
This guard depends on the literal string scanned ~0 bytes in gitleaks log output. Nothing asserts that the summary line is present at all. Three changes make the check silently stop working, with no failure:
- gitleaks changes the summary wording or the unit in a later release.
- someone lowers
--log-level info, which removes the summary line. - the summary moves to stdout-only or stderr-only in a way that the current
2>&1capture no longer covers.
In each case the step reports green while performing no zero-byte verification. That is the "not inspected reads as clean" failure this workflow exists to prevent.
Assert the summary line exists before you interpret it:
🛡️ Proposed change
# A check that passed while scanning zero input is not a pass. gitleaks
# reports the volume it examined; if that is zero, the green tick means
# "not inspected", which is a different claim from "clean".
+ # Assert the summary line FIRST. Without it the grep below cannot
+ # distinguish "scanned plenty" from "wording changed and this check
+ # no longer runs".
+ if ! printf '%s' "$out" | grep -qE 'scanned ~?[0-9]'; then
+ echo "::error::no scan-volume summary in the gitleaks output — this job"
+ echo "::error::can no longer prove the scan examined anything. Update the"
+ echo "::error::pattern in this step for gitleaks ${GITLEAKS_PINNED_VERSION}."
+ exit 1
+ fi
if printf '%s' "$out" | grep -q 'scanned ~0 bytes'; then
echo "::error::the scan examined 0 bytes — treat this as UNKNOWN, not clean"
exit 1
fi
exit "$rc"The pinned version makes this stable today. The assertion is what makes the next version bump report the breakage instead of hiding it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # A check that passed while scanning zero input is not a pass. gitleaks | |
| # reports the volume it examined; if that is zero, the green tick means | |
| # "not inspected", which is a different claim from "clean". | |
| if printf '%s' "$out" | grep -q 'scanned ~0 bytes'; then | |
| echo "::error::the scan examined 0 bytes — treat this as UNKNOWN, not clean" | |
| exit 1 | |
| fi | |
| exit "$rc" | |
| # A check that passed while scanning zero input is not a pass. gitleaks | |
| # reports the volume it examined; if that is zero, the green tick means | |
| # "not inspected", which is a different claim from "clean". | |
| # Assert the summary line FIRST. Without it the grep below cannot | |
| # distinguish "scanned plenty" from "wording changed and this check | |
| # no longer runs". | |
| if ! printf '%s' "$out" | grep -qE 'scanned ~?[0-9]'; then | |
| echo "::error::no scan-volume summary in the gitleaks output — this job" | |
| echo "::error::can no longer prove the scan examined anything. Update the" | |
| echo "::error::pattern in this step for gitleaks ${GITLEAKS_PINNED_VERSION}." | |
| exit 1 | |
| fi | |
| if printf '%s' "$out" | grep -q 'scanned ~0 bytes'; then | |
| echo "::error::the scan examined 0 bytes — treat this as UNKNOWN, not clean" | |
| exit 1 | |
| fi | |
| exit "$rc" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/secret-scan.yml around lines 101 - 108, Update the
gitleaks result handling around the existing zero-byte check to first assert
that the expected scan-summary line is present in out; if it is missing, emit a
workflow error and fail the step as UNKNOWN. Only evaluate the scanned-byte
value after this presence check, preserving the existing failure for zero bytes
and the final exit with rc for valid summaries.
| selftest-binary-scan: | ||
| tags: security | ||
| run: | | ||
| if [ -x scripts/selftest-binary-scan.sh ]; then | ||
| scripts/selftest-binary-scan.sh | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not skip the self-test silently.
If scripts/selftest-binary-scan.sh is missing or loses its executable bit, this job exits 0 and prints nothing. A skipped verification then looks the same as a passing one, which is the failure mode this cohort exists to close. The neighbouring gate at Line 200 calls its script unconditionally.
🔧 Proposed change
selftest-binary-scan:
tags: security
run: |
- if [ -x scripts/selftest-binary-scan.sh ]; then
- scripts/selftest-binary-scan.sh
- fi
+ if [ -f scripts/selftest-binary-scan.sh ]; then
+ bash ./scripts/selftest-binary-scan.sh
+ else
+ echo "✗ scripts/selftest-binary-scan.sh is missing — the binary secret gate is unverified"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selftest-binary-scan: | |
| tags: security | |
| run: | | |
| if [ -x scripts/selftest-binary-scan.sh ]; then | |
| scripts/selftest-binary-scan.sh | |
| fi | |
| selftest-binary-scan: | |
| tags: security | |
| run: | | |
| if [ -f scripts/selftest-binary-scan.sh ]; then | |
| bash ./scripts/selftest-binary-scan.sh | |
| else | |
| echo "✗ scripts/selftest-binary-scan.sh is missing — the binary secret gate is unverified" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lefthook.yml` around lines 240 - 245, Update the selftest-binary-scan hook to
invoke scripts/selftest-binary-scan.sh unconditionally, removing the executable
check so missing or non-executable scripts cause the job to fail rather than
silently pass. Keep the existing security tag and hook structure unchanged.
| canary_stdin_ok() { | ||
| local rc=0 | ||
| printf 'token %s\n' "$CANARY_TOKEN" \ | ||
| | gitleaks stdin --no-banner --redact --config "$CONFIG" >/dev/null 2>&1 || rc=$? | ||
| [ "$rc" -eq 1 ] | ||
| } | ||
|
|
||
| # Returns: | ||
| # 0 = archive traversal works | ||
| # 1 = gitleaks cannot traverse archives | ||
| # 2 = python3 is absent, so the probe could not be built | ||
| # 3 = gitleaks scanned the probe and found nothing -> the config lost the rule | ||
| # 4 = python3 is present but building the zip failed | ||
| # | ||
| # Four codes for four causes, and the contract is spelled out because the first | ||
| # version of this split collapsed 2 and 4 into one code while the caller printed | ||
| # "python3 missing" for both -- reintroducing, at smaller scale, the exact | ||
| # cause-conflation this function was split up to fix. | ||
| canary_archive_ok() { | ||
| local rc=0 z="$TMPDIR_SCAN/canary.zip" | ||
| command -v python3 >/dev/null 2>&1 || return 2 | ||
| python3 - "$z" "$CANARY_TOKEN" <<'PY' 2>/dev/null || return 4 | ||
| import sys, zipfile | ||
| with zipfile.ZipFile(sys.argv[1], "w", zipfile.ZIP_DEFLATED) as z: | ||
| z.writestr("payload.txt", "token " + sys.argv[2]) | ||
| PY | ||
| gitleaks dir --no-banner --redact --config "$CONFIG" \ | ||
| --max-archive-depth "$MAX_ARCHIVE_DEPTH" "$z" >/dev/null 2>&1 || rc=$? | ||
| rm -f "$z" | ||
| # rc=1 means gitleaks found the canary: the traversal path works. rc=0 means it | ||
| # scanned and found nothing, which on a file containing the token means this | ||
| # repo's .gitleaks.toml no longer carries the rule the canary matches -- a | ||
| # config problem, not a version problem, and it must not be reported as one. | ||
| [ "$rc" -eq 1 ] && return 0 | ||
| [ "$rc" -eq 0 ] && return 3 | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report a missing gitleaks as its own cause.
Neither canary checks that gitleaks is on PATH. If the binary is absent, the shell returns 127, both canaries report bad, and the gate prints "needs 8.x". That sends the developer to an upgrade for a tool that is not installed. The PDF branch already names its missing extractor at Line 295; apply the same treatment here.
🔧 Proposed fix: add a distinct "not installed" state
canary_stdin_ok() {
local rc=0
+ command -v gitleaks >/dev/null 2>&1 || return 2
printf 'token %s\n' "$CANARY_TOKEN" \
| gitleaks stdin --no-banner --redact --config "$CONFIG" >/dev/null 2>&1 || rc=$?
[ "$rc" -eq 1 ]
} canary_archive_ok() {
local rc=0 z="$TMPDIR_SCAN/canary.zip"
+ command -v gitleaks >/dev/null 2>&1 || return 5
command -v python3 >/dev/null 2>&1 || return 2Then map the new codes in the caller: 5) archive_canary=nogitleaks ;; with the message "gitleaks is not installed", and change the PDF branch to distinguish canary_stdin_ok return 2 from return 1. Update the return-code contract comment at Lines 105-110 in the same change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/scan-staged-binaries.sh` around lines 98 - 134, Detect a missing
gitleaks executable explicitly in both canary_stdin_ok and canary_archive_ok,
returning a distinct status code while preserving their existing outcomes for
scan failures and configuration issues. Update the return-code contract comment
for canary_archive_ok, map the new archive status in the caller as
archive_canary=nogitleaks, and report “gitleaks is not installed”; also
distinguish canary_stdin_ok’s missing-gitleaks status from its normal failure in
the PDF branch.
1432159 to
b1ed8e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/secret-scan.yml (1)
15-18: 🔒 Security & Privacy | 🔵 TrivialVerify that PR scan enforcement is independent of the PR ref.
Line 15 runs this workflow for pull requests. Lines 52-61 and 111-112 load the scanner contract and self-test from that same checked-out ref. A PR can change the workflow, rules, pins, and self-test together. Confirm that an organization or repository policy requires an independent protected scan for PRs. Otherwise this scan is reproducible after merge, but it is not authoritative for untrusted PRs. GitHub evaluates workflows from the event-associated ref, and its workflow-execution protections exist to apply policy outside an individual workflow file. (docs.github.com)
Also applies to: 52-61, 108-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/secret-scan.yml around lines 15 - 18, Ensure pull_request executions of the secret-scan workflow are enforced by an organization or repository-level protected policy independent of the PR’s checked-out ref, so changes to the workflow, scanner contract, rules, pins, or self-test cannot bypass enforcement. Update the workflow configuration around on.pull_request and the referenced scan/self-test steps only as needed to integrate with that independent protection, while preserving push scans for main.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/scan-staged-binaries.sh`:
- Around line 313-341: Guard both git cat-file blob reads in the staged-binary
and PDF branches so failures do not terminate the scan under set -e. When either
read fails, add the affected path to unknown with a clear blob-read failure
reason and continue to the next file, preserving the requirement that every
staged file is classified and the existing downstream processing for successful
reads.
In `@scripts/selftest-binary-scan.sh`:
- Around line 281-291: Update the SVG test around new_repo c13 to require the
scanner to return a nonzero status and report “SECRET” in diagram.svg. Retain
the existing rejection of “NOT INSPECTED,” so the test fails both when SVG is
treated as opaque and when its content is silently ignored.
---
Nitpick comments:
In @.github/workflows/secret-scan.yml:
- Around line 15-18: Ensure pull_request executions of the secret-scan workflow
are enforced by an organization or repository-level protected policy independent
of the PR’s checked-out ref, so changes to the workflow, scanner contract,
rules, pins, or self-test cannot bypass enforcement. Update the workflow
configuration around on.pull_request and the referenced scan/self-test steps
only as needed to integrate with that independent protection, while preserving
push scans for main.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 613452e7-b733-4b1e-b53d-7a842f161484
📒 Files selected for processing (6)
.github/workflows/secret-scan.yml.gitleaks.tomllefthook.ymlscripts/gitleaks-guard.shscripts/scan-staged-binaries.shscripts/selftest-binary-scan.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/gitleaks-guard.sh
b1ed8e9 to
120da41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/selftest-binary-scan.sh (1)
157-161: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCase 2 depends on the exact gitleaks banner text.
The assertion matches the literal string
no leaks found. If gitleaks changes that wording, the case fails and reports "raw-byte scan now finds it", which names the wrong cause. The same conflation is called out at Line 155 for the fixture. Assert the exit status instead, and keep the string match only as a secondary signal.♻️ Proposed refactor
-if gitleaks stdin --no-banner --redact --config .gitleaks.toml < secret.pdf 2>&1 \ - | grep -q "no leaks found"; then +RAW_RC=0 +gitleaks stdin --no-banner --redact --config .gitleaks.toml < secret.pdf >/dev/null 2>&1 || RAW_RC=$? +if [ "$RAW_RC" -eq 0 ]; then ok "raw-byte scan of the same PDF finds nothing (why pdftotext is required)" -else bad "raw-byte scan now finds it — revisit the pdftotext dependency" ""; fi +elif [ "$RAW_RC" -eq 1 ]; then + bad "raw-byte scan now finds it — revisit the pdftotext dependency" "" +else bad "raw-byte probe errored (rc=$RAW_RC), cause unknown" ""; fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/selftest-binary-scan.sh` around lines 157 - 161, Update case 2 in the raw-byte scan around gitleaks so it asserts the command’s exit status as the primary success condition, while retaining the “no leaks found” grep only as a secondary signal. Adjust the failure message to distinguish a gitleaks wording change from an actual leak detection, consistent with the fixture assertion near this case.scripts/scan-staged-binaries.sh (1)
333-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared stdin canary and rename it.
The same seven lines appear in the PDF branch and the SVG branch. The variable is named
pdf_canary, but it now gates every stdin route, including SVG. Extract one helper and rename the memo tostdin_canary. The two copies can otherwise drift apart.♻️ Proposed refactor
+require_stdin_canary() { # returns 1 and records the cause when unusable + local path="$1" + if [ -z "$stdin_canary" ]; then + if canary_stdin_ok; then stdin_canary=ok; else stdin_canary=bad; fi + fi + [ "$stdin_canary" = ok ] && return 0 + unknown+=("$path (gitleaks on PATH has no working \`stdin\` scan — needs 8.x)") + return 1 +}Then each branch calls
require_stdin_canary "$path" || continue.Also applies to: 355-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/scan-staged-binaries.sh` around lines 333 - 339, Extract the duplicated stdin-canary logic from the PDF and SVG branches into a shared require_stdin_canary helper that accepts the path, uses the stdin_canary memo, and records the existing unknown result before signaling failure. Replace both branch-local blocks with require_stdin_canary "$path" || continue, and rename all remaining pdf_canary references to stdin_canary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/scan-staged-binaries.sh`:
- Around line 279-281: Update the report rewrite around esc_probe, esc_label,
and the following sed command to handle embedded newlines in staged filenames
and labels. Escape newlines in both operands before interpolation, or replace
the rewrite with an equivalent awk-based substitution that passes both values as
data, while preserving delimiter, BRE metacharacter, and ampersand escaping and
ensuring the scan reaches the summary.
In `@scripts/selftest-binary-scan.sh`:
- Around line 307-323: Update Case 13’s PRIMARY_RC handling after invoking
gitleaks-guard.sh to inspect PRIMARY_OUT for a guard error such as version
rejection before classifying the result. Report guard failures separately, and
only emit the “primary gate now scans .svg” failure when the command succeeded
as a guard invocation but returned a scan finding.
---
Nitpick comments:
In `@scripts/scan-staged-binaries.sh`:
- Around line 333-339: Extract the duplicated stdin-canary logic from the PDF
and SVG branches into a shared require_stdin_canary helper that accepts the
path, uses the stdin_canary memo, and records the existing unknown result before
signaling failure. Replace both branch-local blocks with require_stdin_canary
"$path" || continue, and rename all remaining pdf_canary references to
stdin_canary.
In `@scripts/selftest-binary-scan.sh`:
- Around line 157-161: Update case 2 in the raw-byte scan around gitleaks so it
asserts the command’s exit status as the primary success condition, while
retaining the “no leaks found” grep only as a secondary signal. Adjust the
failure message to distinguish a gitleaks wording change from an actual leak
detection, consistent with the fixture assertion near this case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f40b1871-b67e-411e-8223-bc8dcd0d6a8f
📒 Files selected for processing (3)
scripts/scan-staged-binaries.shscripts/selftest-binary-scan.shscripts/test-gitleaks-guard.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test-gitleaks-guard.sh
| set +e | ||
| PRIMARY_OUT=$("$SCRIPT_DIR/gitleaks-guard.sh" git --staged --redact --no-banner --verbose 2>&1) | ||
| PRIMARY_RC=$? | ||
| set -e | ||
| run_sut | ||
| if [ "$SUT_RC" -ne 0 ] && grep -q "SECRET in diagram.svg" <<<"$SUT_OUT"; then | ||
| ok "svg: secret in a staged .svg is detected (via the stdin route)" | ||
| else | ||
| bad "svg NOT detected — it is XML text with no gate (rc=$SUT_RC)" "$SUT_OUT" | ||
| fi | ||
| # Record the upstream exclusion as a fact rather than an assumption: if this ever | ||
| # starts failing, gitleaks changed its built-in allowlist and the extra route can go. | ||
| if [ "$PRIMARY_RC" -eq 0 ]; then | ||
| ok "confirmed: the primary gate still cannot see .svg (upstream allowlist)" | ||
| else | ||
| bad "primary gate now scans .svg — TEXT_ALLOWLISTED_EXTS may be redundant" "$PRIMARY_OUT" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the exit codes gitleaks-guard.sh uses for a version rejection versus a scan finding.
fd -t f 'gitleaks-guard.sh' --exec rg -n -C4 'exit [0-9]|VERSION|floor|too old|require'Repository: Jmeg8r/diffusiongemma-benchmark
Length of output: 14132
🏁 Script executed:
#!/bin/bash
# Inspect the staged-scan helper, the self-test case, and all relevant guard call sites
# to determine whether PRIMARY_RC can distinguish a finding from a guard failure.
fd -t f 'selftest-binary-scan.sh|scan-staged-binaries.sh|gitleaks-guard.sh' --exec sh -c '
for f do
echo "===== $f ====="
sed -n "1,130p" "$f" | cat -n
echo
done
' sh
printf '%s\n' '===== relevant self-test references ====='
rg -n -C8 'PRIMARY_OUT|PRIMARY_RC|gitleaks-guard|run_sut|svg NOT detected|upstream allowlist' scripts/selftest-binary-scan.shRepository: Jmeg8r/diffusiongemma-benchmark
Length of output: 38053
Distinguish guard failures from scan findings in Case 13.
gitleaks-guard.sh returns 1 for version rejection and for scanner findings. Check PRIMARY_OUT for a guard error before reporting that the primary gate scans .svg; otherwise a rejected version produces the wrong failure message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/selftest-binary-scan.sh` around lines 307 - 323, Update Case 13’s
PRIMARY_RC handling after invoking gitleaks-guard.sh to inspect PRIMARY_OUT for
a guard error such as version rejection before classifying the result. Report
guard failures separately, and only emit the “primary gate now scans .svg”
failure when the command succeeded as a guard invocation but returned a scan
finding.
120da41 to
78a1b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/secret-scan.yml:
- Around line 143-149: Update the binary-file staging pipeline in the
secret-scan workflow to preserve NUL-delimited paths from git diff through git
add, removing the newline-based tr/grep/while processing that can corrupt
filenames. Use a NUL-aware filtering and iteration approach while keeping the
existing binary extensions, diff filters, and scan-staged-binaries.sh invocation
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a182a066-1945-4fc3-9c4b-3048921559d2
📒 Files selected for processing (5)
.github/workflows/secret-scan.yml.gitleaks.tomllefthook.ymlscripts/scan-staged-binaries.shscripts/selftest-binary-scan.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- lefthook.yml
78a1b39 to
aa178cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/secret-scan.yml:
- Around line 202-204: Update the notices in the workflow step to scope the
limitation to the full-history scan, and state that binary scanning covers only
changed supported files. Include all supported formats—pdf, xlsx, docx, pptx,
svg, and zip—and remove the inaccurate claim that no scanner reads the listed
binaries.
- Around line 15-19: Update the workflow triggers and job structure so the
required secret scan runs from protected default-branch code, using a required
protected-branch workflow or pull_request_target that treats pull-request
content only as data. Keep proposed scanner configuration, pins, helper scripts,
and self-tests validated in a separate non-authoritative pull-request job, and
ensure the authoritative job never executes pull-request files.
- Around line 137-149: Update the “Scan this PR's binary files” step to select
the base commit from github.event.before for push events, while retaining the
merge-base logic for pull requests. Pass the event-before SHA through env,
reject an all-zero initial-push SHA, and verify the SHA is reachable before
assigning it to base; fail closed when validation fails so the binary scan
cannot be skipped.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fb24994-c085-4816-a196-ea2bfecd6b95
📒 Files selected for processing (1)
.github/workflows/secret-scan.yml
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [main] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow files ---'
git ls-files '.github/workflows/secret-scan.yml' 'scripts/gitleaks*' '.gitleaks.toml'
printf '%s\n' '--- workflow outline and relevant sections ---'
wc -l .github/workflows/secret-scan.yml
sed -n '1,70p' .github/workflows/secret-scan.yml
sed -n '150,220p' .github/workflows/secret-scan.yml
printf '%s\n' '--- scanner configuration and helper scripts ---'
for f in .gitleaks.toml scripts/gitleaks-version.env scripts/gitleaks-guard.sh; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
sed -n '1,240p' "$f"
fi
done
printf '%s\n' '--- self-test references and workflow expressions ---'
rg -n 'self.?test|gitleaks|github\.(base_ref|event|sha|ref)|checkout|pull_request_target|secret-scan|required' \
.github/workflows .gitleaks.toml scripts 2>/dev/null || trueRepository: Jmeg8r/diffusiongemma-benchmark
Length of output: 44813
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,240p' .github/workflows/secret-scan.ymlRepository: Jmeg8r/diffusiongemma-benchmark
Length of output: 10840
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact review locations ---'
nl -ba .github/workflows/secret-scan.yml | sed -n '13,45p;60,135p'
printf '%s\n' '--- static call-graph check ---'
python3 - <<'PY'
from pathlib import Path
workflow = Path(".github/workflows/secret-scan.yml").read_text()
required = [
"on:\n pull_request:",
"actions/checkout@",
"scripts/test-gitleaks-guard.sh",
"scripts/selftest-binary-scan.sh",
"scripts/scan-staged-binaries.sh",
"scripts/gitleaks-guard.sh",
"scripts/gitleaks-version.env",
".gitleaks.toml",
]
for item in required:
print(f"{item!r}: {item in workflow}")
checkout = workflow.index("actions/checkout@")
first_execution = min(
workflow.index("scripts/test-gitleaks-guard.sh"),
workflow.index("scripts/selftest-binary-scan.sh"),
workflow.index("scripts/scan-staged-binaries.sh"),
workflow.index("scripts/gitleaks-guard.sh"),
)
print("checkout_before_pr_file_execution:", checkout < first_execution)
print("job_name_context:", "jobs:\n scan:" in workflow)
PYRepository: Jmeg8r/diffusiongemma-benchmark
Length of output: 599
🌐 Web query:
GitHub Actions official documentation pull_request workflow runs workflow file from merge commit pull_request_target base branch context required status checks
💡 Result:
The pull_request_target event is a specialized GitHub Actions trigger designed for scenarios that require elevated privileges—such as accessing repository secrets or write permissions—when processing pull requests [1][2][3]. Contextual Differences Unlike the standard pull_request event, which runs in the context of the pull request's merge commit (a temporary merge of the head and base branches), the pull_request_target event runs in the context of the repository's default branch [1][4][2]. This ensures that the workflow file itself is always sourced from the trusted default branch, preventing attackers from modifying workflow logic via a pull request to gain unauthorized access [5][4]. Security and Checkout Behavior Because pull_request_target operates with the base repository's secrets and tokens, it is highly sensitive to malicious code injection, often referred to as a "pwn request" [6][2]. If a workflow is configured to check out and execute code from the untrusted fork (e.g., using actions/checkout with the pull request head SHA), that untrusted code will run with the workflow's elevated privileges [5][6][3]. To mitigate these risks, recent security updates have significantly tightened behavior: - As of June 2026, actions/checkout v7 by default refuses to fetch fork pull request code in pull_request_target workflows [6]. - The workflow execution context is fixed to the default branch to prevent outdated workflow files from being exploited [4]. - If a workflow genuinely requires testing code from a fork, developers must explicitly opt out of these protections [5][6]. This should be treated as a major security decision, as it potentially grants the fork's code access to the repository's environment secrets [6]. When building workflows for this event, it is recommended to use the github.base_ref and github.head_ref contexts to conditionally execute jobs based on the target or source branches [1][7]. If checking out code is necessary, it is safer to rely on immutable references (such as the specific commit SHA) rather than volatile merge refs if possible, though strict validation is required when handling untrusted fork inputs [6][8].
Citations:
- 1: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 2: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
- 3: https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/
- 4: https://github.blog/changelog/2025-11-07-actions-pull_request_target-and-environment-branch-protections-changes/
- 5: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
- 6: https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/
- 7: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 8: Checking out a merge commit in
pull_request_targetworkflows actions/checkout#518
Run the required scan from protected default-branch code.
A pull request controls this workflow and the checked-out scanner configuration, version pins, helper scripts, and self-tests. It can change the authoritative job to return success without performing the required scan.
Use a protected-branch required workflow, or a pull_request_target workflow that scans pull-request content only as data. Keep validation of proposed scanner changes in a separate non-authoritative pull-request job. Do not execute pull-request files in the authoritative job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/secret-scan.yml around lines 15 - 19, Update the workflow
triggers and job structure so the required secret scan runs from protected
default-branch code, using a required protected-branch workflow or
pull_request_target that treats pull-request content only as data. Keep proposed
scanner configuration, pins, helper scripts, and self-tests validated in a
separate non-authoritative pull-request job, and ensure the authoritative job
never executes pull-request files.
|
1. Stale pytest step blocks every push in shell-only repos. This branch carries: elif [ -d tests ]; then
pytest -q --maxfail=1A 2. Missing today's Quick check on this branch: grep -c collect_rc lefthook.yml # want 4, currently 0
grep -c 'elif \[ -d tests \]' lefthook.yml # want 0, currently 1
grep -c 'diff-filter=ACMRT' lefthook.yml # want 1, currently 0Take both blocks from No urgency — this PR is |
gitleaks' built-in allowlist, inherited via [extend] useDefault, skips pdf/xlsx/docx/bin/exe BY PATH, so the secrets gate reported "no leaks found" after reading ~0 bytes of them. scripts/scan-staged-binaries.sh closes it by changing the PATH gitleaks sees rather than the config: OOXML is copied to a *.zip temp name and traversed with --max-archive-depth, PDFs go through pdftotext into `gitleaks stdin`. .gitleaks.toml stays the single source of truth with every built-in rule live, so no rule is duplicated and none can drift. Raw-byte PDF scanning was evaluated and rejected: content streams are deflated, so a raw scan reads 0 bytes and reports clean. The scans also now route through scripts/gitleaks-guard.sh, which enforces a version floor -- allowlist semantics differ across gitleaks versions, so a scanner whose version you cannot name has coverage you cannot state. Both gates report N-scanned and fail closed: UNKNOWN, never clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aa178cf to
a363222
Compare
1 participant
What
Puts this repo's local quality-gate kit under version control, and closes a hole in it.
Two commits:
chore(security)—.gitleaks.tomlandlefthook.ymlexisted on disk but were nevercommitted. They ran locally, so nothing surfaced the gap, but they were unversioned,
unreviewed, and would vanish on a fresh clone or
git clean. Committed byte-identical tothe shared kit, so this is an accurate baseline.
fix(security)— the binary secret-scanning fix, plus the scanner version floor itdepends on.
Why the fix is needed
gitleaks' built-in allowlist, inherited via
[extend] useDefault = true, skipspdf/xlsx/docx/bin/exeby path. The pre-commit secrets gate therefore printedno leaks foundafter reading ~0 bytes of them.Verified on gitleaks 8.30.1: a real compressed PDF containing a live AWS key pair passed
both the gitleaks and aikido gates untouched.
How it is fixed
scripts/scan-staged-binaries.shdefeats the allowlist by changing the path gitleakssees rather than the config — OOXML is copied to a
*.ziptemp name and traversed with--max-archive-depth; PDFs go throughpdftotextintogitleaks stdin..gitleaks.tomlstays the single source of truth with every built-in rule live, so no rule is duplicated
and none can drift.
Raw-byte PDF scanning was evaluated and rejected: content streams are deflated, so a raw
scan reads 0 bytes and reports clean — a false green, worse than no gate. The self-test
asserts that directly.
The scans also route through
scripts/gitleaks-guard.sh, which enforces a version floor:allowlist semantics differ across gitleaks versions, so a scanner whose version you cannot
name has coverage you cannot state.
Reporting contract
Both gates report N-scanned and fail closed. A file that should have been readable but
was not comes back
UNKNOWN, never clean. Formats with no text layer (images,fbx,exe,scanned PDFs) are reported as
NOT INSPECTEDrather than passed silently.Verification
scripts/selftest-binary-scan.sh— 10 cases, run in this repo, all passingscripts/test-gitleaks-guard.sh— coverage claims asserted, run in this repo, passingwith renamed extensions
🤖 Generated with Claude Code
Summary by CodeRabbit