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

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

Aperta
#729 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
ci-cd security
Lingua principale
TypeScript
Stelle
143
Fork
46
Merge medio
3g 10h
PR unite (30g)
24

Descrizione

## 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).

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Inizia eseguendo `mise run security:sast` su `main` e ispeziona `cdk/src/handlers/shared/orchestration-comment-trigger.ts:221` e `scripts/linear_epic.py:89`. Leggi `AGENTS.md`, `mise.toml`, `security.yml` e `security-pr.yml`, oltre alle issue correlate #540 e #722, per determinare se i finding sono falsi positivi e come si inseriscono il pinning o il diff-scoping. Il lavoro è completato quando il comando termina con codice di uscita 0 su `main`, le soppressioni sono inline e giustificate e pre-push non rifiuta i finding al di fuori delle modifiche del contributor.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
python, typescript
Ambito
ci-cd, security, tooling
Tipo di issue
Bug
Difficoltà
4/5
Tempo stimato
3-5 giorni
Stato di attività
Tranquilla
Chiarezza
Abbastanza chiara
Idoneità per principianti
48/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.