fix(leak-scan): repair four rules that could never fire

actions/leak-scan/run.sh

@@ -62,6 +62,34 @@ path_scan_status="pass"
 #
 # Pre-existing content is not left unguarded: all-refs mode still reads whole
 # files across every ref, which is the mode built for that question.
+# Emit the added lines of a unified diff read on stdin, dropping any whose file
+# path starts with one of the space-separated prefixes in $1.
+#
+# EXCLUDE_PATHS is documented as "Only effective in pr-diff mode", but it was
+# only ever wired into the path scan -- the pr-diff deny-list ignored it
+# entirely, so callers passing exclude-paths to suppress a known false-positive
+# class silently got no exclusion at all.
+#
+# Path prefixes are matched as literal string prefixes, matching the path scan's
+# documented semantics. substr is used rather than $2 so paths containing spaces
+# are handled.
+filter_added_lines() {
+  local excludes="$1"
+  awk -v excludes="$excludes" '
+    BEGIN { n = split(excludes, ex, " "); skip = 0 }
+    /^\+\+\+ / {
+      path = substr($0, 5)
+      sub(/^b\//, "", path)
+      skip = 0
+      for (i = 1; i <= n; i++) {
+        if (ex[i] != "" && index(path, ex[i]) == 1) { skip = 1; break }
+      }
+      next
+    }
+    /^\+/ { if (!skip) print substr($0, 2) }
+  '
+}
+
 run_diff_deny_list_scan() {
   local patterns_file="${GW_ROOT}/data/leak-patterns.json"
   [[ -f "$patterns_file" ]] || fail "E_MISSING_LEAK_PATTERNS: ${patterns_file} not found"
@@ -81,9 +109,23 @@ run_diff_deny_list_scan() {
 
   # Added lines only, with the leading marker stripped so a pattern anchored at
   # line start still behaves. --unified=0 keeps context lines out.
-  local added
+  # EXCLUDE_PATHS is applied here, per its documented "pr-diff mode" contract.
+  local added all_added
+  all_added=$(git diff --unified=0 "$BASE_REF" "$HEAD_REF" -- $PATHS 2>/dev/null \
+    | filter_added_lines "" || true)
   added=$(git diff --unified=0 "$BASE_REF" "$HEAD_REF" -- $PATHS 2>/dev/null \
-    | grep -E '^\+' | grep -vE '^\+\+\+' | sed 's/^+//' || true)
+    | filter_added_lines "${EXCLUDE_PATHS:-}" || true)
+
+  if [[ -n "${EXCLUDE_PATHS:-}" ]]; then
+    local before after excluded
+    before=$(printf '%s\n' "$all_added" | grep -c . || true)
+    after=$(printf '%s\n' "$added" | grep -c . || true)
+    excluded=$(( before - after ))
+    if [[ "$excluded" -gt 0 ]]; then
+      printf '::notice::leak-scan: excluded %d added line(s) matching EXCLUDE_PATHS prefixes\n' \
+        "$excluded"
+    fi
+  fi
 
   if [[ -z "$added" ]]; then
     printf '::notice::leak-scan: no added lines in scope\n'
@@ -95,7 +137,10 @@ run_diff_deny_list_scan() {
   for pattern in "${all_patterns[@]}"; do
     local count
     # Count only; matching content is never printed.
-    count=$(printf '%s\n' "$added" | grep -c -E "$pattern" || true)
+    # -e is required, not stylistic: a pattern beginning with '-'
+    # (-----BEGIN .* PRIVATE KEY-----) is otherwise parsed as an option and
+    # grep exits with a usage error, so the private-key rule never fired.
+    count=$(printf '%s\n' "$added" | grep -c -E -e "$pattern" || true)
     [[ "$count" -gt 0 ]] && { path_scan_status="fail"; matched=$((matched + count)); }
   done
 
@@ -164,7 +209,7 @@ run_path_deny_list_scan() {
         --include='*.json' \
         --include='*.sh' \
         --include='*.md' \
-        -l -E "$pattern" \
+        -l -E -e "$pattern" \
         "$scan_path" 2>/dev/null || true)
       if [[ -n "$matches" ]]; then
         path_scan_status="fail"
@@ -256,8 +301,14 @@ run_pr_diff_scan() {
       filtered_files=$(printf '%s\n' "$filtered_files" \
         | grep -v "^${excl_prefix}" || true)
     done
-    local excluded_count
-    excluded_count=$(( $(printf '%s\n' "$changed_files" | grep -c .) - $(printf '%s\n' "$filtered_files" | grep -c . || echo 0) ))
+    # `grep -c .` prints 0 AND exits 1 on no match, so `|| echo 0` appended a
+    # second "0" and the arithmetic saw "1 - 0\n0" -- a syntax error that killed
+    # the scan whenever EXCLUDE_PATHS excluded every changed file. Capture the
+    # counts first and let `|| true` absorb the exit status.
+    local before_count after_count excluded_count
+    before_count=$(printf '%s\n' "$changed_files" | grep -c . || true)
+    after_count=$(printf '%s\n' "$filtered_files" | grep -c . || true)
+    excluded_count=$(( before_count - after_count ))
     if [[ $excluded_count -gt 0 ]]; then
       printf '::notice::leak-scan: excluded %d path(s) matching EXCLUDE_PATHS prefixes\n' \
         "$excluded_count"

data/leak-patterns.json

@@ -3,28 +3,28 @@
   "categories": {
     "ipv4_literal": {
       "patterns": [
-        "\\b\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\b"
+        "\\b[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\b"
       ]
     },
     "ipv6_literal": {
       "patterns": [
-        "\\b(?:[0-9a-fA-F]{1,4}:){4,7}[0-9a-fA-F]{1,4}\\b",
-        "\\b(?:[0-9a-fA-F]{1,4}:)+:"
+        "\\b([0-9a-fA-F]{1,4}:){4,7}[0-9a-fA-F]{1,4}\\b",
+        "\\b([0-9a-fA-F]{1,4}:)+:"
       ]
     },
     "cgnat": {
       "patterns": [
         "100\\.6[4-9]\\.",
-        "100\\.[7-9]\\d\\.",
-        "100\\.1[01]\\d\\.",
+        "100\\.[7-9][0-9]\\.",
+        "100\\.1[01][0-9]\\.",
         "100\\.12[0-7]\\."
       ]
     },
     "rfc1918": {
       "patterns": [
         "192\\.168\\.",
         "^10\\.",
-        "172\\.(1[6-9]|2\\d|3[01])\\."
+        "172\\.(1[6-9]|2[0-9]|3[01])\\."
       ]
     },
     "k8s_join_tokens": {

tests/test_leak_patterns_grep_compatible.py

@@ -0,0 +1,112 @@
+"""Every deny-list pattern must work under the engine that actually runs it.
+
+run.sh matches with `grep -E` (POSIX ERE). The rest of the suite matches with
+Python's `re` (Perl-flavoured). Those engines disagree, and the disagreement is
+silent: `\\d`, `\\s`, `\\w` and `(?:...)` are all valid in Python and meaningless
+in ERE, so a pattern using them passes every Python test while matching nothing
+in production.
+
+That is not hypothetical. Before this module existed, ipv4_literal, both
+ipv6_literal patterns, two cgnat patterns and one rfc1918 pattern were dead in
+the shell path, and the ssh_keys private-key pattern made grep exit with a usage
+error because it begins with '-'. The deny-list reported "no match" for a
+private key in a diff.
+
+These tests therefore shell out to grep rather than trusting `re`.
+"""
+
+from __future__ import annotations
+
+import json
+import shutil
+import subprocess
+import unittest
+from pathlib import Path
+
+ROOT = Path(__file__).resolve().parents[1]
+PATTERNS = json.loads((ROOT / "data" / "leak-patterns.json").read_text(encoding="utf-8"))
+
+PERL_ONLY = ("\\d", "\\w", "\\s", "\\D", "\\W", "\\S", "(?:", "(?=", "(?!", "(?<")
+
+# One string per category that the category exists to catch. If grep does not
+# match these, the guard is not guarding.
+MUST_MATCH = {
+    "ipv4_literal": ["10.42.0.0/16", "addr 192.0.2.7", "  - 169.254.0.0/16"],
+    "ipv6_literal": ["fd7a:115c:a1e0::1", "2001:0db8:85a3:0000:0000:8a2e:0370:7334"],
+    "cgnat": ["100.64.1.2", "100.75.0.1", "100.115.3.4", "100.127.0.1"],
+    "rfc1918": ["192.168.0.99", "10.0.0.1", "172.16.0.0/12", "172.25.1.1", "172.31.9.9"],
+    "ssh_keys": [
+        "-----BEGIN OPENSSH PRIVATE KEY-----",
+        "-----BEGIN RSA PRIVATE KEY-----",
+        "ssh-ed25519 AAAAC3Nz",
+    ],
+    "k8s_join_tokens": ["  apiServerEndpoint: 1.2.3.4:6443", "node-token: abc"],
+    "vault_refs": ["vaultPolicy: agents", "approle login"],
+    "hardware_ids": ["00:1a:2b:3c:4d:5e", "/dev/sda1", "by-id/wwn-0x5000"],
+    "provider_ids": ["zone-id: Z123", "tailscale-device: foo"],
+}
+
+
+def all_patterns():
+    for category, body in PATTERNS["categories"].items():
+        for pattern in body["patterns"]:
+            yield category, pattern
+
+
+def grep_matches(pattern: str, text: str) -> bool:
+    """Match exactly as run.sh does: grep -E -e, so a leading '-' is a pattern."""
+    result = subprocess.run(
+        ["grep", "-c", "-E", "-e", pattern],
+        input=f"{text}\n",
+        capture_output=True,
+        text=True,
+    )
+    if result.returncode not in (0, 1):
+        raise AssertionError(
+            f"grep failed for {pattern!r}: rc={result.returncode} {result.stderr.strip()}"
+        )
+    return result.returncode == 0
+
+
+@unittest.skipIf(shutil.which("grep") is None, "grep unavailable")
+class TestPatternsAreEreCompatible(unittest.TestCase):
+    def test_no_perl_only_constructs(self) -> None:
+        for category, pattern in all_patterns():
+            with self.subTest(category=category, pattern=pattern):
+                for token in PERL_ONLY:
+                    self.assertNotIn(
+                        token,
+                        pattern,
+                        f"{token} is Perl-only and matches nothing under grep -E",
+                    )
+
+    def test_grep_accepts_every_pattern_without_error(self) -> None:
+        """A pattern grep rejects is a rule that can never fire."""
+        for category, pattern in all_patterns():
+            with self.subTest(category=category, pattern=pattern):
+                result = subprocess.run(
+                    ["grep", "-c", "-E", "-e", pattern],
+                    input="harmless\n",
+                    capture_output=True,
+                    text=True,
+                )
+                self.assertIn(
+                    result.returncode,
+                    (0, 1),
+                    f"grep rejected {pattern!r}: {result.stderr.strip()}",
+                )
+                self.assertEqual("", result.stderr.strip())
+
+    def test_each_category_matches_what_it_exists_to_catch(self) -> None:
+        for category, samples in MUST_MATCH.items():
+            patterns = PATTERNS["categories"][category]["patterns"]
+            for sample in samples:
+                with self.subTest(category=category, sample=sample):
+                    self.assertTrue(
+                        any(grep_matches(p, sample) for p in patterns),
+                        f"{category} did not match {sample!r} under grep -E",
+                    )
+
+
+if __name__ == "__main__":
+    unittest.main()