Skip to content

sec(scripts): anchor .gitallowed entries, drop bare placeholders - #343

Merged
cristim merged 4 commits into
mainfrom
sec/gitallowed-anchor-340
Oct 12, 2026
Merged

cristim merged 4 commits into
mainfrom
sec/gitallowed-anchor-340

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Closes #340

Git-secrets matches allowed patterns against the whole path:line:content line, so the 18 bare account-ID/UUID entries in .gitallowed exempted any line containing them from every detector (a real secret next to 123456789012 scanned clean). Port of cloud-commitments-cli#2172 / mcp#57.

  • .gitallowed: bare entries removed; replaced by ^path:line:content$-anchored entries for the lines a real scan flags (markdown/comment rules, gofmt-aligned struct lines, synthetic account-ID fixtures). Placeholder digits are bracketed so entries do not match themselves; banner text is a closed word shape that cannot spell a key or password assignment.
  • scripts/test-git-secrets-allowlist.sh: structural check that every entry is ^...$, secret-plus-placeholder must-catch cases, and a file-argument scan (git ls-files -z | xargs -0 git secrets --scan, what pre-commit runs; macOS git grep ignores \s).
  • scripts/setup-git-secrets.sh: corrected the say() comment. The rest of the hardening (say shim, hook verification, no keyword --allowed block, PEM fix) and the pre-commit.yml SHA pin plus self-test step were already on main (sec(scripts): git-secrets allowlist matched whole lines and whitelisted most of the repo #86).

Verification (macOS, git-secrets 1.3.0, HOME-isolated clean clone of head d5c76b6): self-test 44 PASS / 0 FAIL; against main's .gitallowed 36 PASS / 8 FAIL. pre-commit run --all-files with CI-style --register-aws --global: Scan for AWS secrets, detect-private-key, markdownlint, gocyclo, go vet pass. go-mod-tidy/go-fmt fail only because the isolated HOME lacks the Go toolchain; docker, gosec, trivy and test hooks skipped locally (docker daemon hangs). Mutation check: AKIA key or password appended to banner/account lines is still caught.

🤖 Generated with Claude Code

cristim and others added 2 commits October 12, 2026 19:46
…scan

Add the structural check (every .gitallowed entry is ^...$ anchored), the
secret-plus-placeholder must-catch cases, and the CI-equivalent
`git ls-files -z | xargs -0 git secrets --scan` over all tracked files.
Correct the say() comment in setup-git-secrets.sh (hang, not silent exit).

Refs #340

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
git-secrets matches allowed patterns against the whole path:line:content
line, so the 18 bare account-ID/UUID entries exempted any line containing
them from every detector. Replace them with path-and-line-anchored entries
for the lines a real scan flags; placeholder digits are bracketed so the
file does not match itself.

Closes #340

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 3 billable files and costs up to $0.75.

  • 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 23 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 61 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-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 3611bdd9-065f-45be-9f9d-b9a0310a80c4

📥 Commits

Reviewing files that changed from the base of the PR and between 2c1e87b and 99a8e9b.


📒 Files selected for processing (3)
  • .gitallowed
  • 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 and others added 2 commits October 12, 2026 20:22
… entry

GNU grep (CI) rejected the "Dry-run" banner that BSD grep accepted; reproduced locally with ggrep.

Refs #340

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

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Independent merge-gate review (Opus 5.5, claude-opus-5-5): APPROVE at 99a8e9b

Reviewed d5c76b6, then abb64b1 (my gh pr update-branch, merge of main 2c1e87b only; PR patch vs main byte-identical before/after), then the author's fix 99a8e9b (one-line .gitallowed change).

Finding at abb64b1 (now fixed): CI pre-commit failed in the self-test's file-argument scan on pkg/exchange/auto_test.go:764: // ─── Dry-run tests ───: the banner entry's hyphen token [a-z]+-[a-z]+ rejects a capitalised Dry-run. Reproduced locally by running the self-test with GNU grep first on PATH (43 PASS / 1 FAIL, same line); the macOS BSD grep run passed 44/0 because BSD grep flags only 3 of the 6 banner lines in that file (the cli #2172 lesson again: verify on the CI grep). I validated [A-Za-z][a-z]*-[a-z]+ independently (44/0 under both GNU and BSD grep); 99a8e9b's .gitallowed is byte-identical to that validated file.

Evidence (throwaway clones, isolated HOME):

  • Self-test on head patch: 44 PASS / 0 FAIL (BSD and GNU grep); CI pre-commit run 38291269652 on 99a8e9b: 44 PASS, 0 FAIL, Scan for AWS secrets ... Passed.
  • Same self-test with main's .gitallowed: 36 PASS / 8 FAIL (3 secret-plus-placeholder cases x2 paths, the unanchored-entry check, the file-argument scan), as claimed.
  • All 18 bare entries removed; every active entry is ^...$-anchored on path:line:content; account-ID digits bracketed, .gitallowed self-scan clean. Banner shape: uppercase runs are limited to [A-Z]{1,3}/ID, digits only in [A-Z][0-9]:, no quotes or =, so it cannot form a 20-char AKIA run or a password/key assignment (the widened token adds only one capital before lowercase).
  • Direct probes: allowed pkg/ladder Scope fixture line scans clean (file-arg and no-arg); the same line with an AKIA key appended is caught.
  • Main already has the sec(scripts): git-secrets allowlist matched whole lines and whitelisted most of the repo #86 hardening (say shim, hook verification, no keyword --allowed block, git-secrets SHA pin, self-test step in pre-commit.yml); the PR touches only .gitallowed, the self-test, and the say() comment. No real or realistic secrets in the diff (fixtures assembled at runtime).
  • CI: all checks green on 99a8e9b; PR CLEAN, up to date with main; labels mirror sec(scripts): .gitallowed bare placeholder entries exempt whole lines from every secret detector (see mcp#24/PR #57) #340.

Non-blocking follow-up: the case "key appended to an allowed account-ID fixture line" uses cmd/helpers_test.go / accountID:, which no entry allows, so it fails even without the key and does not exercise an allowed line. A probe on a really-allowed line (above) shows the behaviour is correct; the test could target e.g. a pkg/ladder/*_test.go Scope line.

Mutation sweep: none (tooling change, per policy).

@cristim
cristim merged commit 4c75a84 into main Oct 12, 2026
15 checks passed
@cristim
cristim deleted the sec/gitallowed-anchor-340 branch October 17, 2026 13:24
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 (see mcp#24/PR #57)

1 participant