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

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

Open
#729 0 comments 0 reactions 0 assignees View on GitHub
ci-cd security
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.