diff --git a/docs/TOOLS.md b/docs/TOOLS.md index 5038902..49e3fac 100644 --- a/docs/TOOLS.md +++ b/docs/TOOLS.md @@ -64,6 +64,11 @@ bash scripts/check-env.sh --file .env # what is configured, before anything re bash scripts/secrets.sh --tracked # what is already committed ``` +`npm run verify` runs that scan too, as guard `20-secrets`, together with +`--built dist/` over the bundle the build just produced. A line that has to show +a credential shape, such as a usage example, carries `secrets-ok:` and a reason +on the same line; that excuses the one line, visibly. + Then [`architecture/GUARDS.md`](architecture/GUARDS.md) before you write a check of your own — how to write one that can actually fail. diff --git a/docs/architecture/GUARDS.md b/docs/architecture/GUARDS.md index 9eff008..a571fba 100644 --- a/docs/architecture/GUARDS.md +++ b/docs/architecture/GUARDS.md @@ -109,6 +109,16 @@ tr '\0' '\n' < /proc/$PID/environ | grep -c . # 0 here means "could not read" This is the confident-absence failure one level up: the same trap as a screen rendering a failed query as a count of zero, applied to your own diagnosis. +**The same trap inside a scanner.** A matcher that discards its errors turns +"could not run" into "found nothing". `grep "$pattern" 2>/dev/null` answers a +pattern it cannot read with exit 2 and no output, and a loop reading its matches +sees an empty list. `scripts/secrets.sh` ran its private-key pattern exactly that +way from the day the rule was written: the pattern starts with dashes, grep took +it for an option, and the scanner reported a clean tree over a staged private +key until 2026-09-10 (#235). Two rules follow. Pass every pattern with `-e`. And +compile each pattern once before trusting its silence, exiting 2 when one cannot +be read, which is what `secrets.sh` now does. + ## 5. A guard that is often wrong is worse than none A check with a high false-positive rate trains everybody to skip its output, diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 60a14fe..3b182f4 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -141,7 +141,7 @@ each row says what it does *here*. | Path | What it is | | --- | --- | | `scripts/check-env.sh` | which of the 17 Zoho / reCAPTCHA / CORS / rate-limit variables are set and plausible, before the server reads them. Exit 2 means nothing was checked | -| `scripts/secrets.sh` | credential shapes in a staged diff, and `--tracked` for a whole-tree audit. **`--built dist/` is the one that matters here**: `VITE_RECAPTCHA_SITE_KEY` is inlined into the bundle at build time, so the repository scan cannot see what users receive. Carries this project's own shapes, and one **tightened** pattern — see below | +| `scripts/secrets.sh` | credential shapes in a staged diff, and `--tracked` for a whole-tree audit. **`--built dist/` is the one that matters here**: `VITE_RECAPTCHA_SITE_KEY` is inlined into the bundle at build time, so the repository scan cannot see what users receive. `npm run verify` runs both, as guard `20-secrets`. Every pattern is compiled before the scan, because a pattern grep cannot read produces the same silence as a clean tree (#235). A line that must show a credential shape carries `secrets-ok:` and its reason. Carries this project's own shapes, and one **tightened** pattern, described below | | `scripts/verify.sh` | every check this project has, in one table. Honestly thin — there is no test suite, and it says so rather than printing a green row | | `scripts/doc-triggers.py` | which documents a pending change fires, read from the `Governs:` headers. Run it before committing, not after | | `scripts/forgejo-issue.py` | files and closes issues in the tracker convention, refusing malformed ones before they are filed | diff --git a/docs/security/SECURITY.md b/docs/security/SECURITY.md index 91e8fc3..d0fa0c1 100644 --- a/docs/security/SECURITY.md +++ b/docs/security/SECURITY.md @@ -53,7 +53,9 @@ so: that is the one that went, and that is why. credentials in the tracked tree — including in the history, which a `git log -p` search covers and a directory listing does not. `scripts/secrets.sh --tracked` does the tree; `--built dist/` does the bundle, which is the artifact users -actually receive and the one the repository scan never sees. +actually receive and the one the repository scan never sees. `npm run verify` +runs both, as guard `20-secrets`. Until 2026-09-10 neither could see a private +key: the scanner never ran that pattern (#235). **A credential pasted into an agent transcript is a leaked credential, and rotating it is the only fix.** Deleting the message does not help and neither diff --git a/docs/security/SECURITY_CHECKLIST.md b/docs/security/SECURITY_CHECKLIST.md index ea1409a..26fee14 100644 --- a/docs/security/SECURITY_CHECKLIST.md +++ b/docs/security/SECURITY_CHECKLIST.md @@ -19,8 +19,7 @@ is short enough to finish rather than a document to skim. ## Before a release -- [ ] `bash scripts/secrets.sh --tracked` is clean — proves nothing credential-shaped is committed -- [ ] `npm run build && bash scripts/secrets.sh --built dist/` is clean — proves the *bundle* is clean, which the tracked scan cannot tell you +- [ ] `npm run verify` passes guard `20-secrets`. It runs `scripts/secrets.sh --tracked`, which proves nothing credential-shaped is committed, and `--built dist/` over the bundle the build just made, which proves the *bundle* is clean, something the tracked scan cannot tell you - [ ] `bash scripts/check-env.sh --file .env` exits 0 — proves every variable the server reads is set and shaped right, before it reads them. **Exit 2 is not a pass** - [ ] `npm audit` shows no high or critical advisory in production dependencies — proves no known-exploitable code ships - [ ] `bash scripts/preflight.sh` is clean against **both** front doors — `queuenorth.com` by default and `PREFLIGHT_ORIGIN=https://qn.isnull.dev` for the other. Proves headers, CSP and TLS survived the deploy on two separate ingresses that can rot independently @@ -73,3 +72,4 @@ with. | 2026-05-11 → 2026-06-14 | The Zoho WebToLead tokens `xnQsjsdp` and `xmIwtLD` were hardcoded in `index.html` and later `src/pages/Contact.jsx`, and reached four commits on what was then a **public** repository, before being moved to environment variables at `05b27d2`. Low severity — they are public-by-design form identifiers a browser renders anyway — and deliberately **not** rewritten out of the history, which is now private | `scripts/secrets.sh` in the `pre-commit` hook, scanning the staged diff. This is the finding that makes the hook worth having in a project with no test suite to run beside it | | 2026-08-18 | Nobody had ever checked whether the reCAPTCHA **secret** key had reached git. It had not — zero commits, zero tracked files — but "we would have noticed" is not a check | `secrets.sh --tracked` is now the first thing run in a fresh clone, per `docs/TOOLS.md` | | 2026-08-18 | The live lead database had no backup and no restore had ever been attempted, and nothing in any document said so | `scripts/backup.sh` and `scripts/restore-check.sh`, **both run against production the same day**: a verified snapshot was taken from the running container and replayed into a scratch database — 2 tables, 3 rows, under a second. `docs/OPERATIONS.md` now carries a real *Last verified restore* date. What is still missing is a schedule and an off-machine copy, tracked in `Batch 15` | +| 2026-09-10 | `scripts/secrets.sh` had never run its private-key pattern: grep read the leading dashes as an option and `2>/dev/null` hid the error, so a staged private key passed the pre-commit hook. Its NAME=value pattern matched nothing at all until 2026-08-29, and missed quoted values until 2026-09-10. Nothing ran the bundle scan this checklist asked for | Every pattern is compiled before the scan, and an unreadable one exits 2 instead of passing; every grep takes `-e`; quoted values are caught; a documentation line is excused only by a visible `secrets-ok:` note; guard `20-secrets` now scans `dist/` as well (#235) | diff --git a/scripts/check-env.sh b/scripts/check-env.sh index 92936e3..b865fab 100755 --- a/scripts/check-env.sh +++ b/scripts/check-env.sh @@ -58,7 +58,7 @@ # is the container, and the database is elsewhere # NODE_ENV=Production matches no `=== "production"` anywhere, so every # branch takes its development arm in production -# JWT_SECRET=devsecret the eight characters someone typed to get the dev +# JWT_SECRET=devsecret the eight characters someone typed to get the dev (secrets-ok: example) # server up, now signing real sessions # API_URL=https://api.example.com/ # one trailing slash; every joined path is `//v1/...` @@ -247,12 +247,12 @@ SPEC=( # API_KEY, KEYCLOAK_SECRET and MONKEY_HOST alike. The last is a false positive # and costs nothing: its value is described rather than shown. # --------------------------------------------------------------------------- -SECRET_NAME_PATTERN='SECRET|TOKEN|PASSWORD|PASSWD|PWD|CREDENTIAL|PRIVATE|SALT|SIGNATURE|SIGNING|AUTH|KEY|DSN|COOKIE' +SECRET_NAME_PATTERN='SECRET|TOKEN|PASSWORD|PASSWD|PWD|CREDENTIAL|PRIVATE|SALT|SIGNATURE|SIGNING|AUTH|KEY|DSN|COOKIE' # secrets-ok: names, not a value # Values a `secret-min-length` variable must not be, compared in lower case and # never echoed. These are the strings typed to make the dev server start, which # then travel to production inside a copied .env and satisfy every length rule. -PLACEHOLDER_SECRETS='changeme change-me change_me changethis secret mysecret supersecret password passwd hunter2 test testing example placeholder todo tbd xxx xxxx admin dev devsecret development your-secret-here your_secret_here notasecret 123456 12345678 abc123' +PLACEHOLDER_SECRETS='changeme change-me change_me changethis secret mysecret supersecret password passwd hunter2 test testing example placeholder todo tbd xxx xxxx admin dev devsecret development your-secret-here your_secret_here notasecret 123456 12345678 abc123' # secrets-ok: known-weak values # Redaction applies to NAMES read out of a file too, not only to values. # diff --git a/scripts/secrets.sh b/scripts/secrets.sh index 1289aa8..2effe05 100755 --- a/scripts/secrets.sh +++ b/scripts/secrets.sh @@ -26,7 +26,7 @@ # bash scripts/secrets.sh # staged changes (use in pre-commit) # bash scripts/secrets.sh --tracked # everything tracked, for an audit # bash scripts/secrets.sh --built dist/ # the artifact users receive -# SECRETS_PATTERN_FILE=src/lib/log.ts bash scripts/secrets.sh +# SECRETS_PATTERN_FILE=src/lib/log.ts bash scripts/secrets.sh # secrets-ok: a path # bash scripts/secrets.sh --allow docs/examples/ # # ## --built, and why the repository is the wrong place to stop @@ -56,7 +56,16 @@ # in the history** after you delete it, and this will not tell you that — the # fix there is a rotation, not a scan. Rotate first, then clean up. # -# Exit codes: 0 nothing found. 1 a candidate found. 2 nothing was scanned. +# ## Excusing one line +# +# A line that has to show a credential SHAPE, such as a usage example or a list +# of known-weak values, carries `secrets-ok:` and a reason on the same line. +# That excuses that one line and nothing else, and it stays visible in review +# and to grep, which an --allow path does not. There is no blanket exemption for +# comments: a key pasted into a comment is still a leaked key. +# +# Exit codes: 0 nothing found. 1 a candidate found. 2 nothing was scanned, or a +# pattern could not be read, which is the same thing. # --------------------------------------------------------------------------- set -uo pipefail @@ -167,10 +176,20 @@ PATTERNS=( '//[^/@[:space:]:"'"'"'{},<>]+:[^/@[:space:]"'"'"'{},<>]+@' '[?&](token|key|secret|password|access_token|api_key)=[^&[:space:]"]+' '\b(Bearer|Basic)[[:space:]]+[A-Za-z0-9._~+/=-]{20,}' # an authorization header - # Anchored to the start of a line or an `export`, because unanchored it - # matched `access_token = $1` in SQL and `apiKey=` in a property list — three - # findings in src/ that were column names, not credentials. - '(^|[^A-Za-z0-9_])[A-Z0-9_]*(SECRET|TOKEN|PASSWORD|PASSWD|PASS|API_KEY)[A-Z0-9_]*=[^[:space:]"'"'"'$][^[:space:]"'"'"']{7,}' + # NAME=value, where NAME is upper-case and contains SECRET, TOKEN, PASS or + # API_KEY. Three things about it were learned the hard way: + # + # - It must not be anchored to the start of a line. Neither scan mode ever + # presents one: a staged diff starts every line with "+", and --tracked + # prefixes each with its file name. Anchored, it matched nothing at all, so + # the name now merely must not continue an identifier. + # - Case matters. Lower-case `access_token = $1` in SQL and `apiKey=` in a + # property list are column and field names, not credentials. + # - The value may be quoted. Unquoted-only, it missed API_KEY="...", which is + # how a real secret is most often written down. A value starting with `$` + # is a variable reference and one starting with `<` is a placeholder, and + # neither is flagged. + '(^|[^A-Za-z0-9_])[A-Z0-9_]*(SECRET|TOKEN|PASSWORD|PASSWD|PASS|API_KEY)[A-Z0-9_]*=["'"'"']?[^[:space:]"'"'"'$<][^[:space:]"'"'"']{7,}' '-----BEGIN [A-Z ]*PRIVATE KEY-----' '\bghp_[A-Za-z0-9]{20,}' # GitHub '\bxox[baprs]-[A-Za-z0-9-]{10,}' # Slack @@ -277,6 +296,19 @@ else done < <(git ls-files) fi +# Every pattern must compile before its silence means anything. grep answers an +# unreadable pattern with exit 2, and the scan below discards grep's errors, so +# a broken pattern reads as "no credential shapes", the same silence as a clean +# tree. The private-key pattern was exactly that for as long as it existed: it +# starts with dashes, grep took it for an option, and nothing said so. +for pattern in "${PATTERNS[@]}" "${NOTED_PATTERNS[@]}"; do + grep -qE -e "$pattern" /dev/null + if [ "$?" -eq 2 ]; then + say "grep cannot read the pattern '${pattern:0:40}', so nothing was scanned." + exit 2 + fi +done + if [ -z "$CONTENT" ]; then # "Nothing to scan" means two different things and collapsing them makes a # DELETE-ONLY COMMIT IMPOSSIBLE. In staged mode CONTENT is built from ADDED @@ -304,6 +336,9 @@ for pattern in "${PATTERNS[@]}"; do done [ -n "$skip" ] && continue + # One line excused on purpose, with its reason beside it. See the header. + case "$hit" in *secrets-ok:*) continue ;; esac + # The match is masked, then the line is truncated. Truncation alone was not # enough and used to be all there was: it bounds how much of a LONG value # reaches the terminal and prints a short one whole, so the scanner @@ -317,7 +352,7 @@ for pattern in "${PATTERNS[@]}"; do [ -n "$masked" ] || masked="[a line matching a credential pattern, unprintable]" printf ' %.120s…\n' "$masked" found=$((found + 1)) - done < <(printf '%s\n' "$CONTENT" | grep -nEI "$pattern" 2>/dev/null | head -20) + done < <(printf '%s\n' "$CONTENT" | grep -nEI -e "$pattern" 2>/dev/null | head -20) done # The public-by-design tier. Printed, counted, and deliberately not fatal. @@ -332,7 +367,7 @@ if [ "$MODE" = "built" ]; then fi printf ' %.120s…\n' "$hit" noted=$((noted + 1)) - done < <(printf '%s\n' "$CONTENT" | grep -oEI "$pattern" 2>/dev/null | sort -u | head -20) + done < <(printf '%s\n' "$CONTENT" | grep -oEI -e "$pattern" 2>/dev/null | sort -u | head -20) done if [ "$noted" -gt 0 ]; then diff --git a/scripts/verify.d/20-secrets b/scripts/verify.d/20-secrets index f7bc824..765bc49 100755 --- a/scripts/verify.d/20-secrets +++ b/scripts/verify.d/20-secrets @@ -1,6 +1,6 @@ #!/usr/bin/env bash # -# Credentials in the tracked tree. +# Credentials in the tracked tree, and in the bundle users receive. # # `scripts/secrets.sh` runs on the staged diff from the pre-commit hook, which # is the cheap moment. This is the whole-tree version, run as part of verify so @@ -11,9 +11,25 @@ # commits of a then-public repository for a month, and a staged-diff scan # installed afterwards would never have mentioned them. # +# It also scans dist/, which 10-build has just produced. A key can reach the +# bundle from an environment variable inlined at build time without ever being +# committed, and the tracked scan cannot see that. SECURITY_CHECKLIST.md listed +# this as a manual release check for months; nothing ran it. +# # Exit 0 clean, 1 findings, 2 the scanner could not run. set -uo pipefail cd "$(git rev-parse --show-toplevel)" || exit 1 -[ -f scripts/secrets.sh ] || { echo "secrets: scripts/secrets.sh is missing — nothing was scanned." >&2; exit 2; } -bash scripts/secrets.sh --tracked +[ -f scripts/secrets.sh ] || { echo "secrets: scripts/secrets.sh is missing, so nothing was scanned." >&2; exit 2; } + +status=0 +bash scripts/secrets.sh --tracked || status=$? + +if [ ! -d dist ]; then + echo "secrets: dist/ is missing, so the built output was not scanned. Run 10-build first." >&2 + exit 2 +fi + +bash scripts/secrets.sh --built dist/ || { rc=$?; [ "$rc" -gt "$status" ] && status=$rc; } + +exit "$status"