fix(web_search): address code-review findings (answers, SSRF, secret, cold-start)

Code review of the SearXNG integration surfaced four actionable issues; fixes:

#2 (confirmed) — answers parsing emitted raw answer-object JSON. SearXNG
`answers` are objects ({"answer":"...","url":...,"template":...}), not bare
strings, so strings.Trim(rawJSON,'"') left the {...} blob intact. Decode the
`answer` field out of each object instead (also drops the fragile Trim). The
test mock used a string array, masking this — corrected to objects.

#3 (defense-in-depth) — the http.Client had no CheckRedirect. A compromised
or misconfigured SearXNG could 3xx the client toward an internal/metadata
endpoint (SSRF). Install the same per-hop re-classification guard browser and
http_batch use, capped at 10 hops. New TestWebSearch_RedirectToInternalBlocked.

#1 (hardening) — the mounted settings.yml hardcoded secret_key
"change-me-in-dot-env". Deeper tracing showed SEARXNG_SECRET *does* override
the file at app load (searx/settings_defaults.py environ_name), so the env
wiring worked — but the placeholder defeated SearXNG's built-in "secret_key
is not changed" warning, which only fires for the canonical "ultrasecretkey".
Switch the placeholder + the compose env fallback to "ultrasecretkey" so an
unset secret is loudly flagged rather than silently weak. Comments/.env.example
corrected to describe the real (app-load) override mechanism.

#4 (reliability) — `depends_on: [searxng]` only waits for container start, so
the first web_search after `compose up` could race SearXNG readiness. Rather
than a Docker healthcheck (whose probe tooling I can't verify in the upstream
image — a broken probe would deadlock odek startup), make the tool resilient:
retry only on ECONNREFUSED (the precise "up but not yet listening" signal),
2 extra attempts with a 1s backoff. Timeouts / genuine-down fail fast.

#5 (overlay whole-pointer replace) intentionally deferred — it is consistent
with the existing Skills/Memory/Transcription/Vision merge behavior; fixing
only web_search would be inconsistent.

Full suite green under -race; vet/gofmt clean; compose config validates.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: jkyberneees

cmd/odek/web_search_tool.go

@@ -2,11 +2,13 @@ package main
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
 	"net/url"
 	"strings"
+	"syscall"
 	"time"
 
 	"github.com/BackendStack21/odek"
@@ -18,6 +20,15 @@ import (
 // metasearch JSON payload is small; this guards against a misbehaving backend).
 const maxSearXNGBody = 4 << 20 // 4 MiB
 
+// Cold-start retry: how many extra attempts (and the delay between them) to
+// make when SearXNG refuses the connection — covers the compose startup race
+// where the container is up but the app isn't yet listening. Vars (not consts)
+// so tests can shrink the delay.
+var (
+	searxngConnectRetries = 2
+	searxngRetryDelay     = time.Second
+)
+
 // ═════════════════════════════════════════════════════════════════════════
 // web_search Tool (SearXNG JSON API backend)
 // ═════════════════════════════════════════════════════════════════════════
@@ -33,11 +44,30 @@ func newWebSearchTool(dc danger.DangerousConfig, cfg config.WebSearchConfig) *we
 	if timeout <= 0 {
 		timeout = 15
 	}
-	return &webSearchTool{
-		dangerousConfig: dc,
-		cfg:             cfg,
-		client:          &http.Client{Timeout: time.Duration(timeout) * time.Second},
+	t := &webSearchTool{dangerousConfig: dc, cfg: cfg}
+	t.client = &http.Client{
+		Timeout:       time.Duration(timeout) * time.Second,
+		CheckRedirect: t.checkRedirect,
 	}
+	return t
+}
+
+// checkRedirect re-classifies every redirect hop. The configured base_url is
+// trusted, but a compromised, buggy, or misconfigured SearXNG could 3xx the
+// client toward an internal/metadata endpoint (SSRF). Re-classifying each hop —
+// the same guard browser/http_batch install — closes that. Installing
+// CheckRedirect disables Go's implicit 10-hop cap, so we re-impose it.
+func (t *webSearchTool) checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	target := req.URL.String()
+	if err := t.dangerousConfig.CheckOperation(danger.ToolOperation{
+		Name: "web_search", Resource: target, Risk: danger.ClassifyURL(target),
+	}, nil); err != nil {
+		return fmt.Errorf("redirect to %s blocked: %w", target, err)
+	}
+	return nil
 }
 
 func (t *webSearchTool) Name() string { return "web_search" }
@@ -82,7 +112,11 @@ type searxngResponse struct {
 		Content string `json:"content"`
 		Engine  string `json:"engine"`
 	} `json:"results"`
-	Answers     []json.RawMessage `json:"answers"`
+	// SearXNG answers are objects ({"answer": "...", "url": ..., "template": ...}),
+	// not bare strings — decode the answer text out of each.
+	Answers []struct {
+		Answer string `json:"answer"`
+	} `json:"answers"`
 	Infoboxes   []json.RawMessage `json:"infoboxes"`
 	Suggestions []string          `json:"suggestions"`
 }
@@ -159,8 +193,8 @@ func (t *webSearchTool) Call(argsJSON string) (result string, err error) {
 	}
 	out.Count = len(out.Results)
 	for _, a := range resp.Answers {
-		if s := strings.TrimSpace(string(a)); s != "" && s != "null" {
-			out.Answers = append(out.Answers, strings.Trim(s, `"`))
+		if s := strings.TrimSpace(a.Answer); s != "" {
+			out.Answers = append(out.Answers, s)
 		}
 	}
 
@@ -198,7 +232,18 @@ func (t *webSearchTool) query(query, category string) (*searxngResponse, error)
 	}
 	req.Header.Set("Accept", "application/json")
 
-	httpResp, err := t.client.Do(req)
+	// Retry only on connection-refused — the precise signal that the SearXNG
+	// sidecar is up as a container but not yet accepting connections (the
+	// startup race when both come up together under compose). Other errors
+	// (timeouts, DNS, genuine "down") fail fast on the first attempt.
+	var httpResp *http.Response
+	for attempt := 0; ; attempt++ {
+		httpResp, err = t.client.Do(req)
+		if err == nil || attempt >= searxngConnectRetries || !errors.Is(err, syscall.ECONNREFUSED) {
+			break
+		}
+		time.Sleep(searxngRetryDelay)
+	}
 	if err != nil {
 		return nil, fmt.Errorf("cannot reach SearXNG at %s — is the service running? (%v)", t.cfg.BaseURL, err)
 	}

cmd/odek/web_search_tool_test.go

@@ -6,6 +6,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/BackendStack21/odek/internal/config"
 	"github.com/BackendStack21/odek/internal/danger"
@@ -41,7 +42,10 @@ func mockSearXNG(t *testing.T, results int) (*httptest.Server, *string) {
 			})
 		}
 		resp["results"] = rs
-		resp["answers"] = []string{"42"}
+		// SearXNG answers are objects with an "answer" field, not bare strings.
+		resp["answers"] = []map[string]any{
+			{"answer": "42", "url": nil, "template": "answer/legacy.html"},
+		}
 		_ = json.NewEncoder(w).Encode(resp)
 	}))
 	t.Cleanup(srv.Close)
@@ -146,6 +150,12 @@ func TestWebSearch_JSONDisabled403(t *testing.T) {
 }
 
 func TestWebSearch_BackendUnreachable(t *testing.T) {
+	// Connection-refused triggers the cold-start retry; shrink the delay so the
+	// test exercises the retry path without the 1s production backoff.
+	orig := searxngRetryDelay
+	searxngRetryDelay = time.Millisecond
+	t.Cleanup(func() { searxngRetryDelay = orig })
+
 	// Point at a closed port on localhost.
 	tool := newWebSearchTool(allowAllDanger(), config.WebSearchConfig{BaseURL: "http://127.0.0.1:1"})
 	raw, _ := tool.Call(`{"query":"x"}`)
@@ -155,6 +165,31 @@ func TestWebSearch_BackendUnreachable(t *testing.T) {
 	}
 }
 
+func TestWebSearch_RedirectToInternalBlocked(t *testing.T) {
+	// A compromised/misconfigured SearXNG that 302s toward an internal host must
+	// be stopped by the CheckRedirect guard, not followed (SSRF defense).
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound)
+	}))
+	t.Cleanup(srv.Close)
+
+	// Deny network egress so the redirect-hop re-classification blocks the hop.
+	dc := danger.DangerousConfig{Classes: map[danger.RiskClass]danger.Action{
+		danger.NetworkEgress: danger.Allow, // initial query allowed
+		danger.SystemWrite:   danger.Deny,  // internal/metadata target denied
+	}}
+	tool := newWebSearchTool(dc, config.WebSearchConfig{BaseURL: srv.URL})
+
+	raw, _ := tool.Call(`{"query":"x"}`)
+	out := decodeWebSearch(t, raw)
+	if out.Error == "" {
+		t.Fatalf("expected redirect to internal host to be blocked, got results: %+v", out)
+	}
+	if !strings.Contains(out.Error, "blocked") && !strings.Contains(out.Error, "cannot reach") {
+		t.Errorf("expected a redirect-blocked error, got %q", out.Error)
+	}
+}
+
 func TestWebSearch_DeniedByPolicy(t *testing.T) {
 	// NetworkEgress denied → the tool returns the denial as an error result.
 	dc := danger.DangerousConfig{Classes: map[danger.RiskClass]danger.Action{

docker/.env.example

@@ -63,6 +63,8 @@ GIT_COMMITTER_EMAIL=you@example.com
 
 # ── Web search (SearXNG sidecar; see docs/DOCKER_COMPOSE_USER_GUIDE.md) ──
 # The compose file runs a private SearXNG instance backing the `web_search`
-# tool. Set a strong random secret (e.g. `openssl rand -hex 32`). Required by
-# SearXNG; the instance is internal-only (no host port) but set it anyway.
+# tool. SearXNG reads this as server.secret_key at app load and it overrides
+# settings.yml. Set a strong random secret (e.g. `openssl rand -hex 32`). The
+# instance is internal-only (no host port) but set it anyway — left unset,
+# SearXNG runs with the "ultrasecretkey" sentinel and logs a warning.
 SEARXNG_SECRET=change-me-run-openssl-rand-hex-32

docker/docker-compose.yml

@@ -108,7 +108,11 @@ services:
     image: searxng/searxng:2026.6.8-f3fab143b   # pinned; bump deliberately
     environment:
       - SEARXNG_BASE_URL=http://searxng:8080/
-      - SEARXNG_SECRET=${SEARXNG_SECRET:-change-me-in-dot-env}
+      # SearXNG reads server.secret_key from this env var at app load, overriding
+      # settings.yml. Set SEARXNG_SECRET in .env. The fallback is the canonical
+      # "ultrasecretkey" sentinel so an unset secret triggers SearXNG's own
+      # "secret_key is not changed" warning rather than a silent weak literal.
+      - SEARXNG_SECRET=${SEARXNG_SECRET:-ultrasecretkey}
     volumes:
       - ./searxng/settings.yml:/etc/searxng/settings.yml:ro
     # SearXNG needs outbound internet to query upstream engines (Google, Bing,

docker/searxng/settings.yml

@@ -19,8 +19,13 @@ general:
   enable_metrics: false
 
 server:
-  # Must be overridden from the default; injected via $SEARXNG_SECRET in compose.
-  secret_key: "change-me-in-dot-env"
+  # SearXNG reads server.secret_key from the SEARXNG_SECRET env var at app load
+  # (searx/settings_defaults.py: environ_name='SEARXNG_SECRET'), which overrides
+  # this value — set SEARXNG_SECRET in .env. Keep this as the canonical
+  # "ultrasecretkey" placeholder so that, if SEARXNG_SECRET is ever left unset,
+  # SearXNG logs its built-in "secret_key is not changed" warning instead of
+  # silently running with a custom-but-weak literal.
+  secret_key: "ultrasecretkey"
   bind_address: "0.0.0.0"
   port: 8080
   # Single trusted consumer on a private network — disable the bot limiter