diff --git a/actions/leak-scan/run.sh b/actions/leak-scan/run.sh index d3f4401..52526e5 100755 --- a/actions/leak-scan/run.sh +++ b/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" diff --git a/data/leak-patterns.json b/data/leak-patterns.json index 9420085..677ba94 100644 --- a/data/leak-patterns.json +++ b/data/leak-patterns.json @@ -3,20 +3,20 @@ "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]\\." ] }, @@ -24,7 +24,7 @@ "patterns": [ "192\\.168\\.", "^10\\.", - "172\\.(1[6-9]|2\\d|3[01])\\." + "172\\.(1[6-9]|2[0-9]|3[01])\\." ] }, "k8s_join_tokens": { diff --git a/tests/test_leak_patterns_grep_compatible.py b/tests/test_leak_patterns_grep_compatible.py new file mode 100644 index 0000000..3fd7f91 --- /dev/null +++ b/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()