From 9b548a92fa65c16c76e4cf05773d1e4deaa7faff Mon Sep 17 00:00:00 2001 From: Ben Mitchell Date: Fri, 21 Aug 2026 14:36:27 +1200 Subject: [PATCH 1/4] feat(launchpad): secret-material detection via gitleaks (#67) Implements the engine and allowlist location ADR-0006 decided: gitleaks, driven by a single .gitleaks.toml at the repo root extending the default ruleset. PR-diff path fails the run on any finding; scheduled full-history path reports (WARN, never FAIL) so pre-existing history findings don't permanently redden the audit -- no baseline snapshot is used, since a regenerated baseline has no field for a reason and can silently swallow a real finding, per ADR-0006's own rejection of that mechanism. Custom rules cover what gitleaks' default ruleset provably misses (verified empirically in ADR-0006 before this task started): Nostr nsec/hex private keys including BUZZ_PRIVATE_KEY, glibc crypt hashes ($1/$5/$6/$y$, including the optional rounds=N$ segment), BUZZ_S3_* access/secret keys, and a Postgres URL with an embedded password. SSH private keys and registry tokens are covered by gitleaks' own default rules, confirmed against fixtures rather than assumed. Verified locally before commit: - All 7 required material categories fire against synthetic fixtures (12 findings across 7 rule IDs, checked directly against the JSON report). - The two known false-positive classes -- the dev-deployment-SOP.md documentation placeholders and Cargo.lock checksums -- produce zero findings against the real repo, confirmed by filtering the actual scan output, not assumed from the config. - Full-history scan: 5729 commits, 16s -- comfortably inside the 3-minute PR budget even on the path that's explicitly allowed to exceed it. - Real first-run baseline measured: 222 findings across history, spot- checked several directly (test fixtures, Helm chart test placeholders, a Rust test helper's literal test password) -- consistent with the file list being dominated by *_test.* paths. Decided behavior: WARN, visible every run, no baseline file. Not silently triaged to zero here; remediating or allowlisting the 222 is follow-on work this task surfaces rather than resolves. - 42 harness tests pass together; the full security_audit.py entrypoint runs end-to-end locally reproducing what CI will do. Fixtures are synthetic (a real-but-unused SSH key generated solely for this purpose; every other value fabricated) and excluded from the live scan by an explicit, commented allowlist entry -- not by accident of path. --- .../workflows/launchpad-security-audit.yml | 41 ++-- .gitleaks.toml | 116 +++++++++++ .../secrets/crypt_hashes.txt | 5 + .../secrets/env_assignment.env | 5 + .../secrets/nostr_keys.txt | 7 + .../secrets/postgres_url.txt | 4 + .../secrets/registry_token.txt | 5 + .../secrets/s3_minio_keys.txt | 5 + .../secrets/ssh_private_key.txt | 11 ++ launchpad/scripts/security_audit_registry.py | 2 + .../scripts/security_audit_secrets_check.py | 184 ++++++++++++++++++ launchpad/scripts/test_no_model.py | 2 + .../test_security_audit_secrets_check.py | 163 ++++++++++++++++ 13 files changed, 539 insertions(+), 11 deletions(-) create mode 100644 .gitleaks.toml create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/crypt_hashes.txt create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/env_assignment.env create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/nostr_keys.txt create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/postgres_url.txt create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/registry_token.txt create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/s3_minio_keys.txt create mode 100644 launchpad/scripts/security_audit_fixtures/secrets/ssh_private_key.txt create mode 100644 launchpad/scripts/security_audit_secrets_check.py create mode 100644 launchpad/scripts/test_security_audit_secrets_check.py diff --git a/.github/workflows/launchpad-security-audit.yml b/.github/workflows/launchpad-security-audit.yml index 97bf5e47e8b..27bb409a6df 100644 --- a/.github/workflows/launchpad-security-audit.yml +++ b/.github/workflows/launchpad-security-audit.yml @@ -12,21 +12,19 @@ name: launchpad — security audit # matching. Off the hour to avoid GitHub's top-of-hour queue. # workflow_dispatch manual reruns, and the mechanism used to capture the # self-test evidence this task's PR links to. -# pull_request filtered to the paths the audit can actually be affected -# by — the audit scripts themselves, any workflow, and -# .gitignore, which the (future) ignore-coverage check -# reads. Unfiltered would run on every unrelated PR for no -# benefit; too narrow risks a check landing that no PR -# trigger ever exercises. +# pull_request UNFILTERED by path as of #67. #66 scoped this to +# ".github/**"/"launchpad/**"/".gitignore" because no +# check existed yet that cared about anything else. +# #67's secret scan has to see every file a PR touches — +# a leaked credential is exactly as real in crates/ or +# desktop/ as it is here — so a path filter narrow +# enough to be cheap is also narrow enough to miss the +# one thing this check exists to catch. on: schedule: - cron: "17 3 * * *" workflow_dispatch: {} - pull_request: - paths: - - ".github/**" - - "launchpad/**" - - "**/.gitignore" + pull_request: {} # Read-only, and nothing more. No repository secret is referenced anywhere in # this file — a check that needed one would be a check this workflow cannot run @@ -47,6 +45,27 @@ jobs: persist-credentials: false fetch-depth: 0 + # Pinned to a specific release and verified against gitleaks' own + # published checksum before extraction — this is the one third-party + # binary this workflow trusts, and it never runs with any credential + # in scope, so a supply-chain compromise here still cannot reach a + # secret. No gitleaks-maintained Action is used, to keep this + # workflow's trust surface to "one pinned, checksummed binary" rather + # than a third party's Action code at whatever version they push next. + - name: Install gitleaks (pinned, checksum-verified) + env: + GITLEAKS_VERSION: "8.30.1" + GITLEAKS_SHA256: "551f6fc83ea457d62a0d98237cbad105af8d557003051f41f3e7ca7b3f2470eb" + run: | + set -euo pipefail + curl -fsSL -o gitleaks.tar.gz \ + "https://github.com/gitleaks/gitleaks/releases/download/v${GITLEAKS_VERSION}/gitleaks_${GITLEAKS_VERSION}_linux_x64.tar.gz" + echo "${GITLEAKS_SHA256} gitleaks.tar.gz" | sha256sum -c - + tar -xzf gitleaks.tar.gz gitleaks + sudo install -m 0755 gitleaks /usr/local/bin/gitleaks + rm gitleaks.tar.gz gitleaks + gitleaks version + # The harness's own controls, scoped to this task's tests only — the # unfiltered `unittest discover` other launchpad-*.yml checks use would # also run every unrelated script's test suite in this directory, which diff --git a/.gitleaks.toml b/.gitleaks.toml new file mode 100644 index 00000000000..3ced20bbc34 --- /dev/null +++ b/.gitleaks.toml @@ -0,0 +1,116 @@ +title = "launchpad-26/buzz secret-scanning config" + +# Engine and allowlist location decided in ADR-0006 (launchpad/decisions/). +# Extends gitleaks' built-in ruleset rather than replacing it — SSH private +# keys, GitHub PATs and generic high-entropy .env-shaped assignments are +# already covered by the default rules (verified empirically against the +# fixtures in launchpad/scripts/security_audit_fixtures/secrets/ before this +# file was written). The rules below cover only what the default ruleset +# does not: Nostr key material, glibc crypt hashes, this fork's BUZZ_S3_* +# naming, and a Postgres URL with an embedded password. + +[extend] +useDefault = true + +[[rules]] +id = "nostr-nsec-private-key" +description = "Nostr nsec1 bech32-encoded private key" +regex = '''nsec1[023456789acdefghjklmnpqrstuvwxyz]{58}''' +tags = ["key", "nostr"] + +[[rules]] +id = "buzz-private-key" +description = "BUZZ_PRIVATE_KEY or a similarly-named 64-hex-character private key assignment" +# Every group below is non-capturing (?:...). A capturing group here silently +# drops the finding entirely once [extend] useDefault = true merges in +# gitleaks' default ruleset — reproduced and isolated empirically before this +# file was written (a bare (a|b) group matches standalone but reports zero +# leaks the moment useDefault is active; (?:a|b) is unaffected). No known +# gitleaks issue was found describing this, so treat every future rule here +# the same way: no capturing groups, ever, unless secretGroup is set to use one. +regex = '''(?i)[A-Z0-9_]*(?:PRIVATE_KEY|NSEC|SECKEY)[A-Z0-9_]*\s*=\s*['"]?[0-9a-fA-F]{64}['"]?''' +tags = ["key", "nostr"] + +[[rules]] +id = "glibc-crypt-hash" +# $1/$5/$6 optionally carry a rounds=N$ parameter before the salt; $y$ +# (yescrypt) carries its own short encoded parameter block instead. Both +# shapes are one or two extra $-delimited segments before the final hash, so +# the middle is a repeated group rather than a fixed salt-then-hash count. +description = "glibc/Unix crypt hash ($1$, $5$, $6$, or $y$)" +regex = '''\$(?:1|5|6|y)\$(?:[./A-Za-z0-9=]+\$){1,2}[./A-Za-z0-9]{20,}''' +tags = ["password", "crypt"] + +[[rules]] +id = "buzz-s3-minio-key" +description = "BUZZ_S3_ACCESS_KEY or BUZZ_S3_SECRET_KEY assignment" +# {6,} on the value, not \S+: launchpad/deploy/archived/runbooks/dev-deployment-SOP.md +# documents these as a markdown table cell, `` `BUZZ_S3_ACCESS_KEY=` ``, where the +# character immediately after = is a bare backtick — \S+ matched that one +# punctuation character as if it were a credential. Requiring a real value +# shape excludes it without an allowlist entry, and still matches every +# fixture and every real assignment this rule exists for. +regex = '''BUZZ_S3_(?:ACCESS|SECRET)_KEY\s*=\s*['"]?[A-Za-z0-9_\-]{6,}['"]?''' +tags = ["key", "s3", "minio"] +# .env.example is, by repository-wide convention, entirely placeholder +# values meant to be copied and replaced — verified against .env.example +# and deploy/compose/.env.example, both CHANGE_ME-or-dev-value templates, +# never a deployed .env. Scoped to that one filename shape, not the repo. +[rules.allowlist] +paths = ['''.*\.env\.example$'''] + +[[rules]] +id = "postgres-url-with-password" +description = "Postgres connection URL with an embedded password" +regex = '''postgres(?:ql)?://[^:@/\s]+:[^@\s]+@[^\s]+''' +tags = ["password", "postgres"] +# The overwhelming majority of matches on this codebase are the standard +# local dev/test credential postgres://buzz:buzz_dev@localhost:5432/buzz +# (verified: scripts/run-tests.sh, crates/buzz-db/src/usage.rs, and ~100 +# more files as of the first full-history scan run for #67) — a local-only +# connection string is not a production credential regardless of what the +# password looks like. Scoped to the host being loopback, not to any file +# or the value itself, so a real leaked URL pointed at a real host still +# fires. scripts/run-tests.sh already marks this same string +# `# sadscan:disable np.postgres.1` for a different, Block-internal +# scanner; this is gitleaks' equivalent for the same known-safe value. +[rules.allowlist] +regexes = ['''postgres(?:ql)?://[^:@/\s]+:[^@\s]+@(?:localhost|127\.0\.0\.1)(?::\d+)?/'''] + +# Fixtures prove every rule above fires (#67's own definition of done) and +# must therefore be readable by the scanner, but must never themselves be +# reported as a finding of the live audit — they are synthetic, not leaked. +# +# #67 as filed named two placeholders in dev-deployment-SOP.md (an +# ssh-ed25519 public-key fragment at a since-renumbered line, and a $6$ +# prefix mentioned in prose) as needing an exclusion. Verified against the +# file's current content and location +# (launchpad/deploy/archived/runbooks/dev-deployment-SOP.md — moved out of +# launchpad/deploy/ entirely by the deploy-method archival that landed after +# #67 was filed): neither of those two strings matches any rule in this +# config any more, so neither needs an entry here. What the file's current +# content does trip is `<64 hex characters>` — a literal bracketed +# placeholder idiom this SOP uses for "a value goes here", not a value +# itself — via gitleaks' own default generic-api-key rule. +[allowlist] +description = "Paths this scan reads but never reports on, and one documentation placeholder idiom" +paths = [ + '''launchpad/scripts/security_audit_fixtures/secrets/.*''', + # Same class of false positive .intersect/sadscan.yaml already documents + # for this exact file ("Cargo registry checksums are integrity hashes + # generated by Cargo, not payment data") — here it trips gitleaks' + # generic-api-key entropy heuristic instead of sadscan's payment-card + # rule, but the underlying content and reasoning are the same. Verified + # present in both Cargo.lock files in this repo (root and + # desktop/src-tauri/) — one finding each, both checksum data. + '''(^|/)Cargo\.lock$''', +] +regexes = [ + '''<[A-Za-z0-9 ]+>''', + # dev-deployment-SOP.md:1079's worked-example "Public key:" value — a + # fixed, literal walkthrough value, explicitly labelled public (not the + # $6$/secret-key output on adjacent lines, which stay placeholder text + # and trip nothing). Matched literally, not by shape, so this entry + # cannot accidentally swallow any other 64-hex string anywhere else. + '''38980a43aba04331ba61b5e7b64b90e250cd411d042050eaf102a408acc6c379''', +] diff --git a/launchpad/scripts/security_audit_fixtures/secrets/crypt_hashes.txt b/launchpad/scripts/security_audit_fixtures/secrets/crypt_hashes.txt new file mode 100644 index 00000000000..61180fd6aae --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/crypt_hashes.txt @@ -0,0 +1,5 @@ +# Fixture for #67. Synthetic (not derived from any real password/host) glibc +# crypt hashes, shaped correctly for the $6$ (SHA-512 crypt) and $y$ +# (yescrypt) rules. Excluded from the audit's scan path in .gitleaks.toml. +SIX=$6$rounds=656000$Zq7wYkT2fixtureSalt$k4h2mNtQpXz9r1sJd8vLwYbGcHfEeUoAiSm3NkTpQrXyLzAaVbCcDdFfGgHhIiJjKkLlMm +Y=$y$j9T$fixtureSaltForY$K7pQmR9xTvLnZaWbCcDdEeFfGgHhIiJjKkLlMmNn diff --git a/launchpad/scripts/security_audit_fixtures/secrets/env_assignment.env b/launchpad/scripts/security_audit_fixtures/secrets/env_assignment.env new file mode 100644 index 00000000000..d6b47a730c3 --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/env_assignment.env @@ -0,0 +1,5 @@ +# Fixture for #67. A .env-shaped file with a real-looking (synthetic) secret +# value, to prove an .env-shaped assignment rule fires on file shape, not +# just on specific known variable names. Excluded from the audit's scan path +# in .gitleaks.toml. +SOME_SERVICE_API_TOKEN=fx_9Kj2mNpQr7VbXzYcWdEeFfGgHhIiJjKkLlMmNnOoPp diff --git a/launchpad/scripts/security_audit_fixtures/secrets/nostr_keys.txt b/launchpad/scripts/security_audit_fixtures/secrets/nostr_keys.txt new file mode 100644 index 00000000000..f0d25901141 --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/nostr_keys.txt @@ -0,0 +1,7 @@ +# Fixture for #67. Synthetic Nostr key material — a bech32 nsec shape and a +# 64-hex private-key shape, including the BUZZ_PRIVATE_KEY env-var name this +# fork uses. None of these decode to or were ever a real key. Excluded from +# the audit's scan path in .gitleaks.toml. +RELAY_OWNER_NSEC=nsec180cvv07tjdrrgpa0j7j7tmnyl2yr6yr7l8j4s3evf6u64th6gkwsyjh6w6 +BUZZ_PRIVATE_KEY=4c9f2e8a1b6d3f705e9c8a2b7d4f1e6c3a9b8d5f2e7c4a1b6d9f3e8c5a2b7d4f +BARE_HEX_KEY=9a3f7c2e5b8d1f4a6c9e2b5d8f1a4c7e9b2d5f8a1c4e7b9d2f5a8c1e4b7d9f2a diff --git a/launchpad/scripts/security_audit_fixtures/secrets/postgres_url.txt b/launchpad/scripts/security_audit_fixtures/secrets/postgres_url.txt new file mode 100644 index 00000000000..8c9c9ddf0a6 --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/postgres_url.txt @@ -0,0 +1,4 @@ +# Fixture for #67. A synthetic Postgres connection URL with an embedded +# password, matching the shape deploy/compose's POSTGRES_* variables could +# be assembled into. Excluded from the audit's scan path in .gitleaks.toml. +DATABASE_URL=postgres://buzzuser:fx7Kj2mNpQr7VbXzYcWd@db.internal:5432/buzz diff --git a/launchpad/scripts/security_audit_fixtures/secrets/registry_token.txt b/launchpad/scripts/security_audit_fixtures/secrets/registry_token.txt new file mode 100644 index 00000000000..103b048906f --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/registry_token.txt @@ -0,0 +1,5 @@ +# Fixture for #67. A synthetic GitHub personal-access-token shape, standing +# in for "container registry tokens" — ghcr.io auth uses a GitHub PAT, and +# gitleaks' default ruleset already recognizes the ghp_ prefix shape. +# Excluded from the audit's scan path in .gitleaks.toml. +GHCR_TOKEN=ghp_fx9Kj2mNpQr7VbXzYcWdEeFfGgHhIiJjKkLl00 diff --git a/launchpad/scripts/security_audit_fixtures/secrets/s3_minio_keys.txt b/launchpad/scripts/security_audit_fixtures/secrets/s3_minio_keys.txt new file mode 100644 index 00000000000..96b42ae3653 --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/s3_minio_keys.txt @@ -0,0 +1,5 @@ +# Fixture for #67. Synthetic S3/MinIO-shaped access-key and secret-key pair, +# matching this fork's BUZZ_S3_* naming (see deploy/compose/.env.example). +# Excluded from the audit's scan path in .gitleaks.toml. +BUZZ_S3_ACCESS_KEY=AKIAFX2K9MPQRSTUVWXY +BUZZ_S3_SECRET_KEY=fx7Kj2mNpQr7VbXzYcWdEeFfGgHhIiJjKkLlMmNn diff --git a/launchpad/scripts/security_audit_fixtures/secrets/ssh_private_key.txt b/launchpad/scripts/security_audit_fixtures/secrets/ssh_private_key.txt new file mode 100644 index 00000000000..8d5035a07de --- /dev/null +++ b/launchpad/scripts/security_audit_fixtures/secrets/ssh_private_key.txt @@ -0,0 +1,11 @@ +# Fixture for #67. A real key, generated solely for this fixture, never used +# for any access anywhere, and excluded from the audit's scan path in +# .gitleaks.toml. Proves the SSH-private-key rule (gitleaks' built-in +# "private-key" rule) actually fires. +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW +QyNTUxOQAAACDRTit6NTW83Hhmyqk0Vkh3dNEWE+JDMhx7fWFKhvs+fQAAAKCXixD+l4sQ +/gAAAAtzc2gtZWQyNTUxOQAAACDRTit6NTW83Hhmyqk0Vkh3dNEWE+JDMhx7fWFKhvs+fQ +AAAEDMfjdbOnF3oFC9jYn1YcZ9OZLggtFCfdJsbMclndkw4dFOK3o1NbzceGbKqTRWSHd0 +0RYT4kMyHHt9YUqG+z59AAAAF2ZpeHR1cmUtb25seS1uZXZlci11c2VkAQIDBAUG +-----END OPENSSH PRIVATE KEY----- diff --git a/launchpad/scripts/security_audit_registry.py b/launchpad/scripts/security_audit_registry.py index 1a0dfa8b9c8..a1c076cc896 100644 --- a/launchpad/scripts/security_audit_registry.py +++ b/launchpad/scripts/security_audit_registry.py @@ -12,7 +12,9 @@ """ from security_audit_selftest_check import run as harness_self_test +from security_audit_secrets_check import run as secret_material_scan CHECKS = [ harness_self_test, + secret_material_scan, ] diff --git a/launchpad/scripts/security_audit_secrets_check.py b/launchpad/scripts/security_audit_secrets_check.py new file mode 100644 index 00000000000..57e980401db --- /dev/null +++ b/launchpad/scripts/security_audit_secrets_check.py @@ -0,0 +1,184 @@ +#!/usr/bin/env python3 +"""Secret-material detection for #62, using the engine and allowlist location +ADR-0006 decided: gitleaks, driven by .gitleaks.toml at the repo root. + +Two modes, selected by GITHUB_EVENT_NAME rather than two registered checks, +so the registry still carries one line for this whole feature: + + pull_request Scans only the commits this PR adds — fetches the PR's + base ref at run time and scopes gitleaks to + FETCH_HEAD..HEAD, so history predating the PR is never + re-scanned on every push. Reports FAIL on any finding: + this is the gate. + schedule / anything Full git history, no range restriction. Reports WARN on + else (workflow_ any finding, never FAIL — #67's definition of done + dispatch, local) calls this path "reports findings", distinct from the + PR path's "fails the run". A repository with pre- + existing findings must not be permanently red on this + path; it must be visibly, honestly not-clean. + +Neither path prints a matched secret value anywhere: gitleaks runs with +--redact, which cleans both stdout and the JSON report gitleaks itself +produces, and this check's own CheckResult.detail is built only from file +paths, line numbers and rule ids — never the report's "Secret"/"Match" fields. + +Local run: python3 security_audit_secrets_check.py [repo-root] runs the +full-history (reporting) path directly, without the harness, for a quick +manual check outside CI. +""" + +import json +import os +import subprocess +import sys +import time +from pathlib import Path +from typing import List, Optional, Tuple + +from security_audit_core import CheckResult, Status + +NAME = "gitleaks-secret-scan" +GITLEAKS_CONFIG = ".gitleaks.toml" + +# Generous but bounded: the PR path must stay inside #66's 3-minute overall +# budget for the harness; the full-history path is explicitly allowed to +# exceed it, so it gets a much longer allowance rather than none at all — +# an unbounded subprocess is its own failure mode. +_PR_TIMEOUT_SECONDS = 120 +_FULL_HISTORY_TIMEOUT_SECONDS = 1200 + + +def _run_gitleaks( + repo_root: Path, log_opts: Optional[str], timeout: int +) -> Tuple[Optional[List[dict]], Optional[str]]: + """Run gitleaks; (findings, None) on success, (None, reason) if it could not run. + + Deliberately does NOT pass --exit-code 0: gitleaks' real exit code (0 clean, + 1 leaks found) is how a genuine engine failure (bad config, crash — any + other code) is told apart from "it ran and found something", which + --exit-code 0 would erase. + """ + config_path = repo_root / GITLEAKS_CONFIG + if not config_path.is_file(): + return None, f"{GITLEAKS_CONFIG} not found at {config_path}" + + cmd = [ + "gitleaks", + "detect", + "--config", + str(config_path), + "--report-format", + "json", + "--report-path", + "-", + "--redact", + "--no-banner", + ] + if log_opts: + cmd.append(f"--log-opts={log_opts}") + + try: + # encoding/errors explicit, not text=True's platform default: gitleaks + # emits UTF-8 (file paths, findings) and Windows' default subprocess + # text decoding is the system locale (cp1252 on this machine), which + # crashes decoding it. Reproduced locally before this fix landed. + result = subprocess.run( + cmd, + cwd=repo_root, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=timeout, + ) + except FileNotFoundError: + return None, "gitleaks is not installed or not on PATH" + except subprocess.TimeoutExpired: + return None, f"gitleaks did not finish within {timeout}s" + except OSError as exc: + return None, f"could not run gitleaks: {exc}" + + if result.returncode not in (0, 1): + stderr = (result.stderr or "").strip()[-1000:] + return None, f"gitleaks exited {result.returncode}: {stderr}" + + try: + findings = json.loads(result.stdout) if result.stdout.strip() else [] + except json.JSONDecodeError as exc: + return None, f"could not parse gitleaks report: {exc}" + + return findings, None + + +def _summarize(findings: List[dict], limit: int = 8) -> str: + """file:line (rule-id), never the matched value — see module docstring.""" + locations = sorted( + {f"{f.get('File', '?')}:{f.get('StartLine', '?')} ({f.get('RuleID', '?')})" for f in findings} + ) + shown = ", ".join(locations[:limit]) + if len(locations) > limit: + shown += f", and {len(locations) - limit} more" + return shown + + +def _scan_pr_diff(repo_root: Path) -> CheckResult: + base_ref = os.environ.get("GITHUB_BASE_REF", "") + if not base_ref: + return CheckResult( + NAME, Status.INDETERMINATE, "GITHUB_BASE_REF is unset; cannot scope a diff scan" + ) + + try: + subprocess.run( + ["git", "fetch", "--depth=1", "origin", base_ref], + cwd=repo_root, + check=True, + capture_output=True, + timeout=60, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError) as exc: + return CheckResult(NAME, Status.INDETERMINATE, f"could not fetch base ref {base_ref!r}: {exc}") + + findings, error = _run_gitleaks(repo_root, log_opts="FETCH_HEAD..HEAD", timeout=_PR_TIMEOUT_SECONDS) + if error is not None: + return CheckResult(NAME, Status.INDETERMINATE, error) + if findings: + return CheckResult( + NAME, + Status.FAIL, + f"{len(findings)} finding(s) in this PR: {_summarize(findings)}", + ) + return CheckResult(NAME, Status.PASS, "no secret material found in this PR's commits") + + +def _scan_full_history(repo_root: Path) -> CheckResult: + start = time.monotonic() + findings, error = _run_gitleaks(repo_root, log_opts=None, timeout=_FULL_HISTORY_TIMEOUT_SECONDS) + elapsed = time.monotonic() - start + if error is not None: + return CheckResult(NAME, Status.INDETERMINATE, error) + if findings: + # WARN, not FAIL: a pre-existing finding is reported, per #67's + # definition of done, not treated as this run's fault. See D-13-style + # reasoning in the module docstring — no baseline snapshot exists to + # silently swallow these; they are visible every run until someone + # allowlists or remediates each one. + return CheckResult( + NAME, + Status.WARN, + f"{len(findings)} finding(s) across full history in {elapsed:.0f}s: {_summarize(findings)}", + ) + return CheckResult(NAME, Status.PASS, f"full history clean in {elapsed:.0f}s") + + +def run(repo_root: Path) -> CheckResult: + if os.environ.get("GITHUB_EVENT_NAME") == "pull_request": + return _scan_pr_diff(repo_root) + return _scan_full_history(repo_root) + + +if __name__ == "__main__": + root = Path(sys.argv[1]) if len(sys.argv) > 1 else Path.cwd() + outcome = _scan_full_history(root) + print(f"[{outcome.status.value}] {outcome.name} - {outcome.detail}") + sys.exit(1 if outcome.status is Status.FAIL else 0) diff --git a/launchpad/scripts/test_no_model.py b/launchpad/scripts/test_no_model.py index b2e5fcb636f..72cfbe722a5 100644 --- a/launchpad/scripts/test_no_model.py +++ b/launchpad/scripts/test_no_model.py @@ -89,6 +89,8 @@ "same reason as security_audit.py", "security_audit_selftest_check.py": "the #62 security-audit self-test check; " "same reason as security_audit.py", + "security_audit_secrets_check.py": "the #67 secret-material detection check; " + "same reason as security_audit.py", } diff --git a/launchpad/scripts/test_security_audit_secrets_check.py b/launchpad/scripts/test_security_audit_secrets_check.py new file mode 100644 index 00000000000..aa412bd24aa --- /dev/null +++ b/launchpad/scripts/test_security_audit_secrets_check.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +"""Controls for the #67 secret-material check. + +Mocks subprocess.run throughout — no real gitleaks invocation, no network. +.gitleaks.toml's rules themselves are proven against real fixtures with the +real gitleaks binary separately (see the PR body / commit history for that +evidence); this suite is about the check script's own branching and error +handling, which is what a future edit is most likely to quietly break. +""" + +import json +import subprocess +import unittest +from pathlib import Path +from unittest.mock import patch + +from security_audit_core import Status +from security_audit_secrets_check import run, _run_gitleaks, _scan_full_history, _scan_pr_diff + + +def _completed(returncode, stdout="[]", stderr=""): + return subprocess.CompletedProcess(args=["gitleaks"], returncode=returncode, stdout=stdout, stderr=stderr) + + +_FINDING = {"File": "a.env", "StartLine": 3, "RuleID": "buzz-s3-minio-key", "Secret": "should-never-appear"} + + +class RunGitleaksTest(unittest.TestCase): + def test_missing_config_is_indeterminate_without_running_gitleaks(self): + with patch("security_audit_secrets_check.subprocess.run") as mock_run: + findings, error = _run_gitleaks(Path("/nonexistent-repo-root"), None, 10) + mock_run.assert_not_called() + self.assertIsNone(findings) + self.assertIn(".gitleaks.toml", error) + + def test_clean_run_returns_empty_findings(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "[]") + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertEqual(findings, []) + self.assertIsNone(error) + + def test_leaks_found_exit_code_one_is_still_success(self): + # gitleaks exits 1 when it finds something — that is the normal + # "it worked and found leaks" outcome, not an engine failure. + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", + return_value=_completed(1, json.dumps([_FINDING])), + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertIsNone(error) + self.assertEqual(len(findings), 1) + + def test_unexpected_exit_code_is_indeterminate(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(2, "", "config error") + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertIsNone(findings) + self.assertIn("config error", error) + + def test_binary_missing_is_indeterminate(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", side_effect=FileNotFoundError() + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertIsNone(findings) + self.assertIn("not installed", error) + + def test_timeout_is_indeterminate(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", + side_effect=subprocess.TimeoutExpired("gitleaks", 10), + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertIsNone(findings) + self.assertIn("10s", error) + + def test_malformed_json_is_indeterminate(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "not json") + ): + findings, error = _run_gitleaks(Path("."), None, 10) + self.assertIsNone(findings) + self.assertIn("could not parse", error) + + +class ScanFullHistoryTest(unittest.TestCase): + def test_clean_history_passes(self): + with patch("security_audit_secrets_check._run_gitleaks", return_value=([], None)): + result = _scan_full_history(Path(".")) + self.assertEqual(result.status, Status.PASS) + + def test_findings_warn_not_fail(self): + # The scheduled/full-history path REPORTS, per #67's definition of + # done — it must never turn a pre-existing finding into a build + # failure, which is what distinguishes it from the PR path. + with patch("security_audit_secrets_check._run_gitleaks", return_value=([_FINDING], None)): + result = _scan_full_history(Path(".")) + self.assertEqual(result.status, Status.WARN) + self.assertNotIn("should-never-appear", result.detail) + + def test_engine_error_is_indeterminate(self): + with patch("security_audit_secrets_check._run_gitleaks", return_value=(None, "boom")): + result = _scan_full_history(Path(".")) + self.assertEqual(result.status, Status.INDETERMINATE) + + +class ScanPrDiffTest(unittest.TestCase): + def test_missing_base_ref_is_indeterminate(self): + with patch.dict("os.environ", {}, clear=True): + result = _scan_pr_diff(Path(".")) + self.assertEqual(result.status, Status.INDETERMINATE) + self.assertIn("GITHUB_BASE_REF", result.detail) + + def test_fetch_failure_is_indeterminate(self): + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", + side_effect=subprocess.CalledProcessError(1, "git"), + ): + result = _scan_pr_diff(Path(".")) + self.assertEqual(result.status, Status.INDETERMINATE) + self.assertIn("launchpad", result.detail) + + def test_clean_diff_passes(self): + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0) + ), patch("security_audit_secrets_check._run_gitleaks", return_value=([], None)): + result = _scan_pr_diff(Path(".")) + self.assertEqual(result.status, Status.PASS) + + def test_finding_in_diff_fails_the_run(self): + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0) + ), patch("security_audit_secrets_check._run_gitleaks", return_value=([_FINDING], None)): + result = _scan_pr_diff(Path(".")) + self.assertEqual(result.status, Status.FAIL) + self.assertNotIn("should-never-appear", result.detail) + self.assertIn("a.env:3", result.detail) + + +class RunDispatchTest(unittest.TestCase): + def test_pull_request_event_uses_diff_scan(self): + with patch.dict("os.environ", {"GITHUB_EVENT_NAME": "pull_request"}), patch( + "security_audit_secrets_check._scan_pr_diff" + ) as mock_diff, patch("security_audit_secrets_check._scan_full_history") as mock_full: + run(Path(".")) + mock_diff.assert_called_once() + mock_full.assert_not_called() + + def test_other_events_use_full_history_scan(self): + for event in ("schedule", "workflow_dispatch", ""): + with patch.dict("os.environ", {"GITHUB_EVENT_NAME": event}), patch( + "security_audit_secrets_check._scan_pr_diff" + ) as mock_diff, patch("security_audit_secrets_check._scan_full_history") as mock_full: + run(Path(".")) + mock_diff.assert_not_called() + mock_full.assert_called_once() + + +if __name__ == "__main__": + unittest.main() From 8a73711dd5d9bae84dcb7940ec8609ca38829ec2 Mon Sep 17 00:00:00 2001 From: Ben Mitchell Date: Fri, 21 Aug 2026 15:55:50 +1200 Subject: [PATCH 2/4] fix(launchpad): address review-code Blocker + High findings on #271 (PR #271) _scan_pr_diff reported INDETERMINATE on every infrastructure failure (unset GITHUB_BASE_REF, git fetch failure, gitleaks engine error), and security_audit_core.exit_code() treats INDETERMINATE the same as PASS - so a PR whose scan never actually ran went green on the gate path. Contradicts ADR-0008's "indeterminate must never render as pass". Fixed to FAIL; _scan_full_history's own INDETERMINATE is unchanged since that path already WARNs rather than FAILs on findings by design. Also closes three test gaps review-code flagged as High: no test asserted --redact was actually passed to gitleaks, no test asserted _run_gitleaks's log_opts/timeout call args (the FETCH_HEAD..HEAD PR-scoping guarantee was unverified), and RunDispatchTest discarded run()'s return value entirely, so a dropped `return` would go unnoticed here despite crashing format_report elsewhere. --- .../scripts/security_audit_secrets_check.py | 15 ++- .../test_security_audit_secrets_check.py | 103 ++++++++++++++++-- 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/launchpad/scripts/security_audit_secrets_check.py b/launchpad/scripts/security_audit_secrets_check.py index 57e980401db..f060bd7730c 100644 --- a/launchpad/scripts/security_audit_secrets_check.py +++ b/launchpad/scripts/security_audit_secrets_check.py @@ -122,10 +122,19 @@ def _summarize(findings: List[dict], limit: int = 8) -> str: def _scan_pr_diff(repo_root: Path) -> CheckResult: + # This path is the gate (module docstring): security_audit_core.exit_code() + # only fails the run on Status.FAIL, treating INDETERMINATE the same as + # PASS. So every "couldn't actually run the scan" case here must be FAIL, + # never INDETERMINATE — an unscanned PR going green would silently violate + # ADR-0008's rule that indeterminate must never render as pass. This is + # deliberately asymmetric with _scan_full_history below, whose own + # INDETERMINATE is correct: that path already reports pre-existing findings + # as WARN rather than FAIL by design, so a same-shaped infra failure there + # isn't gating anything a merge depends on. base_ref = os.environ.get("GITHUB_BASE_REF", "") if not base_ref: return CheckResult( - NAME, Status.INDETERMINATE, "GITHUB_BASE_REF is unset; cannot scope a diff scan" + NAME, Status.FAIL, "GITHUB_BASE_REF is unset; cannot scope a diff scan" ) try: @@ -137,11 +146,11 @@ def _scan_pr_diff(repo_root: Path) -> CheckResult: timeout=60, ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError) as exc: - return CheckResult(NAME, Status.INDETERMINATE, f"could not fetch base ref {base_ref!r}: {exc}") + return CheckResult(NAME, Status.FAIL, f"could not fetch base ref {base_ref!r}: {exc}") findings, error = _run_gitleaks(repo_root, log_opts="FETCH_HEAD..HEAD", timeout=_PR_TIMEOUT_SECONDS) if error is not None: - return CheckResult(NAME, Status.INDETERMINATE, error) + return CheckResult(NAME, Status.FAIL, error) if findings: return CheckResult( NAME, diff --git a/launchpad/scripts/test_security_audit_secrets_check.py b/launchpad/scripts/test_security_audit_secrets_check.py index aa412bd24aa..bd1a9b939d4 100644 --- a/launchpad/scripts/test_security_audit_secrets_check.py +++ b/launchpad/scripts/test_security_audit_secrets_check.py @@ -41,6 +41,45 @@ def test_clean_run_returns_empty_findings(self): self.assertEqual(findings, []) self.assertIsNone(error) + def test_redact_flag_is_always_passed(self): + # review-code High: nothing previously asserted the actual gitleaks + # command args, so dropping --redact (the flag this module's docstring + # leans on for "never prints a secret value") would still pass every + # other test in this class. + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "[]") + ) as mock_run: + _run_gitleaks(Path("."), None, 10) + cmd = mock_run.call_args.args[0] + self.assertIn("--redact", cmd) + + def test_log_opts_is_appended_when_given(self): + # review-code High: the FETCH_HEAD..HEAD PR-scoping guarantee was + # never checked against the actual command built — swapping the two + # timeout constants or dropping the scoping would leave every test + # green. + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "[]") + ) as mock_run: + _run_gitleaks(Path("."), "FETCH_HEAD..HEAD", 10) + cmd = mock_run.call_args.args[0] + self.assertIn("--log-opts=FETCH_HEAD..HEAD", cmd) + + def test_log_opts_omitted_when_none(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "[]") + ) as mock_run: + _run_gitleaks(Path("."), None, 10) + cmd = mock_run.call_args.args[0] + self.assertFalse(any(arg.startswith("--log-opts=") for arg in cmd)) + + def test_timeout_is_forwarded_to_subprocess(self): + with patch("security_audit_secrets_check.Path.is_file", return_value=True), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0, "[]") + ) as mock_run: + _run_gitleaks(Path("."), None, 42) + self.assertEqual(mock_run.call_args.kwargs["timeout"], 42) + def test_leaks_found_exit_code_one_is_still_success(self): # gitleaks exits 1 when it finds something — that is the normal # "it worked and found leaks" outcome, not an engine failure. @@ -92,6 +131,18 @@ def test_clean_history_passes(self): result = _scan_full_history(Path(".")) self.assertEqual(result.status, Status.PASS) + def test_scans_full_history_unscoped_with_full_history_timeout(self): + # review-code High: nothing previously asserted _run_gitleaks's own + # call args here — the module's central claim (full history is + # unscoped, and gets the long timeout, not the PR-diff one) was + # unverified. Swapping the two timeout constants would leave every + # test in this file green. + import security_audit_secrets_check as mod + + with patch("security_audit_secrets_check._run_gitleaks", return_value=([], None)) as mock_gl: + _scan_full_history(Path(".")) + mock_gl.assert_called_once_with(Path("."), log_opts=None, timeout=mod._FULL_HISTORY_TIMEOUT_SECONDS) + def test_findings_warn_not_fail(self): # The scheduled/full-history path REPORTS, per #67's definition of # done — it must never turn a pre-existing finding into a build @@ -108,21 +159,33 @@ def test_engine_error_is_indeterminate(self): class ScanPrDiffTest(unittest.TestCase): - def test_missing_base_ref_is_indeterminate(self): + def test_missing_base_ref_fails_the_run(self): + # Was INDETERMINATE. security_audit_core.exit_code() treats + # INDETERMINATE the same as PASS, so an unscanned PR on the gate path + # went green — review-code Blocker, contradicts ADR-0008's + # "indeterminate must never render as pass". with patch.dict("os.environ", {}, clear=True): result = _scan_pr_diff(Path(".")) - self.assertEqual(result.status, Status.INDETERMINATE) + self.assertEqual(result.status, Status.FAIL) self.assertIn("GITHUB_BASE_REF", result.detail) - def test_fetch_failure_is_indeterminate(self): + def test_fetch_failure_fails_the_run(self): with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( "security_audit_secrets_check.subprocess.run", side_effect=subprocess.CalledProcessError(1, "git"), ): result = _scan_pr_diff(Path(".")) - self.assertEqual(result.status, Status.INDETERMINATE) + self.assertEqual(result.status, Status.FAIL) self.assertIn("launchpad", result.detail) + def test_engine_error_fails_the_run(self): + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0) + ), patch("security_audit_secrets_check._run_gitleaks", return_value=(None, "boom")): + result = _scan_pr_diff(Path(".")) + self.assertEqual(result.status, Status.FAIL) + self.assertIn("boom", result.detail) + def test_clean_diff_passes(self): with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( "security_audit_secrets_check.subprocess.run", return_value=_completed(0) @@ -130,6 +193,20 @@ def test_clean_diff_passes(self): result = _scan_pr_diff(Path(".")) self.assertEqual(result.status, Status.PASS) + def test_scans_only_the_pr_range_with_pr_timeout(self): + # review-code High: the FETCH_HEAD..HEAD PR-scoping guarantee (the + # module's central claim) was never checked against _run_gitleaks's + # actual call args. + import security_audit_secrets_check as mod + + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0) + ), patch("security_audit_secrets_check._run_gitleaks", return_value=([], None)) as mock_gl: + _scan_pr_diff(Path(".")) + mock_gl.assert_called_once_with( + Path("."), log_opts="FETCH_HEAD..HEAD", timeout=mod._PR_TIMEOUT_SECONDS + ) + def test_finding_in_diff_fails_the_run(self): with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( "security_audit_secrets_check.subprocess.run", return_value=_completed(0) @@ -142,21 +219,31 @@ def test_finding_in_diff_fails_the_run(self): class RunDispatchTest(unittest.TestCase): def test_pull_request_event_uses_diff_scan(self): + # review-code High: this test previously discarded run()'s return + # value entirely, so a dropped `return` (silent None) would crash + # format_report uncaught elsewhere, taking down the whole audit + # report's output, and this suite would stay green. + sentinel = object() with patch.dict("os.environ", {"GITHUB_EVENT_NAME": "pull_request"}), patch( - "security_audit_secrets_check._scan_pr_diff" + "security_audit_secrets_check._scan_pr_diff", return_value=sentinel ) as mock_diff, patch("security_audit_secrets_check._scan_full_history") as mock_full: - run(Path(".")) + result = run(Path(".")) mock_diff.assert_called_once() mock_full.assert_not_called() + self.assertIs(result, sentinel) def test_other_events_use_full_history_scan(self): for event in ("schedule", "workflow_dispatch", ""): + sentinel = object() with patch.dict("os.environ", {"GITHUB_EVENT_NAME": event}), patch( "security_audit_secrets_check._scan_pr_diff" - ) as mock_diff, patch("security_audit_secrets_check._scan_full_history") as mock_full: - run(Path(".")) + ) as mock_diff, patch( + "security_audit_secrets_check._scan_full_history", return_value=sentinel + ) as mock_full: + result = run(Path(".")) mock_diff.assert_not_called() mock_full.assert_called_once() + self.assertIs(result, sentinel) if __name__ == "__main__": From faa3538adbdc5b940195eab617c1a7aedddb233d Mon Sep 17 00:00:00 2001 From: Ben Mitchell Date: Fri, 21 Aug 2026 16:04:45 +1200 Subject: [PATCH 3/4] fix(launchpad): drop --depth=1 on the PR-diff base-ref fetch, real bug hit on this PR's own CI run Reproduced live: after the previous commit's fix made infra failures FAIL instead of INDETERMINATE, this PR's own CI run failed with "98 finding(s) in this PR" spanning AGENTS.md, Justfile, and NIP spec docs -- years-old content nowhere near this PR's actual diff. Root cause: `git fetch --depth=1 origin base_ref` grafts a new shallow boundary onto that one ref, regardless of the checkout already having full history (this workflow's actions/checkout uses fetch-depth: 0). Once base_ref has advanced past this branch's own merge-base - true for almost any real PR, since branches don't rebase on every base push - git can no longer see the shared ancestor as reachable from the shallow, parent-less FETCH_HEAD, and `git log FETCH_HEAD..HEAD` silently expands from "this PR's own commits" to the entire history reachable from HEAD. Confirmed directly against this repo: with --depth=1, FETCH_HEAD..HEAD went from 2 commits to 2,484. Removing --depth=1 (a plain `git fetch origin base_ref`, matching what the checkout already guarantees) resolves it back to 2 commits, and the real gitleaks binary run end-to-end against this branch now reports PASS. Added a regression test asserting the fetch command never requests a shallow depth. --- .../scripts/security_audit_secrets_check.py | 18 ++++++++++++++++- .../test_security_audit_secrets_check.py | 20 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/launchpad/scripts/security_audit_secrets_check.py b/launchpad/scripts/security_audit_secrets_check.py index f060bd7730c..fbd65deeb9a 100644 --- a/launchpad/scripts/security_audit_secrets_check.py +++ b/launchpad/scripts/security_audit_secrets_check.py @@ -138,8 +138,24 @@ def _scan_pr_diff(repo_root: Path) -> CheckResult: ) try: + # No --depth=1 here, deliberately: this repo's checkout already has full + # history (the workflow's actions/checkout uses fetch-depth: 0), and a + # --depth=1 fetch of base_ref grafts a NEW shallow boundary onto that one + # ref regardless of the rest of the repo being fully cloned. Once + # base_ref has advanced past this branch's own merge-base -- true for + # almost every real PR, since branches don't get rebased on every push + # to the base -- git can no longer see that older shared commit as an + # ancestor of the shallow, parent-less FETCH_HEAD, and + # `git log FETCH_HEAD..HEAD` silently expands from "this PR's own + # commits" to the entire history reachable from HEAD. Reproduced + # directly against this repo: a --depth=1 fetch turned a correct + # 2-commit range into 2,484 commits, and the PR-diff scan reported 98 + # "findings in this PR" that were actually years-old content in + # AGENTS.md, Justfile and the NIP spec docs. A plain fetch (full + # history, matching what the checkout already guarantees) resolves the + # same range correctly. subprocess.run( - ["git", "fetch", "--depth=1", "origin", base_ref], + ["git", "fetch", "origin", base_ref], cwd=repo_root, check=True, capture_output=True, diff --git a/launchpad/scripts/test_security_audit_secrets_check.py b/launchpad/scripts/test_security_audit_secrets_check.py index bd1a9b939d4..2c9ea8af250 100644 --- a/launchpad/scripts/test_security_audit_secrets_check.py +++ b/launchpad/scripts/test_security_audit_secrets_check.py @@ -193,6 +193,26 @@ def test_clean_diff_passes(self): result = _scan_pr_diff(Path(".")) self.assertEqual(result.status, Status.PASS) + def test_fetch_does_not_request_shallow_depth(self): + # Regression for a real bug reproduced directly against this repo: + # `git fetch --depth=1 origin ` grafts a new shallow + # boundary onto that ref even in an already-fully-cloned repo (the + # workflow's own fetch-depth: 0), which broke FETCH_HEAD..HEAD's + # ancestry computation once base_ref had advanced past this branch's + # merge-base -- a correct 2-commit range silently became 2,484 + # commits, and real PR runs reported "findings in this PR" that were + # actually years-old content unrelated to the diff. + with patch.dict("os.environ", {"GITHUB_BASE_REF": "launchpad"}), patch( + "security_audit_secrets_check.subprocess.run", return_value=_completed(0) + ) as mock_run, patch( + "security_audit_secrets_check._run_gitleaks", return_value=([], None) + ): + _scan_pr_diff(Path(".")) + fetch_call = mock_run.call_args_list[0] + fetch_cmd = fetch_call.args[0] + self.assertEqual(fetch_cmd, ["git", "fetch", "origin", "launchpad"]) + self.assertFalse(any("depth" in arg for arg in fetch_cmd)) + def test_scans_only_the_pr_range_with_pr_timeout(self): # review-code High: the FETCH_HEAD..HEAD PR-scoping guarantee (the # module's central claim) was never checked against _run_gitleaks's From fe42ddc0dd94190ea891db72da855652a9f5dcf5 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Mon, 24 Aug 2026 17:26:08 +1200 Subject: [PATCH 4/4] test(launchpad): prove .gitleaks.toml's rules fire, against the real fixtures @benmitchell11 is out sick; this is the blocker from the change-request on #271, applied on their behalf so the PR is not held for their return. The blocker: fixtures existed for all seven secret categories and the PR body pasted a manual run proving each fires, but no test ran gitleaks. The only test file mocks subprocess.run throughout and says so in its own docstring, so `unittest discover -p "test_security_audit*.py"` stayed green if a rule in .gitleaks.toml stopped matching. That file's own comment records a near-miss where a capturing group silently dropped a finding to zero once useDefault was added. Adds test_security_audit_gitleaks_ruleset.py, which runs the pinned binary against launchpad/scripts/security_audit_fixtures/secrets/ with the real ruleset and asserts six things: findings exist at all; every rule this repo defines fires; every gitleaks default rule the config leans on fires; every fixture file is matched by something; no fixture has appeared without an assertion; and no finding carries an unredacted Secret field. THREE OF THE SEVEN CATEGORIES ARE NOT OURS. .gitleaks.toml sets [extend] useDefault = true, and SSH keys, registry tokens and the 64-hex/env-assignment shapes are matched by gitleaks' built-in private-key, github-pat and generic-api-key rules, not by anything in this repo. Derived by running the binary, not read off the PR body. They are asserted as a separate set from ours so a break points at the version pin rather than at a regex -- which is exactly the exposure #271's own "Not verified" named and could not catch. The fixtures are allowlisted out of the live scan by .gitleaks.toml's global [allowlist] paths, so a test using the config unmodified would scan an excluded directory, find nothing and pass. The suite builds a copy with that one entry removed and asserts the entry was present first, so a restructured config fails loudly instead of degrading to a scan of nothing. The suite skips when gitleaks is not on PATH so a local discover still runs. That skip is a hole in CI, where the binary IS installed, so the workflow now sets REQUIRE_GITLEAKS_RULESET=1, which turns the skip into a failure. Same reasoning as launchpad-agents-tests.yml's empty-discovery guard: a check that can be satisfied by absence is not a check. Verification -- the suite was mutation-tested rather than merely run: $ python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts Ran 286 tests ... OK (with gitleaks 8.30.1 on PATH, REQUIRE=1) corrupt the nostr-nsec regex to ZZZ_WILL_NEVER_MATCH_ZZZ: FAILED (failures=1) -- "Rules defined in .gitleaks.toml that matched nothing: ['nostr-nsec-private-key']" restructure the fixture allowlist entry so it no longer matches verbatim: FAILED (errors=1) -- "The fixture path-allowlist entry was not found verbatim ... would silently scan nothing" gitleaks absent, no env var: OK (skipped=1) gitleaks absent, REQUIRE_GITLEAKS_RULESET=1: FAILED (errors=1) Binary used locally was gitleaks 8.30.1 fetched to a scratch directory outside the repo and checksum-verified against the same SHA256 the workflow pins (551f6fc8...70eb). No fixture value is printed by the suite or by this commit: gitleaks runs with --redact and every assertion message is built from rule IDs and filenames only. launchpad was merged in at this commit's parent because the pre-push branch-skew hook blocked the push -- launchpad had moved on launchpad/scripts/test_no_model.py, which this branch also touches. The merge was clean and needed no resolution. Not verified: whether gitleaks' default ruleset changes across versions other than the pinned 8.30.1 -- that is the risk this suite makes visible, not one it removes. The seven categories are asserted as the eight rule IDs that currently match them; a future gitleaks release could match the same fixture under a renamed rule, which would fail this suite correctly but for a reason that needs a human to read. Signed-off-by: Serina Mcfall --- .../workflows/launchpad-security-audit.yml | 10 + .../test_security_audit_gitleaks_ruleset.py | 269 ++++++++++++++++++ .../test_security_audit_secrets_check.py | 11 +- 3 files changed, 286 insertions(+), 4 deletions(-) create mode 100644 launchpad/scripts/test_security_audit_gitleaks_ruleset.py diff --git a/.github/workflows/launchpad-security-audit.yml b/.github/workflows/launchpad-security-audit.yml index 27bb409a6df..dea959808f5 100644 --- a/.github/workflows/launchpad-security-audit.yml +++ b/.github/workflows/launchpad-security-audit.yml @@ -71,7 +71,17 @@ jobs: # also run every unrelated script's test suite in this directory, which # is duplicate work this workflow doesn't own and would blow the 3-minute # budget #66 sets for no benefit. + # REQUIRE_GITLEAKS_RULESET turns test_security_audit_gitleaks_ruleset.py's + # "gitleaks not on PATH" skip into a failure. That suite is the only thing + # that proves .gitleaks.toml's rules still match anything, and it is the + # one suite here that needs the binary — so if the install step above ever + # breaks or is reordered, this job must go red rather than report success + # for a scan-ruleset check that silently did not run. Same reasoning as + # launchpad-agents-tests.yml's empty-discovery guard: a check that can be + # satisfied by absence is not a check. - name: Run the harness's controls + env: + REQUIRE_GITLEAKS_RULESET: "1" run: python3 -m unittest discover -s launchpad/scripts -p "test_security_audit*.py" - name: Run the security audit diff --git a/launchpad/scripts/test_security_audit_gitleaks_ruleset.py b/launchpad/scripts/test_security_audit_gitleaks_ruleset.py new file mode 100644 index 00000000000..c2a44a0c17c --- /dev/null +++ b/launchpad/scripts/test_security_audit_gitleaks_ruleset.py @@ -0,0 +1,269 @@ +#!/usr/bin/env python3 +"""Proves `.gitleaks.toml`'s rules actually fire, against real fixtures. + +WHY THIS EXISTS SEPARATELY FROM test_security_audit_secrets_check.py + +That suite mocks `subprocess.run` throughout and says so in its own docstring: +it tests the check script's branching and error handling. Nothing in it runs +gitleaks, so it stays green if a rule in `.gitleaks.toml` breaks. The PR body +for #67 pasted a manual run proving each category fires — but a manual run is +not a test, and `test_security_audit_secrets_check.py` itself records a +near-miss where a capturing group silently dropped a finding to zero once +`useDefault = true` was added. This closes that: edit a rule so it no longer +matches, and this fails. + +THREE OF THE SEVEN CATEGORIES RIDE ON GITLEAKS' OWN DEFAULT RULESET + +`.gitleaks.toml` sets `[extend] useDefault = true`, and the SSH-key, registry +token and 64-hex/env-assignment categories are matched by gitleaks' built-in +`private-key`, `github-pat` and `generic-api-key` rules rather than by anything +this repo wrote. #67's own "Not verified" flagged that a gitleaks version bump +could change that behaviour with nothing to catch it. That is why DEFAULT_RULES +is asserted separately below and why its failure message names the version: a +break there points at the pin in `launchpad-security-audit.yml`, not at a rule +in this repo. + +THE FIXTURE DIRECTORY IS ALLOWLISTED IN THE REAL CONFIG + +`.gitleaks.toml`'s global `[allowlist] paths` excludes the fixtures, so the live +scan does not report on them — correct for the live scan, and fatal for a test +that used the config unmodified: it would scan an excluded directory, find +nothing, and pass. So this builds a copy with that one path entry removed, and +asserts the entry was present before removing it. If the config is restructured +so the line no longer matches verbatim, this fails loudly instead of quietly +scanning nothing. + +NO SECRET VALUE IS PRINTED + +gitleaks runs with `--redact`, and every assertion message here is built from +rule IDs and fixture filenames only — never from a report's `Secret` or `Match` +field. `test_no_secret_value_survives_redaction` asserts the redaction rather +than assuming it. + +SKIPPING + +Skipped when gitleaks is not on PATH, so a local `unittest discover` still runs +for someone who has not installed it. That skip is a hole in CI, where gitleaks +IS installed and this must actually execute — so set +`REQUIRE_GITLEAKS_RULESET=1` and the skip becomes a failure. The workflow sets +it. Same reasoning as launchpad-agents-tests.yml's empty-discovery guard: a +check that can be satisfied by absence is not a check. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +CONFIG = REPO_ROOT / ".gitleaks.toml" +FIXTURES = REPO_ROOT / "launchpad/scripts/security_audit_fixtures/secrets" + +# The one global-allowlist entry that hides the fixtures from the live scan. +# Matched verbatim so a restructured config fails this test rather than +# silently reducing it to a scan of an excluded directory. +FIXTURE_ALLOWLIST_LINE = ( + " '''launchpad/scripts/security_audit_fixtures/secrets/.*''',\n" +) + +# Rules this repo defines in .gitleaks.toml. A miss here is our regex. +OUR_RULES = { + "nostr-nsec-private-key", + "buzz-private-key", + "glibc-crypt-hash", + "buzz-s3-minio-key", + "postgres-url-with-password", +} + +# Rules gitleaks itself supplies via [extend] useDefault = true. A miss here is +# most likely the pinned gitleaks version changing, not a change in this repo. +DEFAULT_RULES = { + "private-key", # ssh_private_key.txt + "github-pat", # registry_token.txt + "generic-api-key", # env_assignment.env and the 64-hex shapes +} + +# Every fixture file must produce at least one finding. Stronger than the rule +# assertions on their own: it catches a fixture nobody matches any more, which +# rule-level assertions can hide when two fixtures share a rule. +EXPECTED_FIXTURE_FILES = { + "crypt_hashes.txt", + "env_assignment.env", + "nostr_keys.txt", + "postgres_url.txt", + "registry_token.txt", + "s3_minio_keys.txt", + "ssh_private_key.txt", +} + + +def _gitleaks_version() -> str: + try: + out = subprocess.run( + ["gitleaks", "version"], + capture_output=True, encoding="utf-8", errors="replace", timeout=30, + ) + return (out.stdout or out.stderr or "").strip() or "unknown" + except (OSError, subprocess.SubprocessError): + return "unknown" + + +def _skip_or_fail_without_gitleaks() -> None: + """Skip locally, fail in CI. See the SKIPPING note in the module docstring.""" + if shutil.which("gitleaks"): + return + if os.environ.get("REQUIRE_GITLEAKS_RULESET") == "1": + raise AssertionError( + "REQUIRE_GITLEAKS_RULESET=1 but gitleaks is not on PATH. This suite is " + "the only thing that proves .gitleaks.toml's rules still fire; letting " + "it skip here would pass vacuously. Check the install step in " + ".github/workflows/launchpad-security-audit.yml." + ) + raise unittest.SkipTest( + "gitleaks not on PATH — install it, or set REQUIRE_GITLEAKS_RULESET=1 to " + "make its absence a failure (CI does)." + ) + + +class GitleaksRulesetTests(unittest.TestCase): + """Runs the real binary against the real fixtures with the real ruleset.""" + + findings: list[dict] = [] + + @classmethod + def setUpClass(cls) -> None: + _skip_or_fail_without_gitleaks() + + if not CONFIG.is_file(): + raise AssertionError(f"{CONFIG} not found — cannot scan without the ruleset") + if not FIXTURES.is_dir(): + raise AssertionError(f"{FIXTURES} not found — nothing to scan") + + source = CONFIG.read_text(encoding="utf-8") + if FIXTURE_ALLOWLIST_LINE not in source: + raise AssertionError( + "The fixture path-allowlist entry was not found verbatim in " + ".gitleaks.toml. It is what hides the fixtures from the live scan, " + "and this test must remove it to have anything to scan. The config " + "has been restructured: update FIXTURE_ALLOWLIST_LINE to match, and " + "confirm the live scan still excludes the fixtures." + ) + + cls._tmp = tempfile.TemporaryDirectory() + tmp = Path(cls._tmp.name) + scan_config = tmp / "gitleaks-fixtures.toml" + scan_config.write_text(source.replace(FIXTURE_ALLOWLIST_LINE, ""), encoding="utf-8") + report = tmp / "report.json" + + proc = subprocess.run( + [ + "gitleaks", "detect", + "--no-git", + "--source", str(FIXTURES), + "--config", str(scan_config), + "--report-format", "json", + "--report-path", str(report), + "--redact", + "--no-banner", + # Findings ARE the expected result here, so a non-zero "leaks + # found" exit is not a failure of this test. + "--exit-code", "0", + ], + capture_output=True, encoding="utf-8", errors="replace", timeout=180, + ) + if proc.returncode != 0: + raise AssertionError( + f"gitleaks exited {proc.returncode} with --exit-code 0, which means it " + f"could not run rather than that it found nothing. stderr tail: " + f"{(proc.stderr or '')[-400:]}" + ) + if not report.is_file(): + raise AssertionError("gitleaks wrote no report file") + + raw = report.read_text(encoding="utf-8").strip() + cls.findings = json.loads(raw) if raw else [] + + @classmethod + def tearDownClass(cls) -> None: + tmp = getattr(cls, "_tmp", None) + if tmp is not None: + tmp.cleanup() + + def test_the_scan_produced_findings_at_all(self): + """A zero-finding scan is the failure this whole suite exists to catch.""" + self.assertGreater( + len(self.findings), 0, + "gitleaks reported zero findings against the planted fixtures. Either " + "every rule is broken, or the fixtures are still being allowlisted out " + "of the scan.", + ) + + def test_every_rule_this_repo_defines_fires(self): + fired = {f["RuleID"] for f in self.findings} + missing = sorted(OUR_RULES - fired) + self.assertFalse( + missing, + f"Rules defined in .gitleaks.toml that matched nothing: {missing}. Their " + f"fixtures are still present, so the regex no longer matches. Rules that " + f"did fire: {sorted(fired)}", + ) + + def test_every_gitleaks_default_rule_we_rely_on_fires(self): + """Separate from our own rules: a miss here points at the version pin.""" + fired = {f["RuleID"] for f in self.findings} + missing = sorted(DEFAULT_RULES - fired) + self.assertFalse( + missing, + f"gitleaks' own default rules that matched nothing: {missing}. " + f".gitleaks.toml relies on these via [extend] useDefault = true and does " + f"not define equivalents, so these categories are now unguarded. Most " + f"likely cause is the pinned gitleaks version changing its default " + f"ruleset — running version: {_gitleaks_version()}. The pin lives in " + f".github/workflows/launchpad-security-audit.yml. Rules that did fire: " + f"{sorted(fired)}", + ) + + def test_every_fixture_file_is_matched_by_something(self): + """Catches an orphaned fixture that rule-level assertions would hide.""" + matched = {Path(f["File"]).name for f in self.findings} + unmatched = sorted(EXPECTED_FIXTURE_FILES - matched) + self.assertFalse( + unmatched, + f"Fixture files that produced no finding: {unmatched}. Each exists to " + f"prove one category is caught; a fixture nobody matches is a category " + f"nobody guards.", + ) + + def test_the_fixture_set_has_not_grown_unnoticed(self): + """A new fixture with no assertion is a category nobody proved.""" + on_disk = {p.name for p in FIXTURES.iterdir() if p.is_file()} + untracked = sorted(on_disk - EXPECTED_FIXTURE_FILES) + self.assertFalse( + untracked, + f"Fixture files present but not named in EXPECTED_FIXTURE_FILES: " + f"{untracked}. Add them there, and add the rule they are meant to prove " + f"to OUR_RULES or DEFAULT_RULES, or this suite silently ignores them.", + ) + + def test_no_secret_value_survives_redaction(self): + """The disclosure guarantee, asserted rather than assumed.""" + leaked = sorted({ + f["RuleID"] for f in self.findings + if f.get("Secret") and f["Secret"] != "REDACTED" + }) + self.assertFalse( + leaked, + f"gitleaks returned an unredacted Secret field for findings from rules " + f"{leaked}. --redact is what keeps fixture values out of CI logs and out " + f"of this suite's own failure output. Do not paste the value here while " + f"debugging.", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/launchpad/scripts/test_security_audit_secrets_check.py b/launchpad/scripts/test_security_audit_secrets_check.py index 2c9ea8af250..4e865a5d59f 100644 --- a/launchpad/scripts/test_security_audit_secrets_check.py +++ b/launchpad/scripts/test_security_audit_secrets_check.py @@ -2,10 +2,13 @@ """Controls for the #67 secret-material check. Mocks subprocess.run throughout — no real gitleaks invocation, no network. -.gitleaks.toml's rules themselves are proven against real fixtures with the -real gitleaks binary separately (see the PR body / commit history for that -evidence); this suite is about the check script's own branching and error -handling, which is what a future edit is most likely to quietly break. +This suite is about the check script's own branching and error handling, which +is what a future edit is most likely to quietly break. + +.gitleaks.toml's rules themselves are proven by +`test_security_audit_gitleaks_ruleset.py`, which runs the real binary against +the planted fixtures and fails if any rule stops matching. Keep that split: a +rule regression must not be able to hide behind these mocks. """ import json