Skip to content

sec(scripts): catch DSN credentials and register the full git-secrets set in CI - #83

Merged
cristim merged 1 commit into
mainfrom
sec/58-git-secrets-dsn
Oct 17, 2026
Merged

cristim merged 1 commit into
mainfrom
sec/58-git-secrets-dsn

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

This fixes the git-secrets gaps reported in #58. Every root cause was reproduced on macOS, and on Linux (debian:bookworm with git-secrets 1.3.0 built from the tag CI uses).

  • DSNs were never flagged. git-secrets 1.3.0 greps with -w (git grep -nwHEI and grep -nwHEI). -w needs a non-word character after the match, but every DSN pattern ended in @ with the host right after it. The regex was fine; -w defeated it. The three DSN lines become one pattern that ends on a host character. It covers postgres, postgresql, mysql, mongodb and mongodb+srv.
  • Spaced assignments were missed on macOS. On macOS, --cached goes through git grep -E, which uses libc POSIX regex with no \s. The local commit hook therefore missed account_id = "..." and spaced aws_secret_access_key = .... A plain file scan uses BSD grep -E, which accepts \s. Those \s patterns come from --register-aws and can't be edited, so I added two POSIX-class twins. Linux CI was not affected.
  • camelCase accountId: was already matched. The miss reported in sec(scripts): DSN detector coverage audit and git-secrets scan-path inconsistencies (#24 part 2) #58 was the same space problem. A test case now guards it.
  • The inverted 40-char pattern is deleted. [^A-Za-z0-9/+=]{40}[^A-Za-z0-9/+=] has its ^ inside the brackets, so it matched 41 non-base64 characters. That is what flagged the markdown table rows, and it could never detect a key. The upstream AWS patterns and the quoted aws(...) pattern cover real keys.
  • CI registered only the AWS patterns. pre-commit.yml now runs scripts/setup-git-secrets.sh instead of git secrets --register-aws --global. CI now enforces the full set and scans the whole tree. The "at least one pattern" assertion moves to local config and fails on an empty pattern list.
  • Tests and allowlist:
    • The self-test gains must-catch cases: the five schemes, a %40-encoded password, an IPv6 host, a quoted DSN, spaced account_id, camelCase accountId: "..." and spaced aws_secret_access_key.
    • It also gains must-not-catch cases: ${DB_PASS}, <password>, a DSN with no password, and https://user:pw@host.
    • The one fixture that tripped the full set is split from adjacent literals.
    • The .gitallowed entries that the old DSN lines needed to stop matching themselves are removed.

Closes #58

If you already ran the setup script

Your .git/config keeps the old inverted and DSN patterns. Re-running the script fails on duplicate patterns; that's #79. Refresh with:

git config --unset-all secrets.patterns; git config --unset-all secrets.allowed; bash scripts/setup-git-secrets.sh

This was verified in a scratch repo: the old patterns are gone and the script exits 0. It also drops any secrets.allowed entries you added by hand. --register-aws re-adds the AWS example allowlist, and the versioned .gitallowed is untouched.

Accepted false negatives

These DSN shapes are deliberately not caught, because the cost of covering them outweighs the value:

  • a password starting with $, < or { (placeholders are excluded by design);
  • an unencoded / in the password;
  • an empty user;
  • an uppercase scheme;
  • a word-character prefix such as xpostgres://.

Decisions

I made these as the driver. The owner can object on this PR:

  • delete the inverted pattern rather than allowlist the table rows;
  • register the full set in CI;
  • add the macOS-only POSIX twins;
  • add no extra schemes, such as redis or amqp.

The sibling repos share the setup script and likely have the same gaps. They're a follow-up after this lands.

Verification (local; macOS plus Linux via docker, no real secrets)

Check macOS Linux
Fail before: new self-test, old setup script and .gitallowed 83 PASS, 20 FAIL (DSN and spaced cases) 14 FAIL (the 7 DSN cases, direct and hook)
Pass after 103 PASS, 0 FAIL 103 PASS, 0 FAIL
  • Mutation probes, each in a scratch copy:
    • restoring the old @-terminated DSN ending gives 14 FAIL (the 7 DSN cases), on Linux and macOS;
    • deleting the POSIX twins gives 6 FAIL on macOS (the 3 spaced cases).
  • Whole-tree scans, in a fresh clone of the committed tree: the setup script exits 0. File mode over git ls-files, scan -r and --cached are all clean.
  • Linters: actionlint is clean on pre-commit.yml, and all pre-commit hooks pass. shellcheck only repeats existing notes: SC2034 on unused colour variables, and an intentional SC2016 on the ${DB_PASS} literal. No repo gate runs shellcheck on scripts/.

Review

The plan had two independent passes (Sonnet, high effort, non-author). Pass 1 made three changes, all applied:

  • split the fixture instead of adding an allowlist entry;
  • add the refresh recipe;
  • list the accepted false negatives.

The diff then had two passes from the same reviewer, both with no actionable findings at the exact staged tree 56cc55e. The reviewer re-ran the fail-before, pass-after, mutation and whole-tree checks itself, on both platforms. It also confirmed that the --list assertion can't pass vacuously, that the step order is unchanged, and that the scan can't hang in Actions.

CodeRabbit was not pinged because the org's CR quota is exhausted.

🤖 Generated with Claude Code

… set in CI

git-secrets greps with -w, which needs a non-word character after the
match. The DSN patterns ended in `@`, with the host right after it, so
real postgres, mysql and mongodb DSNs were never flagged. End the
pattern on the host instead, and cover postgresql and mongodb+srv.

On macOS the --cached path uses git grep's POSIX regex, which has no
\s, so the commit hook missed spaced account_id and
aws_secret_access_key assignments. Register POSIX-class twins of the
two upstream \s patterns.

Delete the inverted 40-char pattern. Its `^` sits inside the bracket,
so it matched 41 non-base64 characters (markdown table rows) and could
never detect a key.

CI registered only the AWS patterns. Run the setup script instead, so
CI enforces the full set and scans the whole tree.

Add must-catch and must-not-catch self-test cases. Split the one
fixture that tripped the full set, and drop the .gitallowed entries
that the old DSN lines needed to stop matching themselves.

Closes #58

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 17, 2026

Copy link
Copy Markdown

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 29 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 75 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-mcp/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 2db52b57-e47d-43cd-b99f-47ded133d98d
📥 Commits

Reviewing files that changed from the base of the PR and between 987bbf5 and d480e9c.

📒 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 added impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/s Hours type/security Security finding triaged Item has been triaged labels Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Merge-driver verdict: CLEAN at d480e9c063855b70147d6c7ddcb2fe2b379caffd

An independent adversarial reviewer (Sonnet, high effort, cold brief, not the author) reviewed the full diff. It found no blocking findings. All git-secrets registration ran in throwaway repos with a temporary HOME. The global secrets.patterns was verified unchanged.

Local verification (macOS, git 2.50.1, Homebrew git-secrets):

  • Self-test (fixture): scripts/test-git-secrets-allowlist.sh gives 103 PASS, 0 FAIL.
  • Extra cases (fixture):
    • Caught: postgres, mysql and mongodb+srv DSNs with credentials, including no-port, no-path and special-character passwords.
    • Not flagged: postgres://localhost/db and ${VAR} passwords.
  • Deleted inverted pattern (fixture): it never matched a 40-char base64 string, only 41 non-key characters. Real keys are still caught by --register-aws plus the new [[:space:]] twin, which also fixes the spaced form on macOS.
  • Full tree (real): git secrets --scan -r, --cached and a per-file scan all exit 0 with the PR's setup script. The removed .gitallowed entries were self-matches of the old DSN lines.
  • Linux path: covered by CI. The pre-commit job ran the new setup-git-secrets.sh registration and scan on Linux and passed at this SHA.
  • Workflow lint: actionlint is clean. shellcheck reports only pre-existing or intended notes.

Non-blocking notes:

  • Some DSN forms are not caught:
  • Docs placeholders like postgres://USER:PASSWORD@HOST would be flagged, and .gitallowed has no placeholder entry yet.
  • git secrets --list | grep -q . can pass with zero patterns, because --list also prints providers. Earlier set -e steps make that unlikely to matter.

Gate items:

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Gate met at d480e9c063855b70147d6c7ddcb2fe2b379caffd: all CI is green, including Unit Tests, and the merge state is CLEAN. Merging.

@cristim
cristim merged commit ee3f199 into main Oct 17, 2026
13 checks passed
@cristim
cristim deleted the sec/58-git-secrets-dsn branch October 17, 2026 17:14
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/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/security Security finding urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(scripts): DSN detector coverage audit and git-secrets scan-path inconsistencies (#24 part 2)

1 participant