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

bug(security): security:sast fails on main — 2 semgrep findings from #695 block pre-push

Offen
#729 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
ci-cd security
Vorherrschende Sprache
TypeScript
Sterne
143
Forks
46
Ø Merge
3 T. 10 Std.
Gemergte PRs (30 T.)
24

Beschreibung

## Problem

`mise run security:sast` **fails on `main`**, so the local pre-push hook (`hooks:pre-push:security` → `mise run security`) is red for everyone, on every branch, regardless of what the branch changes.

Reproduced on `main` @ `a94be94c` and independently on two separate worktrees (a near-main branch and a branch whose only diff is one `mise.toml` line):

```
$ mise run security:sast
❯❱ javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp
221┆ return new RegExp(`(^|[^a-z0-9])${esc}([^a-z0-9]|$)`, 'i').test(text);
❯❱ python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
89┆ with urllib.request.urlopen(req, timeout=30) as r:
exit=1
```

| Finding | Location |
| --- | --- |
| `detect-non-literal-regexp` | `cdk/src/handlers/shared/orchestration-comment-trigger.ts:221` |
| `dynamic-urllib-use-detected` | `scripts/linear_epic.py:89` |

Both lines are present **verbatim on `main`**, last touched by #695 (`4357c353`, "land the carved orchestration arc on main (S2–S8)"). Neither has an inline `nosemgrep`.

## Why this matters

- **It blocks pushes that CI would pass.** `security:sast` is *not* a required status check on the `main` ruleset (`14980587` requires only `build (agentcore)` and `Secrets, deps, and workflow scan`), and `security-pr.yml` explicitly defers the heavy SAST suite to `security.yml`. So the **local gate is stricter than the merge gate** — the same asymmetry class as #721, on another surface.
- Contributors learn to reach for `--no-verify`, which is how a *real* finding eventually gets waved through.

## Likely cause: unpinned rules, not new code

`security:sast` runs `semgrep scan --config auto ...`, which resolves rule packs from the **remote registry at scan time**, so this can flip red with no code change. That is exactly the failure mode #540 ("`semgrep = \"latest\"` + `--config auto` lets registry changes break CI") and #722 (pin scanner toolchain) predict. Local semgrep is currently **1.172.0**, unpinned.

Worth confirming whether these two rules are newly-added/newly-tightened upstream rather than something #695 introduced — if so, the durable fix is pinning (#540/#722) and this issue is the concrete instance that justifies it.

## Assessment of the two findings

Both look like defensible false positives, but that should be a deliberate call, not an implicit one:

- `orchestration-comment-trigger.ts:221` — the interpolated segment is escaped on the immediately preceding line (`phrase.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')`), so it is not attacker-controlled regex syntax. A ReDoS argument would need the input bound checked.
- `linear_epic.py:89` — a maintainer-run script hitting the Linear API; the URL is not user-supplied.

## Proposed fix

1. Triage each finding; if a false positive, add an inline `nosemgrep: -- ` on the offending line (the convention AGENTS.md already prescribes for `security:sast:masking`).
2. Pin semgrep + rule packs so registry drift cannot redden the suite again — defer to #540/#722 rather than duplicating.
3. Consider whether the **pre-push** SAST gate should be diff-scoped (like `security:secrets:range` after #721) so a contributor is never blocked by a pre-existing finding in code they did not touch.

## Acceptance criteria

- `mise run security:sast` exits 0 on `main`.
- Any retained suppression is inline, rule-scoped, and carries a written justification.
- Pre-push does not fail on findings that the required merge checks would not fail on.

## How this surfaced

Found while landing #721 (PR #728): after the gitleaks scope fix cleared the original block, the pre-push hook still failed on these two SAST findings. Verified against the diff — PR #728 changes only `mise.toml`, and the findings reproduce on untouched `main`. Each other gate passes (`security:secrets`, `:range`, `security:deps`, `security:gh-actions`, `//cdk:compile`), so #728 was pushed with `--no-verify` and this filed instead.

Related: #540, #722 (pinning), #542 (`security:sast:masking` red on main — separate gate, same suite), #721 / #723 (the gitleaks half of this asymmetry), #695 (introduced the flagged lines).

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Führe zunächst `mise run security:sast` auf `main` aus und untersuche `cdk/src/handlers/shared/orchestration-comment-trigger.ts:221` und `scripts/linear_epic.py:89`. Lies `AGENTS.md`, `mise.toml`, `security.yml` und `security-pr.yml` sowie die zugehörigen Issues #540 und #722, um festzustellen, ob es sich bei den Findings um False Positives handelt und wie Pinning oder Diff-Scoping dazu passen. Erledigt bedeutet, dass der Befehl auf `main` mit Exit-Code 0 endet, Suppressions inline und begründet sind und pre-push Findings außerhalb der Änderungen des Contributors nicht ablehnt.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
python, typescript
Bereich
ci-cd, security, tooling
Issue-Typ
Bug
Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Aktivitätsstatus
Ruhig
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
48/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.