aws-samples / aws-samples/sample-autonomous-cloud-coding-agents
bug(security): security:sast fails on main — 2 semgrep findings from #695 block pre-push
- Dominant language
- TypeScript
- Stars
- 143
- Forks
- 46
- Avg merge
- 3d 9h
- Merged PRs (30d)
- 20
Description
## 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).
Contributor guide
Research direction
Start by running `mise run security:sast` on `main` and inspect `cdk/src/handlers/shared/orchestration-comment-trigger.ts:221` and `scripts/linear_epic.py:89`. Read `AGENTS.md`, `mise.toml`, `security.yml`, and `security-pr.yml`, plus related issues #540 and #722, to determine whether the findings are false positives and how pinning or diff-scoping fits. Done means the command exits 0 on `main`, suppressions are inline and justified, and pre-push does not reject findings outside the contributor’s changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, typescript
- Domain
- ci-cd, security, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100