From 01523f634b76654811230c56000638abe2f6c148 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Mon, 5 Oct 2026 13:27:07 -0400 Subject: [PATCH] Reject malformed search bodies and report unranked candidates SearXNG now requires a results array and Brave requires "type": "search" with a results array in any web object, so `{}` is ErrSearchResponse instead of an empty search. Rank no longer drops failed candidates: it returns the ranked ones with ErrCandidatesUnranked joined with each unranked candidate's URL and reason, or ErrNoUsableRecipe when none rank. Adds an HTTPS ranking test through the SSRF-safe Fetcher. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 16 +++-- internal/recipes/rank.go | 26 +++++++-- internal/recipes/rank_test.go | 100 +++++++++++++++++++++++++++++++- internal/recipes/search.go | 34 ++++++++--- internal/recipes/search_test.go | 23 +++++++- 5 files changed, 177 insertions(+), 22 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f9cd825..44418ec 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -248,15 +248,21 @@ automatic** (D-021). key decrypted from `recipe_search_settings.api_key_ct` (`recipes.NewSearchProvider`). Redirects are not followed; 401/403 is `ErrSearchRefused`, 429 `ErrSearchRateLimited`, other non-2xx a - `SearchStatusError`, and a body that is not the provider's JSON, over - 1 MB or with a result lacking an absolute http(s) URL `ErrSearchResponse`. + `SearchStatusError`, and a body that is not the provider's JSON (SearXNG + without a `results` array; Brave without `"type": "search"` or with a + `web` object lacking `results` — Brave omits `web` when nothing matched), + over 1 MB or with a result lacking an absolute http(s) URL + `ErrSearchResponse`. - Ranking (`recipes.Rank`) fetches and extracts the top 5 candidates concurrently, rejects any whose recipe lacks ingredients or an image, and orders the rest by the token similarity of the recipe's title to the dish name (`harness.Similarity`, 0–1) plus a quarter for each of ingredients, - image, steps and total minutes; ties keep search order. When every - candidate is rejected the error is `ErrNoUsableRecipe` joined with each - reason. + image, steps and total minutes; ties keep search order. No unranked + candidate is dropped silently: when some are ranked, they are returned + with `ErrCandidatesUnranked`; when none are, the error is + `ErrNoUsableRecipe`. Either is joined with each unranked candidate's URL + and reason (fetch failure, no valid recipe, missing ingredients or + image), so the caller decides per item what a failure means. - Photos are cached locally (`photos`) and processed with `golang.org/x/image`. diff --git a/internal/recipes/rank.go b/internal/recipes/rank.go index e025f35..8d3c254 100644 --- a/internal/recipes/rank.go +++ b/internal/recipes/rank.go @@ -20,9 +20,13 @@ var ( ErrNoIngredients = errors.New("recipes: recipe has no ingredients") // ErrNoImage rejects a candidate whose recipe has no image. ErrNoImage = errors.New("recipes: recipe has no image") - // ErrNoUsableRecipe is returned when Rank rejects every candidate; it - // wraps each candidate's reason. + // ErrNoUsableRecipe is returned when Rank ranks no candidate; it is + // joined with each candidate's reason. ErrNoUsableRecipe = errors.New("recipes: no candidate holds a usable recipe") + // ErrCandidatesUnranked is returned, alongside the ranked candidates, + // when Rank ranks some candidates but not all; it is joined with each + // unranked candidate's reason. + ErrCandidatesUnranked = errors.New("recipes: some candidates were not ranked") ) // PageFetcher fetches a candidate's page; *Fetcher satisfies it. @@ -42,8 +46,15 @@ type Ranked struct { // Rank fetches and extracts the first RankDepth candidates concurrently and // returns those whose recipe has ingredients and an image, highest Score -// first; equal scores keep search order. When every candidate is rejected -// the error is ErrNoUsableRecipe joined with each rejection's reason. +// first; equal scores keep search order. +// +// Every candidate that is not ranked — its fetch failed, its page holds no +// valid recipe, or the recipe lacks ingredients or an image — is reported, +// never dropped silently: when some are ranked the error is +// ErrCandidatesUnranked, returned together with the ranked ones; when none +// are it is ErrNoUsableRecipe with no results. Either is joined with each +// unranked candidate's URL and reason. A cancelled ctx returns only its +// error. func Rank(ctx context.Context, f PageFetcher, dish string, cands []Candidate) ([]Ranked, error) { if len(cands) > RankDepth { cands = cands[:RankDepth] @@ -68,7 +79,7 @@ func Rank(ctx context.Context, f PageFetcher, dish string, cands []Candidate) ([ } var ranked []Ranked - reasons := []error{ErrNoUsableRecipe} + var reasons []error for i, o := range outcomes { if o.err != nil { reasons = append(reasons, fmt.Errorf("%s: %w", cands[i].URL, o.err)) @@ -77,9 +88,12 @@ func Rank(ctx context.Context, f PageFetcher, dish string, cands []Candidate) ([ ranked = append(ranked, o.ranked) } if len(ranked) == 0 { - return nil, errors.Join(reasons...) + return nil, errors.Join(append([]error{ErrNoUsableRecipe}, reasons...)...) } sort.SliceStable(ranked, func(a, b int) bool { return ranked[a].Score > ranked[b].Score }) + if len(reasons) > 0 { + return ranked, errors.Join(append([]error{ErrCandidatesUnranked}, reasons...)...) + } return ranked, nil } diff --git a/internal/recipes/rank_test.go b/internal/recipes/rank_test.go index b70c67b..a8d91ce 100644 --- a/internal/recipes/rank_test.go +++ b/internal/recipes/rank_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "net/http" + "strings" "sync" "testing" ) @@ -66,8 +68,11 @@ func TestRankOrder(t *testing.T) { {URL: "https://f.example/toodeep"}, } got, err := Rank(context.Background(), f, "Chicken curry", cands) - if err != nil { - t.Fatal(err) + if !errors.Is(err, ErrCandidatesUnranked) || !errors.Is(err, ErrNoImage) || !errors.Is(err, ErrNoIngredients) { + t.Errorf("err = %v, want ErrCandidatesUnranked with ErrNoImage and ErrNoIngredients", err) + } + if errors.Is(err, ErrNoUsableRecipe) { + t.Errorf("err = %v, want not ErrNoUsableRecipe when some are ranked", err) } want := []struct { url string @@ -119,6 +124,29 @@ func TestRankTiesKeepSearchOrder(t *testing.T) { } } +// TestRankReportsFetchFailure checks a failed fetch is reported with the +// candidates that were ranked, never swallowed. +func TestRankReportsFetchFailure(t *testing.T) { + f := &fakeFetcher{pages: map[string][]byte{ + "https://a.example/1": recipePage("Beef Stew", true, true, true, true), + }} + cands := []Candidate{{URL: "https://a.example/1"}, {URL: "https://b.example/missing"}} + got, err := Rank(context.Background(), f, "beef stew", cands) + if len(got) != 1 || got[0].Candidate.URL != "https://a.example/1" { + t.Errorf("ranked = %+v, want only https://a.example/1", got) + } + if !errors.Is(err, ErrCandidatesUnranked) { + t.Errorf("err = %v, want ErrCandidatesUnranked", err) + } + var se *StatusError + if !errors.As(err, &se) || se.Status != 404 || se.URL != "https://b.example/missing" { + t.Errorf("err = %v, want the 404 StatusError for https://b.example/missing", err) + } + if err != nil && !strings.Contains(err.Error(), "https://b.example/missing") { + t.Errorf("err = %v, want it to name the failed candidate", err) + } +} + func TestRankAllRejected(t *testing.T) { f := &fakeFetcher{pages: map[string][]byte{ "https://a.example/noimg": recipePage("Chicken Curry", true, false, true, true), @@ -157,3 +185,71 @@ func TestRankContextCancelled(t *testing.T) { t.Errorf("err = %v, want context.Canceled", err) } } + +// TestRankOverHTTP ranks fixture candidates served over HTTPS through the +// SSRF-safe Fetcher: every host, a subdomain the test server's certificate +// covers, resolves to a public test address the test +// dialer routes to one httptest server, which serves pages by host and path. +func TestRankOverHTTP(t *testing.T) { + t.Parallel() + pages := map[string][]byte{ + "best.example.com/curry": recipePage("Chicken Curry", true, true, true, true), + "easy.example.com/curry": recipePage("Easy Chicken Curry", true, true, true, false), + "thai.example.com/curry": recipePage("Thai Green Curry", true, true, true, true), + "noimg.example.com/curry": recipePage("Chicken Curry", true, false, true, true), + } + env := newTestEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, ok := pages[r.Host+r.URL.Path] + if !ok { + http.Error(w, "broken", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "text/html") + w.Write(body) + }), map[string][][]string{ + "best.example.com": {{publicIP}}, + "easy.example.com": {{publicIP}}, + "thai.example.com": {{publicIP}}, + "noimg.example.com": {{publicIP}}, + "broken.example.com": {{publicIP}}, + }) + cands := []Candidate{ + {URL: "https://thai.example.com/curry"}, + {URL: "https://broken.example.com/curry"}, + {URL: "https://easy.example.com/curry"}, + {URL: "https://noimg.example.com/curry"}, + {URL: "https://best.example.com/curry"}, + } + + got, err := Rank(t.Context(), env.f, "Chicken curry", cands) + + want := []struct { + url string + score float64 + }{ + {"https://best.example.com/curry", 2}, + {"https://easy.example.com/curry", 1.75}, + {"https://thai.example.com/curry", 1.25}, + } + if len(got) != len(want) { + t.Fatalf("got %d ranked, want %d: %+v (err %v)", len(got), len(want), got, err) + } + for i, w := range want { + if got[i].Candidate.URL != w.url || got[i].Score != w.score { + t.Errorf("rank %d = %s (%.2f), want %s (%.2f)", i, got[i].Candidate.URL, got[i].Score, w.url, w.score) + } + } + if got[0].Recipe.Title != "Chicken Curry" || got[0].Recipe.ImageURL != "https://img.example.com/dish.jpg" { + t.Errorf("top recipe = %+v", got[0].Recipe) + } + if !errors.Is(err, ErrCandidatesUnranked) || !errors.Is(err, ErrNoImage) { + t.Errorf("err = %v, want ErrCandidatesUnranked with ErrNoImage", err) + } + var se *StatusError + if !errors.As(err, &se) || se.Status != http.StatusInternalServerError { + t.Errorf("err = %v, want the 500 StatusError for broken.example.com", err) + } + if n := len(env.dials()); n != len(cands) { + t.Errorf("dialed %d times, want %d", n, len(cands)) + } +} diff --git a/internal/recipes/search.go b/internal/recipes/search.go index 6a17a86..6aec8e0 100644 --- a/internal/recipes/search.go +++ b/internal/recipes/search.go @@ -36,8 +36,10 @@ var ( // ErrSearchRateLimited is returned for HTTP 429. ErrSearchRateLimited = errors.New("recipes: search provider rate limit reached") // ErrSearchResponse is returned for a response body that is not the - // provider's JSON shape, is over MaxSearchBodyBytes, or holds a result - // without an absolute http(s) URL. + // provider's JSON shape — SearXNG without a results array, Brave without + // "type": "search" or with a web object lacking a results array — is + // over MaxSearchBodyBytes, or holds a result without an absolute http(s) + // URL. ErrSearchResponse = errors.New("recipes: malformed search response") ) @@ -126,8 +128,10 @@ func (s *SearXNG) Search(ctx context.Context, query string, n int) ([]Candidate, if err != nil { return nil, fmt.Errorf("recipes: build SearXNG request: %w", err) } + // Results is a pointer so a body without the array is told apart from + // an empty search. var body struct { - Results []struct { + Results *[]struct { URL string `json:"url"` Title string `json:"title"` Content string `json:"content"` @@ -136,8 +140,11 @@ func (s *SearXNG) Search(ctx context.Context, query string, n int) ([]Candidate, if err := doSearch(s.client, req, "SearXNG", &body); err != nil { return nil, err } + if body.Results == nil { + return nil, fmt.Errorf("%w: SearXNG body has no results array", ErrSearchResponse) + } var out []Candidate - for _, r := range body.Results { + for _, r := range *body.Results { out = append(out, Candidate{URL: r.URL, Title: r.Title, Snippet: r.Content}) } return candidates("SearXNG", out, n) @@ -177,9 +184,13 @@ func (b *Brave) Search(ctx context.Context, query string, n int) ([]Candidate, e return nil, fmt.Errorf("recipes: build Brave request: %w", err) } req.Header.Set("X-Subscription-Token", b.key) + // Brave leaves out "web" when nothing matched, so a search response is + // recognised by its "type"; a web object present must hold the results + // array. var body struct { - Web struct { - Results []struct { + Type string `json:"type"` + Web *struct { + Results *[]struct { URL string `json:"url"` Title string `json:"title"` Description string `json:"description"` @@ -189,8 +200,17 @@ func (b *Brave) Search(ctx context.Context, query string, n int) ([]Candidate, e if err := doSearch(b.client, req, "Brave", &body); err != nil { return nil, err } + if body.Type != "search" { + return nil, fmt.Errorf("%w: Brave body type is %q, not \"search\"", ErrSearchResponse, body.Type) + } + if body.Web == nil { + return nil, nil + } + if body.Web.Results == nil { + return nil, fmt.Errorf("%w: Brave web object has no results array", ErrSearchResponse) + } var out []Candidate - for _, r := range body.Web.Results { + for _, r := range *body.Web.Results { out = append(out, Candidate{URL: r.URL, Title: r.Title, Snippet: r.Description}) } return candidates("Brave", out, n) diff --git a/internal/recipes/search_test.go b/internal/recipes/search_test.go index b3a181a..8bda597 100644 --- a/internal/recipes/search_test.go +++ b/internal/recipes/search_test.go @@ -166,8 +166,11 @@ func TestSearchErrors(t *testing.T) { return errors.As(err, &se) && se.Status == http.StatusFound }}, {"not JSON", http.StatusOK, ``, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, - {"result without URL", http.StatusOK, `{"results":[{"title":"x"}],"web":{"results":[{"title":"x"}]}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, - {"relative URL", http.StatusOK, `{"results":[{"url":"/x"}],"web":{"results":[{"url":"/x"}]}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, + {"result without URL", http.StatusOK, `{"type":"search","results":[{"title":"x"}],"web":{"results":[{"title":"x"}]}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, + {"relative URL", http.StatusOK, `{"type":"search","results":[{"url":"/x"}],"web":{"results":[{"url":"/x"}]}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, + {"empty object", http.StatusOK, `{}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, + {"null results", http.StatusOK, `{"type":"search","results":null,"web":{"results":null}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, + {"results not an array", http.StatusOK, `{"type":"search","results":{},"web":{"results":{}}}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, {"body too large", http.StatusOK, `{"results":[],"pad":"` + strings.Repeat("x", MaxSearchBodyBytes) + `"}`, func(err error) bool { return errors.Is(err, ErrSearchResponse) }}, } for _, tc := range tests { @@ -189,6 +192,22 @@ func TestSearchErrors(t *testing.T) { } } +// TestSearchNoMatches checks a well-formed response with nothing found is an +// empty search, not an error. +func TestSearchNoMatches(t *testing.T) { + providers := map[string]SearchProvider{ + "SearXNG": newSearXNGServer(t, http.StatusOK, `{"query":"x recipe","results":[]}`, nil), + "Brave without web": newBraveServer(t, "secret-key", http.StatusOK, `{"type":"search","query":{"original":"x recipe"}}`, nil), + "Brave empty web": newBraveServer(t, "secret-key", http.StatusOK, `{"type":"search","web":{"type":"search","results":[]}}`, nil), + } + for name, p := range providers { + got, err := p.Search(context.Background(), "x", 3) + if err != nil || len(got) != 0 { + t.Errorf("%s: got %v, %v; want no candidates and no error", name, got, err) + } + } +} + func TestSearchContextCancelled(t *testing.T) { s := newSearXNGServer(t, http.StatusOK, searxngBody, nil) ctx, cancel := context.WithCancel(context.Background())