Skip to content

no-secrets: deleting the Slack pattern leaves the gate green, and its tree scan passes on an empty file list #387

Description

@vladimirrott

tests/release/no-secrets.test.sh runs in docs-and-hygiene, one of the five
required checks. It guards scripts/check_no_secrets.sh, the scanner
.githooks/pre-commit runs over staged content before a commit object exists.

Most of the guard holds up. Eight of the nine patterns in the scanner have a
positive case, the fixture side is covered, and the "never echo the credential"
assertion from #228 carries its own mutation proof. Three gaps sit in the parts
nobody exercised, and a fourth turned up while measuring them.

Everything below ran against a throwaway clone of 049bc1f, one mutation at a
time, restored between each. The harness is two lines:

$ cat /tmp/ns-audit/run.sh
#!/usr/bin/env bash
# usage: run.sh <label>
cd /tmp/ns-audit/tree
out="$(bash tests/release/no-secrets.test.sh 2>&1)"; rc=$?
printf '=== %s === rc=%d\n%s\n' "$1" "$rc" "$out"

1. The Slack pattern has no positive case, so deleting it is silent

check_no_secrets.sh carries nine PATTERNS entries. POSITIVES in the guard
synthesises seven keys and an eighth case covers AWS. Slack is the one left
over.

$ sed -i '/"Slack token:xox\[baprs\]-\[A-Za-z0-9-\]{20,}"/d' scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh M1-slack-pattern-deleted
=== M1-slack-pattern-deleted === rc=0
ok: catches real-shaped credentials, ignores this repo's fixtures
ok: the whole tracked tree scans clean, and findings never echo the secret

Delete any other pattern and the guard names it. That calibration is what makes
the green above worth reading:

$ sed -i '/"Groq:gsk_\[A-Za-z0-9\]{40,}"/d' scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh M2-groq-pattern-deleted
=== M2-groq-pattern-deleted === rc=1
FAIL: a gsk_AAAA… credential (56 chars) was NOT caught

$ sed -i '/"AWS access key:AKIA\[0-9A-Z\]{16}"/d' scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh M3-aws-pattern-deleted
=== M3-aws-pattern-deleted === rc=1
FAIL: an AWS access key was NOT caught

$ sed -i 's|"OpenAI:sk-\[A-Za-z0-9\]{40,}"|"OpenAI:sk-[A-Za-z0-9]{400,}"|' scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh M4-openai-length-400
=== M4-openai-length-400 === rc=1
FAIL: a sk-AAAAA… credential (51 chars) was NOT caught

Slack tokens are not a hypothetical class here.
crates/sysknife-brain/src/prefs.rs:41-42 redacts xoxb- and xoxp- out of
user preferences, so the codebase already treats them as credentials in the
other direction. Both the pattern and the case list arrived in bf8d412 on
2026-08-08, so the hole is as old as the file.

2. The tracked-tree scan reports "clean" when it scans nothing

Section 3 builds its file list from git ls-files and never checks the list
came back with anything in it. The scanner with no arguments exits 0:

$ scripts/check_no_secrets.sh; echo "rc=$?"
rc=0

So the assertion the file header calls "the assertion that keeps this scanner
usable" passes over an empty list. Take git ls-files away and the step stays
green while printing the opposite of what happened:

$ mv .git /tmp/ns-audit/parked-dotgit
$ /tmp/ns-audit/run.sh S2-tracked-file-list-empty
=== S2-tracked-file-list-empty === rc=0
fatal: not a git repository (or any of the parent directories): .git
ok: catches real-shaped credentials, ignores this repo's fixtures
ok: the whole tracked tree scans clean, and findings never echo the secret

The fatal: goes to stderr. Same shape as cargo test NAME -- --exact matching
nothing and exiting 0.

The other two input-side breaks fail loudly, which is worth recording:

$ mv scripts/check_no_secrets.sh /tmp/ns-audit/parked.sh
$ /tmp/ns-audit/run.sh S1-scanner-missing
=== S1-scanner-missing === rc=1
FAIL: /tmp/ns-audit/tree/scripts/check_no_secrets.sh not found or not executable

