From 720be38a7e688ea874803494a73fd9c4f2dc6b1e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Jul 2026 04:33:03 +0000 Subject: [PATCH] =?UTF-8?q?chore(audit):=20weekly=20cloud=20audit=20?= =?UTF-8?q?=E2=80=94=20fix=204=20fail-open=20/=20data-loss=20defects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Weekly security/quality audit. Four verified defects fixed, each with a regression test; three consecutive audit rounds (the last dry). 1. security/scanners.go · parseGovulncheck — fail-open dependency scan. Text-mode govulncheck emits no "Vulnerability #" line on a network/build/ timeout error, so a failed scan was indistinguishable from a clean one and routed to `ran` (clean) instead of `failed` — defeating even require_scanners. Switched to `govulncheck -json` with exit-code + config- handshake detection so a failed run is reported failed. 2. web/auth.go · RequireToken — fail-open auth. RequireToken("") set AllowUnauthenticated, silently serving an unauthenticated dashboard, contradicting its own doc, NewAuthMiddleware's panic contract, and CLAUDE.md. Now inherits the panic-on-empty-token guarantee. 3. engine/conflict_resolver.go · handleBinaryConflict — silent data loss. Small binary conflicts used `git checkout --ours`, but during a rebase --ours is the base, so the story's binary asset change was discarded whenever the base also touched the file. Changed to --theirs (story side), matching the text/ JSON deterministic paths. 4. engine/verification_loop.go · checkTests/parseGoTestJSON — completion gate fail-open. A Go test binary that fails to COMPILE emits only package-level events (empty Test) and go build ignores test files, so a non-compiling suite parsed as (0,0,0)=clean → REQ_COMPLETED on cross-story drift. Now counts package build-fail events and treats a non-zero runner exit with no per-test failure as red. Docs: corrected the CLAUDE.md binary-policy line and conflict-resolver comments to match the fixed --theirs semantics. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LFb5kxYoibXzSz3YGnmJrU --- CLAUDE.md | 2 +- internal/engine/conflict_resolver.go | 26 ++-- .../engine/conflict_resolver_extended_test.go | 78 ++++++++++++ internal/engine/verification_loop.go | 69 +++++++--- internal/engine/verification_loop_test.go | 52 +++++++- internal/engine/wiring_test.go | 2 +- internal/security/coverage_gaps_test.go | 19 ++- internal/security/scanners.go | 120 +++++++++++++++--- internal/security/scanners_exec_test.go | 46 ++++++- internal/security/scanners_test.go | 58 ++++++--- internal/web/auth.go | 13 +- internal/web/auth_test.go | 15 +++ 12 files changed, 411 insertions(+), 89 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a0ea9dd..1d3ef34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,7 +83,7 @@ Tier 4: Pause (human intervention required) `internal/engine/conflict_resolver.go` uses a 3-tier strategy during rebase: 1. **Binary detection** (`internal/git/conflict.go`): `IsBinaryConflict` runs `git diff --numstat HEAD -- `; if the output is `-\t-\t`, the file is binary. `SniffBinary` is the fallback (null-byte check in first 8 KB) for newly-added unmerged files not yet in HEAD. Binary files are NEVER sent to an LLM. -2. **Binary policy**: Compiled/oversized files (`server`, `main`, `*.exe`, >500 KB) are removed via `git rm` and emit `STORY_CONFLICT_BINARY_REMOVED`. Smaller binaries are resolved with `git checkout --ours` (story branch version wins) and emit `STORY_CONFLICT_BINARY`. +2. **Binary policy**: Compiled/oversized files (`server`, `main`, `*.exe`, >500 KB) are removed via `git rm` and emit `STORY_CONFLICT_BINARY_REMOVED`. Smaller binaries are resolved with `git checkout --theirs` (story branch version wins — during a rebase `--theirs` is the replayed story commit, `--ours` is the base) and emit `STORY_CONFLICT_BINARY`. (Previously `--ours`, which during a rebase is the base side — that silently discarded the story's binary change when the base also touched the file.) 3. **Senior fast-path**: Text conflicts are sent to the Senior model. If the resolved content still contains `<<<<<<<` markers, the result is discarded and the Tech Lead is tried. 4. **Tech Lead escalation**: Triggered when (a) the Senior fails, (b) resolved content still has conflict markers, or (c) the conflict spans >3 files (integration-level). The Tech Lead prompt includes the requirement title/text, story acceptance criteria, `depends_on` story titles, sibling story titles, and the last 3 `git log` subjects for the file. Emits `STORY_CONFLICT_ESCALATED`. diff --git a/internal/engine/conflict_resolver.go b/internal/engine/conflict_resolver.go index ad30e29..0736cf2 100644 --- a/internal/engine/conflict_resolver.go +++ b/internal/engine/conflict_resolver.go @@ -151,8 +151,11 @@ func (cr *ConflictResolver) RebaseWithResolution(ctx context.Context, storyID, w continue } - // Generated lock-file check: resolve deterministically (story branch - // version via --ours, then staged by the bulk StageFiles below). + // Generated lock-file check: resolve deterministically by keeping the + // base side (--ours, which during a rebase is the upstream/base), then + // staged by the bulk StageFiles below. Which side wins is immaterial + // here — a lock conflict means both sides diverged from the merged + // manifest, so the post-merge build regenerates the lock regardless. // Lock files like package-lock.json are huge and machine-generated — // sending them to the LLM blows the pipeline timeout for no benefit; // the post-merge build/QA validates dependencies. This was the root @@ -463,7 +466,8 @@ func deepMergeJSON(ours, theirs any) any { // handleBinaryConflict applies a deterministic policy for binary-file conflicts // without invoking the LLM: // - Oversized (>500 KB) or compiled binary names (server, main, *.exe) → git rm -// - All others → git checkout --ours (story branch version wins) +// - All others → git checkout --theirs (story branch version wins; during a +// rebase --theirs is the replayed story commit, --ours is the base) // generatedLockFiles are package-manager lock files that are machine-generated // and must never be LLM-resolved — they are large and deterministic, so a // conflict is resolved by taking one side and letting the build regenerate. @@ -503,15 +507,21 @@ func (cr *ConflictResolver) handleBinaryConflict(storyID, worktreePath, absPath, return nil } - // Take the story branch version (--ours during rebase = the branch being rebased). - log.Printf("[conflict-resolver] taking --ours for binary %s in %s", file, storyID) - cmd := exec.Command("git", "checkout", "--ours", "--", file) + // Keep the story branch version. During `git rebase ` the story + // commit is replayed ONTO the base, so the conflict sides are inverted + // relative to a merge: --ours = base/upstream, --theirs = the story commit + // being replayed (pinned by git.TestConflictSides_OursIsBaseTheirsIsStory, + // and matching the text/JSON deterministic paths, which all keep theirs). + // Using --ours here silently discarded the story's binary asset change + // whenever the base also touched the file — real data loss. + log.Printf("[conflict-resolver] taking --theirs (story branch version) for binary %s in %s", file, storyID) + cmd := exec.Command("git", "checkout", "--theirs", "--", file) cmd.Dir = worktreePath if out, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("git checkout --ours %s: %w (%s)", file, err, strings.TrimSpace(string(out))) + return fmt.Errorf("git checkout --theirs %s: %w (%s)", file, err, strings.TrimSpace(string(out))) } cr.emitBinaryEvent(storyID, file, state.EventStoryConflictBinary, - "binary conflict: took --ours (story branch version)") + "binary conflict: took --theirs (story branch version)") return nil } diff --git a/internal/engine/conflict_resolver_extended_test.go b/internal/engine/conflict_resolver_extended_test.go index 0305257..37b35a9 100644 --- a/internal/engine/conflict_resolver_extended_test.go +++ b/internal/engine/conflict_resolver_extended_test.go @@ -1,6 +1,7 @@ package engine import ( + "bytes" "context" "fmt" "os" @@ -665,3 +666,80 @@ func TestBinaryConflict_NoLLMCall(t *testing.T) { t.Error("expected STORY_CONFLICT_BINARY or STORY_CONFLICT_BINARY_REMOVED event to be emitted") } } + +// TestBinaryConflict_SmallBinaryKeepsStoryVersion pins the data-loss regression: +// a small (kept) binary asset changed by BOTH the story and the base must resolve +// to the STORY's version. During a rebase the sides are inverted (--ours=base, +// --theirs=story), so the previous `git checkout --ours` silently reverted the +// story's asset change to the base version and the requirement merged with the +// story's work lost. The resolver must take --theirs. +func TestBinaryConflict_SmallBinaryKeepsStoryVersion(t *testing.T) { + bareDir := filepath.Join(t.TempDir(), "remote.git") + if err := exec.Command("git", "init", "--bare", bareDir).Run(); err != nil { + t.Fatalf("init bare: %v", err) + } + cloneDir := filepath.Join(t.TempDir(), "clone") + if err := exec.Command("git", "clone", bareDir, cloneDir).Run(); err != nil { + t.Fatalf("clone: %v", err) + } + runGit := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = cloneDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v (%s)", args, err, out) + } + } + runGit("config", "user.email", "test@test.com") + runGit("config", "user.name", "Test") + + // A small binary asset (null bytes => detected binary) with a non-compiled + // name, so it takes the kept-small-binary path, not the git-rm removal path. + asset := filepath.Join(cloneDir, "logo.png") + base := []byte{0x89, 'P', 'N', 'G', 0x00, 0x01, 0x02} + if err := os.WriteFile(asset, base, 0o644); err != nil { + t.Fatal(err) + } + runGit("add", ".") + runGit("commit", "-m", "init") + runGit("push", "origin", "main") + + // Main modifies the asset. + if err := os.WriteFile(asset, append(append([]byte{}, base...), 0xFF), 0o644); err != nil { + t.Fatal(err) + } + runGit("add", ".") + runGit("commit", "-m", "main updates logo") + runGit("push", "origin", "main") + + // Story branch (from before main's change) makes a DIFFERENT change. + storyVersion := append(append([]byte{}, base...), 0xAA, 0xBB) + runGit("checkout", "-b", "feature", "HEAD~1") + if err := os.WriteFile(asset, storyVersion, 0o644); err != nil { + t.Fatal(err) + } + runGit("add", ".") + runGit("commit", "-m", "story updates logo") + runGit("fetch", "origin", "main") + + senior := llm.NewReplayClient() // zero responses — errors if called + techLead := llm.NewReplayClient() // binary must never reach an LLM + es := newEventStoreForTest(t) + cr := NewConflictResolver(senior, "senior-model", techLead, "tl-model", 4096, nil, es) + + if err := cr.RebaseWithResolution(context.Background(), "s-binary-keep", cloneDir, "origin/main"); err != nil { + t.Fatalf("RebaseWithResolution: %v", err) + } + if senior.CallCount() > 0 || techLead.CallCount() > 0 { + t.Errorf("binary conflict must not reach an LLM (senior=%d, tl=%d)", senior.CallCount(), techLead.CallCount()) + } + + got, err := os.ReadFile(asset) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, storyVersion) { + t.Errorf("small binary conflict must keep the STORY version %v, got %v (base was %v+0xFF) — story asset change was lost", + storyVersion, got, base) + } +} diff --git a/internal/engine/verification_loop.go b/internal/engine/verification_loop.go index d2ec68f..a552120 100644 --- a/internal/engine/verification_loop.go +++ b/internal/engine/verification_loop.go @@ -154,35 +154,48 @@ func checkTests(repoDir string) (passing, failing, total int) { } cmd.Dir = repoDir - out, _ := cmd.CombinedOutput() + out, runErr := cmd.CombinedOutput() output := string(out) if fileExists(filepath.Join(repoDir, "go.mod")) { - return parseGoTestJSON(output) - } - - // Parse test results (simplified — count PASS/FAIL lines) - for _, line := range strings.Split(output, "\n") { - if strings.Contains(line, "\"numPassedTests\"") || strings.Contains(line, "PASS:") { - passing++ - } - if strings.Contains(line, "\"numFailedTests\"") || strings.Contains(line, "FAIL:") { - failing++ + passing, failing, total = parseGoTestJSON(output) + } else { + // Parse test results (simplified — count PASS/FAIL lines) + for _, line := range strings.Split(output, "\n") { + if strings.Contains(line, "\"numPassedTests\"") || strings.Contains(line, "PASS:") { + passing++ + } + if strings.Contains(line, "\"numFailedTests\"") || strings.Contains(line, "FAIL:") { + failing++ + } } - } - // Fallback: parse Jest summary line - if strings.Contains(output, "Tests:") { - for _, line := range strings.Split(output, "\n") { - if strings.Contains(line, "Tests:") && strings.Contains(line, "passed") { - // Parse "Tests: X failed, Y passed, Z total" - _, _ = fmt.Sscanf(line, "Tests: %d failed, %d passed, %d total", &failing, &passing, &total) // partial parse keeps zero counters - break + // Fallback: parse Jest summary line + if strings.Contains(output, "Tests:") { + for _, line := range strings.Split(output, "\n") { + if strings.Contains(line, "Tests:") && strings.Contains(line, "passed") { + // Parse "Tests: X failed, Y passed, Z total" + _, _ = fmt.Sscanf(line, "Tests: %d failed, %d passed, %d total", &failing, &passing, &total) // partial parse keeps zero counters + break + } } } + total = passing + failing + } + + // Fail closed: a non-zero test-runner exit with no attributable per-test + // failure means the suite could not be EVALUATED — a compile error (test + // files aren't built by `go build ./...`), a panic before any test ran, a + // vet failure, or the runner (jest/vitest) failing to launch. The per-test + // parsers see nothing to count and would otherwise report (0 failing) = + // green, so the completion gate would emit REQ_COMPLETED on a suite that + // never ran. Treat it as a failure so the gate blocks instead. + if runErr != nil && failing == 0 { + log.Printf("[verify] test runner exited non-zero (%v) with 0 per-test failures — treating as compile/infra failure, not a clean run", runErr) + failing = 1 + total = passing + failing } - total = passing + failing log.Printf("[verify] tests: %d passing, %d failing, %d total", passing, failing, total) return passing, failing, total } @@ -197,7 +210,21 @@ func parseGoTestJSON(output string) (passing, failing, total int) { Action string `json:"Action"` Test string `json:"Test"` } - if err := json.Unmarshal([]byte(line), &evt); err != nil || evt.Test == "" { + if err := json.Unmarshal([]byte(line), &evt); err != nil { + continue + } + // Package-level events have an empty Test. Almost all are ignorable + // (start/output/pass), but "build-fail" is critical: `go test -json` + // emits it when a test binary fails to COMPILE — e.g. a merged story's + // _test.go references a symbol another story renamed or removed. Because + // `go build ./...` (checkBuild) does NOT compile test files, this is the + // ONLY signal that cross-story drift broke the test suite. Counting it as + // a failure is what stops the completion gate from reporting a suite that + // does not even compile as green. + if evt.Test == "" { + if evt.Action == "build-fail" { + failing++ + } continue } switch evt.Action { diff --git a/internal/engine/verification_loop_test.go b/internal/engine/verification_loop_test.go index 936b985..3ed914b 100644 --- a/internal/engine/verification_loop_test.go +++ b/internal/engine/verification_loop_test.go @@ -1,6 +1,11 @@ package engine -import "testing" +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) func TestShouldRunFixCycle_WhenTestsFail(t *testing.T) { result := VerificationResult{ @@ -22,3 +27,48 @@ func TestParseGoTestJSONCountsIndividualTests(t *testing.T) { t.Fatalf("expected 1 pass, 1 fail, 2 total; got pass=%d fail=%d total=%d", passing, failing, total) } } + +// TestParseGoTestJSON_BuildFailIsCounted pins the fail-open fix: a test binary +// that fails to COMPILE emits a package-level "build-fail" (empty Test) and no +// per-test events. Previously all such events were skipped, so a non-compiling +// suite parsed as (0,0,0) = clean. It must now count as failing. +func TestParseGoTestJSON_BuildFailIsCounted(t *testing.T) { + output := `{"ImportPath":"pkg [pkg.test]","Action":"build-output","Output":"# pkg\n"} +{"ImportPath":"pkg [pkg.test]","Action":"build-output","Output":"./x_test.go:4:8: undefined: Foo\n"} +{"ImportPath":"pkg [pkg.test]","Action":"build-fail"} +{"Action":"fail","Package":"pkg","FailedBuild":"pkg [pkg.test]"}` + _, failing, _ := parseGoTestJSON(output) + if failing == 0 { + t.Fatalf("a package build-fail must count as failing, got failing=0 (fail-open)") + } +} + +// TestCheckTests_BrokenTestFileIsNotClean is the end-to-end regression for the +// completion-gate fail-open. Production code builds (go build ./... ignores test +// files), but a _test.go references an undefined symbol — exactly the cross-story +// drift the completion gate exists to catch. checkTests must report failing > 0 +// so ShouldRunFixCycle blocks REQ_COMPLETED instead of reporting green. +func TestCheckTests_BrokenTestFileIsNotClean(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go toolchain not on PATH") + } + dir := t.TempDir() + write := func(name, content string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + write("go.mod", "module example.com/x\n\ngo 1.26\n") + write("main.go", "package x\n\n// Add is real production code and compiles fine.\nfunc Add(a, b int) int { return a + b }\n") + // Broken test: references a symbol that does not exist (cross-story drift). + write("x_test.go", "package x\n\nimport \"testing\"\n\nfunc TestAdd(t *testing.T) {\n\t_ = NonexistentSymbol\n}\n") + + if !checkBuild(dir) { + t.Fatalf("go build ./... must pass — it does not compile test files") + } + _, failing, _ := checkTests(dir) + if failing == 0 { + t.Fatalf("a test file that fails to compile must count as failing, got failing=0 (fail-open: REQ_COMPLETED on an un-compilable suite)") + } +} diff --git a/internal/engine/wiring_test.go b/internal/engine/wiring_test.go index b033b22..42a67ea 100644 --- a/internal/engine/wiring_test.go +++ b/internal/engine/wiring_test.go @@ -2309,7 +2309,7 @@ func TestWiring_StoryConflictBinary_ProjectsWithoutError(t *testing.T) { evt := state.NewEvent(state.EventStoryConflictBinary, "conflict-resolver", "s-wiring-bin-1", map[string]any{ "file": "server", - "reason": "binary conflict: took --ours (story branch version)", + "reason": "binary conflict: took --theirs (story branch version)", }) if err := store.Project(evt); err != nil { diff --git a/internal/security/coverage_gaps_test.go b/internal/security/coverage_gaps_test.go index 2043e90..5ab071c 100644 --- a/internal/security/coverage_gaps_test.go +++ b/internal/security/coverage_gaps_test.go @@ -193,14 +193,23 @@ func TestParseNpmAudit_FallsBackToMapKeyForName(t *testing.T) { } } -func TestParseGovulncheck_MalformedLinesSkipped(t *testing.T) { - out := []byte("Vulnerability #1 without colon\nVulnerability #2: \nVulnerability #3: GO-2025-999\n") - fs, err := parseGovulncheck(out) +func TestParseGovulncheckJSON_ImportOnlyFindingsNotCalled(t *testing.T) { + // Module- and package-level findings (no function in the top trace frame) are + // import-only and must NOT be reported; only the symbol-level (called) finding + // counts. A clean handshake (config) with no called vuln is a clean scan. + out := []byte(`{"config":{"scanner_name":"govulncheck"}} +{"finding":{"osv":"GO-2025-111","trace":[{"module":"example.com/dep","package":"example.com/dep"}]}} +{"finding":{"osv":"GO-2025-111","trace":[{"module":"example.com/dep","package":"example.com/dep","function":"Vulnerable"}]}} +{"finding":{"osv":"GO-2025-222","trace":[{"module":"example.com/other","package":"example.com/other"}]}}`) + fs, sawConfig, err := parseGovulncheckJSON(out) if err != nil { t.Fatal(err) } - if len(fs) != 1 || fs[0].RuleID != "GO-2025-999" { - t.Errorf("malformed lines must be skipped, valid ones kept: %+v", fs) + if !sawConfig { + t.Error("config handshake must be detected") + } + if len(fs) != 1 || fs[0].RuleID != "GO-2025-111" { + t.Errorf("only the called (symbol-level) vuln must be reported: %+v", fs) } } diff --git a/internal/security/scanners.go b/internal/security/scanners.go index dab6f38..694d731 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -1,13 +1,16 @@ package security import ( - "bufio" "bytes" "context" "encoding/json" + "errors" + "fmt" + "io" "log" "os/exec" "path/filepath" + "sort" "strconv" "strings" "time" @@ -309,24 +312,53 @@ func parseNpmAudit(out []byte) ([]Finding, error) { return findings, nil } -func parseGovulncheck(out []byte) ([]Finding, error) { - var findings []Finding - sc := bufio.NewScanner(bytes.NewReader(out)) - sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) - for sc.Scan() { - line := strings.TrimSpace(sc.Text()) - // Lines look like: "Vulnerability #1: GO-2024-1234" - if !strings.HasPrefix(line, "Vulnerability #") { - continue +// parseGovulncheckJSON parses the `govulncheck -json` message stream (a sequence +// of concatenated JSON objects) and extracts CALLED vulnerabilities — those with +// a symbol-level trace frame (trace[0].function set). Unlike govulncheck's text +// mode, the JSON stream lets us positively tell "ran clean" from "failed to run": +// a successful run always emits a leading `config` handshake message. sawConfig +// reports whether that handshake was seen; a decode error on a non-EOF token +// (truncated/garbage output, e.g. a plain-text fatal error appended after a +// network failure) is returned so the caller can route the scan to `failed` +// instead of reporting a broken dependency scan as clean. +func parseGovulncheckJSON(out []byte) (findings []Finding, sawConfig bool, err error) { + type traceFrame struct { + Function string `json:"function"` + } + var msg struct { + Config json.RawMessage `json:"config"` + Finding *struct { + OSV string `json:"osv"` + Trace []traceFrame `json:"trace"` + } `json:"finding"` + } + called := map[string]bool{} + dec := json.NewDecoder(bytes.NewReader(out)) + for { + msg.Config = nil + msg.Finding = nil + if e := dec.Decode(&msg); e != nil { + if errors.Is(e, io.EOF) { + break + } + return nil, sawConfig, e } - idx := strings.LastIndex(line, ":") - if idx < 0 { - continue + if len(msg.Config) > 0 { + sawConfig = true } - id := strings.TrimSpace(line[idx+1:]) - if id == "" { - continue + // govulncheck emits one finding per OSV per trace granularity + // (module, package, symbol). The symbol-level finding — the only one + // with a function in its top trace frame — is the "called" signal. + if f := msg.Finding; f != nil && f.OSV != "" && len(f.Trace) > 0 && f.Trace[0].Function != "" { + called[f.OSV] = true } + } + ids := make([]string, 0, len(called)) + for id := range called { + ids = append(ids, id) + } + sort.Strings(ids) + for _, id := range ids { findings = append(findings, Finding{ Tool: "govulncheck", RuleID: id, @@ -338,7 +370,51 @@ func parseGovulncheck(out []byte) ([]Finding, error) { Source: "scanner", }) } - return findings, sc.Err() + return findings, sawConfig, nil +} + +// runGovulncheck runs govulncheck in JSON mode and, crucially, treats a failed +// run as a failure rather than a clean scan. The four JSON scanners (gosec, +// gitleaks, semgrep, npm-audit) get failure detection for free — a crash emits +// non-JSON that fails json.Unmarshal, routing them to `failed`. govulncheck's +// text mode had no such signal: a network/build/timeout error simply produced +// output with no "Vulnerability #" line, indistinguishable from a clean scan, so +// a dependency scan that never ran was reported as scanned-clean (defeating even +// `require_scanners: true`, which only inspects skipped/failed). In -json mode +// govulncheck exits non-zero ONLY on a tool error — vulnerabilities found do NOT +// set a non-zero code — and always emits a `config` handshake on a real run, so +// both a non-zero exit and a missing/garbled stream are surfaced as errors. +func runGovulncheck(ctx context.Context, repoDir string) ([]Finding, error) { + ctx, cancel := context.WithTimeout(ctx, scannerTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "govulncheck", "-json", "./...") + cmd.Dir = repoDir + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + runErr := cmd.Run() + + findings, sawConfig, perr := parseGovulncheckJSON(stdout.Bytes()) + if perr != nil { + return nil, fmt.Errorf("govulncheck: unparseable JSON output (scan did not complete): %w", perr) + } + if runErr != nil { + return nil, fmt.Errorf("govulncheck: %w: %s", runErr, strings.TrimSpace(tailString(stderr.String(), 300))) + } + if !sawConfig { + return nil, fmt.Errorf("govulncheck: no config message on stdout (scan did not run)") + } + return findings, nil +} + +// tailString returns the last n bytes of s (used to bound scanner stderr in +// error messages so a large trace can't flood logs). +func tailString(s string, n int) string { + if len(s) <= n { + return s + } + return s[len(s)-n:] } // Run executes the scanner against repoDir and returns parsed findings. A @@ -347,6 +423,12 @@ func parseGovulncheck(out []byte) ([]Finding, error) { // caller can log and continue (graceful degradation — one tool failing never // aborts the scan). func (s Scanner) Run(ctx context.Context, repoDir string) ([]Finding, error) { + // govulncheck runs in JSON mode with its own exit-code handling so a failed + // run is reported as failed, never as a clean scan (see runGovulncheck). + if s.Kind == ScannerGovulncheck { + return runGovulncheck(ctx, repoDir) + } + ctx, cancel := context.WithTimeout(ctx, scannerTimeout) defer cancel() @@ -354,8 +436,6 @@ func (s Scanner) Run(ctx context.Context, repoDir string) ([]Finding, error) { switch s.Kind { case ScannerGosec: cmd = exec.CommandContext(ctx, "gosec", "-fmt=json", "-quiet", "./...") - case ScannerGovulncheck: - cmd = exec.CommandContext(ctx, "govulncheck", "./...") case ScannerGitleaks: cmd = exec.CommandContext(ctx, "gitleaks", "detect", "--no-banner", "--report-format", "json", "--report-path", "/dev/stdout") case ScannerSemgrep: @@ -371,8 +451,6 @@ func (s Scanner) Run(ctx context.Context, repoDir string) ([]Finding, error) { switch s.Kind { case ScannerGosec: return parseGosec(out, repoDir) - case ScannerGovulncheck: - return parseGovulncheck(out) case ScannerGitleaks: return parseGitleaks(out, repoDir) case ScannerSemgrep: diff --git a/internal/security/scanners_exec_test.go b/internal/security/scanners_exec_test.go index e5919d7..ba1195c 100644 --- a/internal/security/scanners_exec_test.go +++ b/internal/security/scanners_exec_test.go @@ -47,10 +47,13 @@ const fakeGitleaksOut = `[{"Description":"AWS access key","File":"config.env","S const fakeSemgrepOut = `{"results":[{"check_id":"go.lang.security.audit.sqli","path":"db.go","start":{"line":42},"extra":{"message":"SQL built from input","severity":"ERROR","metadata":{"cwe":["CWE-89"],"owasp":["A03:2021 - Injection"]}}}]}` -const fakeGovulncheckOut = `Scanning your code and 42 packages across 7 dependent modules for known vulnerabilities... - -Vulnerability #1: GO-2024-1234 - A bad thing in some module.` +// govulncheck -json emits a stream of JSON objects and (unlike text mode) exits +// 0 when it runs successfully — even with findings. The config handshake proves +// the run completed; the symbol-level finding is the "called" vuln. +const fakeGovulncheckOut = `{"config":{"protocol_version":"v1.0.0","scanner_name":"govulncheck"}} +{"progress":{"message":"Scanning your code..."}} +{"osv":{"id":"GO-2024-1234"}} +{"finding":{"osv":"GO-2024-1234","trace":[{"module":"example.com/dep","package":"example.com/dep","function":"Boom"}]}}` const fakeNpmAuditOut = `{"vulnerabilities":{"lodash":{"name":"lodash","severity":"high","range":"<4.17.21","via":[]}}}` @@ -62,7 +65,7 @@ func installAllFakeScanners(t *testing.T) string { fakeTool(t, bin, "gosec", fakeGosecOut, 1) fakeTool(t, bin, "gitleaks", fakeGitleaksOut, 1) fakeTool(t, bin, "semgrep", fakeSemgrepOut, 0) - fakeTool(t, bin, "govulncheck", fakeGovulncheckOut, 1) + fakeTool(t, bin, "govulncheck", fakeGovulncheckOut, 0) // -json exits 0 even with findings fakeTool(t, bin, "npm", fakeNpmAuditOut, 1) // Keep /bin:/usr/bin so the fake scripts can find cat; no real security // scanner is ever installed there, so LookPath still resolves only fakes. @@ -142,6 +145,39 @@ func TestRunScanners_FailedToolIsReportedNotSwallowed(t *testing.T) { } } +func TestRunScanners_GovulncheckFailureNotReportedClean(t *testing.T) { + // Regression: govulncheck's text mode reported a failed run (e.g. a blocked + // vuln.go.dev fetch) as a clean scan because its output carried no + // "Vulnerability #" line. In -json mode a failed run exits non-zero and/or + // emits a garbled stream, so it must now land in `failed`, never `ran`. + bin := installAllFakeScanners(t) + const netFail = `{"config":{"scanner_name":"govulncheck"}} +{"progress":{"message":"Fetching vulnerabilities from the database..."}} +govulncheck: fetching vulnerabilities: Get "https://vuln.go.dev/index/modules.json.gz": Forbidden` + fakeTool(t, bin, "govulncheck", netFail, 1) + repo := seedRepo(t, map[string]string{"go.mod": "module example.com/x\n"}) + + findings, ran, _, failed := RunScanners(context.Background(), repo) + + failedSet := map[ScannerKind]bool{} + for _, k := range failed { + failedSet[k] = true + } + if !failedSet[ScannerGovulncheck] { + t.Fatalf("a failed govulncheck must be reported in failed, got failed=%v", failed) + } + for _, k := range ran { + if k == ScannerGovulncheck { + t.Error("a failed govulncheck must NOT be counted as a clean run") + } + } + for _, f := range findings { + if f.Tool == "govulncheck" { + t.Errorf("a failed govulncheck must contribute no findings, got %+v", f) + } + } +} + func TestRunScanners_MissingToolsSkippedVisibly(t *testing.T) { bin := t.TempDir() fakeTool(t, bin, "gitleaks", fakeGitleaksOut, 1) // only gitleaks installed diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index b1e8031..ef95c8a 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -176,32 +176,52 @@ func TestParseNpmAudit(t *testing.T) { } } -func TestParseGovulncheck(t *testing.T) { - // govulncheck text output (the human format): we extract called vulns. - out := []byte(`=== Symbol Results === - -Vulnerability #1: GO-2024-1234 - A flaw in net/http allows request smuggling. - More info: https://pkg.go.dev/vuln/GO-2024-1234 - Module: golang.org/x/net - Found in: golang.org/x/net@v0.10.0 - Fixed in: golang.org/x/net@v0.17.0 - -Vulnerability #2: GO-2023-5678 - Another issue. - More info: https://pkg.go.dev/vuln/GO-2023-5678 +func TestParseGovulncheckJSON(t *testing.T) { + // govulncheck -json stream: config handshake + two called (symbol-level) vulns. + out := []byte(`{"config":{"protocol_version":"v1.0.0","scanner_name":"govulncheck"}} +{"progress":{"message":"Scanning..."}} +{"osv":{"id":"GO-2024-1234"}} +{"finding":{"osv":"GO-2024-1234","trace":[{"module":"golang.org/x/net","package":"golang.org/x/net/http2","function":"Server.ServeConn"}]}} +{"finding":{"osv":"GO-2023-5678","trace":[{"module":"example.com/dep","package":"example.com/dep","function":"Do"}]}} `) - got, err := parseGovulncheck(out) + got, sawConfig, err := parseGovulncheckJSON(out) if err != nil { - t.Fatalf("parseGovulncheck: %v", err) + t.Fatalf("parseGovulncheckJSON: %v", err) + } + if !sawConfig { + t.Fatal("config handshake must be detected on a real run") } if len(got) != 2 { - t.Fatalf("expected 2 vuln findings, got %d", len(got)) + t.Fatalf("expected 2 called-vuln findings, got %d (%+v)", len(got), got) } - if got[0].RuleID != "GO-2024-1234" { - t.Errorf("expected GO-2024-1234, got %q", got[0].RuleID) + // Findings are emitted in sorted RuleID order. + if got[0].RuleID != "GO-2023-5678" || got[1].RuleID != "GO-2024-1234" { + t.Errorf("unexpected finding ids/order: %+v", got) } if got[0].Severity != SeverityHigh { t.Errorf("dependency CVE should be high, got %v", got[0].Severity) } } + +// TestParseGovulncheckJSON_FailedRunIsNotClean pins the fail-open regression: +// govulncheck's text mode reported a network/build/timeout error (output with no +// "Vulnerability #" line) as a clean scan. The -json parser must instead surface +// a decode error (garbage/truncated stream) or a missing config handshake so the +// scan is routed to `failed`, never masqueraded as scanned-clean. +func TestParseGovulncheckJSON_FailedRunIsNotClean(t *testing.T) { + // (a) A real failure: config + progress emitted, then a plain-text fatal error + // (the exact shape of a blocked vuln.go.dev fetch). Must error, not return clean. + partial := []byte(`{"config":{"scanner_name":"govulncheck"}} +{"progress":{"message":"Fetching vulnerabilities from the database..."}} +govulncheck: fetching vulnerabilities: Get "https://vuln.go.dev/index/modules.json.gz": Forbidden +`) + if _, _, err := parseGovulncheckJSON(partial); err == nil { + t.Error("a truncated stream with a trailing fatal error must return an error, not a clean scan") + } + + // (b) Empty output (tool crashed before any handshake) yields no config, so the + // caller (runGovulncheck) rejects it as "scan did not run". + if _, sawConfig, err := parseGovulncheckJSON(nil); err != nil || sawConfig { + t.Errorf("empty output: want (sawConfig=false, err=nil), got sawConfig=%v err=%v", sawConfig, err) + } +} diff --git a/internal/web/auth.go b/internal/web/auth.go index c98d17e..9777362 100644 --- a/internal/web/auth.go +++ b/internal/web/auth.go @@ -176,14 +176,13 @@ func (a *authenticator) Rotate() (string, error) { // RequireToken is a thin compatibility wrapper for callers that don't // need the bootstrap-nonce flow. Equivalent to NewAuthMiddleware with -// only Token set (or AllowUnauthenticated when token is empty — only -// the AllowUnauthenticated path is explicit; empty token still panics -// in NewAuthMiddleware, mirroring the safety guarantee). +// only Token set, so it inherits the same safety guarantee: an empty +// token PANICS rather than silently serving an unauthenticated +// dashboard. Callers that genuinely want no auth must go through +// NewAuthMiddleware with an explicit AllowUnauthenticated=true — the +// fail-open path is never reachable by leaving the token blank. func RequireToken(token string, next http.Handler) http.Handler { - mw := NewAuthMiddleware(AuthOptions{ - Token: token, - AllowUnauthenticated: token == "", - }) + mw := NewAuthMiddleware(AuthOptions{Token: token}) return mw(next) } diff --git a/internal/web/auth_test.go b/internal/web/auth_test.go index ee16d7f..8a4e78d 100644 --- a/internal/web/auth_test.go +++ b/internal/web/auth_test.go @@ -100,6 +100,21 @@ func TestNewAuthMiddleware_AllowUnauthenticatedExplicit(t *testing.T) { // ─── RequireToken — bypass paths + missing/wrong token ─────────────────── +// TestRequireToken_EmptyTokenPanics pins the safety contract: RequireToken must +// NOT silently fail open on a blank token. Previously it set +// AllowUnauthenticated: token == "", so RequireToken("") served an +// unauthenticated dashboard — contradicting its own doc, NewAuthMiddleware's +// panic guarantee, and CLAUDE.md. An empty token is misconfiguration and must +// panic, exactly like NewAuthMiddleware(AuthOptions{}). +func TestRequireToken_EmptyTokenPanics(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("RequireToken(\"\") must panic, not fail open to an unauthenticated dashboard") + } + }() + RequireToken("", http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) +} + func TestRequireToken_AllowsBypassPaths(t *testing.T) { h := RequireToken("the-token", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK)