fix: ScanDir SSRF self-allowlist, Flush requeue, remove dead RateLimiter, FailOn contract

.github/workflows/ci.yml

@@ -32,7 +32,7 @@ concurrency:
   cancel-in-progress: true
 
 env:
-  GO_VERSION: "1.26.5"
+  GO_VERSION: "1.26.6"
   GOPROXY: "https://proxy.golang.org,direct"
   GOPRIVATE: "github.com/GrayCodeAI/*"
   GONOSUMDB: "github.com/GrayCodeAI/*"

CHANGELOG.md

@@ -7,6 +7,43 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ---
 
+## [Unreleased]
+
+### Removed
+- **Dead exported `RateLimiter` type** (`ratelimit.go`). It had no
+  callers — the crawler rate-limits via its own internal per-crawl
+  limiter (`internal/crawler/rate.go`) — and its `Close` panicked when
+  called twice. Removed along with its test file; the now-unused
+  `golang.org/x/time` dependency is dropped from `go.mod`. This is a
+  minor API removal of code that never worked in a real scan; the
+  crawler's actual rate limiting is unchanged.
+
+### Fixed
+- **`FindingsStore.Flush` dropped the batch when the sink errored.** The
+  buffer was swapped out before `StoreBatch`, so a failing sink silently
+  lost every buffered entry. A failed flush now re-queues its batch at
+  the front of the buffer (entries added in the meantime keep their
+  order behind it) and still returns the error, so the next `Flush`
+  retries the same entries. The buffer stays bounded: re-queued batches
+  are capped at `maxBufferEntries` (10,000); overflow drops the oldest
+  entries and is reported via the new `FindingsStore.Dropped` counter.
+- **`Scanner.ScanDir` was blocked by its own SSRF protection.** The
+  temporary file server behind `ScanDir` listens on `127.0.0.1`, which the
+  crawler rejects under the default configuration, so default-options
+  scans (including the MCP `inspect_scan_dir` tool) returned a silently
+  empty report. The crawler now accepts an exact `host:port` private-IP
+  allowlist (`crawler.Config.PrivateIPAllowlist`) honored at both the
+  dialer and URL-validation layers, and `ScanDir` registers its ephemeral
+  listener address for the duration of that scan only. User-supplied URLs
+  and all other private addresses remain blocked.
+- **`ToContractReport` lost the configured fail threshold at the contract
+  layer.** The conversion field-copied `FailOn`, leaving the contract's
+  `FailOnSet` false, so a user-configured below-critical threshold did not
+  take effect in `verify.Report.Failed`. The converter now calls
+  `SetFailOn` so the threshold is recorded as explicitly configured.
+
+---
+
 ## [0.1.3] - 2026-07-04
 
 ### Changed

contracts.go

@@ -42,12 +42,16 @@ func ToContractReport(r *Report) *verifycontracts.Report {
 	if r == nil {
 		return nil
 	}
-	return &verifycontracts.Report{
+	res := &verifycontracts.Report{
 		Target:      r.Target,
 		Findings:    ToContractFindings(r.Findings),
 		Stats:       toContractStats(r.Stats),
 		CrawledURLs: r.CrawledURLs,
 		Duration:    r.Duration,
-		FailOn:      r.FailOn,
 	}
+	// Set through the method so the threshold is recorded as explicitly
+	// configured; a bare field copy leaves FailOnSet false and the contract
+	// Failed() would fall back to its default threshold.
+	res.SetFailOn(r.FailOn)
+	return res
 }

contracts_test.go

@@ -39,3 +39,30 @@ func TestToContractReport(t *testing.T) {
 		t.Fatalf("unexpected findings conversion: %+v", got.Findings)
 	}
 }
+
+func TestToContractReport_FailOnThresholdTakesEffect(t *testing.T) {
+	t.Parallel()
+
+	base := func(sev Severity) *Report {
+		return &Report{
+			Target: "https://example.com",
+			Findings: []Finding{
+				{Check: "security", Severity: sev, URL: "https://example.com/", Message: "m"},
+			},
+			FailOn: SeverityMedium,
+		}
+	}
+
+	low := ToContractReport(base(SeverityLow))
+	if !low.FailOnSet {
+		t.Fatal("converted report must record the threshold as explicitly set")
+	}
+	if low.Failed() {
+		t.Fatal("low finding must not fail a Medium threshold")
+	}
+
+	high := ToContractReport(base(SeverityHigh))
+	if !high.Failed() {
+		t.Fatal("high finding must fail a Medium threshold")
+	}
+}