$ chmod -x scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh S3-scanner-not-executable
=== S3-scanner-not-executable === rc=1
FAIL: /tmp/ns-audit/tree/scripts/check_no_secrets.sh not found or not executable

3. An allowlist entry no pattern can reach

ALLOWED_EXAMPLES exempts two literals by exact match. The second is AWS's
published secret access key, and no regex in PATTERNS produces it: the AWS
rule is AKIA[0-9A-Z]{16}, which matches the access key ID and nothing else.

$ printf 'k = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"\n' > /tmp/ns-audit/awssec.txt
$ scripts/check_no_secrets.sh /tmp/ns-audit/awssec.txt; echo "rc=$?"
rc=0

$ sed -i '/"wJalrXUtnFEMI\/K7MDENG\/bPxRfiCYEXAMPLEKEY"/d' scripts/check_no_secrets.sh
$ /tmp/ns-audit/run.sh M8-aws-secret-allowlist-deleted
=== M8-aws-secret-allowlist-deleted === rc=0
ok: catches real-shaped credentials, ignores this repo's fixtures
ok: the whole tracked tree scans clean, and findings never echo the secret

Removing it changes nothing. It reads as coverage of AWS secret keys that the
scanner does not have. No pattern covers that format because it is 40 characters
of base64, which matches every checksum and blob in the tree.

4. A dirty tree exits 141, not 1

Turned up while testing the above. Section 3's diagnostic pipes the scanner into
head -10 under set -o pipefail, so once the scan prints more than ten
findings the scanner takes SIGPIPE and the file aborts before fail=1 and
before the summary. repro.sh is that branch lifted out verbatim, run over
fifteen files each holding a synthesised Groq key:

#!/usr/bin/env bash
set -euo pipefail
CHECK="$1"; shift
fail=0
if ! "$CHECK" "$@" >/dev/null 2>&1; then
    echo "FAIL: the tracked tree does not pass its own secret scan:"
    "$CHECK" "$@" 2>&1 | head -10
    fail=1
fi
echo "reached the end, fail=$fail"
$ bash repro.sh ./check_no_secrets.sh f*.txt; echo "rc=$?"
FAIL: the tracked tree does not pass its own secret scan:
SECRET: Groq key in f10.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f11.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f12.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f13.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f14.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f15.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f1.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f2.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f3.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f4.txt — starts gsk_AAA…, 56 chars
rc=141

reached the end never prints. The step still goes red, so nothing is hidden.
The exit code names a signal, the list is cut at ten with no sign there were
more. repro-fixed.sh is the same file with head -10 <<< "$scan_out" in place
of the pipe, and it fixes both:

$ bash repro-fixed.sh ./check_no_secrets.sh f*.txt; echo "rc=$?"
FAIL: the tracked tree does not pass its own secret scan:
SECRET: Groq key in f10.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f11.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f12.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f13.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f14.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f15.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f1.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f2.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f3.txt — starts gsk_AAA…, 56 chars
SECRET: Groq key in f4.txt — starts gsk_AAA…, 56 chars
reached the end, fail=1
rc=0

Scope

Derive the coverage instead of listing it, which is the direction the other
guards in this directory have already moved:

  • Key each positive case by the provider label the scanner prints, parse
    PATTERNS out of check_no_secrets.sh, and fail when the two sets differ in
    either direction. A pattern added without a case, and a pattern deleted while
    its case stays, both go red and name the provider.
  • Require each case to be reported under its own provider name, so a case that
    trips a neighbouring pattern cannot stand in for a deleted one.
  • Add the missing Slack case.
  • Assert the tracked list contains scripts/check_no_secrets.sh before trusting
    a clean scan of it.
  • Assert every ALLOWED_EXAMPLES entry is matched by some pattern, and drop the
    AWS secret-key entry with a comment saying why no pattern covers that format.
  • Replace the | head -10 diagnostic with a here-string.

This does not stop somebody deleting a pattern and its case in one commit.
Nothing cheap does. It turns a one-line accident into a two-file deliberate act,
which is the whole of what is claimed for it.

