From c437339bda9f17aa20a479c1f2f5e29be4da27f1 Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Fri, 15 May 2026 18:04:10 +0800 Subject: [PATCH] Separate observation from evaluation in blacklist sources Each source's Query() method previously set r.Listed and r.Severity, embedding verdict logic inside the prober. Evaluation now lives in a dedicated Evaluate(SourceResult) (bool, string) method per source, keeping Query() as pure observation. A package-level EvaluateResult() helper looks up the source by ID and delegates to its Evaluate method; rules.go, report.go, types.go, and provider.go all call this instead of reading pre-set r.Listed/r.Severity values. An unknownSource sentinel handles results whose source is no longer registered. --- checker/dnsbl.go | 9 +++++++-- checker/openphish.go | 9 +++++++-- checker/provider.go | 2 +- checker/report.go | 16 +++++++++------- checker/report_test.go | 9 +++++---- checker/rule.go | 29 +++++++++++++++++++++++++---- checker/safebrowsing.go | 9 +++++++-- checker/source.go | 22 ++++++++++++++++++++++ checker/types.go | 4 ++-- checker/urlhaus.go | 9 +++++++-- checker/urlhaus_test.go | 12 ++++++++---- checker/virustotal.go | 28 +++++++++++++++++----------- checker/virustotal_test.go | 9 ++++++--- 13 files changed, 123 insertions(+), 44 deletions(-) diff --git a/checker/dnsbl.go b/checker/dnsbl.go index e0d7dff..34fa68a 100644 --- a/checker/dnsbl.go +++ b/checker/dnsbl.go @@ -160,8 +160,6 @@ func (s *dnsblSource) queryOne(ctx context.Context, registered string, z DNSBLZo res.BlockedQuery = true return res } - res.Listed = true - res.Severity = SeverityCrit if len(res.Reasons) == 0 { res.Reasons = append(res.Reasons, "Listed (no detail decoded)") } @@ -176,6 +174,13 @@ func (s *dnsblSource) queryOne(ctx context.Context, registered string, z DNSBLZo return res } +func (*dnsblSource) Evaluate(r SourceResult) (bool, string) { + if r.Enabled && !r.BlockedQuery && r.Error == "" && len(r.Evidence) > 0 { + return true, SeverityCrit + } + return false, "" +} + func (*dnsblSource) Diagnose(res SourceResult) Diagnosis { return Diagnosis{ Severity: SeverityCrit, diff --git a/checker/openphish.go b/checker/openphish.go index 065652a..12ab8dc 100644 --- a/checker/openphish.go +++ b/checker/openphish.go @@ -63,8 +63,6 @@ func (s *openPhishSource) Query(ctx context.Context, domain, registered string, // Fall through with whatever the cache could provide. } if len(urls) > 0 { - res.Listed = true - res.Severity = SeverityCrit res.Reasons = []string{"Phishing"} for _, u := range urls { res.Evidence = append(res.Evidence, Evidence{Label: "URL", Value: u}) @@ -73,6 +71,13 @@ func (s *openPhishSource) Query(ctx context.Context, domain, registered string, return []SourceResult{res} } +func (*openPhishSource) Evaluate(r SourceResult) (bool, string) { + if r.Enabled && r.Error == "" && len(r.Evidence) > 0 { + return true, SeverityCrit + } + return false, "" +} + func (*openPhishSource) Diagnose(res SourceResult) Diagnosis { urls := make([]string, 0, len(res.Evidence)) for _, e := range res.Evidence { diff --git a/checker/provider.go b/checker/provider.go index fd4e0ea..de33c02 100644 --- a/checker/provider.go +++ b/checker/provider.go @@ -37,7 +37,7 @@ func (p *blacklistProvider) ExtractMetrics(ctx sdk.ReportContext, collectedAt ti continue } v := 0.0 - if r.Listed { + if listed, _ := EvaluateResult(r); listed { v = 1 } metrics = append(metrics, sdk.CheckMetric{ diff --git a/checker/report.go b/checker/report.go index 5649812..c823cf7 100644 --- a/checker/report.go +++ b/checker/report.go @@ -101,7 +101,7 @@ func diagnose(d *BlacklistData) []Diagnosis { var out []Diagnosis for _, r := range d.Results { - if !r.Listed { + if listed, _ := EvaluateResult(r); !listed { continue } if s, ok := byID[r.SourceID]; ok { @@ -189,7 +189,7 @@ func buildSections(d *BlacklistData) []sourceSection { // subject sources have at most one). Plain sources skip this. if dr, ok := byID[id].(DetailRenderer); ok { for _, r := range results { - if !r.Listed && len(r.Details) == 0 { + if listed, _ := EvaluateResult(r); !listed && len(r.Details) == 0 { continue } html, err := dr.RenderDetail(r) @@ -210,7 +210,7 @@ func sectionStatus(results []SourceResult) (string, string) { if r.Enabled { enabled++ } - if r.Listed { + if l, _ := EvaluateResult(r); l { listed++ } else if r.Error != "" { errs++ @@ -238,11 +238,12 @@ func subjectStatusLabel(r SourceResult) string { switch { case !r.Enabled: return "Disabled" - case r.Listed: - return "LISTED" case r.Error != "": return "Error" } + if listed, _ := EvaluateResult(r); listed { + return "LISTED" + } return "Clean" } @@ -250,11 +251,12 @@ func subjectStatusClass(r SourceResult) string { switch { case !r.Enabled: return "muted" - case r.Listed: - return r.Severity case r.Error != "": return "warn" } + if listed, severity := EvaluateResult(r); listed { + return severity + } return "ok" } diff --git a/checker/report_test.go b/checker/report_test.go index 1ff5fad..3215745 100644 --- a/checker/report_test.go +++ b/checker/report_test.go @@ -15,8 +15,9 @@ func TestDiagnoseAndReportRender(t *testing.T) { { SourceID: "dnsbl", SourceName: "Spamhaus DBL", Subject: "dbl.spamhaus.org", - Enabled: true, Listed: true, Severity: SeverityCrit, - Reasons: []string{"Phishing domain"}, + Enabled: true, + Reasons: []string{"Phishing domain"}, + Evidence: []Evidence{{Label: "Return code", Value: "127.0.1.4"}}, LookupURL: "https://check.spamhaus.org/results/?query=example.com", RemovalURL: "https://www.spamhaus.org/dbl/removal/", }, @@ -27,7 +28,7 @@ func TestDiagnoseAndReportRender(t *testing.T) { }, { SourceID: "openphish", SourceName: "OpenPhish feed", - Enabled: true, Listed: true, Severity: SeverityCrit, + Enabled: true, Evidence: []Evidence{{Label: "URL", Value: "http://example.com/login"}}, }, }, @@ -66,7 +67,7 @@ func TestHeadline(t *testing.T) { } func TestSectionStatus(t *testing.T) { - if l, c := sectionStatus([]SourceResult{{Enabled: true, Listed: true, Severity: SeverityCrit}}); c != "crit" || !strings.HasPrefix(l, "LISTED") { + if l, c := sectionStatus([]SourceResult{{SourceID: "openphish", Enabled: true, Evidence: []Evidence{{Label: "URL", Value: "http://evil.com/"}}}}); c != "crit" || !strings.HasPrefix(l, "LISTED") { t.Errorf("sectionStatus listed = %q/%q", l, c) } if l, c := sectionStatus([]SourceResult{{Enabled: true}}); c != "ok" || l != "Clean" { diff --git a/checker/rule.go b/checker/rule.go index d389026..72d5c38 100644 --- a/checker/rule.go +++ b/checker/rule.go @@ -40,18 +40,28 @@ func (*sourceRule) Evaluate(ctx context.Context, obs sdk.ObservationGetter, opts }} } + byID := make(map[string]Source, len(Sources())) + for _, s := range Sources() { + byID[s.ID()] = s + } + out := make([]sdk.CheckState, 0, len(data.Results)) for _, r := range data.Results { - out = append(out, evaluateOne(r)) + src, ok := byID[r.SourceID] + if !ok { + src = unknownSource{} + } + out = append(out, evaluateOne(r, src)) } return out } -func evaluateOne(r SourceResult) sdk.CheckState { +func evaluateOne(r SourceResult, src Source) sdk.CheckState { subj := r.SourceName if r.Subject != "" && r.Subject != r.SourceName { subj = r.SourceName + " / " + r.Subject } + listed, severity := src.Evaluate(r) switch { case !r.Enabled: return sdk.CheckState{ @@ -72,9 +82,9 @@ func evaluateOne(r SourceResult) sdk.CheckState { Message: subj + ": query failed: " + r.Error, Code: "source_error", } - case r.Listed: + case listed: return sdk.CheckState{ - Status: severityToStatus(r.Severity), + Status: severityToStatus(severity), Subject: subj, Message: fmt.Sprintf("Listed in %s: %s", subj, joinNonEmpty(r.Reasons, "; ")), Code: "source_listed", @@ -95,6 +105,17 @@ func evaluateOne(r SourceResult) sdk.CheckState { } } +// unknownSource is a sentinel used when a SourceResult references a source ID +// that is no longer in the registry. Evaluate always returns (false, ""). +type unknownSource struct{} + +func (unknownSource) ID() string { return "" } +func (unknownSource) Name() string { return "unknown" } +func (unknownSource) Options() SourceOptions { return SourceOptions{} } +func (unknownSource) Query(_ context.Context, _, _ string, _ sdk.CheckerOptions) []SourceResult { return nil } +func (unknownSource) Diagnose(_ SourceResult) Diagnosis { return Diagnosis{} } +func (unknownSource) Evaluate(_ SourceResult) (bool, string) { return false, "" } + func severityToStatus(sev string) sdk.Status { switch sev { case SeverityCrit: diff --git a/checker/safebrowsing.go b/checker/safebrowsing.go index a9a9c03..be489a7 100644 --- a/checker/safebrowsing.go +++ b/checker/safebrowsing.go @@ -124,8 +124,6 @@ func (s *safeBrowsingSource) Query(ctx context.Context, domain, registered strin if len(parsed.Matches) == 0 { return []SourceResult{res} } - res.Listed = true - res.Severity = SeverityCrit res.Reference = "https://transparencyreport.google.com/safe-browsing/search?url=" + registered seenType := map[string]bool{} for _, m := range parsed.Matches { @@ -143,6 +141,13 @@ func (s *safeBrowsingSource) Query(ctx context.Context, domain, registered strin return []SourceResult{res} } +func (*safeBrowsingSource) Evaluate(r SourceResult) (bool, string) { + if r.Enabled && r.Error == "" && len(r.Evidence) > 0 { + return true, SeverityCrit + } + return false, "" +} + func (*safeBrowsingSource) Diagnose(res SourceResult) Diagnosis { return Diagnosis{ Severity: SeverityCrit, diff --git a/checker/source.go b/checker/source.go index 793d919..cc8b15e 100644 --- a/checker/source.go +++ b/checker/source.go @@ -48,6 +48,13 @@ type Source interface { // generic report wraps it with the title bar and severity styling. // Called only when SourceResult.Listed is true. Diagnose(res SourceResult) Diagnosis + + // Evaluate inspects an already-collected SourceResult and returns + // whether the domain is considered listed and at what severity. + // Implementations must read observation fields only (Evidence, + // Reasons, Error, Enabled, BlockedQuery, Details) and must never + // consult r.Listed or r.Severity. + Evaluate(r SourceResult) (listed bool, severity string) } // DetailRenderer is an optional interface a Source can implement when @@ -144,3 +151,18 @@ func Sources() []Source { copy(out, registry) return out } + +// EvaluateResult looks up the source that produced r from the registry +// and delegates to its Evaluate method. Returns (false, "") when the +// source is not found — a safe default that never promotes a stale +// Listed=true value. +func EvaluateResult(r SourceResult) (bool, string) { + registryMu.RLock() + defer registryMu.RUnlock() + for _, s := range registry { + if s.ID() == r.SourceID { + return s.Evaluate(r) + } + } + return false, "" +} diff --git a/checker/types.go b/checker/types.go index 4022687..48ba06c 100644 --- a/checker/types.go +++ b/checker/types.go @@ -31,7 +31,7 @@ type BlacklistData struct { func (d *BlacklistData) TotalHits() int { n := 0 for _, r := range d.Results { - if r.Listed { + if listed, _ := EvaluateResult(r); listed { n++ } } @@ -43,7 +43,7 @@ func (d *BlacklistData) TotalHits() int { func (d *BlacklistData) FilterListed() []SourceResult { out := make([]SourceResult, 0, len(d.Results)) for _, r := range d.Results { - if r.Listed { + if listed, _ := EvaluateResult(r); listed { out = append(out, r) } } diff --git a/checker/urlhaus.go b/checker/urlhaus.go index 4636ac8..9d9341e 100644 --- a/checker/urlhaus.go +++ b/checker/urlhaus.go @@ -120,8 +120,6 @@ func (s *urlhausSource) Query(ctx context.Context, domain, registered string, op if len(parsed.URLs) == 0 { return []SourceResult{res} } - res.Listed = true - res.Severity = SeverityCrit threats := map[string]bool{} details := urlhausDetails{} for _, u := range parsed.URLs { @@ -148,6 +146,13 @@ func (s *urlhausSource) Query(ctx context.Context, domain, registered string, op return []SourceResult{res} } +func (*urlhausSource) Evaluate(r SourceResult) (bool, string) { + if r.Enabled && r.Error == "" && len(r.Evidence) > 0 { + return true, SeverityCrit + } + return false, "" +} + func (*urlhausSource) Diagnose(res SourceResult) Diagnosis { online := 0 for _, e := range res.Evidence { diff --git a/checker/urlhaus_test.go b/checker/urlhaus_test.go index 0a10921..9128efd 100644 --- a/checker/urlhaus_test.go +++ b/checker/urlhaus_test.go @@ -24,8 +24,9 @@ func TestURLhausSource_NoResults(t *testing.T) { t.Fatalf("expected 1 result, got %d", len(results)) } r := results[0] - if !r.Enabled || r.Listed || r.Error != "" { - t.Fatalf("expected enabled+clean, got %+v", r) + listed, _ := s.Evaluate(r) + if !r.Enabled || listed || r.Error != "" { + t.Fatalf("expected enabled+clean, got %+v, Evaluate listed=%v", r, listed) } } @@ -48,8 +49,11 @@ func TestURLhausSource_Listed(t *testing.T) { s := &urlhausSource{endpoint: srv.URL} r := s.Query(context.Background(), "example.com", "example.com", sdk.CheckerOptions{"enable_urlhaus": true, "urlhaus_auth_key": "k"})[0] - if !r.Listed || len(r.Evidence) != 1 { - t.Fatalf("expected 1 listed evidence, got %+v", r) + if len(r.Evidence) != 1 { + t.Fatalf("expected 1 evidence item, got %+v", r) + } + if listed, severity := s.Evaluate(r); !listed || severity != SeverityCrit { + t.Errorf("expected Evaluate()=(true, crit), got (%v, %q)", listed, severity) } if r.Evidence[0].Status != "online" { t.Errorf("evidence status = %q", r.Evidence[0].Status) diff --git a/checker/virustotal.go b/checker/virustotal.go index 5213df9..bbd946e 100644 --- a/checker/virustotal.go +++ b/checker/virustotal.go @@ -146,17 +146,6 @@ func (s *virusTotalSource) Query(ctx context.Context, domain, registered string, return d.Vendors[i].Engine < d.Vendors[j].Engine }) res.Details = mustJSON(d) - - if d.Malicious == 0 && d.Suspicious == 0 { - // Clean. - return []SourceResult{res} - } - res.Listed = true - if d.Malicious > 0 { - res.Severity = SeverityCrit - } else { - res.Severity = SeverityWarn - } for _, v := range d.Vendors { res.Reasons = append(res.Reasons, v.Engine) res.Evidence = append(res.Evidence, Evidence{ @@ -167,6 +156,23 @@ func (s *virusTotalSource) Query(ctx context.Context, domain, registered string, return []SourceResult{res} } +func (*virusTotalSource) Evaluate(r SourceResult) (bool, string) { + var d vtDetails + if len(r.Details) == 0 { + return false, "" + } + if err := json.Unmarshal(r.Details, &d); err != nil { + return false, "" + } + if d.Malicious == 0 && d.Suspicious == 0 { + return false, "" + } + if d.Malicious > 0 { + return true, SeverityCrit + } + return true, SeverityWarn +} + func (*virusTotalSource) Diagnose(res SourceResult) Diagnosis { var d vtDetails _ = json.Unmarshal(res.Details, &d) diff --git a/checker/virustotal_test.go b/checker/virustotal_test.go index aba955c..e53d2cc 100644 --- a/checker/virustotal_test.go +++ b/checker/virustotal_test.go @@ -46,8 +46,8 @@ func TestVTSource_Listed(t *testing.T) { s := &virusTotalSource{endpoint: endpoint} r := s.Query(context.Background(), "example.com", "example.com", sdk.CheckerOptions{"virustotal_api_key": "k"})[0] - if !r.Listed || r.Severity != SeverityCrit { - t.Errorf("expected listed+crit, got %+v", r) + if listed, severity := s.Evaluate(r); !listed || severity != SeverityCrit { + t.Errorf("expected Evaluate()=(true, crit), got (%v, %q)", listed, severity) } var d vtDetails if err := json.Unmarshal(r.Details, &d); err != nil { @@ -72,9 +72,12 @@ func TestVTSource_NotFound(t *testing.T) { s := &virusTotalSource{endpoint: endpoint} r := s.Query(context.Background(), "example.com", "example.com", sdk.CheckerOptions{"virustotal_api_key": "k"})[0] - if r.Listed || r.Error != "" { + if r.Error != "" { t.Errorf("404 should be quiet not-listed: %+v", r) } + if listed, severity := s.Evaluate(r); listed || severity != "" { + t.Errorf("Evaluate() on clean result = (%v, %q), want (false, \"\")", listed, severity) + } if !strings.Contains(r.Reference, "example.com") { t.Errorf("reference URL missing: %+v", r) }