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
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