Out of scope here: no-secrets.test.sh is one of the seven scripts CI runs and
scripts/ci-local.sh does not, so nobody exercises any of this before pushing.
That belongs to #346 and the patch below leaves ci-local.sh alone.

The patch

diff --git a/scripts/check_no_secrets.sh b/scripts/check_no_secrets.sh
index 0d9783a..5ef0281 100755
--- a/scripts/check_no_secrets.sh
+++ b/scripts/check_no_secrets.sh
@@ -33,8 +33,12 @@ set -euo pipefail
 # Literal values that look like credentials and are published as examples by
 # their own vendors. Exact matches only — never a prefix.
 ALLOWED_EXAMPLES=(
-    "AKIAIOSFODNN7EXAMPLE"                      # AWS docs, every IAM example
-    "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"  # its matching secret
+    "AKIAIOSFODNN7EXAMPLE"  # AWS docs, every IAM example
+    # AWS's matching secret example was listed here too. No pattern above can
+    # produce it -- an AWS secret access key is 40 characters of base64, which
+    # matches every checksum and blob in the tree -- so the exemption exempted
+    # nothing. An allowlist entry no pattern reaches reads as coverage the
+    # scanner does not have.
 )
 
 # provider:regex. Bodies are sized to the real format so fixtures fall short.
diff --git a/tests/release/no-secrets.test.sh b/tests/release/no-secrets.test.sh
index 7cab257..f8b4252 100755
--- a/tests/release/no-secrets.test.sh
+++ b/tests/release/no-secrets.test.sh
@@ -15,6 +15,11 @@
 #
 # No real credential appears in this file. The positive cases are synthesised at
 # the right length from a fixed filler character.
+#
+# The positive cases are keyed by the provider label the scanner prints, and
+# that key set is compared against the PATTERNS array in the scanner itself.
+# A hand-written list of cases drifts from the list it is meant to cover: the
+# Slack pattern went untested that way, and deleting it left this file green.
 set -euo pipefail
 
 ROOT="$(cd "$(dirname "$0")/../.." && pwd)"
@@ -26,34 +31,71 @@ trap 'rm -rf "$tmp"' EXIT
 
 fail=0
 
+# --- 0. Read the scanner's own pattern and allowlist tables -----------------
+# Everything below is checked against these, so an unreadable table is a hard
+# stop rather than a silently empty comparison.
+read_array() {
+    # $1 = array name in the scanner. Emits one element body per line.
+    sed -n "/^$1=(/,/^)/p" "$CHECK" | sed -nE 's/^[[:space:]]*"(.*)"([[:space:]]+#.*)?$/\1/p'
+}
+
+mapfile -t scanner_providers < <(read_array PATTERNS | sed -E 's/:.*$//' | sort -u)
+mapfile -t scanner_regexes < <(read_array PATTERNS | sed -E 's/^[^:]*://')
+mapfile -t scanner_allowlist < <(read_array ALLOWED_EXAMPLES)
+
+if [ "${#scanner_providers[@]}" -eq 0 ] || [ "${#scanner_allowlist[@]}" -eq 0 ]; then
+    echo "FAIL: could not read PATTERNS / ALLOWED_EXAMPLES out of $CHECK"
+    echo "      (${#scanner_providers[@]} pattern(s), ${#scanner_allowlist[@]} allowlist entry/entries)"
+    exit 1
+fi
+
 # --- 1. Real-shaped credentials must be caught ------------------------------
 # Synthesised, never a live key: prefix + filler at the real body length.
