Skip to content

sec(scripts): anchor .gitallowed entries and add allowlist self-test (#2170) - #2172

Merged
cristim merged 7 commits into
mainfrom
sec/gitallowed-anchor-2170
Oct 12, 2026
Merged

cristim merged 7 commits into
mainfrom
sec/gitallowed-anchor-2170

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Closes #2170. Port of the cloud-commitments-mcp fix (PR #57).

  • .gitallowed: removed the 18 bare account-ID/UUID entries (git-secrets matches allowlist regexes against the whole path:line:content line, so any line containing one was exempt from every detector). Remaining entries are anchored on path and whole line.
  • scripts/setup-git-secrets.sh: removed the 20 keyword --allowed entries (same defect); added the no-op say() shim. Root cause of the hang: git-secrets 1.3.0 calls say, which on macOS resolves to /usr/bin/say (text-to-speech) and blocks the install. Hooks are now verified after install so a failure is loud. Dropped the type.*service_account detector, as mcp did.
  • scripts/test-git-secrets-allowlist.sh: new self-test (HOME-isolated, runtime-assembled synthetic fixtures), with must-catch cases for placeholder plus secret on one line and a check that every active entry is anchored.
  • CI: pin git-secrets to the verified 1.3.0 SHA and run the self-test.

Evidence (local, real git-secrets binary): self-test 41 PASS / 0 FAIL. Against the old .gitallowed it gives 29 PASS / 12 FAIL, including "account-id placeholder with a key on the same line" and "UUID placeholder with a password on the same line" scanning clean. setup-git-secrets.sh now completes on a clone of this branch, with the repo-wide scan clean.

The local actionlint pre-commit hook was skipped on the workflow commit (Docker daemon unresponsive on this machine); CI runs it.

🤖 Generated with Claude Code

cristim and others added 5 commits October 12, 2026 19:00
Ported from cloud-commitments-mcp, plus a structural check that every
.gitallowed entry is anchored and must-catch cases with a placeholder and a
secret on one line.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…all hang (#2170)

Port of the mcp fix. The keyword --allowed entries matched whole
path:line:content lines. A no-op say() shim stops git-secrets calling
macOS /usr/bin/say, which hung the install; hooks are verified after
install, and the PEM and regex detectors are fixed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Bare account-ID/UUID entries exempted any line containing them from every
detector. Keep only path-and-line anchored entries.

Closes #2170

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The repo-wide scan in setup-git-secrets.sh flags separator lines and
truncated PEM literals once the bare entries are gone. Each is anchored on
path and whole line. Fixtures in the self-test no longer spell a
password assignment literally.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/s Hours type/security Security finding triaged Item has been triaged labels Oct 12, 2026
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 4 billable files and costs up to $1.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 10 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 62 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: c5d655d8-dce7-41e0-a6ec-2c5806d6aec0

📥 Commits

Reviewing files that changed from the base of the PR and between 17ed955 and a29d9ce.


📒 Files selected for processing (4)
  • .gitallowed
  • .github/workflows/pre-commit.yml
  • scripts/setup-git-secrets.sh
  • scripts/test-git-secrets-allowlist.sh

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 12, 2026 •

Copy link
Copy Markdown
Member Author

Independent merge-gate review (Opus 5.5): CHANGES REQUESTED

Reviewed head: 5806c3c375570dce9a03255bf6451884e74ef3ce (base main @ 17ed9555). Not merging.

Blocking: removing the bare .gitallowed entries breaks the CI git-secrets hook (real regression)

Run pre-commit hooks fails on this SHA (run 38282720535, step Run pre-commit, hook Scan for AWS secrets), while main @ 17ed955 is green. 23 test-fixture lines are flagged by the --register-aws account-ID pattern now that the bare 123456789012 / 111111111111 / ... entries are gone:
cmd/helpers_test.go:88,103,114, cmd/multi_service_filters_test.go:375,382,389,396,403, cmd/multi_service_helpers_test.go:655,656,680,696-698,770,831-833, cmd/multi_service_test.go:1139, cmd/savings_plan_purchase_test.go:40,57,164,188.

Reproduced locally (macOS, git-secrets 1.3.0, HOME-isolated clone, setup script's patterns): git ls-files -z | xargs -0 git secrets --scan -> rc=1, the same 23 lines.

Why the PR's "repo-wide scan clean" evidence missed it: git secrets --scan with no file args uses git grep -E, which does not honour \s in the AWS account-ID pattern here; with file args (what the pre-commit framework passes, entry: git-secrets --scan + filenames) it uses plain grep -E, which does. Same pattern, same file: git grep -nwHEI "$P" -- cmd/helpers_test.go -> 0 matches, grep -nwHEI "$P" cmd/helpers_test.go -> 3.

Fix options: (a) add path-and-whole-line anchored entries for these fixture shapes (e.g. ^cmd/[a-z_]+_test\.go:[0-9]+:...$ covering the accountID: "NNN", / {Account: "NNN", AccountName: ""}, / Account: "NNN", Term: lines), or (b) change the fixtures so they do not match the account-ID pattern. Re-verify with the file-argument scan above (and ideally make the self-test exercise the file-argument path too), not the no-args scan.

Verified OK (evidence)

  • Self-test on head: 41 PASS, 0 FAIL (5s). Head script + main's .gitallowed: 29 PASS, 12 FAIL, incl. "account-id placeholder with a key on the same line" and "UUID placeholder with a password on the same line" scanning clean. CI step Run git-secrets allowlist self-test: success.
  • All active .gitallowed entries are ^...$ anchored; all but the truncated-PEM entry also anchor the path (that one anchors ^[^:]+:[0-9]+: + whole line, ported from mcp; non-blocking).
  • Dropping the 20 keyword --allowed entries is correct in intent, but on the file-argument path the same 23 lines above were also covered by the old _test\.go keyword: file-arg scan with head .gitallowed = 23 findings, with the 20 old keywords re-added = 0. So the anchored replacements for those lines are missing from both removals, not just the bare entries.
  • Hang root cause: git secrets --install -f calls say (git-secrets:208); Git 2.50's git-sh-setup has no say, so it resolves to /usr/bin/say, which blocks on audio output (timeout 20 /usr/bin/say hi -> 124; say -o file -> 0). Old script: rc=124 after 150s, stuck at git secrets --install -f. New script: rc=0 in 1s, with hook verification. Non-blocking nit: the comment says /usr/bin/say "exits 0 by accident" on macOS; here it hangs.
  • ad82d68ee924906a0401dfd48de5057731a9bc84 is a real awslabs/git-secrets commit ("Prepare for 1.3.0 release") and is what tag 1.3.0 points to (gh api).
  • Fixtures are runtime-assembled synthetic strings; scripts use set -euo pipefail, mktemp -d + trap cleanup, quoted expansions; no injection paths found.
  • Labels mirror sec(scripts): .gitallowed bare placeholder entries exempt whole lines from every secret detector; no allowlist self-test (see mcp#24/PR #57) #2170. Mutation sweep not run (CI/tooling change; policy).

)

The AWS account-ID detector in CI flags placeholder IDs in Go test tables
once the bare entries are gone. Each is anchored on path, placeholder digits
and whole line; a self-test case checks an appended key is still caught.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Re-gate (Opus 5.5) at 1f4d6f7f51d30149bcccc8f8426e532a2b4b3a46: CHANGES REQUESTED

Not merging. The new commit fixes the 23 fixture lines but adds three new account-ID literals that the same AWS account-ID detector flags.

Blocking: CI self-test fails on this SHA

Run 38283717393, step Run git-secrets allowlist self-test: FAIL: .gitallowed self-scan (direct scan) and (installed hook), 41 PASS, 2 FAIL, exit 1. So Run pre-commit never ran on this SHA either.

Reproduced locally (HOME-isolated clone, setup script's patterns, git ls-files -z | xargs -0 git secrets --scan): rc=1, 3 findings:

  • .gitallowed:90: ...Account: "111111111111", Term... (the new savings_plan entry is a literal that matches the account-ID pattern)
  • .gitallowed:91: ...r\.Account = "222222222222" ...
  • scripts/test-git-secrets-allowlist.sh:192: accountID: "123456789012", k := ... (literal in the new fixture)

Locally on macOS the self-test still passes (43/0) only because git grep -E there ignores \s in that pattern (see my previous comment); GNU grep and git grep on the Linux runner do not.

Suggested fix: in .gitallowed, break the digit runs with the same single-character bracket trick the file already uses for its other entries (for example 11111111111[1], 22222222222[2]). That stops the "NNNNNNNNNNNN" literal from matching the detector while still matching the same text. The two-digit alternation entries at lines 88-89 are not flagged today because (accountID|Account): puts no quote directly before the digits; check them too. In the self-test, build the line 192 fixture from $ACCT, as lines 184-186 already do. Then re-verify with the file-argument scan above, which matches the pre-commit and CI path.

Otherwise re-verified on this SHA

  • Self-test with main's .gitallowed: 30 PASS / 13 FAIL. Both "secret plus placeholder on the same line" cases are still caught, so the hole stays closed. The new "key appended to an allowed account-ID fixture line" case is a good addition.
  • All active entries are ^...$ anchored, and the new ones pin the path, the placeholder digits and the whole line. Not broad.
  • Non-blocking: the self-test does not yet run a file-argument scan of the tree, and the say comment ("exits 0 by accident" on macOS; it hangs here) is unchanged.
  • Labels mirror sec(scripts): .gitallowed bare placeholder entries exempt whole lines from every secret detector; no allowlist self-test (see mcp#24/PR #57) #2170. No mutation sweep (tooling change, per policy).

…ent scan to self-test (#2170)

The new account-ID entries and one self-test fixture were themselves
flagged by the AWS account-ID pattern under the file-argument scan the
pre-commit framework uses. Bracket one digit per entry, build the fixture
from $ACCT, and make the self-test run that CI-equivalent scan over all
tracked files. Correct the say() comment: on macOS /usr/bin/say can hang on
audio output.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Addressed both gate rounds on a29d9ce: the three placeholder digit runs flagged by the file-argument scan are bracketed (11111111111[1], 22222222222[2], fixture built from $ACCT); the self-test now runs the file-argument scan over all tracked files (44 PASS / 0 FAIL; the new check failed 43/1 before the bracketing); say() comment corrected. Verified in a clean clone of this head: pre-commit run --all-files with the AWS patterns registered, 0 failures (actionlint and hadolint skipped locally, Docker daemon unresponsive; CI runs them), and git ls-files -z | xargs -0 git secrets --scan rc=0.

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Re-gate (Opus 5.5) at a29d9ce42d6df89d2acb0c373f82a5f481f57214: APPROVED, merging

Both previous blocking findings are fixed. Evidence on this exact SHA:

  • CI: every check passes. In Run pre-commit hooks (run 38284789322), Run git-secrets allowlist self-test prints 44 PASS, 0 FAIL and the Scan for AWS secrets hook passes. Build, Lint Code, Lint Workflows, Unit Tests, Integration Tests, Security Scanning, Snyk and CI Success all pass. mergeStateStatus CLEAN.
  • Local check (macOS, git-secrets 1.3.0, HOME-isolated clone, setup script's 15 patterns): git ls-files -z | xargs -0 git secrets --scan gives rc=0 and 0 findings. The no-argument scan also gives rc=0.
  • Self-test on this head: 44 PASS / 0 FAIL. With main's bare-entry .gitallowed it gives 30 PASS / 14 FAIL, and both "secret plus placeholder on the same line" cases are caught, so the hole stays closed.
  • All active .gitallowed entries are ^...$ anchored (structural check: 0 unanchored). The bracketed-digit entries no longer match themselves, since the file-argument scan of .gitallowed is clean. The new fixture entries pin path, digits and whole line.
  • The self-test now includes a file-argument scan of all tracked files (the same path as the pre-commit framework). The say comment now describes the hang correctly. The fixture at line 192 is built at runtime from $ACCT.
  • Previously verified, unchanged: the git-secrets pin ad82d68e is tag 1.3.0 of awslabs/git-secrets; the hang root cause is /usr/bin/say blocking on audio, fixed by the say no-op plus hook verification; no real secrets; set -euo pipefail, mktemp and quoted variables are used.
  • Labels mirror sec(scripts): .gitallowed bare placeholder entries exempt whole lines from every secret detector; no allowlist self-test (see mcp#24/PR #57) #2170. No mutation sweep (tooling change, per policy).

@cristim
cristim merged commit e0381af into main Oct 12, 2026
12 checks passed
@cristim
cristim deleted the sec/gitallowed-anchor-2170 branch October 17, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(scripts): .gitallowed bare placeholder entries exempt whole lines from every secret detector; no allowlist self-test (see mcp#24/PR #57)

1 participant