Pular para conteúdo

AUR-548 — Semgrep entra no gate como SAST multilinguagem

Status

Implemented in internal/analysis/semgrep.go (the Semgrep adapter), internal/config/sast.go (quality_gates.sast), internal/config/central.go (central-policy precedence) and cmd/aurumcode/aur548.go (runSASTPass, applySASTGate), wired into both cmd/aurumcode/main.go's runReview (--base) and cmd/aurumcode/pr.go's runPRReview (--pr). Behavior proof: cmd/aurumcode/aur548_test.go::TestAUR548* (including the --pr cases, driven through runPRReview with the same httptest GitHub mock pattern aur537_test.go/aur515_test.go already use); acceptance: tests/acceptance/AUR-548.sh.

--pr's own scan target is the verified checkout, never the wrong tree. runPRReview reuses AUR-515/536's own verification exactly as computed for the codebase-context pass (codebaseContextMismatch + verifiedCleanCheckoutReason, mismatch/verifiedDir in pr.go): when the local checkout is confirmed to be the pull request's own head commit (same repository, same HEAD, clean tree), Semgrep scans that exact, verified directory. When it is not — a different repository, a divergent HEAD, an unclean tree, or simply unverifiable — Semgrep is never invoked at all, and the SAST pass reports its own inconclusive reason (sast_unverified_checkout) instead, following gate.inconclusive the same way every other Semgrep failure mode does.