+# Each case names the provider the scanner is expected to report, so a case that
+# happens to trip a different pattern cannot stand in for a deleted one.
 make_key() { printf '%s%s' "$1" "$(head -c "$2" < /dev/zero | tr '\0' 'A')"; }
 
 declare -a POSITIVES=(
-    "$(make_key 'gsk_' 52)"          # Groq
-    "$(make_key 'sk-' 48)"           # OpenAI classic
-    "$(make_key 'sk-proj-' 64)"      # OpenAI project
-    "$(make_key 'sk-ant-' 95)"       # Anthropic
-    "$(make_key 'ghp_' 36)"          # GitHub PAT
-    "$(make_key 'github_pat_' 82)"   # GitHub fine-grained
-    "$(make_key 'AIza' 35)"          # Google
+    "Groq|$(make_key 'gsk_' 52)"
+    "OpenAI|$(make_key 'sk-' 48)"
+    "OpenAI project|$(make_key 'sk-proj-' 64)"
+    "Anthropic|$(make_key 'sk-ant-' 95)"
+    "GitHub PAT|$(make_key 'ghp_' 36)"
+    "GitHub fine-grained PAT|$(make_key 'github_pat_' 82)"
+    "Google API key|$(make_key 'AIza' 35)"
+    "Slack token|$(make_key 'xoxb-' 24)"
+    # AWS keys are fixed-length, so the example and a real one are the same
+    # shape: only the exact-match allowlist separates them.
+    "AWS access key|$(printf 'AKIA%s' '0123456789ABCDEF')"
 )
-for key in "${POSITIVES[@]}"; do
+
+for entry in "${POSITIVES[@]}"; do
+    provider="${entry%%|*}"
+    key="${entry#*|}"
     printf 'API_KEY = "%s"\n' "$key" > "$tmp/leak.txt"
-    if "$CHECK" "$tmp/leak.txt" >/dev/null 2>&1; then
+    if out="$("$CHECK" "$tmp/leak.txt" 2>&1)"; then
         echo "FAIL: a ${key:0:8}… credential ($(printf '%s' "$key" | wc -c) chars) was NOT caught"
         fail=1
+    elif ! grep -qF "SECRET: $provider key" <<< "$out"; then
+        echo "FAIL: the ${key:0:8}… case was caught, but not by the '$provider' pattern"
+        fail=1
     fi
 done
 
-# AWS keys are fixed-length, so the example and a real one are the same shape:
-# only the exact-match allowlist separates them.
-printf 'aws = "AKIA%s"\n' "0123456789ABCDEF" > "$tmp/aws.txt"
-if "$CHECK" "$tmp/aws.txt" >/dev/null 2>&1; then
-    echo "FAIL: an AWS access key was NOT caught"
+# --- 1b. Every scanner pattern must have a case above -----------------------
+mapfile -t case_providers < <(printf '%s\n' "${POSITIVES[@]}" | cut -d'|' -f1 | sort -u)
+
+while IFS= read -r provider; do
+    [ -n "$provider" ] || continue
+    echo "FAIL: the scanner has a '$provider' pattern and this file never exercises it"
     fail=1
