From 0ba727d77d0a21922e670340394308e292fa72a4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 05:33:52 +0000 Subject: [PATCH] fix(scripts): widen DSN detectors and list test credential placeholders Audit follow-up to #1972 (issue #368). The bare placeholder block was already replaced with anchored entries in #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-#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 #368 Co-Authored-By: Claude Opus 5.5 --- .gitallowed | 67 ++++++++++------- scripts/setup-git-secrets.sh | 30 +++++++- scripts/test-git-secrets-allowlist.sh | 102 ++++++++++++++++++++++++-- 3 files changed, 162 insertions(+), 37 deletions(-) diff --git a/.gitallowed b/.gitallowed index c1aec0906..96febcdc6 100644 --- a/.gitallowed +++ b/.gitallowed @@ -12,32 +12,30 @@ # it instead of silencing the scanner. Test fixtures should use synthetic # placeholders; add an entry only for a line a real scan flags. -# scripts/setup-git-secrets.sh registers its detectors as literal regex text -# and three of them (Azure connection string, PostgreSQL and MySQL DSN -# detectors, verified empirically -- MongoDB's does not) match their own -# registration line. Each entry below is anchored on the path, the line -# number, AND the entire line as it appears in that file (including the -# trailing comment), with a trailing $ -- not just the "git secrets --add -# '" command prefix, and not just the added pattern with the anchor left -# open at the end. A prefix-only anchor whitelists a secret appended to -# ANY registration line in this file; an anchor missing the trailing $ -# whitelists a secret appended to the END of one of these three specific -# lines. Both are the #1972 hole reintroduced at a smaller scale, and both -# were caught only by a fixture exercising exactly that mutation -- add -# one for any future entry in this block that isn't already covered. +# scripts/setup-git-secrets.sh registers its detectors as literal regex text, +# and one of them (the Azure connection string) matches its own registration +# line. The DSN detectors used to as well, until #368 widened their schemes to +# alternations, whose registration text no longer spells out a literal +# "scheme://" for the detector to find. The entry below is anchored on the path, +# the line number, AND the entire line as it appears in that file (including +# the trailing comment), with a trailing $ -- not just the "git secrets --add +# '" command prefix, and not just the added pattern with the anchor left open +# at the end. A prefix-only anchor whitelists a secret appended to ANY +# registration line in this file; an anchor missing the trailing $ whitelists +# a secret appended to the END of this specific line. Both are the #1972 hole +# reintroduced at a smaller scale, and both were caught only by a fixture +# exercising exactly that mutation -- add one for any future entry in this +# block that isn't already covered. # -# Each entry below also replaces one letter of its own trigger substring -# with a single-character bracket expression, e.g. "Protoco[l]" for -# "Protocol". This is a no-op for matching (as a regex, [l] matches the -# literal letter l exactly like l would) but it breaks the contiguous -# trigger span each entry is built from, so .gitallowed does not need to -# allow-list ITSELF against the very patterns these entries exist to -# suppress (confirmed empirically: without this, the .gitallowed -# self-scan case below fails -- and note this comment must not spell any -# of those spans out literally either, for the same reason). +# The entry also replaces one letter of its own trigger substring with a +# single-character bracket expression, e.g. "Protoco[l]" for "Protocol". This +# is a no-op for matching (as a regex, [l] matches the literal letter l exactly +# like l would) but it breaks the contiguous trigger span the entry is built +# from, so .gitallowed does not need to allow-list ITSELF against the very +# pattern the entry exists to suppress (confirmed empirically: without this, +# the .gitallowed self-scan case fails -- and note this comment must not spell +# that span out literally either, for the same reason). ^scripts/setup-git-secrets\.sh:[0-9]+:add_pattern 'DefaultEndpointsProtoco[l]=https' # Azure Connection String$ -^scripts/setup-git-secrets\.sh:[0-9]+:add_pattern 'postgre[s]://\[\^:\]\+:\[\^@\]\+@' # PostgreSQL$ -^scripts/setup-git-secrets\.sh:[0-9]+:add_pattern 'mysq[l]://\[\^:\]\+:\[\^@\]\+@' # MySQL$ # Truncated PEM placeholder used by scripts/test-git-secrets-allowlist.sh # (and by any fixture in this repo needing a non-secret PEM-shaped string): @@ -71,9 +69,13 @@ # each placeholder is bracketed so this file does not match its own entries. # A single account field per line, quoted placeholder only: ^[^:]+(_test\.go|\.test\.ts):[0-9]+:[[:space:]]*(AccountID|accountID|account_id|Account):[[:space:]]*["'](12345678901[2]|11111111111[1]|11112222333[3]|22222222222[2]|33333333333[3]|44444444444[4]|55555555555[5]|66666666666[6]|77777777777[7]|99988877766[6]|99998888777[7]|99999999999[9])["'],?$ -# Test-fixture credential fields: one field, one short quoted value, nothing -# else on the line, in a test file only. -^[^:]+(_test\.go|\.test\.ts):[0-9]+:[[:space:]]*(password|api_key):[[:space:]]*["'][^"']{0,40}["'],?$ +# Test-fixture credential fields: one field, one quoted placeholder from the +# list, nothing else on the line, in a test file only. The values are listed +# rather than matched by shape (#368): a shape such as "any value up to 40 +# characters" also passes a real password or a 32-character API key pasted +# into a test file. To use a new placeholder, add it to the list. +^[^:]+(_test\.go|\.test\.ts):[0-9]+:[[:space:]]*password:[[:space:]]*["'](C0mpl3x!S3cur3|lowercases3!|My\$ecur3#S3cur3!|MyAdmin@12345678|MyQwerty123!|NoNumberSecure!|NoSpecialS3cur3|pa111ssword|pass###word|pass1111word|password|password11|password123|S3cure1111xyz!|S3cureAaaa123!|S3cureXyz###1|S3cureXyz11!|SecurePass123!|smtp_password|StrongS3cur3!|testpass|UPPERCASES3!|cGFzc3dvcmQ=)["'],?$ +^[^:]+(_test\.go|\.test\.ts):[0-9]+:[[:space:]]*api_key:[[:space:]]*["'](full-api-key-value|full-key-value|new-api-key-12345|test-key)["'],?$ # Placeholder credentials that only the word-bounded detectors (#328) flag: # a detector glued to a word, e.g. the "password" of db_password or a DSN @@ -95,6 +97,17 @@ ^terraform/profiles/README\.md:[0-9]+:[[:space:]]*export[[:space:]]+TF_VAR_database_passwor[d]="secret123"[[:space:]]*$ ^terraform/profiles/README\.md:[0-9]+:[[:space:]]*database_passwor[d][[:space:]]+=[[:space:]]+"secret123"[[:space:]]*$ +# Lines the DSN detectors flag since #368 added the postgresql://, pgx5:// and +# other schemes. None holds a real credential: the local docker-compose +# database password that docs/DEVELOPMENT.md documents, a ${DB_PASSWORD} shell +# reference in an audit quote, and an audit sentence that names the pgx5 scheme +# in prose, where the detector runs on to a later ":" and "@". Same whole-line +# anchoring as the blocks around it; the scheme is bracketed so this file does +# not match the detector itself. +^docs/DEVELOPMENT\.md:[0-9]+:[[:space:]]*-database[[:space:]]+"pgx[5]://cudly:cudly_local_dev@(localhost|postgres):5432/cudly\?sslmode=disable"[[:space:]]+(up|version|down[[:space:]]+1|\\)[[:space:]]*$ +^docs/audits/codebase-audit-2026-09-02\.md:[0-9]+:[[:space:]]*DB_URL="pgx[5]://cudly:\$\{DB_PASSWORD\}@\$\{DB_ENDPOINT\}:5432/cudly\?sslmode=require"[[:space:]]*$ +^docs/audits/codebase-audit-2026-09-02\.md:[0-9]+:-[[:space:]]+verdict:[[:space:]]+CONFIRMED[[:space:]]+—[[:space:]]+Dockerfile\.dev:22-33[[:space:]]+downloads[[:space:]]+the[[:space:]]+v4\.17\.0[[:space:]]+release[[:space:]]+tarball,[[:space:]]+while[[:space:]]+Dockerfile:55-85[[:space:]]+explains[[:space:]]+that[[:space:]]+upstream[[:space:]]+tarballs[[:space:]]+carry[[:space:]]+upstream's[[:space:]]+own[[:space:]]+toolchain[[:space:]]+and[[:space:]]+dependency[[:space:]]+pins[[:space:]]+\("how[[:space:]]+issue[[:space:]]+#1833's[[:space:]]+stdlib[[:space:]]+CVEs[[:space:]]+reached[[:space:]]+the[[:space:]]+runtime[[:space:]]+image"\)[[:space:]]+and[[:space:]]+builds[[:space:]]+migrate[[:space:]]+as[[:space:]]+a[[:space:]]+package[[:space:]]+of[[:space:]]+this[[:space:]]+module[[:space:]]+at[[:space:]]+the[[:space:]]+version[[:space:]]+`go\.mod`[[:space:]]+resolves[[:space:]]+\(v4\.19\.1,[[:space:]]+go\.mod:106\)[[:space:]]+with[[:space:]]+`-tags=pgx5`\.[[:space:]]+The[[:space:]]+divergence[[:space:]]+is[[:space:]]+worse[[:space:]]+than[[:space:]]+the[[:space:]]+finding[[:space:]]+states:[[:space:]]+the[[:space:]]+prod[[:space:]]+binary[[:space:]]+registers[[:space:]]+only[[:space:]]+the[[:space:]]+`pgx[5]://`[[:space:]]+scheme[[:space:]]+while[[:space:]]+the[[:space:]]+dev[[:space:]]+tarball[[:space:]]+is[[:space:]]+the[[:space:]]+default[[:space:]]+`postgre[s]://`[[:space:]]+build,[[:space:]]+so[[:space:]]+the[[:space:]]+same[[:space:]]+migration[[:space:]]+command[[:space:]]+is[[:space:]]+not[[:space:]]+portable[[:space:]]+between[[:space:]]+the[[:space:]]+two[[:space:]]+images\.[[:space:]]+`go[[:space:]]+install[[:space:]]+github\.com/air-verse/air@v1\.61\.7`[[:space:]]+at[[:space:]]+Dockerfile\.dev:16[[:space:]]+does[[:space:]]+resolve[[:space:]]+air's[[:space:]]+own[[:space:]]+go\.mod,[[:space:]]+as[[:space:]]+claimed\.[[:space:]]*$ + # One-off fixture lines a real scan flags (a synthetic placeholder account ID, # a throwaway test password, a credential-less test DSN). Each entry pins the # file, any line number, and the WHOLE line with a trailing $, so appending a diff --git a/scripts/setup-git-secrets.sh b/scripts/setup-git-secrets.sh index 23a6a3111..9a3ffa64e 100755 --- a/scripts/setup-git-secrets.sh +++ b/scripts/setup-git-secrets.sh @@ -198,10 +198,32 @@ add_pattern 'api[_-]?key[[:space:]]*[=:][[:space:]]*['\''"][^'\''"]{8,}' add_pattern 'secret[_-]?key[[:space:]]*[=:][[:space:]]*['\''"][^'\''"]{8,}' # Secret key with quoted value add_pattern 'BEGIN[[:space:]]((RSA|DSA|EC|OPENSSH|ENCRYPTED)[[:space:]])?PRIVATE[[:space:]]KEY-----' # PEM private keys -# Database connection strings -add_pattern 'postgres://[^:]+:[^@]+@' # PostgreSQL -add_pattern 'mysql://[^:]+:[^@]+@' # MySQL -add_pattern 'mongodb(\+srv)?://[^:]+:[^@]+@' # MongoDB +# Database connection strings (#368). A DSN detector covers each scheme a client +# accepts for that database: libpq's postgresql:// as well as postgres://, the +# pgx/pgx4/pgx5 schemes golang-migrate uses (this repo's migrations use pgx5://), +# and SQLAlchemy-style "+driver" suffixes. The user name may be empty +# (scheme://:password@host is a valid DSN), so it is [^:]* rather than [^:]+. +# URL-encoded credentials and IPv6 hosts need nothing extra: the match ends at +# the "@" before the host. The last detector catches a password passed as a +# query parameter instead of in the user info (?password=, &password=), which +# libpq and the JDBC drivers accept. +add_pattern '(postgres(ql)?|pgx[45]?)(\+[A-Za-z0-9]+)?://[^:]*:[^@]+@' # PostgreSQL +add_pattern '(mysql|mariadb)(\+[A-Za-z0-9]+)?://[^:]*:[^@]+@' # MySQL +add_pattern 'mongodb(\+srv)?://[^:]*:[^@]+@' # MongoDB +add_pattern '(postgres(ql)?|pgx[45]?|mysql|mariadb)(\+[A-Za-z0-9]+)?://[^[:space:]'\''"]*[?&]password=[^&[:space:]'\''"]+' # DSN password query parameter + +# Clones set up before #368 still carry the narrower DSN detectors, bare (before +# #328) or word-wrapped. Each matches a subset of what its replacement above +# matches, so a re-run drops them instead of scanning for both. Each scheme is +# split into two adjacent quoted strings so this line does not match the +# detectors it names. +for legacy_dsn in 'postgres:''//[^:]+:[^@]+@' 'mysql:''//[^:]+:[^@]+@' 'mongodb(\+srv)?:''//[^:]+:[^@]+@'; do + for legacy_form in "${legacy_dsn}" "${word_chars}(${legacy_dsn})${word_chars}"; do + if git config --local --get-all secrets.patterns | grep -qxF -- "${legacy_form}"; then + git config --local --unset-all --fixed-value secrets.patterns "${legacy_form}" + fi + done +done # Allowed patterns live in .gitallowed (versioned, applied by every scan, including CI). # They are matched against the whole "path:line:content" scanner output line, so an entry diff --git a/scripts/test-git-secrets-allowlist.sh b/scripts/test-git-secrets-allowlist.sh index d73459644..92bb6a004 100755 --- a/scripts/test-git-secrets-allowlist.sh +++ b/scripts/test-git-secrets-allowlist.sh @@ -77,6 +77,28 @@ if [ "$(git config --get-all secrets.patterns | wc -l)" -ne "$patterns_after" ]; echo "setup run over a pre-#328 bare detector changed the pattern count" >&2 exit 1 fi +# A clone set up before #368 has the narrower DSN detectors registered, bare or +# word-wrapped. A re-run must drop them, leaving only the widened ones. The +# schemes are split so this file does not match the detectors. +LEGACY_PG='[A-Za-z0-9_]*(postgres:''//[^:]+:[^@]+@)[A-Za-z0-9_]*' +LEGACY_MYSQL='mysql:''//[^:]+:[^@]+@' +git config --add secrets.patterns "$LEGACY_PG" +git config --add secrets.patterns "$LEGACY_MYSQL" +if ! bash scripts/setup-git-secrets.sh > "$tmp/setup4.log" 2>&1; then + echo "scripts/setup-git-secrets.sh failed with pre-#368 DSN detectors registered:" >&2 + cat "$tmp/setup4.log" >&2 + exit 1 +fi +for legacy in "$LEGACY_PG" "$LEGACY_MYSQL"; do + if git config --get-all secrets.patterns | grep -qxF -- "$legacy"; then + echo "setup run left a pre-#368 DSN detector registered: $legacy" >&2 + exit 1 + fi +done +if [ "$(git config --get-all secrets.patterns | wc -l)" -ne "$patterns_after" ]; then + echo "setup run over pre-#368 DSN detectors changed the pattern count" >&2 + exit 1 +fi # Same HOME-isolation reason: drop the provider before any further scan. git config --unset-all secrets.providers @@ -222,14 +244,14 @@ run_case "secret appended to an unrelated registration line stays caught" 1 \ "$(sed "s/# GCP API Key\$/# GCP API Key ${KEY}/" "$REPO_ROOT/scripts/setup-git-secrets.sh")" cp "$REPO_ROOT/scripts/setup-git-secrets.sh" scripts/setup-git-secrets.sh -# A secret appended to the END of one of the three self-matching lines -# THEMSELVES must also still be caught. An entry anchored on the path and -# pattern but missing a trailing $ would stop matching at the end of the -# pattern it names and let anything appended after it through, which is a -# narrower version of the same hole the entry above closes. +# A secret appended to the END of the self-matching line ITSELF must also +# still be caught. An entry anchored on the path and pattern but missing a +# trailing $ would stop matching at the end of the pattern it names and let +# anything appended after it through, which is a narrower version of the +# same hole the entry above closes. run_case "secret appended to a self-matching line itself stays caught" 1 \ scripts/setup-git-secrets.sh \ - "$(sed "s/# PostgreSQL\$/# PostgreSQL ${KEY}/" "$REPO_ROOT/scripts/setup-git-secrets.sh")" + "$(sed "s/# Azure Connection String\$/# Azure Connection String ${KEY}/" "$REPO_ROOT/scripts/setup-git-secrets.sh")" cp "$REPO_ROOT/scripts/setup-git-secrets.sh" scripts/setup-git-secrets.sh # Must scan clean (exit 0): what the tree contains, and what the allowlist @@ -302,6 +324,74 @@ run_case "postgres DSN with no user info" 0 ac.env \ run_case "password variable reference, no literal" 0 ad.tf \ "db_pass""word = var.db_password" +# DSN detector coverage (#368). Each case scanned clean before #368: the +# canonical postgresql:// scheme, golang-migrate's pgx5:// (this repo's +# migration scheme), SQLAlchemy "+driver" schemes, MariaDB, an empty user +# name, and a password passed as a query parameter. +PGQL="postg""resql://" +PGX5="pg""x5://" +MY="mys""ql" +MONGO="mongo""db://" +PWQ="pass""word=" +run_case "postgresql:// DSN" 1 dsn1.env \ + "DATABASE_URL=${PGQL}app:${DSN_PW}@db.example.com:5432/app" +run_case "pgx5:// DSN" 1 dsn2.sh \ + "migrate -database \"${PGX5}app:${DSN_PW}@db:5432/app?sslmode=require\" up" +run_case "postgresql+driver DSN" 1 dsn3.py \ + "url = '${PGQL%://}+psycopg2://app:${DSN_PW}@db/app'" +run_case "mysql+connector DSN" 1 dsn4.py \ + "url = '${MY}+mysqlconnector://root:${DSN_PW}@localhost/app'" +run_case "mariadb DSN" 1 dsn5.txt \ + "maria""db://root:${DSN_PW}@localhost/app" +run_case "postgres DSN with an empty user name" 1 dsn6.env \ + "DATABASE_URL=${PG}:${DSN_PW}@db.example.com/app" +run_case "mysql DSN with an empty user name" 1 dsn7.txt \ + "${MY}://:${DSN_PW}@localhost/app" +run_case "mongodb DSN with an empty user name" 1 dsn8.txt \ + "${MONGO}:${DSN_PW}@mongo.example.net/app" +run_case "password as a DSN query parameter" 1 dsn9.env \ + "DATABASE_URL=${PG}app@db.example.com/app?sslmode=require&${PWQ}${DSN_PW}" +run_case "password as a JDBC query parameter" 1 dsn10.properties \ + "url=jdbc:${PGQL}db.example.com/app?user=app&${PWQ}${DSN_PW}" +# Already covered before #368, kept as regression guards: URL-encoded +# credentials and an IPv6 host still end the match at the "@". +run_case "postgres DSN with a URL-encoded password" 1 dsn11.env \ + "DATABASE_URL=${PG}app:p%40ss${DSN_PW}@db.example.com/app" +run_case "postgres DSN with an IPv6 host" 1 dsn12.env \ + "DATABASE_URL=${PG}app:${DSN_PW}@[2001:db8::1]:5432/app" +# The widened detectors must leave credential-less DSNs alone. +run_case "pgx5:// DSN with no password" 0 dsn13.sh \ + "DB_URL=\"${PGX5}cudly@\${DB_ENDPOINT}:5432/cudly?sslmode=require\"" +run_case "postgresql:// DSN with no user info" 0 dsn14.env \ + "DATABASE_URL=${PGQL}db.example.com:5432/app" +run_case "sslpassword query parameter is not the password" 0 dsn15.env \ + "DATABASE_URL=${PG}app@db.example.com/app?ssl${PWQ}${DSN_PW}" + +# The docs/DEVELOPMENT.md entry allows only the documented local +# docker-compose password, not any password on the same command line. +run_case "documented local dev DSN stays allowed" 0 docs/DEVELOPMENT.md \ + " -database \"${PGX5}cudly:cudly_local_dev@localhost:5432/cudly?sslmode=disable\" up" +run_case "other password on the documented dev DSN line is caught" 1 docs/DEVELOPMENT.md \ + " -database \"${PGX5}cudly:${DSN_PW}@localhost:5432/cudly?sslmode=disable\" up" + +# Test-fixture credential placeholders (#368). The entry lists the values in +# use; it used to allow any quoted value of up to 40 characters, so a real +# password or a 32-character API key in a test file scanned clean. +PWF="pass""word: " +AKF="api""_key: " +run_case "listed password placeholder in a Go test" 0 fixture_test.go \ + " ${PWF}\"test""pass\"," +run_case "listed password placeholder in a TS test" 0 fixture.test.ts \ + " ${PWF}'SecurePass""123!'," +run_case "listed API key placeholder in a TS test" 0 fixture.test.ts \ + " ${AKF}'test-""key'," +run_case "real-looking password in a Go test is caught" 1 fixture_test.go \ + " ${PWF}\"Xq7#mP2v""L8qR4wZ1\"," +run_case "real-looking API key in a TS test is caught" 1 fixture.test.ts \ + " ${AKF}'9f86d081884c7d65""9a2feaa0c55ad015'," +run_case "listed placeholder outside a test file is caught" 1 fixture.go \ + " ${PWF}\"test""pass\"," + # CI-equivalent check: the pre-commit framework hands git-secrets explicit # file names (plain grep), unlike a no-argument scan (git grep), which # differ on regex features such as \s. Scan every tracked file of the real