diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d0935f8..b1e4b54 100644 --- a/.github/workflows/ci.yml +++ b/.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/*" diff --git a/CHANGELOG.md b/CHANGELOG.md index 62a46c8..7def875 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/browser/go.mod b/browser/go.mod index 3e1bb80..89a3779 100644 --- a/browser/go.mod +++ b/browser/go.mod @@ -1,6 +1,6 @@ module github.com/GrayCodeAI/inspect/browser -go 1.26.5 +go 1.26.6 require ( github.com/GrayCodeAI/inspect v0.1.0 @@ -8,15 +8,14 @@ require ( ) require ( - github.com/GrayCodeAI/hawk-core-contracts v0.1.9 // indirect + github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e // indirect github.com/ysmood/fetchup v0.2.3 // indirect github.com/ysmood/goob v0.4.0 // indirect github.com/ysmood/got v0.40.0 // indirect github.com/ysmood/gson v0.7.3 // indirect github.com/ysmood/leakless v0.9.0 // indirect golang.org/x/net v0.55.0 // indirect - golang.org/x/time v0.15.0 // indirect -gopkg.in/yaml.v3 v3.0.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) replace github.com/GrayCodeAI/inspect => ../ diff --git a/browser/go.sum b/browser/go.sum index b493d2f..cfe4807 100644 --- a/browser/go.sum +++ b/browser/go.sum @@ -1,5 +1,5 @@ -github.com/GrayCodeAI/hawk-core-contracts v0.1.9 h1:uXX/gtNM+3kxSEzu+rZkHykzcEaAbASn1lmPyOGMXvc= -github.com/GrayCodeAI/hawk-core-contracts v0.1.9/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e h1:cG9bLB3rWmMVU/7GwDxUIrEFbGHz7OTD5s9I4K+udr0= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/go-rod/rod v0.116.2 h1:A5t2Ky2A+5eD/ZJQr1EfsQSe5rms5Xof/qj296e+ZqA= github.com/go-rod/rod v0.116.2/go.mod h1:H+CMO9SCNc2TJ2WfrG+pKhITz57uGNYU43qYHh438Mg= github.com/ysmood/fetchup v0.2.3 h1:ulX+SonA0Vma5zUFXtv52Kzip/xe7aj4vqT5AJwQ+ZQ= @@ -18,8 +18,6 @@ github.com/ysmood/leakless v0.9.0 h1:qxCG5VirSBvmi3uynXFkcnLMzkphdh3xx5FtrORwDCU github.com/ysmood/leakless v0.9.0/go.mod h1:R8iAXPRaG97QJwqxs74RdwzcRHT1SWCGTNqY8q0JvMQ= golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= -golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= -golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/contracts.go b/contracts.go index f21597f..e21f270 100644 --- a/contracts.go +++ b/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 } diff --git a/contracts_test.go b/contracts_test.go index bf7f881..894acf9 100644 --- a/contracts_test.go +++ b/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") + } +} diff --git a/findings_store.go b/findings_store.go index 328742c..2b3c87f 100644 --- a/findings_store.go +++ b/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,11 +120,31 @@ 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() @@ -121,6 +152,16 @@ func (s *FindingsStore) Size() int { 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 { diff --git a/findings_store_test.go b/findings_store_test.go index fabe1a7..06438d3 100644 --- a/findings_store_test.go +++ b/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) { diff --git a/go.mod b/go.mod index 26c8fd7..8ab1a61 100644 --- a/go.mod +++ b/go.mod @@ -1,13 +1,12 @@ module github.com/GrayCodeAI/inspect -go 1.26.5 +go 1.26.6 require ( - github.com/GrayCodeAI/hawk-core-contracts v0.1.9 + github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e github.com/GrayCodeAI/hawk-mcpkit v0.1.4 github.com/mark3labs/mcp-go v0.49.0 golang.org/x/net v0.55.0 - golang.org/x/time v0.15.0 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index e00a87b..c216382 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/GrayCodeAI/hawk-core-contracts v0.1.9 h1:uXX/gtNM+3kxSEzu+rZkHykzcEaAbASn1lmPyOGMXvc= -github.com/GrayCodeAI/hawk-core-contracts v0.1.9/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e h1:cG9bLB3rWmMVU/7GwDxUIrEFbGHz7OTD5s9I4K+udr0= +github.com/GrayCodeAI/hawk-core-contracts v0.1.13-0.20260816034142-16ebcfd5ad6e/go.mod h1:BXbh68YrCf+s9HVqND5F8DAvl2MnE5NcOwZZZB56HGA= github.com/GrayCodeAI/hawk-mcpkit v0.1.4 h1:tlhZXKDbI679I7c1feeY/pzErFwndD+R2CQf9sqHAVE= github.com/GrayCodeAI/hawk-mcpkit v0.1.4/go.mod h1:C32HPDRqiDETbVbMIbOTvguek6KImpLCffJjet7sqck= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= @@ -30,8 +30,6 @@ github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zI github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= -golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= -golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/inspect_test.go b/inspect_test.go index ab089a6..39db664 100644 --- a/inspect_test.go +++ b/inspect_test.go @@ -5,6 +5,9 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" + "strings" "testing" "github.com/GrayCodeAI/inspect" @@ -216,3 +219,66 @@ func TestScan_Presets(t *testing.T) { } } } + +// TestScanDir_DefaultOptions is a regression test: ScanDir serves the +// directory on an ephemeral loopback listener and must exempt exactly that +// listener from the crawler's SSRF protection (enabled by default), so the +// scan works without WithAllowPrivateIPs. +func TestScanDir_DefaultOptions(t *testing.T) { + dir := t.TempDir() + html := `Local

Hello

` + if err := os.WriteFile(filepath.Join(dir, "index.html"), []byte(html), 0o644); err != nil { + t.Fatal(err) + } + + s := inspect.NewScanner() // default options: SSRF protection ON, no WithAllowPrivateIPs + report, err := s.ScanDir(context.Background(), dir) + if err != nil { + t.Fatalf("ScanDir failed with default options: %v", err) + } + if report.CrawledURLs < 1 { + t.Fatalf("expected at least 1 crawled URL, got %d", report.CrawledURLs) + } + + // The page must actually have been fetched and analyzed. Before the + // fix, SSRF protection rejected the scanner's own file server and the + // report came back silently empty. The img without an alt attribute + // produces a deterministic a11y finding that requires a parsed body. + hasAltFinding := false + for _, f := range report.Findings { + if f.Check == "a11y" && strings.Contains(f.Message, "alt") { + hasAltFinding = true + } + } + if !hasAltFinding { + t.Errorf("expected the local page to be analyzed (image alt finding); got %d findings — scan likely blocked by its own SSRF guard", len(report.Findings)) + } +} + +// TestScan_LoopbackNotAllowlistedStillBlocked asserts the flip side of the +// ScanDir exemption: a loopback server that is NOT registered in the +// crawl's private-IP allowlist stays rejected under default options. +func TestScan_LoopbackNotAllowlistedStillBlocked(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + fmt.Fprint(w, `X

X

`) + })) + defer srv.Close() + + s := inspect.NewScanner(inspect.Quick) // default SSRF protection, no WithAllowPrivateIPs + report, err := s.Scan(context.Background(), srv.URL) + if err != nil { + t.Fatalf("Scan failed: %v", err) + } + + // Every page fetch is rejected by SSRF protection, so no check can + // analyze page content and the report must carry no findings. (When + // the page is fetched, the Quick preset reports the broken img link, + // and the full check set reports missing headers/meta — so findings + // would be non-empty on a successful fetch.) + if len(report.Findings) != 0 { + for _, f := range report.Findings { + t.Errorf("unexpected finding from blocked scan: [%s] %s", f.Check, f.Message) + } + } +} diff --git a/internal/crawler/crawler.go b/internal/crawler/crawler.go index 6091b50..fbf0872 100644 --- a/internal/crawler/crawler.go +++ b/internal/crawler/crawler.go @@ -37,6 +37,16 @@ type Config struct { Logger *slog.Logger MaxPages int // Maximum number of pages to crawl; 0 means no limit + // PrivateIPAllowlist lists exact "host:port" addresses that are exempt + // from SSRF private-IP blocking at both the dialer and URL-validation + // layers. It exists so a caller that crawls a server it controls (e.g. + // the temporary file server behind Scanner.ScanDir) is not blocked by + // its own SSRF protection. Entries must carry an explicit port, apply + // to that address only, and live for the lifetime of this Config (one + // crawl). User-supplied URLs are unaffected: every other private + // address is still rejected. + PrivateIPAllowlist []string + // CircuitBreaker, when non-nil, gates requests per host. If a host has // accumulated too many consecutive failures the breaker opens and the // crawler skips further requests to that host until the cooldown expires. @@ -103,12 +113,13 @@ type FormInput struct { // Crawler performs concurrent crawling with rate limiting. type Crawler struct { - cfg Config - client *http.Client - seen map[string]bool - mu sync.Mutex - robots *RobotsCache - limiter *rateLimiter + cfg Config + client *http.Client + seen map[string]bool + mu sync.Mutex + robots *RobotsCache + limiter *rateLimiter + privateAllowed map[string]struct{} } // New creates a configured Crawler. @@ -128,23 +139,33 @@ func New(cfg Config) *Crawler { KeepAlive: 30 * time.Second, } + // Exact host:port addresses exempt from private-IP blocking (see + // Config.PrivateIPAllowlist). Shared by the dialer below and + // validateURL so both layers honor the same exemptions. + privateAllowed := make(map[string]struct{}, len(cfg.PrivateIPAllowlist)) + for _, hp := range cfg.PrivateIPAllowlist { + privateAllowed[hp] = struct{}{} + } + transport := &http.Transport{ MaxIdleConns: cfg.Concurrency * 2, MaxIdleConnsPerHost: cfg.Concurrency, IdleConnTimeout: 90 * time.Second, DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { if !cfg.AllowPrivateIPs { - host, _, err := net.SplitHostPort(addr) - if err != nil { - return nil, err - } - ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) - if err != nil { - return nil, err - } - for _, ip := range ips { - if isPrivateIP(ip.IP) { - return nil, fmt.Errorf("SSRF protection: blocked connection to private IP %s", ip.IP) + if _, ok := privateAllowed[addr]; !ok { + host, _, err := net.SplitHostPort(addr) + if err != nil { + return nil, err + } + ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) + if err != nil { + return nil, err + } + for _, ip := range ips { + if isPrivateIP(ip.IP) { + return nil, fmt.Errorf("SSRF protection: blocked connection to private IP %s", ip.IP) + } } } } @@ -165,11 +186,12 @@ func New(cfg Config) *Crawler { } return &Crawler{ - cfg: cfg, - client: client, - seen: make(map[string]bool), - robots: NewRobotsCache(), - limiter: newRateLimiter(cfg.RateLimit), + cfg: cfg, + client: client, + seen: make(map[string]bool), + robots: NewRobotsCache(), + limiter: newRateLimiter(cfg.RateLimit), + privateAllowed: privateAllowed, } } @@ -526,7 +548,8 @@ func (c *Crawler) noRedirectClient() *http.Client { } // validateURL checks the URL for SSRF risks: scheme must be http/https and -// resolved IP must not be in private ranges (unless AllowPrivateIPs is set). +// resolved IP must not be in private ranges (unless AllowPrivateIPs is set +// or the URL's host:port is on the crawl's private-IP allowlist). func (c *Crawler) validateURL(rawURL string) error { parsed, err := url.Parse(rawURL) if err != nil { @@ -535,7 +558,7 @@ func (c *Crawler) validateURL(rawURL string) error { if parsed.Scheme != "http" && parsed.Scheme != "https" { return fmt.Errorf("disallowed URL scheme %q (only http/https allowed)", parsed.Scheme) } - if c.cfg.AllowPrivateIPs { + if c.cfg.AllowPrivateIPs || c.privateAddrAllowed(parsed.Host) { return nil } host := parsed.Hostname() @@ -592,6 +615,18 @@ func isPrivateIP(ip net.IP) bool { return false } +// privateAddrAllowed reports whether hostport is an exact-match entry of +// this crawl's private-IP allowlist (Config.PrivateIPAllowlist). Entries +// are "host:port" strings for servers the caller controls; they are exempt +// from private-IP blocking while every other address stays protected. +func (c *Crawler) privateAddrAllowed(hostport string) bool { + if len(c.privateAllowed) == 0 { + return false + } + _, ok := c.privateAllowed[hostport] + return ok +} + func isRetryable(statusCode int) bool { return statusCode == 429 || statusCode == 500 || statusCode == 502 || statusCode == 503 || statusCode == 504 || statusCode == 0 diff --git a/internal/crawler/crawler_more_test.go b/internal/crawler/crawler_more_test.go index bd261be..b4ef4fe 100644 --- a/internal/crawler/crawler_more_test.go +++ b/internal/crawler/crawler_more_test.go @@ -6,6 +6,7 @@ import ( "net" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -259,6 +260,109 @@ func TestValidateURL_SSRFProtection(t *testing.T) { } } +func TestValidateURL_PrivateIPAllowlist(t *testing.T) { + c := New(Config{ + AllowPrivateIPs: false, + PrivateIPAllowlist: []string{"127.0.0.1:8123"}, + UserAgent: "test", + }) + + // Exact allowlisted host:port is exempt from the private-IP check. + if err := c.validateURL("http://127.0.0.1:8123/index.html"); err != nil { + t.Errorf("expected allowlisted address to pass, got %v", err) + } + + // Same host, different port: not on the allowlist, must stay blocked. + if err := c.validateURL("http://127.0.0.1:8124/index.html"); err == nil { + t.Error("expected error for loopback address not in allowlist") + } + + // Host without an explicit port never matches (allowlist entries are + // exact host:port values), so it must stay blocked. + if err := c.validateURL("http://127.0.0.1/admin"); err == nil { + t.Error("expected error for loopback host without port not in allowlist") + } + + // Other private ranges remain blocked. + if err := c.validateURL("http://192.168.1.10:8123/"); err == nil { + t.Error("expected error for private range not in allowlist") + } +} + +func TestCrawl_PrivateIPAllowlist(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + fmt.Fprint(w, `

local

`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + parsed, err := url.Parse(srv.URL) + if err != nil { + t.Fatal(err) + } + hostPort := parsed.Host // 127.0.0.1: + + // Without an allowlist entry the loopback server must be rejected by + // SSRF protection (validateURL layer). + blocked := New(Config{UserAgent: "test"}) + pages, err := blocked.Crawl(context.Background(), srv.URL+"/") + if err != nil { + t.Fatalf("Crawl failed: %v", err) + } + if len(pages) != 1 || pages[0].Error == nil { + t.Fatalf("expected the crawl page to carry an error, got %+v", pages) + } + if !strings.Contains(pages[0].Error.Error(), "SSRF protection") { + t.Errorf("expected SSRF protection error, got %v", pages[0].Error) + } + + // The dialer layer must enforce the same rule for requests that skip + // validateURL (e.g. robots.txt and sitemap fetches). + _, dialErr := blocked.client.Get(srv.URL + "/") + if dialErr == nil || !strings.Contains(dialErr.Error(), "SSRF protection") { + t.Errorf("expected dialer to block loopback, got %v", dialErr) + } + + // With the server's exact host:port allowlisted for this crawl, both + // layers let the fetch through. + allowed := New(Config{ + UserAgent: "test", + PrivateIPAllowlist: []string{hostPort}, + }) + pages, err = allowed.Crawl(context.Background(), srv.URL+"/") + if err != nil { + t.Fatalf("Crawl failed: %v", err) + } + if len(pages) != 1 { + t.Fatalf("expected 1 page, got %d", len(pages)) + } + if pages[0].Error != nil { + t.Fatalf("expected allowlisted crawl to succeed, got %v", pages[0].Error) + } + if pages[0].StatusCode != http.StatusOK { + t.Errorf("expected status 200, got %d", pages[0].StatusCode) + } + + // A different loopback port stays blocked even on the allowlisted + // crawler: spin up a second server and crawl it directly. + mux2 := http.NewServeMux() + mux2.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + fmt.Fprint(w, `

other

`) + }) + srv2 := httptest.NewServer(mux2) + defer srv2.Close() + pages, err = allowed.Crawl(context.Background(), srv2.URL+"/") + if err != nil { + t.Fatalf("Crawl failed: %v", err) + } + if len(pages) != 1 || pages[0].Error == nil || !strings.Contains(pages[0].Error.Error(), "SSRF protection") { + t.Errorf("expected second server to stay SSRF-blocked, got %+v", pages) + } +} + // --- isRetryable tests --- func TestIsRetryable(t *testing.T) { diff --git a/ratelimit.go b/ratelimit.go deleted file mode 100644 index 0349948..0000000 --- a/ratelimit.go +++ /dev/null @@ -1,140 +0,0 @@ -package inspect - -import ( - "context" - "sync" - "time" - - "golang.org/x/time/rate" -) - -// hostLimit pairs a per-host limiter with the time it was last accessed. -type hostLimit struct { - limiter *rate.Limiter - lastUsed time.Time -} - -// RateLimiter provides per-host rate limiting for crawl requests. Each host -// gets its own token-bucket limiter so that aggressive crawling of one host -// does not throttle requests to others. Stale limiters are cleaned up -// periodically. -type RateLimiter struct { - limits map[string]*hostLimit - mu sync.Mutex - requestsPerSecond float64 - burst int - cleanupInterval time.Duration - stopCleanup chan struct{} -} - -// RateLimiterOption is a functional option for configuring a RateLimiter. -type RateLimiterOption func(*RateLimiter) - -// WithCleanupInterval sets how often stale host limiters are reaped. -// A host is considered stale when it has not been used for 5 minutes. -// The default cleanup interval is 1 minute. -func WithCleanupInterval(d time.Duration) RateLimiterOption { - return func(rl *RateLimiter) { - rl.cleanupInterval = d - } -} - -// NewRateLimiter creates a per-host rate limiter that allows rps requests per -// second with the given burst size. Each host that is seen gets its own -// independent limiter. Background cleanup removes limiters for hosts that -// have not been accessed in 5 minutes. -func NewRateLimiter(rps float64, burst int, opts ...RateLimiterOption) *RateLimiter { - rl := &RateLimiter{ - limits: make(map[string]*hostLimit), - requestsPerSecond: rps, - burst: burst, - cleanupInterval: time.Minute, - stopCleanup: make(chan struct{}), - } - - for _, opt := range opts { - opt(rl) - } - - go rl.cleanupLoop() - - return rl -} - -// getOrCreate returns the limiter for host, creating one if it does not exist. -// Caller must hold rl.mu. -func (rl *RateLimiter) getOrCreate(host string) *hostLimit { - hl, ok := rl.limits[host] - if !ok { - hl = &hostLimit{ - limiter: rate.NewLimiter(rate.Limit(rl.requestsPerSecond), rl.burst), - } - rl.limits[host] = hl - } - hl.lastUsed = time.Now() - return hl -} - -// Wait blocks until a request for host is allowed or ctx is cancelled. -func (rl *RateLimiter) Wait(ctx context.Context, host string) error { - rl.mu.Lock() - hl := rl.getOrCreate(host) - rl.mu.Unlock() - - return hl.limiter.Wait(ctx) -} - -// Allow reports whether a request for host is allowed right now without -// blocking. Returns true if the request may proceed. -func (rl *RateLimiter) Allow(host string) bool { - rl.mu.Lock() - hl := rl.getOrCreate(host) - rl.mu.Unlock() - - return hl.limiter.Allow() -} - -// ActiveHosts returns the number of hosts currently being tracked. -func (rl *RateLimiter) ActiveHosts() int { - rl.mu.Lock() - defer rl.mu.Unlock() - - return len(rl.limits) -} - -// Close stops the background cleanup goroutine. After Close is called the -// limiter should not be used. -func (rl *RateLimiter) Close() { - close(rl.stopCleanup) -} - -// cleanupLoop periodically removes limiters for hosts that have not been -// accessed in the stale threshold (5 minutes). -func (rl *RateLimiter) cleanupLoop() { - ticker := time.NewTicker(rl.cleanupInterval) - defer ticker.Stop() - - for { - select { - case <-ticker.C: - rl.reapStale() - case <-rl.stopCleanup: - return - } - } -} - -const staleThreshold = 5 * time.Minute - -// reapStale removes host limiters that have not been used recently. -func (rl *RateLimiter) reapStale() { - rl.mu.Lock() - defer rl.mu.Unlock() - - now := time.Now() - for host, hl := range rl.limits { - if now.Sub(hl.lastUsed) > staleThreshold { - delete(rl.limits, host) - } - } -} diff --git a/ratelimit_test.go b/ratelimit_test.go deleted file mode 100644 index d5b3573..0000000 --- a/ratelimit_test.go +++ /dev/null @@ -1,263 +0,0 @@ -package inspect - -import ( - "context" - "testing" - "time" -) - -func TestNewRateLimiter(t *testing.T) { - t.Run("creates limiter with default settings", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - if rl == nil { - t.Fatal("NewRateLimiter returned nil") - } - if rl.ActiveHosts() != 0 { - t.Errorf("expected 0 active hosts, got %d", rl.ActiveHosts()) - } - }) - - t.Run("creates limiter with cleanup interval option", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5, WithCleanupInterval(30*time.Second)) - defer rl.Close() - - if rl.cleanupInterval != 30*time.Second { - t.Errorf("expected cleanup interval 30s, got %v", rl.cleanupInterval) - } - }) - - t.Run("default cleanup interval is one minute", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - if rl.cleanupInterval != time.Minute { - t.Errorf("expected cleanup interval 1m, got %v", rl.cleanupInterval) - } - }) -} - -func TestRateLimiterAllow(t *testing.T) { - t.Run("allows first request", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - if !rl.Allow("example.com") { - t.Error("expected first request to be allowed") - } - }) - - t.Run("allows requests within burst", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - for i := 0; i < 5; i++ { - if !rl.Allow("example.com") { - t.Errorf("request %d should be allowed (within burst)", i+1) - } - } - }) - - t.Run("denies request beyond burst", func(t *testing.T) { - rl := NewRateLimiter(10.0, 2) - defer rl.Close() - - // Use up burst - rl.Allow("example.com") - rl.Allow("example.com") - - if rl.Allow("example.com") { - t.Error("expected request beyond burst to be denied") - } - }) - - t.Run("tracks active hosts", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - rl.Allow("host1.com") - rl.Allow("host2.com") - rl.Allow("host3.com") - - if rl.ActiveHosts() != 3 { - t.Errorf("expected 3 active hosts, got %d", rl.ActiveHosts()) - } - }) - - t.Run("each host has independent rate limit", func(t *testing.T) { - rl := NewRateLimiter(10.0, 2) - defer rl.Close() - - // Use up burst for host1 - rl.Allow("host1.com") - rl.Allow("host1.com") - - // host1 should be denied - if rl.Allow("host1.com") { - t.Error("expected host1 to be denied (burst exhausted)") - } - - // host2 should still be allowed (independent limit) - if !rl.Allow("host2.com") { - t.Error("expected host2 to be allowed (independent limit)") - } - }) -} - -func TestRateLimiterWait(t *testing.T) { - t.Run("wait allows first request immediately", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - ctx := context.Background() - start := time.Now() - err := rl.Wait(ctx, "example.com") - duration := time.Since(start) - - if err != nil { - t.Errorf("Wait returned error: %v", err) - } - // Should be nearly instant (< 10ms) - if duration > 10*time.Millisecond { - t.Errorf("Wait took too long: %v", duration) - } - }) - - t.Run("wait respects context cancellation", func(t *testing.T) { - rl := NewRateLimiter(10.0, 1) - defer rl.Close() - - // Use up the single token - rl.Wait(context.Background(), "example.com") - - // Create context with short timeout - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - defer cancel() - - // This should block and then fail due to timeout - err := rl.Wait(ctx, "example.com") - if err == nil { - t.Error("expected Wait to fail with cancelled context") - } - }) - - t.Run("wait blocks when burst exhausted", func(t *testing.T) { - rl := NewRateLimiter(10.0, 1) - defer rl.Close() - - // Use up the single token - rl.Wait(context.Background(), "example.com") - - // This immediate call should fail (no tokens available) - // We need to use a timeout context to avoid infinite wait - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) - defer cancel() - - err := rl.Wait(ctx, "example.com") - if err == nil { - t.Error("expected Wait to fail when no tokens available") - } - }) -} - -func TestRateLimiterActiveHosts(t *testing.T) { - t.Run("starts with zero hosts", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - if rl.ActiveHosts() != 0 { - t.Errorf("expected 0 active hosts, got %d", rl.ActiveHosts()) - } - }) - - t.Run("counts unique hosts", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - rl.Allow("host1.com") - rl.Allow("host2.com") - rl.Allow("host1.com") // duplicate - - if rl.ActiveHosts() != 2 { - t.Errorf("expected 2 active hosts, got %d", rl.ActiveHosts()) - } - }) - - t.Run("allows empty host", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - rl.Allow("") - - if rl.ActiveHosts() != 1 { - t.Errorf("expected 1 active host, got %d", rl.ActiveHosts()) - } - }) -} - -func TestRateLimiterReapStale(t *testing.T) { - t.Run("removes hosts unused for 5 minutes", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5, WithCleanupInterval(10*time.Millisecond)) - - rl.Allow("host1.com") - rl.Allow("host2.com") - - if rl.ActiveHosts() != 2 { - t.Fatalf("expected 2 active hosts, got %d", rl.ActiveHosts()) - } - - // Manually set lastUsed to simulate stale hosts - rl.mu.Lock() - for _, hl := range rl.limits { - hl.lastUsed = time.Now().Add(-6 * time.Minute) - } - rl.mu.Unlock() - - // Trigger cleanup - rl.reapStale() - - if rl.ActiveHosts() != 0 { - t.Errorf("expected 0 active hosts after cleanup, got %d", rl.ActiveHosts()) - } - - rl.Close() - }) - - t.Run("keeps recently used hosts", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5) - defer rl.Close() - - rl.Allow("host1.com") - rl.Allow("host2.com") - - // Manually set one host to be stale - rl.mu.Lock() - rl.limits["host1.com"].lastUsed = time.Now().Add(-6 * time.Minute) - rl.mu.Unlock() - - rl.reapStale() - - if rl.ActiveHosts() != 1 { - t.Errorf("expected 1 active host after cleanup, got %d", rl.ActiveHosts()) - } - - // Should still have host2 - if !rl.Allow("host2.com") { - t.Error("expected host2 to still be available") - } - }) -} - -func TestRateLimiterClose(t *testing.T) { - t.Run("close stops cleanup goroutine", func(t *testing.T) { - rl := NewRateLimiter(10.0, 5, WithCleanupInterval(10*time.Millisecond)) - - rl.Close() - - // Allow should still work after close - if !rl.Allow("example.com") { - t.Error("expected Allow to work after Close") - } - }) -} diff --git a/scanner.go b/scanner.go index c40a52b..18f1d60 100644 --- a/scanner.go +++ b/scanner.go @@ -28,6 +28,15 @@ func NewScanner(opts ...Option) *Scanner { // Scan crawls the target URL and runs all configured checks against the // discovered pages. Returns a complete Report with findings and stats. func (s *Scanner) Scan(ctx context.Context, target string) (*Report, error) { + return s.scan(ctx, target, nil) +} + +// scan is the shared implementation of Scan and ScanDir. allowPrivateAddrs +// lists exact host:port addresses exempt from the crawler's SSRF private-IP +// blocking for this scan only; ScanDir uses it for the ephemeral local file +// server it starts. It must only ever contain addresses the scanner itself +// brought up, never user-supplied input. +func (s *Scanner) scan(ctx context.Context, target string, allowPrivateAddrs []string) (*Report, error) { if s.cfg.timeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, s.cfg.timeout) @@ -37,21 +46,22 @@ func (s *Scanner) Scan(ctx context.Context, target string) (*Report, error) { start := time.Now() crawlCfg := crawler.Config{ - MaxDepth: s.cfg.depth, - Concurrency: s.cfg.concurrency, - Timeout: s.cfg.timeout, - PageTimeout: s.cfg.pageTimeout, - RateLimit: s.cfg.rateLimit, - UserAgent: s.cfg.userAgent, - FollowRedirects: s.cfg.followRedirects, - RespectRobots: s.cfg.respectRobots, - Exclude: s.cfg.exclude, - AuthHeader: s.cfg.authHeader, - AuthValue: s.cfg.authValue, - CookieJar: s.cfg.cookieJar, - AllowPrivateIPs: !s.cfg.blockPrivateIPs, - Logger: s.cfg.logger, - MaxPages: s.cfg.maxPages, + MaxDepth: s.cfg.depth, + Concurrency: s.cfg.concurrency, + Timeout: s.cfg.timeout, + PageTimeout: s.cfg.pageTimeout, + RateLimit: s.cfg.rateLimit, + UserAgent: s.cfg.userAgent, + FollowRedirects: s.cfg.followRedirects, + RespectRobots: s.cfg.respectRobots, + Exclude: s.cfg.exclude, + AuthHeader: s.cfg.authHeader, + AuthValue: s.cfg.authValue, + CookieJar: s.cfg.cookieJar, + AllowPrivateIPs: !s.cfg.blockPrivateIPs, + PrivateIPAllowlist: allowPrivateAddrs, + Logger: s.cfg.logger, + MaxPages: s.cfg.maxPages, } if s.cfg.circuitBreakerOn { @@ -213,5 +223,9 @@ func (s *Scanner) ScanDir(ctx context.Context, dir string) (*Report, error) { return nil, err } defer srv.Close() - return s.Scan(ctx, "http://"+addr) + // The temporary file server listens on loopback, which the crawler's + // SSRF protection (enabled by default) would reject. Exempt exactly + // this listener address for the duration of the scan so ScanDir works + // with default options; every other private address stays blocked. + return s.scan(ctx, "http://"+addr, []string{addr}) }