-fi
+done < <(comm -23 <(printf '%s\n' "${scanner_providers[@]}") <(printf '%s\n' "${case_providers[@]}"))
+
+while IFS= read -r provider; do
+    [ -n "$provider" ] || continue
+    echo "FAIL: this file has a '$provider' case and the scanner has no such pattern"
+    fail=1
+done < <(comm -13 <(printf '%s\n' "${scanner_providers[@]}") <(printf '%s\n' "${case_providers[@]}"))
 
 # --- 2. The repo's own fixtures must NOT be caught --------------------------
 declare -a NEGATIVES=(
@@ -74,14 +116,37 @@ for fixture in "${NEGATIVES[@]}"; do
     fi
 done
 
+# --- 2b. Every allowlist entry must be reachable ----------------------------
+# An exact-match exemption for a string no pattern can produce exempts nothing.
+# It reads as coverage the scanner does not have.
+for example in "${scanner_allowlist[@]}"; do
+    matched=0
+    for regex in "${scanner_regexes[@]}"; do
+        if grep -qE "$regex" <<< "$example"; then matched=1; break; fi
+    done
+    if [ "$matched" != 1 ]; then
+        echo "FAIL: allowlist entry '${example:0:12}…' is matched by no pattern, so it exempts nothing"
+        fail=1
+    fi
+done
+
 # --- 3. The whole tracked tree must be clean --------------------------------
 # The assertion that keeps this scanner usable. If it ever fails, the answer is
 # to shorten the offending fixture, not to loosen a pattern.
 cd "$ROOT"
 mapfile -t tracked < <(git ls-files)
-if ! "$CHECK" "${tracked[@]}" >/dev/null 2>&1; then
+# The scanner exits 0 on an empty argument list, so an empty or truncated
+# `git ls-files` would make this section screen nothing and still pass.
+if ! printf '%s\n' "${tracked[@]}" | grep -qx 'scripts/check_no_secrets.sh'; then
+    echo "FAIL: git ls-files returned ${#tracked[@]} path(s) and not the scanner's own;"
+    echo "      the tracked-tree scan would have screened nothing"
+    fail=1
+elif ! scan_out="$("$CHECK" "${tracked[@]}" 2>&1)"; then
+    # A here-string, not a pipe into head: `"$CHECK" ... | head -10` takes
+    # SIGPIPE once the scan prints more than ten lines, and `set -o pipefail`
+    # then aborts the whole file with 141 and a half-written diagnostic.
     echo "FAIL: the tracked tree does not pass its own secret scan:"
-    "$CHECK" "${tracked[@]}" 2>&1 | head -10
+    head -10 <<< "$scan_out"
     fail=1
 fi
 
@@ -113,5 +178,6 @@ fi
 CHECK="$real_check"
 
 if [ "$fail" != 0 ]; then exit 1; fi
-echo "ok: catches real-shaped credentials, ignores this repo's fixtures"
+echo "ok: every one of ${#scanner_providers[@]} scanner pattern(s) has a positive case, and each is"
+echo "    reported under its own provider name; this repo's fixtures are ignored"
 echo "ok: the whole tracked tree scans clean, and findings never echo the secret"

Verification

$ /tmp/ns-audit/run.sh N0b-patched-baseline
=== N0b-patched-baseline === rc=0
ok: every one of 9 scanner pattern(s) has a positive case, and each is
    reported under its own provider name; this repo's fixtures are ignored
ok: the whole tracked tree scans clean, and findings never echo the secret

Mutation grid against the patched tree, each applied alone:

# Mutation Patched gate says
N0b none rc=0, every one of 9 scanner pattern(s) has a positive case
N1 Slack pattern deleted (the one the current gate passes) rc=1, a xoxb-AAA… credential (29 chars) was NOT caught + this file has a 'Slack token' case and the scanner has no such pattern
N2 Groq pattern deleted rc=1, both messages, naming Groq
N3 a Stripe live key pattern added with no case rc=1, the scanner has a 'Stripe live key' pattern and this file never exercises it
N4 the dead AWS secret allowlist entry put back rc=1, allowlist entry 'wJalrXUtnFEM…' is matched by no pattern, so it exempts nothing
N5 Groq relabelled Groq Cloud in the scanner rc=1, the gsk_AAAA… case was caught, but not by the 'Groq' pattern, plus both set-difference lines
N6 PATTERNS renamed PATTERN_LIST, scanner still working rc=1, could not read PATTERNS / ALLOWED_EXAMPLES out of … (0 pattern(s), 1 allowlist entry/entries)
N7 scanner prints the whole credential rc=1, the scanner echoed the credential it found
N8 git ls-files returns nothing rc=1, git ls-files returned 0 path(s) and not the scanner's own; the tracked-tree scan would have screened nothing

N1 is the mutation this issue exists for. N6 and N8 are the checks on the
guard's own input: the first makes the scanner's table unreadable while leaving
the scanner working, the second takes its file list away.

It applies to 049bc1f without fuzz:

$ git apply --check -v no-secrets-coverage.patch
Checking patch scripts/check_no_secrets.sh...
Checking patch tests/release/no-secrets.test.sh...

Also clean: bash -n, and shellcheck --severity=warning (shellcheck 0.10.0),
which is the command e2e.yml's scripts-lint runs over this file.

One warning for whoever takes this. The first draft put AKIA0123456789ABCDEF
into POSITIVES as a single literal, and section 3 failed on it, because
section 3 scans this file too. Build the AWS case with printf so the source
never holds a whole key. That is the guard working, and it is why the assertion
is there.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingclaimedSomeone has said in the thread that they are working on thishelp wantedExtra attention is neededmediumDifficulty: needs familiarity with one subsystem

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions