chore(audit): weekly cloud audit — 4 SQL read-only bypasses + 4 scanner false-clean masquerades

internal/security/scanners.go

@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"log"
 	"os/exec"
 	"path/filepath"
@@ -179,10 +180,21 @@ func parseGosec(out []byte, repoDir string) ([]Finding, error) {
 				ID string `json:"id"`
 			} `json:"cwe"`
 		} `json:"Issues"`
+		// gosec reports compile/package-load failures in "Golang errors"
+		// (filename → errors) while still emitting valid JSON with an empty
+		// Issues list. Decoding only Issues would return a clean empty scan
+		// for a run that could not actually analyse the code — the same
+		// masquerade the other parsers guard against. Fail closed only when
+		// no issues were produced, so a scan that found issues is never
+		// flipped to failed by a per-file parse error alongside real results.
+		GolangErrors map[string]json.RawMessage `json:"Golang errors"`
 	}
 	if err := json.Unmarshal(out, &doc); err != nil {
 		return nil, err
 	}
+	if len(doc.Issues) == 0 && len(doc.GolangErrors) > 0 {
+		return nil, fmt.Errorf("gosec reported %d package-load error(s) and no issues; scan coverage lost", len(doc.GolangErrors))
+	}
 	findings := make([]Finding, 0, len(doc.Issues))
 	for _, i := range doc.Issues {
 		line, _ := strconv.Atoi(strings.SplitN(i.Line, "-", 2)[0]) // gosec may emit "12-14"
@@ -248,10 +260,36 @@ func parseSemgrep(out []byte, repoDir string) ([]Finding, error) {
 				} `json:"metadata"`
 			} `json:"extra"`
 		} `json:"results"`
+		// semgrep always emits a top-level "errors" array alongside "results".
+		// On a scan-level failure (e.g. `--config auto` cannot load rules on an
+		// offline dispatch host) it emits valid JSON with results:[] and an
+		// error-level entry, and exits non-zero. Decoding only results would
+		// return ([], nil) — counted as a clean `ran` scan rather than `failed`,
+		// hiding total coverage loss from the gate and require_scanners strict
+		// mode. Inspect errors so that masquerade is surfaced.
+		Errors []struct {
+			Level   string `json:"level"`
+			Message string `json:"message"`
+		} `json:"errors"`
 	}
 	if err := json.Unmarshal(out, &doc); err != nil {
 		return nil, err
 	}
+	// Fail closed only when no results were produced AND a fatal error is
+	// present: a scan that returned findings clearly wasn't a total loss, so
+	// non-fatal per-file warnings that coexist with results never flip a
+	// successful scan to failed.
+	if len(doc.Results) == 0 {
+		for _, e := range doc.Errors {
+			if strings.EqualFold(e.Level, "error") {
+				msg := e.Message
+				if msg == "" {
+					msg = "semgrep reported a scan error with no results"
+				}
+				return nil, fmt.Errorf("semgrep scan error: %s", msg)
+			}
+		}
+	}
 	findings := make([]Finding, 0, len(doc.Results))
 	for _, r := range doc.Results {
 		cwe := ""
@@ -279,6 +317,18 @@ func parseSemgrep(out []byte, repoDir string) ([]Finding, error) {
 
 func parseNpmAudit(out []byte) ([]Finding, error) {
 	var doc struct {
+		// npm emits a valid-JSON error envelope (e.g. {"error":{"code":
+		// "ENOLOCK",...}}) when the audit cannot actually run — most commonly
+		// a package.json with no lockfile. That envelope unmarshals cleanly
+		// into a nil Vulnerabilities map, so without inspecting Error the scan
+		// would look like a clean pass and be counted in RunScanners' `ran`
+		// list — exactly the "failed scan masquerading as clean" that the
+		// `failed` list exists to prevent (and that require_scanners strict
+		// mode relies on). Surface it as an error so coverage loss is visible.
+		Error *struct {
+			Code    string `json:"code"`
+			Summary string `json:"summary"`
+		} `json:"error"`
 		Vulnerabilities map[string]struct {
 			Name     string `json:"name"`
 			Severity string `json:"severity"`
@@ -289,6 +339,16 @@ func parseNpmAudit(out []byte) ([]Finding, error) {
 	if err := json.Unmarshal(out, &doc); err != nil {
 		return nil, err
 	}
+	if doc.Error != nil {
+		msg := doc.Error.Summary
+		if msg == "" {
+			msg = doc.Error.Code
+		}
+		if msg == "" {
+			msg = "npm audit reported an error with no detail"
+		}
+		return nil, fmt.Errorf("npm audit error: %s", msg)
+	}
 	findings := make([]Finding, 0, len(doc.Vulnerabilities))
 	for pkg, v := range doc.Vulnerabilities {
 		name := v.Name
@@ -311,10 +371,21 @@ func parseNpmAudit(out []byte) ([]Finding, error) {
 
 func parseGovulncheck(out []byte) ([]Finding, error) {
 	var findings []Finding
+	// govulncheck runs in text mode, so — unlike the JSON scanners — a failure
+	// produces no unmarshal error to route it to `failed`. Its fatal errors
+	// print as "govulncheck: <msg>" (e.g. the vulnerability DB is unreachable
+	// on an offline dispatch host). Without detecting that, a scan that never
+	// ran returns zero findings and is counted as a clean `ran` scan, hiding
+	// total coverage loss from the gate and require_scanners strict mode — the
+	// same masquerade the JSON parsers guard against.
+	var scanErr string
 	sc := bufio.NewScanner(bytes.NewReader(out))
 	sc.Buffer(make([]byte, 0, 64*1024), 1024*1024)
 	for sc.Scan() {
 		line := strings.TrimSpace(sc.Text())
+		if strings.HasPrefix(line, "govulncheck:") && scanErr == "" {
+			scanErr = strings.TrimSpace(strings.TrimPrefix(line, "govulncheck:"))
+		}
 		// Lines look like: "Vulnerability #1: GO-2024-1234"
 		if !strings.HasPrefix(line, "Vulnerability #") {
 			continue
@@ -338,7 +409,15 @@ func parseGovulncheck(out []byte) ([]Finding, error) {
 			Source:   "scanner",
 		})
 	}
-	return findings, sc.Err()
+	if err := sc.Err(); err != nil {
+		return nil, err
+	}
+	// A fatal error with no vulnerability findings means the scan did not run
+	// — surface it as a failure rather than a clean empty result.
+	if len(findings) == 0 && scanErr != "" {
+		return nil, fmt.Errorf("govulncheck error: %s", scanErr)
+	}
+	return findings, nil
 }
 
 // Run executes the scanner against repoDir and returns parsed findings. A

internal/security/scanners_test.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
+	"strings"
 	"testing"
 )
 
@@ -99,6 +100,32 @@ func TestParseGosec(t *testing.T) {
 	}
 }
 
+// TestParseGosec_PackageLoadErrorSurfaced pins that a gosec run that could not
+// analyse the code (compile/package-load failure → valid JSON with empty
+// Issues but a populated "Golang errors" map) is surfaced as a failure rather
+// than swallowed as a clean, zero-issue scan. A scan that produced issues
+// alongside a per-file error is NOT flipped to failed.
+func TestParseGosec_PackageLoadErrorSurfaced(t *testing.T) {
+	failed := []byte(`{"Issues":[],"Golang errors":{"/repo/broken.go":[{"line":1,"column":1,"error":"expected declaration"}]},"Stats":{"files":0,"lines":0}}`)
+	got, err := parseGosec(failed, "/repo")
+	if err == nil {
+		t.Fatalf("parseGosec = nil error, want package-load failure surfaced; findings=%+v", got)
+	}
+	if !strings.Contains(err.Error(), "coverage lost") {
+		t.Errorf("error %q does not signal coverage loss", err.Error())
+	}
+
+	// Issues present alongside a Golang error → still a successful scan.
+	withResults := []byte(`{"Issues":[{"severity":"HIGH","cwe":{"id":"798"},"rule_id":"G101","details":"d","file":"/repo/a.go","line":"1"}],"Golang errors":{"/repo/b.go":[{"line":1,"column":1,"error":"x"}]}}`)
+	fs, err := parseGosec(withResults, "/repo")
+	if err != nil {
+		t.Fatalf("parseGosec with issues + error = %v, want nil", err)
+	}
+	if len(fs) != 1 {
+		t.Fatalf("expected 1 finding, got %d", len(fs))
+	}
+}
+
 func TestParseGitleaks(t *testing.T) {
 	out := []byte(`[
 	  {"Description":"AWS Access Key","File":"config/prod.env","StartLine":3,"RuleID":"aws-access-token","Secret":"AKIAXXXXXXXX","Match":"AKIA..."},
@@ -142,6 +169,33 @@ func TestParseSemgrep(t *testing.T) {
 	}
 }
 
+// TestParseSemgrep_ScanErrorEnvelope pins that a scan-level failure (valid
+// JSON, empty results, an error-level entry — e.g. rules failed to load on an
+// offline host) is surfaced as a failure, not swallowed as a clean empty scan.
+// Otherwise RunScanners counts it in `ran` (clean) instead of `failed`
+// (coverage lost), and require_scanners strict mode would not block.
+func TestParseSemgrep_ScanErrorEnvelope(t *testing.T) {
+	out := []byte(`{"results":[],"errors":[{"level":"error","message":"unable to load config: registry unreachable"}]}`)
+	got, err := parseSemgrep(out, "/repo")
+	if err == nil {
+		t.Fatalf("parseSemgrep = nil error, want scan failure surfaced; findings=%+v", got)
+	}
+	if !strings.Contains(err.Error(), "unable to load config") {
+		t.Errorf("error %q does not name the scan error", err.Error())
+	}
+
+	// A scan that produced findings is NOT flipped to failed by a coexisting
+	// non-fatal (warn-level) error entry — coverage clearly wasn't lost.
+	okOut := []byte(`{"results":[{"check_id":"r","path":"/repo/x.go","start":{"line":1},"extra":{"message":"m","severity":"ERROR","metadata":{}}}],"errors":[{"level":"warn","message":"partial parse"}]}`)
+	fs, err := parseSemgrep(okOut, "/repo")
+	if err != nil {
+		t.Fatalf("parseSemgrep with results + warn = %v, want nil", err)
+	}
+	if len(fs) != 1 {
+		t.Fatalf("expected 1 finding, got %d", len(fs))
+	}
+}
+
 func TestParseNpmAudit(t *testing.T) {
 	out := []byte(`{
 	  "vulnerabilities": {
@@ -176,6 +230,45 @@ func TestParseNpmAudit(t *testing.T) {
 	}
 }
 
+// TestParseNpmAudit_ErrorEnvelope pins that npm's valid-JSON error envelope
+// (emitted when the audit cannot run — e.g. no lockfile) is surfaced as an
+// error rather than swallowed as a clean, empty scan. Otherwise RunScanners
+// counts it in `ran` (clean coverage) instead of `failed` (coverage lost),
+// letting a dependency-CVE scan silently no-op while the security gate — and
+// require_scanners strict mode — believe the tool ran.
+func TestParseNpmAudit_ErrorEnvelope(t *testing.T) {
+	cases := []struct {
+		name string
+		out  string
+		want string
+	}{
+		{
+			name: "ENOLOCK no lockfile",
+			out:  `{"error":{"code":"ENOLOCK","summary":"This command requires an existing lockfile."}}`,
+			want: "This command requires an existing lockfile",
+		},
+		{
+			name: "error with only code",
+			out:  `{"error":{"code":"EAUDITNOPJSON"}}`,
+			want: "EAUDITNOPJSON",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parseNpmAudit([]byte(tc.out))
+			if err == nil {
+				t.Fatalf("parseNpmAudit(%s) = nil error, want failure surfaced; findings=%+v", tc.out, got)
+			}
+			if got != nil {
+				t.Errorf("expected no findings on error envelope, got %+v", got)
+			}
+			if !strings.Contains(err.Error(), tc.want) {
+				t.Errorf("error %q does not mention %q", err.Error(), tc.want)
+			}
+		})
+	}
+}
+
 func TestParseGovulncheck(t *testing.T) {
 	// govulncheck text output (the human format): we extract called vulns.
 	out := []byte(`=== Symbol Results ===
@@ -205,3 +298,29 @@ Vulnerability #2: GO-2023-5678
 		t.Errorf("dependency CVE should be high, got %v", got[0].Severity)
 	}
 }
+
+// TestParseGovulncheck_FatalErrorSurfaced pins that a govulncheck run that
+// could not execute (e.g. the vulnerability DB is unreachable on an offline
+// host) is surfaced as a failure, not swallowed as a clean, zero-finding scan.
+// Text mode has no unmarshal error to route the failure to `failed`, so the
+// "govulncheck: <msg>" fatal line is the signal.
+func TestParseGovulncheck_FatalErrorSurfaced(t *testing.T) {
+	out := []byte("govulncheck: loading vulnerability database: Get \"https://vuln.go.dev/index/db.json\": dial tcp: lookup vuln.go.dev: no such host\n")
+	got, err := parseGovulncheck(out)
+	if err == nil {
+		t.Fatalf("parseGovulncheck = nil error, want fatal surfaced; findings=%+v", got)
+	}
+	if !strings.Contains(err.Error(), "vulnerability database") {
+		t.Errorf("error %q does not name the fatal cause", err.Error())
+	}
+
+	// A clean run (no vulns, no fatal line) is NOT flipped to an error.
+	clean := []byte("=== Symbol Results ===\n\nNo vulnerabilities found.\n")
+	fs, err := parseGovulncheck(clean)
+	if err != nil {
+		t.Fatalf("parseGovulncheck(clean) = %v, want nil", err)
+	}
+	if len(fs) != 0 {
+		t.Errorf("expected 0 findings on a clean scan, got %d", len(fs))
+	}
+}