feat(launchpad): secret-material detection via gitleaks (#67) - #271
feat(launchpad): secret-material detection via gitleaks (#67)#271benmitchell11 wants to merge 3 commits into
Conversation
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.
|
Throwaway-branch proof, per #67's own definition of done ("every rule has been observed failing a real run on a throwaway branch, with the run linked from the PR"): Pushed a disposable branch off this one with a single planted, synthetic secret ( Run: https://github.com/launchpad-26/buzz/actions/runs/32440647262/job/96650516602 Raw output from the actual CI run: File, line, rule ID — never the matched value, on the real workflow, not a local approximation. This demonstrates the PR-diff wiring itself (trigger, exit-code handling, the check going red on github.com) actually works end to end. The other 6 rule categories are proven firing against planted fixtures locally (see PR body Verification) rather than each getting their own throwaway PR — one real CI failure proves the mechanism; the fixture scan proves the ruleset. |
|
Automated review (requested by @serina, via her review-code/review-tests/review-adjudicate pipeline — independent reviewers + adjudicator, each re-verified claims against the actual repo and each other's severities rather than trusting the diff) Blocker
High
Medium / Low (non-blocking, worth a look)
|
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Requesting changes — two Blockers from the automated review posted above, both need fixing before merge:
- security_audit_tracked_files_check.py:56 — the .example exemption reopens the exact hole #68 was filed to close. I ran the actual patterns against the files the code comment cites as justification (.env.example, deploy/compose/.env.example, mobile/.env.json.example) — none of them ever needed the exemption; every pattern is anchored to a filename ending that .example already changes. The exemption's only real effect is suppressing a match inside a tracked seed/ or seed.sample/ directory — the shape #68 documents a real past incident for (a committed SSH public key). A file like launchpad/deploy/archived/seed/authorized_keys.example reports PASS today. Please drop the exemption, or scope it to the .env family the way .gitleaks.toml already does.
- test_security_audit_ignore_coverage_check.py:20 — test_full_coverage_passes's fixture is built by iterating REQUIRED_COVERAGE itself, so it can't fail on a dropped pattern. 8 of the 11 required patterns — including identity.key, id_rsa, id_ed25519 — have no independent test; they could be silently removed from the dict and the suite would stay green. Please add a hardcoded-literal test per unprotected pattern, same shape as the existing *.pem and seed.sample/ tests.
The 5 Medium findings from the same review are non-blocking — happy to file those as follow-up issues too if you want, same as #271's.
#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.
…g 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.
Blocker 1: security_audit_tracked_files_check.py's *.example suffix exemption reopened exactly the hole #68 was filed to close. None of _SENSITIVE_PATTERNS ever matched a .example-suffixed filename in the first place (all end-anchored to .env/.key/.pem/id_rsa/id_ed25519), so the exemption bought nothing there -- but seed/ and seed.sample/ match on directory component, not filename, so a tracked seed/authorized_keys.example previously slipped past it despite being the exact shape #68 documents a real past incident for. Dropped the exemption entirely per review-code's first suggested fix. Also fixed the same --depth=1 shallow-fetch bug from #271 in this file's own newly-hidden-tracked-file diff, found while in there. Blocker 2: test_full_coverage_passes built its fixture by iterating REQUIRED_COVERAGE itself, so it could never fail on a dropped pattern -- 8 of 11 required patterns, including identity.key/id_rsa/ id_ed25519, had no independent test protecting them. Added one hardcoded-literal regression test per previously-unprotected pattern, same shape as the existing *.pem/seed.sample/ tests. Verification: 78 tests pass (up from 61), and a real end-to-end `security_audit.py .` run reports 4 pass / 0 fail / 1 warn (the full-history WARN is expected and unchanged).
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Changes requested — one blocker. Also: the change-request currently on this PR belongs to #275.
Reviewed in a fresh context, with the real pinned gitleaks binary. I am an agent; I do not approve or reject — this flags what needs fixing before @serina-mcfall approves.
First, a status problem worth untangling before anything else
The formal CHANGES_REQUESTED blocking this PR is not about this PR's code. It was submitted 2026-08-21T03:44:36Z and cites security_audit_tracked_files_check.py:56 and test_security_audit_ignore_coverage_check.py:20. Neither file exists in #271's diff. Both live in #275, which carries a textually identical review submitted two minutes earlier at 03:42:38Z — including the line "same as #271's", a self-reference that only makes sense sitting on #275.
So it is not stale and not live; it never applied here, and no change to #271 can address it. Worth dismissing rather than trying to satisfy.
#271's own review did land correctly as an issue comment at 03:34:07Z, raising a real blocker at security_audit_secrets_check.py:143 — INDETERMINATE rendering as PASS on the gate path, against ADR-0008. Commit 8a73711d fixed it, and I verified the current head: every infra-failure branch on the PR-diff path now returns FAIL, and the tests assert --redact is passed and that run()'s return value propagates. That one is genuinely addressed.
Fail-open: correct, and I want to state it plainly since it is the thing that matters most
The gate path fails closed on every infra failure — missing binary, timeout, unexpected exit code, malformed JSON, unset GITHUB_BASE_REF, failed base fetch. _run_gitleaks returns (None, reason) and never a false-clean []. Checked against the harness contract: exit_code() fails the run only on FAIL, which is exactly why the asymmetry is right — the gate path deliberately never returns INDETERMINATE, while the non-gating full-history scan legitimately does. The workflow runs it with no || true. Good design, and the reasoning is visible in the code.
Blocker — the fixtures that prove the rules work are never run by any test
launchpad/scripts/test_security_audit_secrets_check.py and launchpad/scripts/security_audit_fixtures/secrets/
Fixtures exist for all 7 secret categories, and the PR body shows a manual run proving each fires. But that run is not a test. The test file says so itself: "Mocks subprocess.run throughout — no real gitleaks invocation… this suite is about the check script's own branching and error handling." Every test patches subprocess.run or _run_gitleaks. Nothing ever runs the real binary against the fixtures.
So the command CI actually executes — python3 -m unittest discover -s launchpad/scripts -p "test_security_audit*.py" — stays green if a rule breaks.
The failure scenario is concrete and your own comment predicts it: the file notes a near-miss where a capturing group silently dropped a finding to zero once useDefault = true. Edit .gitleaks.toml, break nostr-nsec-private-key, and CI reports OK while a real Nostr private key committed to the repo goes uncaught.
Fix: one test or CI step that runs the checksum-pinned gitleaks against security_audit_fixtures/secrets/ with the real .gitleaks.toml and asserts each of the 7 rule IDs fires at least once — the command already in your Verification section, made repeatable instead of manual.
Blocking because this is the difference between a scanner and a scanner-shaped thing: every other guarantee here is only as good as the ruleset, and nothing watches the ruleset.
Verified clean
- No secret reaches a log.
--redacton every path,_summarize()builds detail strings only fromFile/StartLine/RuleID, and tests assert the fixture's fake secret value is absent fromresult.detail. I confirmed empirically that the real binary returns"Secret": "REDACTED". - History is scanned, not just the working tree —
gitleaks detectthroughout; my own full-history run covered 7,116 commits. Findings there WARN rather than FAIL by explicit design per ADR-0006, so pre-existing secrets never block a merge — a stated trade-off, not a hidden one. - Version pinned and checksum-verified — gitleaks 8.30.1, SHA256 checked against the published checksums.
- Two allowlist entries are sound: the
.env.examplepath scope, and the loopback-only Postgres URL regex.
Non-blocker, filed as #389
Two global allowlist entries are broader than their stated reason. .gitleaks.toml:108-109's <[A-Za-z0-9 ]+> regex is justified by a claim that does not hold — I ran the ruleset with the global block stripped and <64 hex characters> produces zero findings either way; the only line that fires is the adjacent Public key: one, already covered by a separate literal entry. And :106 exempts any Cargo.lock anywhere rather than the two files the comment verifies. I could not construct a working bypass through either, so this is forward-looking risk, not a hole today.
Not verified
gitleaks was not installed here — I fetched the pinned 8.30.1 into a scratch directory (never the repo tree) and checksum-verified it, so the empirical claims above are real runs, not inference. I did not visually confirm the literal final return/sys.exit line of security_audit_core.main(); I inferred it calls exit_code(results) from the module docstring and that being the function's only use. That is the one link in the fail-closed chain I would want a second pair of eyes on.
Dismissing as misfiled -- this review is not about #271. It cites security_audit_tracked_files_check.py:56 and test_security_audit_ignore_coverage_check.py:20; neither file exists in #271's diff. Both are in #275, which carries a textually identical review submitted two minutes earlier (03:42:38Z), including the phrase 'same as #271's' -- a self-reference that only makes sense on #275. No change to #271 could have addressed it. #271's own review landed correctly as an issue comment at 03:34:07Z and its blocker was fixed in 8a73711; a current review has been posted separately.
Requested changes NOT yet done — worth a look soonChecked at head Still open — the fixtures are never exercised by any test CI runs.
So The fix is small: one test or CI step running the checksum-pinned binary against Everything else stands verified and is good work: the gate path fails closed on every infra failure, Flagging rather than nagging — this is the last thing between the PR and mergeable, and it is blocking #275 behind it. |
2 participants
Summary
Adds secret-material detection to the #62 audit, per ADR-0006's engine/allowlist decision: gitleaks driven by
.gitleaks.toml, registered into the harness (not a second workflow), PR-diff path fails on any finding, scheduled full-history path reports without failing.Related issue
Closes #67
Issue type
Task
Agent provenance
Objective
security_audit_secrets_check.py, registered intosecurity_audit_registry.py, running gitleaks against the PR diff (fail on finding) and full git history (report, never fail) with a fork-specific.gitleaks.tomlruleset covering the seven material categories #67 names, proven by planted fixtures.Impacted components
.gitleaks.toml
launchpad/scripts/security_audit_secrets_check.py
launchpad/scripts/test_security_audit_secrets_check.py
launchpad/scripts/security_audit_fixtures/secrets/
launchpad/scripts/security_audit_registry.py
launchpad/scripts/test_no_model.py
.github/workflows/launchpad-security-audit.yml
Approach and rejected alternatives
This branch was originally started off
feat/security-audit-harness(#200's branch) before that merged, per a note left on the branch at the time. Rebuilt on a fresh branch off currentlaunchpadrather than rebasing the old one, since #200 and its own follow-up (#253) had already landed independently and the old branch's base no longer existed.Rejected
--exit-code 0on the gitleaks invocation: gitleaks' real exit code (0 clean, 1 leaks found, anything else a genuine engine failure) is how this check tells "it ran and found nothing" apart from "it crashed" — forcing exit 0 would erase that distinction and let a broken gitleaks invocation silently report clean.Rejected gitleaks'
--baseline-pathmechanism for the first-run noise (222 findings measured across full history, see Verification) in favor of no baseline at all, downgrading the full-history path to WARN instead. ADR-0006 already rejected the baseline mechanism specifically because a regenerated baseline has no field for a reason and can silently accept a real finding; the same logic applies here. The 222 findings stay visible on every scheduled run rather than being triaged away — remediating or allowlisting them individually is follow-on work this task surfaces, not resolves.Verification
Command run:
Raw output (tail):
Command run (proving all 7 required material categories fire, against a config with the fixtures' own path-exclusion temporarily removed):
Raw output (rule IDs and counts only, per the "never print the matched value" rule this task itself enforces):
Every one of the 7 categories #67 names is covered (SSH keys to
private-key; crypt hashes toglibc-crypt-hash, both$6$and$y$shapes; Nostr nsec tonostr-nsec-private-key; 64-hex/BUZZ_PRIVATE_KEYtobuzz-private-key+generic-api-key; S3/MinIO pair tobuzz-s3-minio-key; registry tokens togithub-pat; Postgres URL topostgres-url-with-password).Command run (the real check, real config, real repo, matching CI's exact invocation):
Raw output:
16s for a full-history scan — comfortably inside the 3-minute PR budget even though this is the path explicitly allowed to exceed it. Filtered the JSON report directly (not assumed) to confirm the two known false-positive classes produce zero findings against the real repo:
dev-deployment-SOP.md(0 findings) and bothCargo.lockfiles (0 findings). Fixtures directory: 0 findings (correctly excluded from the live scan).Spot-checked several of the 222 real findings directly against current file content to confirm they're benign test/fixture material, not real secrets — e.g.
deploy/charts/buzz/tests/hpa_test.yaml:9is the literal placeholderpostgres://u:p@h:5432/d, andcrates/buzz-db/src/replica_fence.rs's finding is a test helper building a connection string with the literal test passwordfence_probe_test. (Full-history findings report the file/line as it appeared in the historical commit gitleaks matched, not current HEAD, so a few spot-checks landed on lines that no longer say what the finding implies at today's HEAD — expected behavior for a history scan, not a bug.)Command run:
Raw output:
Confirms the full entrypoint runs end-to-end locally, matching what CI will do, and that a WARN does not fail the overall audit.
Throwaway-branch proof that the PR path actually fails a real run: see the linked comment below — a planted, synthetic secret was pushed to a disposable branch, a PR opened against
launchpad, thelaunchpad — security auditcheck observed failing for real, then closed/deleted.Not verified
Whether every one of the 222 pre-existing full-history findings is genuinely benign — only a handful were spot-checked directly; the rest are visible every scheduled run (by design, WARN not silently triaged) rather than individually confirmed here. Whether gitleaks' default ruleset (
useDefault = true) itself ever changes behavior on a version bump — this PR pins 8.30.1 in the workflow (checksum-verified against gitleaks' own published checksums file) but does not attempt to freeze the ruleset's own content against future gitleaks releases.Security implications
This is a detective control, not preventive — it reports secret material already committed; it does not stop a commit from happening (that's git/GitHub push protection territory, #72) and it does not rotate or remediate anything it finds (explicitly out of scope, per #67 itself). The workflow itself holds no repository secret and needs none —
permissions: contents: readonly — so it can run safely on a fork pull request. Findings never surface a matched value anywhere:--redacton every gitleaks invocation, and the check's own summary is built only from file/line/rule-id fields, never gitleaks' Secret/Match fields — verified directly in the test suite (asserting the fixture's fake secret value never appears in the result).Escalations
None new. The 222-finding first-run baseline is a real, visible signal this task surfaces rather than resolves — remediating or allowlisting individual findings is follow-on work, not blocking this task's own definition of done, which asks for a decided and documented behavior, not zero findings.