Product image: verified. docker build -t aurumcode-aur548-check:tmp -f Dockerfile . builds successfully, and docker run --rm --entrypoint semgrep aurumcode-aur548-check:tmp --version reports 1.172.0 from inside the built image — Alpine/musl was a real theoretical risk (Semgrep's PyPI wheels have historically targeted glibc) but semgrep 1.172.0 ships a musllinux_1_2_x86_64 wheel, so the straightforward pip install on the existing Alpine final stage works with no base-image change needed. The temporary image was removed (docker rmi) after verification.

Independent review follow-up (B1/B2/B3)

A review of the first --pr-wiring commit found three blocking gaps, closed in a follow-up commit, each with its own regression test:

  • B1 — a fatal Semgrep-side failure could pass as a clean scan. The original Semgrep() only ever looked at the runner's own exit code together with whether stdout parsed as JSON; it never read Semgrep's own top-level "errors" array, and it treated ANY non-zero exit whose stdout still parsed as "findings were reported" (true for some other CI integrations that pass --error, but this command never does). A rule-pack download failure prints exactly {"errors":[{...}],"results": []} — a decodable, zero-finding report — at an exit code that can be non-zero OR 0. decodeSemgrepReport now reads Errors explicitly (ErrSemgrepReportedErrors), checked BEFORE the runner's own exit code, and any remaining non-zero exit (no errors array, otherwise a clean report) is ALSO now a failure (AUR-548-MUT-001-ANCHOR in internal/analysis/semgrep.go), since --error is never passed. Tests: TestSemgrepReportedErrorsNeverReadAsClean/ TestSemgrepNonZeroExitWithoutErrorFlagIsFailure (internal/analysis/semgrep_test.go, unit level) and TestAUR548SemgrepReportedErrorsAreInconclusive/ TestAUR548NonZeroExitCleanReportIsInconclusive (cmd/aurumcode/aur548_test.go, end-to-end via runReview).
  • B2 — Semgrep 1.x's real severity vocabulary was unmapped. normalizeSemgrepSeverity only recognized the historical ERROR/WARNING/ INFO spelling; Semgrep 1.x's own documented extra.severity values are CRITICAL/HIGH/MEDIUM/LOW/INFO (plus INVENTORY/EXPERIMENT for non-gating rules), so a real HIGH finding fell through to the "unrecognized" default — which happened to still be "error" today, but was unverified and one careless edit away from silently becoming "info". Now mapped explicitly: CRITICAL/HIGH→error, MEDIUM→warning, LOW/INFO→info, anything else→error. Tests: TestNormalizeSemgrepSeverity (table test, every input) and TestAUR548HighSeverityFailsAtErrorThreshold (end-to-end: a real HIGH finding fails the default fail_on_severity: ERROR threshold).
  • B3 — the scanned tree on --pr is the pull request author's own content. A # nosemgrep comment or a committed .semgrepignore let the author suppress a finding even under a central policy that is supposed to be authoritative over them. analysis.Runner.Semgrep now takes a policyOrigin parameter and, when true, passes TWO flags: --disable-nosem (so a # nosemgrep comment cannot suppress a finding) and --x-ignore-semgrepignore-files (so a committed .semgrepignore, at ANY depth in the tree, cannot hide a file from the scan either). A repository's own, policy-free opt-in never adds either flag: there is no author/policy trust boundary to defend when the repository configured SAST on itself. Tests: TestSemgrepPolicyOriginFlags (unit level) and TestAUR548PolicyOriginDisablesNosem (end-to-end, cmd/aurumcode/aur548_test.go).

Revision history for B3. The first fix (committed, then re-reviewed) instead scanned a sanitized filesystem COPY of the checkout with .semgrepignore stripped (sastScanRoot/copyTreeExcluding). A second review found that copy bypassable and unsafe: copyTreeExcluding used os.ReadFile, which follows symlinks, so a .semgrepignore containing * plus one dangling or tree-escaping symlink restored the original bypass (reproduced: scan == root) and could read arbitrary host paths (/etc/hostname, /proc/self/environ) into the scan tree or hang on a device file (/dev/zero); runSASTPass also silently fell back to scanning the unsanitized root on any copy error, which is itself a fail-OPEN path under a trust boundary. docker run --rm docker.io/semgrep/semgrep@sha256:65dcd4408adda7c183a6b4550cb1e9b19f7f627a6fbb7e0559bd466bedc44d7b semgrep scan --help (the exact digest .board/bootstrap/locks/ scanners.yml pins, confirmed at version 1.172.0, matching the product image's pip-installed version) was then checked for a native flag, per the second review's own suggestion, and found one: --x-ignore-semgrepignore-files ("[INTERNAL] Ignore all '.semgrepignore' files found in the project tree ... THIS OPTION IS NOT PART OF THE SEMGREP API AND MAY CHANGE OR DISAPPEAR WITHOUT NOTICE"). Using Semgrep's own, already-audited target-selection code instead of a second, hand-rolled filesystem copy removes the entire symlink attack surface outright; sastScanRoot/copyTreeExcluding and their tests were deleted. Should this internal flag ever disappear in a future Semgrep version, the invocation simply errors on an unrecognized flag, which this adapter already turns into a non-nil, fail-closed error (B1's own handling) -- never a silent, unprotected scan.

Also from that review: SastConfig.Validate now rejects a rule_packs entry starting with - (would otherwise read as a CLI flag); every Semgrep-sourced Message passes through the redaction filter before publication (runSASTPass's new filter parameter), matching every other publication sink in this codebase; and QualityGatesConfig precedence under ApplyCentralPolicy changed from "any central policy wins the WHOLE quality_gates block" to per-section — a policy that declares quality_gates.sast says nothing about quality_gates.ssor_dtrack or quality_gates.supply_chain, and a repository's own, undeclared sections survive untouched (internal/config/qualitygates.go, internal/config/central.go). TestAUR548PolicyWinsOverRepoDisable still passes under this narrower precedence rule, since that test's policy explicitly declares its own quality_gates.sast.

Per-section precedence surfaced one more gap the second review caught: sastOrigin in main.go/pr.go was derived from centralCfg != nil alone -- true for ANY central policy, even one that governs other sections entirely and never declares quality_gates.sast at all. Under per-section precedence that leaves repoCfg.QualityGates.Sast as the REPOSITORY's own value, which must be labeled "repo" (and must never receive the two policy-only hardening flags above); it was being labeled "policy" instead, just because some unrelated policy happened to exist. Fixed to centralCfg != nil && centralCfg.QualityGates.Sast != nil in both files. Test: TestAUR548PolicyOriginWithoutSastSectionIsRepo.

Problem

The RFC behind this corporate-adoption office (.board/cards/backlog/AUR-549.md .. AUR-551.md's own "Origem" section) asks for multi-language SAST with Semgrep as one of four adoption gates (SAST, SBOM, Dependency-Track inventory, Sigstore/Cosign signing). AurumCode's own go vet-shaped deterministic-tool adapter (internal/analysis.Runner.Vet) already existed but was never wired into either review path (AUR-492's own "linter adapter" card, still ready, not done, at the time this card started) — there was no established pattern for "run an external security scanner over the WHOLE reviewed tree and gate on its findings" in this codebase, only "run a linter over the diff and anchor findings to changed lines" (AUR-492's own scope, explicitly a different problem: internal/analysis.Runner.Analyze's embedded catalog and .Vet are both diff-anchored; Semgrep's own report is not, by design — a SAST pass must see the resulting code, not only the lines that changed).

Design

Why not through evaluateGate

AUR-519's evaluateGate (cmd/aurumcode/policygate.go) is gated behind GateConfig.Declared() (no gate: key at all ⇒ Active=false, no-op) and, inside that, behind gate.fail_on being non-empty before its severity loop ever runs. quality_gates.sast is a separate, independently-configured gate: the RFC's own example config declares quality_gates.sast with no gate: section at all, and AC-001 requires that alone to be enough to fail the check on an ERROR-severity finding. Overloading evaluateGate's own signature to also carry a SAST threshold would have meant changing a function every one of AUR-519/520/521/537/538/543's existing tests calls directly, for a condition (quality_gates.sast) those tests know nothing about — and two sibling cards (AUR-549/550) touch cmd/aurumcode concurrently, in their own worktrees, so a wide signature change here is exactly the kind of edit that turns "additive" into "a guaranteed merge conflict".

Instead, applySASTGate (cmd/aurumcode/aur548.go) computes its own, independent decision from quality_gates.sast, Semgrep's own findings, and the Semgrep execution outcome, and folds it in place into the very same gateDecision struct evaluateGate already returned for this run. Every downstream consumer of that struct — publishPolicyGateStatus, writeComplianceArtifacts (AUR-521's audit record and SARIF document), and runReview's own exit-code section — needed zero changes to also honor a SAST breach or a SAST-inconclusive scan: from their point of view there is exactly one gate decision per run, now possibly informed by two independent, additive sources.

The adapter

internal/analysis.Runner.Semgrep (internal/analysis/semgrep.go) mirrors Runner.Vet's own injected-commandRunner seam: production code (cmd/aurumcode/aur548.go's realSemgrepRunner) execs the semgrep binary from PATH; tests inject a fake executable on PATH instead (no network, no real Semgrep). It runs semgrep scan --json --quiet --metrics=off --disable-version-check --config <pack>... . in the reviewed root and parses the report's results array into analysis.Findings, each carrying RuleID = "semgrep:" + check_id, severity mapped ERROR→error, WARNING→warning, INFO/anything else→error (an unrecognized severity from an external tool must never silently fail open to a lower rank, hence the one asymmetric default), and the file/line Semgrep itself reported. Message is Semgrep's own fixed rule message (extra.message); the matched source line (extra.lines) is never read, so a Semgrep finding can never leak reviewed source text through this adapter.

Three failure modes all become a non-nil error, never an empty finding list: the runner itself failing with nothing parseable afterward (ErrNotFound ⇒ sast_unavailable; any other runner error ⇒ sast_execution_error), invalid JSON output, and valid JSON missing the "results" key entirely ({}) — ErrSemgrepNoResults ⇒ sast_invalid_output. That last case is this card's own guarded-against defect (MUT-001): a naive implementation that read "no results key" the same as "an empty results array" would treat a truncated or wrong-shaped response as a clean, zero-finding pass.

The config

QualityGatesConfig (internal/config/sast.go) is the shared quality_gates: shape this office's four cards agree on: Sast (this card), SsorDtrack (AUR-549/550, SBOM + Dependency-Track — defined here as an empty placeholder struct only, pending those cards), SupplyChain (AUR-551, signing — likewise empty for now). Each is a pointer so an entirely absent section (nil) is distinct from one written with every field at its zero value, and *SastConfig's own methods (IsEnabled, Packs, Threshold, Validate) are nil-safe, so a nil Sast behaves exactly like a declared, disabled one everywhere it is read.

ApplyCentralPolicy (internal/config/central.go) governs QualityGates exactly like Gate/Rules/Ignore/Exceptions: under any central policy, effective.QualityGates = central.QualityGates unconditionally — a repository's own quality_gates.sast is always discarded (with a named warning when it declared one), regardless of whether the policy's own section is present. This is what makes "a repository cannot disable a policy-enabled SAST" (CR-TRUST-001) trivially true: under a policy, the repository's section never has any effect at all, in either direction.

AC-005 by construction, not just by ordering

applySASTGate is handed runSASTPass's own Semgrep-sourced issue slice directly — never result.Issues, the slice the model's parsed JSON response populates. A model reply that recites, contradicts or claims to have removed a Semgrep finding (in its summary field, the only channel available to it) has no code path by which it could reach applySASTGate's input at all. TestAUR548ModelCannotRemoveOrDowngradeFinding proves this with a fixture response that explicitly claims the finding was "removed as a false positive".

Reproducible Semgrep availability

.board/bootstrap/locks/scanners.yml already pins sast_scanner_image: docker.io/semgrep/semgrep@sha256:... at version 1.172.0. The product image installs that same pinned version so aurumcode review can exec semgrep from its own container without a second, separately-orchestrated scanner container (the reusable workflow, .github/workflows/review.yml, runs exactly one container — the AurumCode image itself — mounting the reviewed checkout read-only; there is no docker-in-docker step for a second scanner image today).

Network requirement: semgrep scan --config p/<name> downloads that named pack from Semgrep's own rule registry on every invocation. In a CI runner with outbound network, this is transparent. In a fully network-isolated runner, point quality_gates.sast.rule_packs at local rule files already present in the image or checkout instead of a registry name — the adapter passes whatever string is configured straight to --config, so a local path works identically to a registry pack name.

Acceptance scenarios

  • AC-001: a fake Semgrep returning one ERROR finding fails the gate, naming the check_id and line in the published output, the audit record and the SARIF document.
  • AC-002: a finding below fail_on_severity is produced but does not fail.
  • AC-003: Semgrep absent, erroring, or returning invalid JSON all become inconclusive, following gate.inconclusive — never a silent clean pass.
  • AC-004: rule packs come from the yml, or the RFC's own documented defaults; with no quality_gates.sast section, Semgrep is never invoked.
  • AC-005: a model reply that claims to have removed or downgraded the Semgrep finding has no effect on the gate.

Skeptical mutation

  • MUT-001: treating a Semgrep execution error as zero findings (the exact defect ErrSemgrepNoResults/the runner-error branch in internal/analysis/semgrep.go guards against) must turn AC-003's own tests red. tests/acceptance/AUR-548.sh AC-003-MUT-001 applies this mutation mechanically and checks for the red result.