aws-samples / aws-samples/sample-autonomous-cloud-coding-agents

decision: scheduled gitleaks sweep — drop it (rely on per-PR range) or scope it to main? (blocks #721)

Open
#723 1 comment 0 reactions 1 assignee Claimed by @scottschreckengaust View on GitHub
ci-cd P1 security
Dominant language
TypeScript
Stars
143
Forks
46
Avg merge
3d 9h
Merged PRs (30d)
20

Description

## Decision needed: what to do with the scheduled full-history gitleaks sweep

Blocks/scopes **#721**. Splitting the *decision* out from the *implementation* because there's a real security trade-off that a reviewer/owner should ratify before code lands.

### Background — how the three gitleaks tasks are wired today

| Surface | Task invoked | Scope | Enforcement |
| --- | --- | --- | --- |
| **PR gate** (`security-pr.yml`, **required** check "Secrets, deps, and workflow scan") | `security:secrets:range` | commits in `origin/$BASE..HEAD` (the PR's own diff) | **Blocks merge** (#327/#330) |
| **Scheduled weekly** (`security.yml` → `mise run security`) | `security:secrets` (full) | **all fetched refs** (runner is `fetch-depth: 0`) | Files a failure issue; does **not** gate PRs |
| **Local pre-push** (`hooks:pre-push:security` → `mise run security`) | `security:secrets` (full) | all fetched refs | Local only |

### The problem (surfaced in #721 / #593)
The scheduled `security:secrets` runs bare `gitleaks git . --no-banner --redact` with **no `--log-opts`**, so it scans secrets across **every fetched ref** — including **unmerged, no-PR branches**. A stray/abandoned branch (e.g. an AWS account id in a doc on `feat/agentcore-gateway-mcp`, a 381-commit spike with no PR) reddens the mainline scheduled suite even though nothing reached `main`. Confirmed empirically:
```
gitleaks git . --no-banner --redact → leaks found: 4 (all from an unmerged branch)
gitleaks git . --no-banner --redact --log-opts=HEAD → no leaks found (main history is clean)
```

### The decision
The PR gate already uses the range scan and is a required check — **no change needed there.** The question is only about the **scheduled/pre-push full sweep**:

**Option 1 — Remove the full sweep; rely solely on the per-PR range scan.**
- ✅ Simplest. Nothing to scope; delete `security:secrets` from the scheduled `security` aggregate (and pre-push).
- ⚠️ **Loses the historical backstop.** The range gate only exists since #330 and only scans each PR's *diff going forward*. A secret committed to `main` **before** #330, or introduced via any **non-PR path** (direct push, admin merge, force-push, imported history), would **never** be re-audited. No safety net over `main`'s full history.

**Option 2 — Keep the full sweep, but scope it to the mainline (`--log-opts="HEAD"`).**
- ✅ Retains a full-history backstop over **`main`** (verified: scans main's 279-commit history and **fail-closes** on a planted mainline secret).
- ✅ Excludes other branches' unmerged history — kills the #721 false-positive class.
- ✅ One-line change; pre-push still scans the developer's own branch commits.
- ⚠️ A secret living **only** in unmerged branch history is no longer caught by the sweep — acceptable because the required per-PR range gate catches it **at merge time**, on the actual diff (where you want the gate to fire).
- 🪤 Gotcha discovered while prototyping: scope with **`HEAD`**, not a ref-pattern like `--remotes=origin/main` — the latter matches **no ref** in a plain worktree and walks **0 commits**, silently turning the scan into a fail-**open** no-op. `HEAD` always walks the real checked-out history (which is `main` in the scheduled run).

### Recommendation
**Option 2.** Unlike a redundant pin (where an in-path gate makes a floor pointless), the full sweep is **not** redundant with the range gate: the range gate is recent and diff-only, so it cannot see `main`'s pre-#330 history or non-PR paths. Option 2 keeps that unique backstop while removing the false-positive drag. Option 1 trades away real coverage for marginal simplicity.

### Whichever is chosen, #721 implements it
- Option 1 → drop `security:secrets` from the `security` aggregate (+ pre-push), update docs, note the coverage gap accepted.
- Option 2 → set `security:secrets` `run` to `gitleaks git . --no-banner --redact --log-opts="HEAD"`, comment the rationale + the `--remotes` fail-open trap, keep `:range` and `:staged` unchanged.

### Acceptance for THIS issue
- A maintainer/owner selects Option 1 or Option 2 (or a third approach) with a one-line rationale, so #721 can implement without re-litigating the trade-off.

### References
#721 (implementation), #593 (failure symptom), #327 / #330 (required per-PR range gate), #335 / #336 (prior merge_group range-scoping — the other half of this split), ADR-003 (governance).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.