Repository navigation
Conversation
Audit follow-up to #1972 (issue LeanerCloud#368). The bare placeholder block was already replaced with anchored entries in LeanerCloud#813; what remained: - The shape-based test-credential entry in .gitallowed allowed any quoted password or api_key value up to 40 characters in a test file, so a real password or a 32-character API key pasted into a fixture scanned clean. It now lists the placeholder values in use (on main and the open fork branches). - The DSN detectors missed libpq's postgresql:// scheme, golang-migrate's pgx/pgx5 schemes (used by this repo's migrations), "+driver" schemes, mariadb://, DSNs with an empty user name, and a password passed as a ?password= query parameter. URL-encoded credentials and IPv6 hosts were already covered and are now pinned by tests. - The widened detectors no longer match their own registration lines, so the two DSN self-match entries are gone; a re-run of the setup script drops the pre-LeanerCloud#368 DSN detectors from an existing clone. - Three docs lines the widened detectors flag (local docker-compose password, a ${DB_PASSWORD} reference, prose naming pgx5) get whole-line entries. Closes LeanerCloud#368 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB14. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Collaborator
Author
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB15. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is the follow-up audit to #1972 that #368 asked for. There were two parts.
1. Placeholder entries in
.gitallowedThe bare account-ID/UUID block the issue describes is already gone. #813 (for #804) replaced it with
^path:[0-9]+:...$entries, andscripts/test-git-secrets-allowlist.shnow checks that every entry is anchored. One loose entry was left. It was a shape-based entry for test credentials:It was anchored, but it accepted any quoted value of up to 40 characters. So a real password, or a 32-character API key, pasted into any
_test.go/.test.tsfile scanned clean. It's now two entries, one forpasswordand one forapi_key, and each lists the placeholder values actually in use: 23 passwords and 4 API keys. A new placeholder needs a one-word addition to the list. The account-ID entry was already value-listed and is unchanged.2. DSN detector coverage audit
I probed each detector empirically in a throwaway repo with git-secrets 1.3.0 and an isolated HOME.
postgres://u:p@hostpostgresql://u:p@host(libpq's canonical scheme)pgx5://u:p@host,pgx://(golang-migrate; this repo's migrations usepgx5://)postgresql+psycopg2://,postgres+asyncpg://,mysql+mysqlconnector://mysql+pymysql://mariadb://u:p@hostscheme://:p@host(postgres, mysql, mongodb)?password=/&password=query parameter (libpq URI, JDBC)p%40ss)@[2001:db8::1]:5432)?sslpassword=, credential-lesspgx5://user@hostThe fix is in
scripts/setup-git-secrets.sh:(postgres(ql)?|pgx[45]?)(\+[A-Za-z0-9]+)?.(mysql|mariadb)(\+[A-Za-z0-9]+)?.[^:]*instead of[^:]+in all three detectors.Side effects in
.gitallowed:The PostgreSQL and MySQL registration lines no longer contain a literal
scheme://, so they stop matching themselves. Their two self-match entries were dead and are removed. The Azure entry stays.Three docs lines are now flagged and get whole-line entries:
docs/DEVELOPMENT.md(pgx5://cudly:cudly_local_dev@...)${DB_PASSWORD}shell reference quoted in the 2026-09-02 auditpgx5://in prose, where the detector runs on to a later:and@None of them holds a real credential.
Closes #368
Accepted residual gaps (not fixed here)
Each would need its own design decision and a false-positive pass. File follow-ups if you want any of them:
POSTGRES://,MONGODB://). git-secrets greps case-sensitively, and spelling out[Pp][Oo]...makes the patterns much harder to read. Clients and tools emit lower-case schemes.host=... user=... password=secret). It isn't a URL DSN, and the obvious detector would flag everyfmt.Sprintf("... password=%s")in the tree. The quoted form is already caught by the generic password detector.user:pass@tcp(host:3306)/db, no scheme) and other databases (redis, amqp, mssql/sqlserver, cockroachdb, redshift). These are outside the three detectors the issue scoped.[^:]*can still run across prose on a long line, as in the audit sentence above. I didn't exclude whitespace from it, because that would drop coverage[^:]+already has: a concatenated"postgres://" + user + ":" + "pw" + "@".pre-commitjob registers onlygit secrets --register-aws. The custom detectors, including these DSN detectors, run in CI only through thegit-secrets allowlist self-testjob, which scans every tracked file with them. That was already the case and is unchanged.Fork-branch / fixture compatibility
The brief asked for this before any entry was tightened. I scanned with no allowlist, then applied the old and new
.gitallowed, across these trees:origin/mainfix/issue-{47,54,64,76,153,169,243,310,312,316,326,337,339,342,343,353}Results:
.gitallowedor the setup script.Caveat: the #313 worktree is still in progress and edits
internal/auth/service_password_test.go. If it adds a new password placeholder value, that value has to be added to the list.Verification
All of this is real git-secrets 1.3.0 scan evidence, not mocks. Each run used a fresh
git inittemp repo and an isolated HOME, never the shared git config.bash scripts/test-git-secrets-allowlist.shon this branch: 183 PASS, 0 FAIL.origin/main's.gitallowedandsetup-git-secrets.shgives 157 PASS, 26 FAIL. That's 13 new cases × 2 scan paths (direct scan and installed hook):passwordplaceholders in a Go and a TS test, the listedapi_keyplaceholder, and the documented dev DSN all scan clean. The same placeholder outside a test file is still caught. The whole-tree file-argument scan of all tracked files also passes.setup run left a pre-#368 DSN detector registered.shellcheck scripts/setup-git-secrets.sh scripts/test-git-secrets-allowlist.sh: no new findings. The only findings are the two SC2034 warnings for unusedYELLOW/BLUE, which main has too.git diff --check: clean. No Go, Terraform, workflow or frontend files changed.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
setup-git-secrets.sh, which are on main too.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,triaged,priority/p3,severity/low,urgency/eventually,effort/s,type/security.🤖 Generated with Claude Code