findings_store.go

@@ -35,8 +35,15 @@ type FindingsStore struct {
 	mu        sync.Mutex
 	batchSize int
 	autoFlush bool
+	dropped   int // entries dropped after failed flushes overflowed maxBufferEntries
 }
 
+// maxBufferEntries bounds the buffer when a failed flush re-queues its
+// batch: with a persistently failing sink the buffer would otherwise grow
+// without limit. When the bound is exceeded the oldest entries are
+// dropped (and counted via Dropped); the newest are kept.
+const maxBufferEntries = 10000
+
 // FindingsStoreOption configures a FindingsStore.
 type FindingsStoreOption func(*FindingsStore)
 
@@ -94,6 +101,10 @@ func (s *FindingsStore) Add(ctx context.Context, entry FindingEntry) error {
 
 // Flush sends all buffered entries to the sink and clears the buffer.
 // A nil sink causes Flush to clear the buffer without error.
+// If the sink rejects the batch, the entries are put back at the front of
+// the buffer so the next Flush retries them, and the error is returned.
+// The buffer stays bounded: once it holds maxBufferEntries entries the
+// oldest are dropped (see Dropped).
 func (s *FindingsStore) Flush(ctx context.Context) error {
 	s.mu.Lock()
 	if len(s.buffer) == 0 {
@@ -109,18 +120,48 @@ func (s *FindingsStore) Flush(ctx context.Context) error {
 	}
 
 	if err := s.sink.StoreBatch(ctx, batch); err != nil {
+		// The sink failed; keep the batch buffered (in front of any
+		// entries added while StoreBatch was running) so the next
+		// Flush retries it instead of silently dropping it.
+		s.mu.Lock()
+		s.requeueLocked(batch)
+		s.mu.Unlock()
 		return fmt.Errorf("inspect: flushing findings batch (%d entries): %w", len(batch), err)
 	}
 	return nil
 }
 
+// requeueLocked prepends a failed batch to the buffer. The combined buffer
+// is capped at maxBufferEntries; overflow drops the oldest entries and
+// counts them in s.dropped. Caller must hold s.mu.
+func (s *FindingsStore) requeueLocked(batch []FindingEntry) {
+	combined := make([]FindingEntry, 0, len(batch)+len(s.buffer))
+	combined = append(combined, batch...)
+	combined = append(combined, s.buffer...)
+	if overflow := len(combined) - maxBufferEntries; overflow > 0 {
+		s.dropped += overflow
+		combined = combined[overflow:]
+	}
+	s.buffer = combined
+}
+
 // Size returns the number of entries currently in the buffer.
 func (s *FindingsStore) Size() int {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	return len(s.buffer)
 }
 
+// Dropped returns the number of entries discarded because failed flushes
+// re-queued more entries than the buffer is allowed to hold
+// (maxBufferEntries). It reports entries lost to a persistently failing
+// sink and never resets.
+func (s *FindingsStore) Dropped() int {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.dropped
+}
+
 // ConvertScanResult converts a slice of Finding records from a scan into
 // FindingEntry records for the store.
 func ConvertScanResult(url string, findings []Finding) []FindingEntry {

findings_store_test.go

@@ -3,6 +3,7 @@ package inspect
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -314,6 +315,130 @@ func TestFlushPropagatesBatchError(t *testing.T) {
 	}
 }
 
+// TestFlushRetriesBatchAfterSinkError verifies that a batch rejected by the
+// sink is retained in the buffer and delivered by the next successful
+// Flush, instead of being dropped.
+func TestFlushRetriesBatchAfterSinkError(t *testing.T) {
+	sink := &mockFindingSink{batchErr: errors.New("sink down")}
+	store := NewFindingsStore(sink, WithAutoFlush(false))
+	ctx := context.Background()
+
+	for i := 0; i < 5; i++ {
+		if err := store.Add(ctx, FindingEntry{Message: fmt.Sprintf("entry-%d", i)}); err != nil {
+			t.Fatalf("Add failed: %v", err)
+		}
+	}
+
+	if err := store.Flush(ctx); err == nil {
+		t.Fatal("expected error from Flush with failing sink")
+	}
+	if got := store.Size(); got != 5 {
+		t.Fatalf("expected failed batch of 5 entries to be retained, got buffer size %d", got)
+	}
+
+	// Sink recovers: the next Flush must retry and deliver the same batch.
+	sink.mu.Lock()
+	sink.batchErr = nil
+	sink.mu.Unlock()
+	if err := store.Flush(ctx); err != nil {
+		t.Fatalf("Flush after sink recovery failed: %v", err)
+	}
+	if got := sink.batchCallCount(); got != 2 {
+		t.Errorf("expected 2 StoreBatch calls (failed + retry), got %d", got)
+	}
+	sink.mu.Lock()
+	retry := sink.batchCalls[len(sink.batchCalls)-1]
+	sink.mu.Unlock()
+	if len(retry) != 5 {
+		t.Fatalf("expected retry batch of 5 entries, got %d", len(retry))
+	}
+	for i, e := range retry {
+		want := fmt.Sprintf("entry-%d", i)
+		if e.Message != want {
+			t.Errorf("retry batch entry %d: want %q, got %q", i, want, e.Message)
+		}
+	}
+	if got := store.Size(); got != 0 {
+		t.Errorf("expected empty buffer after successful retry, got %d", got)
+	}
+	if got := store.Dropped(); got != 0 {
+		t.Errorf("expected no dropped entries, got %d", got)
+	}
+}
+
+// TestFlushRequeuesFailedBatchBeforeNewEntries verifies ordering: entries
+// added while a flush is failing end up behind the re-queued batch.
+func TestFlushRequeuesFailedBatchBeforeNewEntries(t *testing.T) {
+	sink := &mockFindingSink{batchErr: errors.New("sink down")}
+	store := NewFindingsStore(sink, WithAutoFlush(false))
+	ctx := context.Background()
+
+	_ = store.Add(ctx, FindingEntry{Message: "first"})
+	_ = store.Add(ctx, FindingEntry{Message: "second"})
+	if err := store.Flush(ctx); err == nil {
+		t.Fatal("expected error from Flush with failing sink")
+	}
+	_ = store.Add(ctx, FindingEntry{Message: "third"})
+
+	sink.mu.Lock()
+	sink.batchErr = nil
+	sink.mu.Unlock()
+	if err := store.Flush(ctx); err != nil {
+		t.Fatalf("Flush after sink recovery failed: %v", err)
+	}
+	sink.mu.Lock()
+	got := sink.batchCalls[len(sink.batchCalls)-1]
+	sink.mu.Unlock()
+	want := []string{"first", "second", "third"}
+	if len(got) != len(want) {
+		t.Fatalf("expected %d entries, got %d", len(want), len(got))
+	}
+	for i, e := range got {
+		if e.Message != want[i] {
+			t.Errorf("entry %d: want %q, got %q", i, want[i], e.Message)
+		}
+	}
+}
+
+// TestFlushRequeueIsBounded verifies the buffer cap when a failed batch is
+// re-queued: overflow drops the oldest entries and counts them in Dropped.
+func TestFlushRequeueIsBounded(t *testing.T) {
+	store := NewFindingsStore(&mockFindingSink{}, WithAutoFlush(false))
+
+	store.mu.Lock()
+	store.buffer = make([]FindingEntry, maxBufferEntries-1)
+	for i := range store.buffer {
+		store.buffer[i].Message = fmt.Sprintf("buffered-%d", i)
+	}
+	batch := make([]FindingEntry, 10)
+	for i := range batch {
+		batch[i].Message = fmt.Sprintf("batch-%d", i)
+	}
+	store.requeueLocked(batch)
+	size := len(store.buffer)
+	dropped := store.dropped
+	first, last := store.buffer[0].Message, store.buffer[len(store.buffer)-1].Message
+	store.mu.Unlock()
+
+	if size != maxBufferEntries {
+		t.Errorf("expected buffer capped at %d entries, got %d", maxBufferEntries, size)
+	}
+	if dropped != 9 {
+		t.Errorf("expected 9 dropped entries, got %d", dropped)
+	}
+	if got := store.Dropped(); got != 9 {
+		t.Errorf("Dropped() = %d, want 9", got)
+	}
+	// The oldest entries (the head of the failed batch) are dropped; the
+	// newest (the tail of the existing buffer) survive.
+	if first != "batch-9" {
+		t.Errorf("first buffered entry = %q, want %q", first, "batch-9")
+	}
+	if last != fmt.Sprintf("buffered-%d", maxBufferEntries-2) {
+		t.Errorf("last buffered entry = %q, want the newest buffered entry", last)
+	}
+}
+
 // TestAutoFlushMultipleBatches verifies that auto-flush triggers correctly
 // across multiple batch boundaries.
 func TestAutoFlushMultipleBatches(t *testing.T) {