Repository navigation
fix(community): screen web_search result URLs in every provider - #7037
Conversation
The Tavily and Exa web_search tools returned provider URLs to the model unfiltered, so a loopback, link-local or metadata-service address in a result reached the agent. Brave and Serper already pass result URLs through safe_public_result_url; do the same here, blanking a non-public URL while keeping the row's title and snippet.
youcom, tencent_wsa, serply (url and scholar pdf_url), groundroute, sofya, ddg_search, fastcrw, firecrawl and searxng handed provider-supplied URLs to the model unfiltered. Route each through safe_public_result_url, as brave, serper, tavily and exa do: a non-public URL is blanked and the row's title and snippet are kept. The test now drives every listed provider's web_search tool with a stubbed provider payload carrying a metadata, loopback or public URL.
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at head 6057125. The 11 providers touched here screen their result URLs correctly: I ran backend/tests/test_image_result_url_safety.py against this exact head (92 passed) and ruff check / ruff format --check are clean on the changed files.
One important gap inline: the title's "every provider" claim does not hold at this head — brave, serper (text results) and webz still return web_search URLs verbatim, and serper's code carries an explicit comment documenting verbatim result links as a deliberate policy, which this PR reverses for 11 providers without reconciling it.
Note for maintainers: #7080 (he-yufeng, "fix(search): screen result URLs across all web_search providers") is an independent PR covering the same URL-screening concern — worth coordinating so the fixes don't conflict or duplicate each other.
| return [patch.object(module, "_get_searxng_client", return_value=client)], _async(module.web_search_tool) | ||
|
|
||
|
|
||
| _TEXT_SEARCH_STUBS = { |
There was a problem hiding this comment.
The "every provider" claim (title and PR body) does not hold at this head. 15 providers expose web_search; this PR covers 11, and three remain unscreened:
community/brave/tools.py— textweb_searchstill emits"url": r.get("url", "")verbatim (~line 176 at this head); only itsimage_searchpasses URLs through_safe_public_url.community/serper/tools.py— organic results still emit"url": r.get("link", "")verbatim (~line 437), and the comment directly above (~lines 431-433) explicitly documents the opposite policy: search links are deliberately not passed through_safe_public_urlbecause they are model citations. This PR silently reverses that documented policy for 11 providers without updating that comment — either the comment or this PR's scope needs to change so the codebase is consistent.community/webz/tools.py—"url": article.get("url", "")(~line 48), no screening at all.
infoquest's web_search also delegates payload shaping to its client, so its result URLs deserve a look too.
Suggestion: add brave/serper/webz stubs to _TEXT_SEARCH_STUBS so the test actually enforces "every provider", or rescope the title/body ("every provider" → the 11 changed providers) and update serper's comment to document the carve-out.
There was a problem hiding this comment.
Brave, Serper and Webz now screen result URLs too (InfoQuest already does on main), with stubs in the test table and the snippet asserted, in 496e5fb
| async def test_text_search_providers_screen_result_urls(provider, url, expected): | ||
| """web_search must not hand a non-public provider URL to the model (#7036). | ||
|
|
||
| Only the provider payload varies; the title and snippet stay in the row. |
There was a problem hiding this comment.
Nit: the docstring says "the title and snippet stay in the row", but only the title is asserted below (line 173). Since the snippet field name differs per provider (snippet vs content), consider asserting survival of the payload text, e.g. assert "c" in json.dumps(payload) after the url assertion, or per-provider snippet keys, so the retention claim is actually enforced.
The remaining text-search providers still returned result URLs verbatim. Serper's comment documented that policy; it now screens organic links like its image results. The shared provider test covers all fourteen providers and asserts the snippet survives screening.
github-actions
Bot
added
risk:medium
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Follow-up at 496e5fb (previous review was at 6057125): the gaps flagged last round are closed. Brave now passes r["url"] and serper passes r["link"] through the shared helper, and serper's comment now documents the screening policy instead of contradicting it; webz screens inside _normalize after the field-type checks (and the helper never raises, so it cannot trip the ValueError skip path). infoquest stayed client-side and its screening arrived via the upstream merge (#7031), with the screen fixture now covering infoquest.infoquest_client — so the "every provider" claim now holds: 15 providers, 14 stubbed in _TEXT_SEARCH_STUBS here plus infoquest's client covered by main's test_infoquest_result_url_safety.py. The snippet-retention nit is fixed too (assert "c" in (content, snippet)). I found nothing else in the new code: brave/serper/webz have no secondary result arrays left unscreened, and the serper domain-filter test correctly expects the unschemed URL to come back blanked. One doc-only nit inline. Coordination note: #7080 (same concern) is still open with a force-pushed head (now 83fa329), so the two PRs still need reconciling.
|
|
||
| import importlib | ||
| from unittest.mock import patch | ||
| import inspect |
There was a problem hiding this comment.
Doc-only follow-up: the line-1 docstring ("The image providers enforce the same string-only result URL policy") predates this commit — the module now also owns the 14-provider text-search screening table. Consider rewording it (e.g. "The image and text-search providers enforce the same string-only result URL policy") so the docstring matches the module's scope. Not blocking.
|
|
…rowser SearXNG is the Compose bundle's default web and image search. Its normalisers only checked that a result URL started with http(s), and frontend-hm renders result `url` and `thumbnail_url` values as links and images, so an untrusted result pointing at an intranet, loopback, link-local or cloud-metadata address reached the model and the person's browser. Port upstream's `safe_public_result_url` (bytedance#6740, bytedance#6819; applied to every provider in bytedance#7037) into url_safety: a pure parse with no DNS or network I/O, which recognises IPv4-mapped/6to4/NAT64 embeddings, decimal/octal/ hex IPv4 spellings and IDNA dot separators. SearXNG now drops web results whose URL fails it and blanks failing image, thumbnail and source URLs. The new normaliser tests fail without the change. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01GujxPsQnaanjqeHp5fwAUn
Fixes #7036. All fifteen web_search providers now screen result URLs (InfoQuest already did on
main).Why
These tools passed provider URLs to the model unfiltered, so a loopback or metadata-service address reached the agent.
What changed
Tavily, Exa, You.com, Tencent WSA, Serply (
urland scholarpdf_url), GroundRoute, Sofya, DuckDuckGo, fastCRW, Firecrawl, SearXNG, Brave, Serper and Webz now pass result URLs throughsafe_public_result_url. A non-public URL is blanked; title and snippet are kept. Serper's verbatim-links comment is updated.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.json(say what it buys us)Screenshots / Recording
N/A, no UI change.
Bug fix verification
backend/tests/test_image_result_url_safety.pystubs each of the fourteen providers with metadata, loopback and public URLs and checks title and snippet survive: 29 failed onupstream/mainsources, 120 passed here.Validation
ruff check .andruff format --check .(whatmake lintruns): clean.make test: 33,977 passed, 24 failed; the same 24 fail onupstream/mainhere (local environment), none in search providers.AI assistance
Tool(s) used: Claude Code
How you used it: AI wrote the fix and tests.