From 1601008979171f62b3b2879671b3a8b9704f09ab Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 08:12:52 -0300 Subject: [PATCH 01/15] Add Firecrawl Search tool to the Atomic Forge --- AGENTS.md | 1 + README.md | 1 + atomic-forge/README.md | 1 + .../tools/firecrawl_search/.coveragerc | 8 + atomic-forge/tools/firecrawl_search/README.md | 61 ++++ .../tools/firecrawl_search/pyproject.toml | 34 ++ .../tools/firecrawl_search/requirements.txt | 3 + .../tests/test_firecrawl_search.py | 317 ++++++++++++++++++ .../firecrawl_search/tool/firecrawl_search.py | 220 ++++++++++++ docs/guides/tools.md | 1 + uv.lock | 38 +++ 11 files changed, 685 insertions(+) create mode 100644 atomic-forge/tools/firecrawl_search/.coveragerc create mode 100644 atomic-forge/tools/firecrawl_search/README.md create mode 100644 atomic-forge/tools/firecrawl_search/pyproject.toml create mode 100644 atomic-forge/tools/firecrawl_search/requirements.txt create mode 100644 atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py create mode 100644 atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py diff --git a/AGENTS.md b/AGENTS.md index ac320a4d..688aa825 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,7 @@ Each example is self-contained and demonstrates specific patterns and capabiliti - `calculator/` - Mathematical computation tool - `datetime_tool/` - Timezone-aware now / parse / convert / shift / diff - `fia_signals/` - Crypto market intelligence (regime, signals, yields, gas, trending, wallet risk) +- `firecrawl_search/` - Web and news search via the Firecrawl API, with optional page content as Markdown - `hackernews_search/` - Hacker News search via the free Algolia API - `pdf_reader/` - PDF text + metadata extraction (local file or URL, page-range support) - `searxng_search/` - Privacy-focused search integration diff --git a/README.md b/README.md index 270f5588..eaa6a167 100644 --- a/README.md +++ b/README.md @@ -349,6 +349,7 @@ Atomic Forge is a collection of tools that can be used with Atomic Agents to ext - Calculator - DateTime - Fía Signals +- Firecrawl Search - Hacker News Search - PDF Reader - SearXNG Search diff --git a/atomic-forge/README.md b/atomic-forge/README.md index d34c5a0f..19304267 100644 --- a/atomic-forge/README.md +++ b/atomic-forge/README.md @@ -18,6 +18,7 @@ The Atomic Forge project includes the following tools: - [Calculator](/atomic-forge/tools/calculator/README.md) - [DateTime](/atomic-forge/tools/datetime_tool/README.md) — timezone-aware now/parse/convert/shift/diff. - [Fía Signals](/atomic-forge/tools/fia_signals/README.md) +- [Firecrawl Search](/atomic-forge/tools/firecrawl_search/README.md) — web and news search via the Firecrawl API, with optional page content as Markdown. - [Hacker News Search](/atomic-forge/tools/hackernews_search/README.md) — search HN stories, comments, Show HN, Ask HN via the free Algolia API. - [PDF Reader](/atomic-forge/tools/pdf_reader/README.md) — extract text and metadata from a local or remote PDF, with page-range filtering. - [SearXNG Search](/atomic-forge/tools/searxng_search/README.md) diff --git a/atomic-forge/tools/firecrawl_search/.coveragerc b/atomic-forge/tools/firecrawl_search/.coveragerc new file mode 100644 index 00000000..c45136ef --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/.coveragerc @@ -0,0 +1,8 @@ +[run] +source = tool +omit = */tests/* + +[report] +exclude_lines = + if __name__ == "__main__": +show_missing = True diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md new file mode 100644 index 00000000..74b1782f --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -0,0 +1,61 @@ +# Firecrawl Search Tool + +## Overview +Searches the web or recent news through the [Firecrawl Search](https://www.firecrawl.dev/search) API and returns each result's title, URL, snippet, and rank, plus the publish date for news. With `include_content=True` the main content of each result page also comes back as Markdown in the same call, so an agent does not need a separate fetch step to read the pages. Requires a Firecrawl API key. + +## Prerequisites and Dependencies +- Python 3.12 or later +- `atomic-agents` +- `pydantic` +- `aiohttp` +- A Firecrawl API key from [firecrawl.dev/app/api-keys](https://www.firecrawl.dev/app/api-keys), set as `FIRECRAWL_API_KEY` or passed in the config + +## Installation +1. Use the Atomic Assembler CLI: run `atomic` and pick `firecrawl_search`. +2. Or copy the `tool/` folder directly into your project. + +## Configuration +- `api_key` (str): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. +- `base_url` (str): API base URL (default `https://api.firecrawl.dev/v2`). +- `location` (str, optional): location to localise results, e.g. `Germany`. +- `time_range` (str, optional): `day`, `week`, `month`, or `year`. Only applies to web results. +- `max_content_chars` (int, optional): truncate each result's `content` to this many characters. No limit by default. +- `timeout` (float): HTTP timeout in seconds (default 75, since requests with `include_content` fetch each page). + +## Input & Output Structure + +### Input Schema +- `queries` (list[str]): search queries to run. Operators such as `site:`, `"exact phrase"`, `-term`, and `filetype:pdf` are supported. +- `search_type` (str): `web` or `news`. Default `web`. +- `max_results_per_query` (int): 1-100 (default 5, kept low because each result can carry a full page with `include_content`). +- `include_content` (bool): also return each result page as Markdown (default `False`). + +### Output Schema +A list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). + +## Usage +If every query fails (for example a wrong API key), `run` raises the error. If only some fail, the others are returned and the failures are logged. + +```python +from tool.firecrawl_search import FirecrawlSearchTool, FirecrawlSearchToolConfig, FirecrawlSearchToolInputSchema + +tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="your-firecrawl-api-key")) + +output = tool.run(FirecrawlSearchToolInputSchema( + queries=["retrieval augmented generation"], + max_results_per_query=3, + include_content=True, +)) + +for item in output.results: + print(item.title, "-", item.url) + print(item.content[:500] if item.content else item.description) +``` + +The request and response formats are documented at [docs.firecrawl.dev/features/search](https://docs.firecrawl.dev/features/search). + +## Contributing +PRs welcome. See the main repo `CONTRIBUTING.md`. + +## License +Same as the main Atomic Agents project. diff --git a/atomic-forge/tools/firecrawl_search/pyproject.toml b/atomic-forge/tools/firecrawl_search/pyproject.toml new file mode 100644 index 00000000..efdc59ab --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/pyproject.toml @@ -0,0 +1,34 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[tool.hatch.build.targets.wheel] +packages = ["tool"] + +[project] +name = "firecrawl-search" +version = "1.0.0" +description = "Web and news search for Atomic Agents via the Firecrawl Search API, with optional page content" +readme = "README.md" +authors = [ + { name = "Juampi (Firecrawl)", email = "juan@sideguide.dev" } +] +requires-python = ">=3.12" +dependencies = [ + "atomic-agents", + "pydantic>=2.10.3,<3.0.0", + "aiohttp>=3.9.0,<4.0.0", +] + +[dependency-groups] +dev = [ + "coverage>=7.6.1,<8.0.0", + "pytest>=8.3.3,<9.0.0", + "pytest-asyncio>=0.23.5,<1.0.0", + "pytest-cov>=5.0.0,<6.0.0", + "python-dotenv>=1.0.0,<2.0.0", + "rich>=13.7.0,<14.0.0", +] + +[tool.uv.sources] +atomic-agents = { workspace = true } diff --git a/atomic-forge/tools/firecrawl_search/requirements.txt b/atomic-forge/tools/firecrawl_search/requirements.txt new file mode 100644 index 00000000..469c7e98 --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/requirements.txt @@ -0,0 +1,3 @@ +atomic-agents>=2.0.0,<3.0.0 +pydantic>=2.10.3,<3.0.0 +aiohttp>=3.9.0,<4.0.0 diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py new file mode 100644 index 00000000..2c781685 --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -0,0 +1,317 @@ +import os +import sys +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) + +from tool.firecrawl_search import ( # noqa: E402 + FirecrawlSearchResultItem, + FirecrawlSearchTool, + FirecrawlSearchToolConfig, + FirecrawlSearchToolInputSchema, + FirecrawlSearchToolOutputSchema, +) + + +WEB_HIT = { + "url": "https://www.python.org/", + "title": "Welcome to Python.org", + "description": "Python is a programming language that lets you work quickly.", + "position": 1, +} + +NEWS_HIT = { + "url": "https://example.com/python-3-14", + "title": "Python 3.14 released", + "snippet": "The Python core team shipped 3.14 today.", + "date": "2 days ago", + "imageUrl": "https://example.com/thumb.png", + "position": 2, +} + +CONTENT_HIT = { + "url": "https://example.com/guide", + "description": "A guide to agents.", + "position": 3, + "markdown": "# Agent guide\n\nBuild agents step by step.", + "metadata": {"title": "Agent guide | Example", "description": "Metadata description", "statusCode": 200}, +} + + +def _mock_session(status: int, payload: dict, reason: str = "OK") -> MagicMock: + session = MagicMock() + response = SimpleNamespace(status=status, reason=reason, json=AsyncMock(return_value=payload)) + session.post.return_value.__aenter__.return_value = response + return session + + +@pytest.fixture +def tool(): + return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="test-key")) + + +def test_to_item_web(): + item = FirecrawlSearchTool._to_item(WEB_HIT, "python") + assert isinstance(item, FirecrawlSearchResultItem) + assert item.query == "python" + assert item.title == "Welcome to Python.org" + assert item.url == "https://www.python.org/" + assert item.description.startswith("Python is a programming language") + assert item.position == 1 + assert item.published is None + assert item.content is None + + +def test_to_item_news(): + """News results carry a snippet and a relative date instead of a description.""" + item = FirecrawlSearchTool._to_item(NEWS_HIT, "python") + assert item.description == "The Python core team shipped 3.14 today." + assert item.published == "2 days ago" + assert item.position == 2 + + +def test_to_item_with_content(): + """With scrapeOptions each result carries markdown and page metadata.""" + item = FirecrawlSearchTool._to_item(CONTENT_HIT, "agents") + assert item.title == "Agent guide | Example" + assert item.description == "A guide to agents." + assert item.content.startswith("# Agent guide") + + +def test_to_item_falls_back_to_url_for_title(): + item = FirecrawlSearchTool._to_item({"url": "https://example.com/untitled"}, "q") + assert item.title == "https://example.com/untitled" + assert item.description is None + + +def test_build_body_defaults(tool): + body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"])) + assert body == {"query": "python", "limit": 5, "sources": ["web"], "origin": "atomic-agents"} + + +def test_build_body_with_options(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", location="Germany", time_range="week")) + params = FirecrawlSearchToolInputSchema(queries=["python"], max_results_per_query=3, include_content=True) + + body = tool._build_body("python", params) + + assert body["sources"] == ["web"] + assert body["limit"] == 3 + assert body["location"] == "Germany" + assert body["tbs"] == "qdr:w" + assert body["scrapeOptions"] == {"formats": ["markdown"], "onlyMainContent": True} + + +def test_build_body_skips_time_range_for_news(): + """The API's tbs filter only applies to web results.""" + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", time_range="day")) + + body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"], search_type="news")) + + assert body["sources"] == ["news"] + assert "tbs" not in body + + +def test_to_item_truncates_content_and_ignores_bad_metadata(): + hit = {**CONTENT_HIT, "metadata": ["not", "a", "dict"]} + + item = FirecrawlSearchTool._to_item(hit, "agents", max_content_chars=7) + + assert item.content == "# Agent" + assert item.title == "https://example.com/guide" + + +@pytest.mark.asyncio +async def test_fetch_builds_request(tool): + session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT]}}) + params = FirecrawlSearchToolInputSchema(queries=["python"]) + + items = await tool._fetch(session, "python", params) + + assert len(items) == 1 + assert items[0].url == "https://www.python.org/" + call_args = session.post.call_args + assert call_args[0][0] == "https://api.firecrawl.dev/v2/search" + assert call_args[1]["json"]["query"] == "python" + + +@pytest.mark.asyncio +async def test_fetch_reads_news_results(tool): + session = _mock_session(200, {"success": True, "data": {"news": [NEWS_HIT] * 4}}) + params = FirecrawlSearchToolInputSchema(queries=["python"], search_type="news", max_results_per_query=2) + + items = await tool._fetch(session, "python", params) + + assert len(items) == 2 + assert items[0].published == "2 days ago" + + +@pytest.mark.asyncio +async def test_fetch_handles_missing_source_key(tool): + """The API omits the source key entirely when there are no results.""" + session = _mock_session(200, {"success": True, "data": {}}) + + items = await tool._fetch(session, "nothing", FirecrawlSearchToolInputSchema(queries=["nothing"])) + + assert items == [] + + +@pytest.mark.asyncio +async def test_fetch_skips_hits_without_url(tool): + session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT, {"title": "no url"}]}}) + + items = await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) + + assert [item.url for item in items] == ["https://www.python.org/"] + + +@pytest.mark.asyncio +async def test_fetch_raises_on_http_error(tool): + session = _mock_session(401, {"success": False, "error": "Unauthorized: Invalid token"}, reason="Unauthorized") + + with pytest.raises(Exception, match="Firecrawl search failed for 'python': 401 Unauthorized: Invalid token"): + await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) + + +@pytest.mark.asyncio +async def test_fetch_raises_when_success_is_false(tool): + session = _mock_session(200, {"success": False}) + + with pytest.raises(Exception, match="Firecrawl search failed for 'python': 200 OK"): + await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) + + +@pytest.mark.asyncio +async def test_fetch_reports_status_for_non_json_body(tool): + session = MagicMock() + response = SimpleNamespace(status=502, reason="Bad Gateway", json=AsyncMock(side_effect=ValueError("not json"))) + session.post.return_value.__aenter__.return_value = response + + with pytest.raises(Exception, match="Firecrawl search failed for 'python': 502 Bad Gateway"): + await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) + + +@pytest.mark.asyncio +async def test_fetch_applies_max_content_chars(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", max_content_chars=5)) + session = _mock_session(200, {"success": True, "data": {"web": [CONTENT_HIT, "not a dict"]}}) + + items = await tool._fetch(session, "agents", FirecrawlSearchToolInputSchema(queries=["agents"], include_content=True)) + + assert [item.content for item in items] == ["# Age"] + + +@pytest.mark.asyncio +async def test_run_async_aggregates_results(tool): + async def fake_fetch(self, session, query, params): + return [FirecrawlSearchTool._to_item(WEB_HIT, query), FirecrawlSearchTool._to_item(NEWS_HIT, query)] + + with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): + out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q1", "q2"])) + + assert isinstance(out, FirecrawlSearchToolOutputSchema) + assert len(out.results) == 4 + assert {r.query for r in out.results} == {"q1", "q2"} + + +@pytest.mark.asyncio +async def test_run_async_skips_failing_query(tool): + async def fake_fetch(self, session, query, params): + if query == "bad": + raise Exception("rate limited") + return [FirecrawlSearchTool._to_item(WEB_HIT, query)] + + with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): + out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad", "good"])) + + assert len(out.results) == 1 + assert out.results[0].query == "good" + + +@pytest.mark.asyncio +async def test_run_async_raises_when_every_query_fails(tool): + """A bad key or exhausted credits should surface as an error, not as an empty result.""" + + async def fake_fetch(self, session, query, params): + raise Exception("Firecrawl search failed for 'q': 401 Unauthorized: Invalid token") + + with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): + with pytest.raises(Exception, match="401"): + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q1", "q2"])) + + +@pytest.mark.asyncio +async def test_run_async_end_to_end_with_mocked_session(tool): + session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT]}}) + + class FakeSession: + def __init__(self, headers=None, timeout=None): + self.headers = headers + + async def __aenter__(self): + return session + + async def __aexit__(self, *args): + return False + + with patch("tool.firecrawl_search.aiohttp.ClientSession", FakeSession): + out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["python"], include_content=True)) + + assert [r.url for r in out.results] == ["https://www.python.org/"] + assert session.post.call_args[1]["json"]["scrapeOptions"]["formats"] == ["markdown"] + + +@pytest.mark.asyncio +async def test_run_async_sends_bearer_header(tool): + captured = {} + + class FakeSession: + def __init__(self, headers=None, timeout=None): + captured["headers"] = headers + + async def __aenter__(self): + return self + + async def __aexit__(self, *args): + return False + + async def fake_fetch(self, session, query, params): + return [] + + with ( + patch("tool.firecrawl_search.aiohttp.ClientSession", FakeSession), + patch.object(FirecrawlSearchTool, "_fetch", fake_fetch), + ): + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + assert captured["headers"]["Authorization"] == "Bearer test-key" + assert captured["headers"]["Content-Type"] == "application/json" + + +@pytest.mark.asyncio +async def test_run_async_requires_api_key(monkeypatch): + monkeypatch.delenv("FIRECRAWL_API_KEY", raising=False) + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) + + with pytest.raises(ValueError, match="FIRECRAWL_API_KEY"): + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + +def test_api_key_falls_back_to_environment(monkeypatch): + monkeypatch.setenv("FIRECRAWL_API_KEY", "env-key") + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) + assert tool.api_key == "env-key" + + +def test_run_invokes_run_async(tool): + sentinel = FirecrawlSearchToolOutputSchema(results=[]) + with patch.object(FirecrawlSearchTool, "run_async", AsyncMock(return_value=sentinel)): + out = tool.run(FirecrawlSearchToolInputSchema(queries=["x"])) + assert out is sentinel + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py new file mode 100644 index 00000000..4ee7a66b --- /dev/null +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -0,0 +1,220 @@ +import asyncio +import logging +import os +from concurrent.futures import ThreadPoolExecutor +from typing import List, Literal, Optional + +import aiohttp +from pydantic import Field + +from atomic_agents import BaseIOSchema, BaseTool, BaseToolConfig + + +logger = logging.getLogger(__name__) + + +################ +# INPUT SCHEMA # +################ +class FirecrawlSearchToolInputSchema(BaseIOSchema): + """ + Search the web or recent news through the Firecrawl Search API. + Use this to find current information on a topic. Set include_content to true to also get + each result page as Markdown in the same call. Requires a Firecrawl API key. + """ + + queries: List[str] = Field( + ..., + description='Search queries to run. Supports operators such as site:, "exact phrase", -term, and filetype:pdf.', + ) + search_type: Literal["web", "news"] = Field( + default="web", + description="Which results to return. 'web' = web pages; 'news' = news articles.", + ) + max_results_per_query: int = Field(default=5, ge=1, le=100, description="Maximum results per query.") + include_content: bool = Field( + default=False, + description="Also return the main content of each result page as Markdown. Slower than a plain search.", + ) + + +#################### +# OUTPUT SCHEMA(S) # +#################### +class FirecrawlSearchResultItem(BaseIOSchema): + """A single Firecrawl search result (web page or news article).""" + + query: str = Field(..., description="The query that produced this result.") + title: str = Field(..., description="Title of the result.") + url: str = Field(..., description="URL of the result.") + description: Optional[str] = Field(None, description="Query-relevant excerpt or snippet for the result.") + position: Optional[int] = Field(None, description="Rank of the result.") + published: Optional[str] = Field(None, description="Publication date string (news only).") + content: Optional[str] = Field(None, description="Page content as Markdown (only when include_content is set).") + + +class FirecrawlSearchToolOutputSchema(BaseIOSchema): + """Output of the Firecrawl search tool.""" + + results: List[FirecrawlSearchResultItem] = Field(..., description="Matching results across all queries.") + + +################# +# CONFIGURATION # +################# +class FirecrawlSearchToolConfig(BaseToolConfig): + """Configuration for the FirecrawlSearchTool.""" + + api_key: str = Field( + default="", description="Firecrawl API key. Falls back to the FIRECRAWL_API_KEY environment variable." + ) + base_url: str = Field(default="https://api.firecrawl.dev/v2", description="Firecrawl API base URL.") + location: Optional[str] = Field( + default=None, description="Location to localise results, e.g. 'Germany' or 'San Francisco,California,United States'." + ) + time_range: Optional[Literal["day", "week", "month", "year"]] = Field( + default=None, description="Only return web results from this time window. Does not apply to news." + ) + max_content_chars: Optional[int] = Field( + default=None, ge=1, description="Truncate each result's content to this many characters. No limit when unset." + ) + timeout: float = Field(default=75.0, ge=1.0, le=120.0, description="HTTP request timeout in seconds.") + + +##################### +# MAIN TOOL & LOGIC # +##################### +class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSearchToolOutputSchema]): + """Tool for searching the web and news via the Firecrawl Search API.""" + + TIME_RANGES = {"day": "qdr:d", "week": "qdr:w", "month": "qdr:m", "year": "qdr:y"} + + def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): + super().__init__(config) + self.api_key = config.api_key or os.getenv("FIRECRAWL_API_KEY", "") + self.base_url = config.base_url.rstrip("/") + self.location = config.location + self.time_range = config.time_range + self.max_content_chars = config.max_content_chars + self.timeout = config.timeout + + @classmethod + def _to_item(cls, hit: dict, query: str, max_content_chars: Optional[int] = None) -> FirecrawlSearchResultItem: + metadata = hit.get("metadata") if isinstance(hit.get("metadata"), dict) else {} + content = hit.get("markdown") + if content and max_content_chars: + content = content[:max_content_chars] + return FirecrawlSearchResultItem( + query=query, + title=hit.get("title") or metadata.get("title") or hit["url"], + url=hit["url"], + description=hit.get("description") or hit.get("snippet") or metadata.get("description"), + position=hit.get("position"), + published=hit.get("date"), + content=content, + ) + + def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dict: + body = { + "query": query, + "limit": params.max_results_per_query, + "sources": [params.search_type], + "origin": "atomic-agents", + } + if self.location: + body["location"] = self.location + if self.time_range and params.search_type == "web": + body["tbs"] = self.TIME_RANGES[self.time_range] + if params.include_content: + body["scrapeOptions"] = {"formats": ["markdown"], "onlyMainContent": True} + return body + + async def _fetch( + self, + session: aiohttp.ClientSession, + query: str, + params: FirecrawlSearchToolInputSchema, + ) -> List[FirecrawlSearchResultItem]: + async with session.post(f"{self.base_url}/search", json=self._build_body(query, params)) as resp: + try: + data = await resp.json(content_type=None) + except ValueError: + data = None + if resp.status != 200 or not isinstance(data, dict) or not data.get("success"): + error = data.get("error") if isinstance(data, dict) else None + raise Exception(f"Firecrawl search failed for '{query}': {resp.status} {error or resp.reason}") + + # The key for the requested source is absent when there are no results. + groups = data.get("data") if isinstance(data.get("data"), dict) else {} + hits = groups.get(params.search_type) or [] + items = [self._to_item(hit, query, self.max_content_chars) for hit in hits if isinstance(hit, dict) and hit.get("url")] + return items[: params.max_results_per_query] + + async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: + if not self.api_key: + raise ValueError( + "Firecrawl API key is missing. Set FirecrawlSearchToolConfig.api_key or the FIRECRAWL_API_KEY " + "environment variable." + ) + + headers = {"Authorization": f"Bearer {self.api_key}", "Content-Type": "application/json"} + timeout = aiohttp.ClientTimeout(total=self.timeout) + async with aiohttp.ClientSession(headers=headers, timeout=timeout) as session: + tasks = [self._fetch(session, q, params) for q in params.queries] + grouped = await asyncio.gather(*tasks, return_exceptions=True) + + # A bad key or exhausted credits fails every query; raise instead of returning an empty list. + failures = [group for group in grouped if isinstance(group, Exception)] + if failures and len(failures) == len(grouped): + raise failures[0] + + results: List[FirecrawlSearchResultItem] = [] + for query, group in zip(params.queries, grouped): + if isinstance(group, Exception): + logger.warning("Firecrawl query '%s' failed: %s", query, str(group) or repr(group)) + continue + results.extend(group) + return FirecrawlSearchToolOutputSchema(results=results) + + def run(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: + with ThreadPoolExecutor() as executor: + return executor.submit(asyncio.run, self.run_async(params)).result() + + +################# +# EXAMPLE USAGE # +################# +if __name__ == "__main__": # pragma: no cover + from dotenv import load_dotenv + from rich.console import Console + + load_dotenv() + console = Console() + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=os.getenv("FIRECRAWL_API_KEY", ""))) + + examples = [ + ("web", False, "atomic agents framework"), + ("news", False, "AI agents"), + ("web", True, "atomic agents framework"), + ] + for search_type, include_content, query in examples: + output = tool.run( + FirecrawlSearchToolInputSchema( + queries=[query], + search_type=search_type, + max_results_per_query=3, + include_content=include_content, + ) + ) + console.rule(f"[bold cyan]{search_type}{' + content' if include_content else ''}") + for item in output.results: + console.print(f"[bold]{item.title}[/bold]") + console.print(item.url) + if item.description: + console.print(item.description[:200]) + if item.published: + console.print(f"[bold]Published:[/bold] {item.published}") + if item.content: + console.print(f"[bold]Content:[/bold] {len(item.content)} chars") + console.print(item.content[:300]) + console.print() diff --git a/docs/guides/tools.md b/docs/guides/tools.md index 5d290363..7d9e2a23 100644 --- a/docs/guides/tools.md +++ b/docs/guides/tools.md @@ -150,6 +150,7 @@ The Atomic Forge ships with several pre-built tools: - **Calculator**: Perform mathematical calculations - **DateTime**: Timezone-aware now / parse / convert / shift / diff (no key required) - **Fía Signals**: Crypto market intelligence — market regime, trading signals, DeFi yields, gas prices, Solana trending tokens, and wallet risk scoring +- **Firecrawl Search**: Web and news search via the Firecrawl API, with optional page content as Markdown - **Hacker News Search**: Search HN stories, comments, Show HN, Ask HN, polls (free Algolia API) - **PDF Reader**: Extract text and metadata from local or remote PDFs, with page-range filtering - **SearXNG Search**: Search the web using SearXNG diff --git a/uv.lock b/uv.lock index 6c39d504..a357fa2f 100644 --- a/uv.lock +++ b/uv.lock @@ -21,6 +21,7 @@ members = [ "example-mcp-server", "fastapi-memory", "fia-signals", + "firecrawl-search", "hackernews-search", "hooks-example", "nested-multimodal", @@ -1329,6 +1330,43 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/76/91/7216b27286936c16f5b4d0c530087e4a54eead683e6b0b73dd0c64844af6/filelock-3.20.0-py3-none-any.whl", hash = "sha256:339b4732ffda5cd79b13f4e2711a31b0365ce445d95d243bb996273d072546a2", size = 16054, upload-time = "2025-10-08T18:03:48.35Z" }, ] +[[package]] +name = "firecrawl-search" +version = "1.0.0" +source = { editable = "atomic-forge/tools/firecrawl_search" } +dependencies = [ + { name = "aiohttp" }, + { name = "atomic-agents" }, + { name = "pydantic" }, +] + +[package.dev-dependencies] +dev = [ + { name = "coverage" }, + { name = "pytest" }, + { name = "pytest-asyncio" }, + { name = "pytest-cov" }, + { name = "python-dotenv" }, + { name = "rich" }, +] + +[package.metadata] +requires-dist = [ + { name = "aiohttp", specifier = ">=3.9.0,<4.0.0" }, + { name = "atomic-agents", editable = "." }, + { name = "pydantic", specifier = ">=2.10.3,<3.0.0" }, +] + +[package.metadata.requires-dev] +dev = [ + { name = "coverage", specifier = ">=7.6.1,<8.0.0" }, + { name = "pytest", specifier = ">=8.3.3,<9.0.0" }, + { name = "pytest-asyncio", specifier = ">=0.23.5,<1.0.0" }, + { name = "pytest-cov", specifier = ">=5.0.0,<6.0.0" }, + { name = "python-dotenv", specifier = ">=1.0.0,<2.0.0" }, + { name = "rich", specifier = ">=13.7.0,<14.0.0" }, +] + [[package]] name = "flake8" version = "7.3.0" From 04921b1f9309dbd7c179c79cd3886020bf786c1e Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 13:31:37 -0300 Subject: [PATCH 02/15] Address review: drop origin field, split tool logic, validate responses, redact key --- atomic-forge/tools/firecrawl_search/README.md | 2 +- .../tests/test_firecrawl_search.py | 107 +++++++++++++++--- .../firecrawl_search/tool/firecrawl_search.py | 99 ++++++++++------ 3 files changed, 157 insertions(+), 51 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 74b1782f..1fbacb90 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -15,7 +15,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec 2. Or copy the `tool/` folder directly into your project. ## Configuration -- `api_key` (str): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. +- `api_key` (str): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. Stored as a secret and kept out of reprs, raised errors, and logs. - `base_url` (str): API base URL (default `https://api.firecrawl.dev/v2`). - `location` (str, optional): location to localise results, e.g. `Germany`. - `time_range` (str, optional): `day`, `week`, `month`, or `year`. Only applies to web results. diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 2c781685..f61e2a70 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -1,9 +1,13 @@ +import logging import os import sys +from contextlib import asynccontextmanager from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch import pytest +from aiohttp import web +from aiohttp.test_utils import TestServer sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -41,6 +45,33 @@ } +DUMMY_KEY = "fc-dummy-secret-0123456789" + + +@asynccontextmanager +async def fake_firecrawl(respond): + """Serve POST /v2/search on a local aiohttp server; `respond(body)` builds each response.""" + seen = [] + + async def handler(request): + body = await request.json() + seen.append({"headers": dict(request.headers), "body": body}) + return respond(body) + + app = web.Application() + app.router.add_post("/v2/search", handler) + server = TestServer(app) + await server.start_server() + try: + yield str(server.make_url("/v2")), seen + finally: + await server.close() + + +def _http_tool(base_url: str, **config) -> FirecrawlSearchTool: + return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY, base_url=base_url, **config)) + + def _mock_session(status: int, payload: dict, reason: str = "OK") -> MagicMock: session = MagicMock() response = SimpleNamespace(status=status, reason=reason, json=AsyncMock(return_value=payload)) @@ -89,7 +120,7 @@ def test_to_item_falls_back_to_url_for_title(): def test_build_body_defaults(tool): body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"])) - assert body == {"query": "python", "limit": 5, "sources": ["web"], "origin": "atomic-agents"} + assert body == {"query": "python", "limit": 5, "sources": ["web"]} def test_build_body_with_options(): @@ -244,24 +275,70 @@ async def fake_fetch(self, session, query, params): @pytest.mark.asyncio -async def test_run_async_end_to_end_with_mocked_session(tool): - session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT]}}) +async def test_http_round_trip(): + def respond(body): + return web.json_response( + {"success": True, "data": {"web": [{**CONTENT_HIT, "url": f"https://example.com/{body['query']}"}]}} + ) - class FakeSession: - def __init__(self, headers=None, timeout=None): - self.headers = headers + async with fake_firecrawl(respond) as (base_url, seen): + out = await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["agents"], include_content=True)) - async def __aenter__(self): - return session + assert [r.url for r in out.results] == ["https://example.com/agents"] + assert out.results[0].content.startswith("# Agent guide") + assert seen[0]["headers"]["Authorization"] == f"Bearer {DUMMY_KEY}" + assert seen[0]["body"]["scrapeOptions"] == {"formats": ["markdown"], "onlyMainContent": True} - async def __aexit__(self, *args): - return False - with patch("tool.firecrawl_search.aiohttp.ClientSession", FakeSession): - out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["python"], include_content=True)) +@pytest.mark.asyncio +@pytest.mark.parametrize( + "payload", + [ + {"success": True, "data": None}, + {"success": True}, + {"success": True, "data": {"web": {"url": "https://example.com/not-a-list"}}}, + {"success": True, "data": {"web": None}}, + {"success": True, "data": [WEB_HIT]}, + ], +) +async def test_http_malformed_success_response_raises(payload): + """A 200 with success=true but no usable result list is an error, not an empty result.""" + async with fake_firecrawl(lambda body: web.json_response(payload)) as (base_url, _): + with pytest.raises(Exception, match="unexpected response shape for 'web' results"): + await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + +@pytest.mark.asyncio +async def test_http_non_json_error_page_reports_status(): + def respond(body): + return web.Response(status=502, text="Bad Gateway", content_type="text/html") + + async with fake_firecrawl(respond) as (base_url, _): + with pytest.raises(Exception, match="Firecrawl search failed for 'q': 502 Bad Gateway"): + await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + +@pytest.mark.asyncio +async def test_http_dummy_key_kept_out_of_repr_errors_and_logs(caplog): + """Even when the provider echoes the key back, it never reaches repr, raised errors, or logs.""" + + def respond(body): + if body["query"] == "bad": + return web.json_response({"success": False, "error": f"Invalid token {DUMMY_KEY}"}, status=401) + return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}) + + async with fake_firecrawl(respond) as (base_url, _): + tool = _http_tool(base_url) + with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): + out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad", "good"])) + with pytest.raises(Exception) as raised: + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad"])) - assert [r.url for r in out.results] == ["https://www.python.org/"] - assert session.post.call_args[1]["json"]["scrapeOptions"]["formats"] == ["markdown"] + assert [r.query for r in out.results] == ["good"] + assert "401 Invalid token [redacted]" in caplog.text + assert "401 Invalid token [redacted]" in str(raised.value) + for text in (caplog.text, str(raised.value), repr(raised.value), repr(tool), repr(tool.config), str(tool.config)): + assert DUMMY_KEY not in text @pytest.mark.asyncio @@ -303,7 +380,7 @@ async def test_run_async_requires_api_key(monkeypatch): def test_api_key_falls_back_to_environment(monkeypatch): monkeypatch.setenv("FIRECRAWL_API_KEY", "env-key") tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) - assert tool.api_key == "env-key" + assert tool._api_key == "env-key" def test_run_invokes_run_async(tool): diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 4ee7a66b..11683d18 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -5,7 +5,7 @@ from typing import List, Literal, Optional import aiohttp -from pydantic import Field +from pydantic import Field, SecretStr from atomic_agents import BaseIOSchema, BaseTool, BaseToolConfig @@ -65,8 +65,9 @@ class FirecrawlSearchToolOutputSchema(BaseIOSchema): class FirecrawlSearchToolConfig(BaseToolConfig): """Configuration for the FirecrawlSearchTool.""" - api_key: str = Field( - default="", description="Firecrawl API key. Falls back to the FIRECRAWL_API_KEY environment variable." + api_key: SecretStr = Field( + default=SecretStr(""), + description="Firecrawl API key. Falls back to the FIRECRAWL_API_KEY environment variable. Hidden in repr.", ) base_url: str = Field(default="https://api.firecrawl.dev/v2", description="Firecrawl API base URL.") location: Optional[str] = Field( @@ -91,27 +92,30 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) - self.api_key = config.api_key or os.getenv("FIRECRAWL_API_KEY", "") + self._api_key = config.api_key.get_secret_value() or os.getenv("FIRECRAWL_API_KEY", "") self.base_url = config.base_url.rstrip("/") self.location = config.location self.time_range = config.time_range self.max_content_chars = config.max_content_chars self.timeout = config.timeout + @staticmethod + def _first(*values): + return next((value for value in values if value), None) + @classmethod def _to_item(cls, hit: dict, query: str, max_content_chars: Optional[int] = None) -> FirecrawlSearchResultItem: - metadata = hit.get("metadata") if isinstance(hit.get("metadata"), dict) else {} + metadata = hit.get("metadata") + metadata = metadata if isinstance(metadata, dict) else {} content = hit.get("markdown") - if content and max_content_chars: - content = content[:max_content_chars] return FirecrawlSearchResultItem( query=query, - title=hit.get("title") or metadata.get("title") or hit["url"], + title=cls._first(hit.get("title"), metadata.get("title"), hit["url"]), url=hit["url"], - description=hit.get("description") or hit.get("snippet") or metadata.get("description"), + description=cls._first(hit.get("description"), hit.get("snippet"), metadata.get("description")), position=hit.get("position"), published=hit.get("date"), - content=content, + content=content[:max_content_chars] if content else None, ) def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dict: @@ -119,7 +123,6 @@ def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dic "query": query, "limit": params.max_results_per_query, "sources": [params.search_type], - "origin": "atomic-agents", } if self.location: body["location"] = self.location @@ -129,6 +132,32 @@ def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dic body["scrapeOptions"] = {"formats": ["markdown"], "onlyMainContent": True} return body + def _redact(self, text: str) -> str: + return text.replace(self._api_key, "[redacted]") if self._api_key else text + + def _error(self, query: str, status: int, detail: str) -> Exception: + return Exception(self._redact(f"Firecrawl search failed for '{query}': {status} {detail}")) + + @staticmethod + async def _read_json(resp: aiohttp.ClientResponse): + try: + return await resp.json(content_type=None) + except ValueError: + return None + + def _check_success(self, query: str, status: int, reason: Optional[str], data) -> None: + payload = data if isinstance(data, dict) else {} + if status != 200 or not payload.get("success"): + raise self._error(query, status, payload.get("error") or reason) + + def _extract_hits(self, query: str, data: dict, search_type: str) -> list: + # The key for the requested source is absent when there are no results. + groups = data.get("data") + hits = groups.get(search_type, []) if isinstance(groups, dict) else None + if not isinstance(hits, list): + raise self._error(query, 200, f"unexpected response shape for '{search_type}' results") + return hits + async def _fetch( self, session: aiohttp.ClientSession, @@ -136,45 +165,45 @@ async def _fetch( params: FirecrawlSearchToolInputSchema, ) -> List[FirecrawlSearchResultItem]: async with session.post(f"{self.base_url}/search", json=self._build_body(query, params)) as resp: - try: - data = await resp.json(content_type=None) - except ValueError: - data = None - if resp.status != 200 or not isinstance(data, dict) or not data.get("success"): - error = data.get("error") if isinstance(data, dict) else None - raise Exception(f"Firecrawl search failed for '{query}': {resp.status} {error or resp.reason}") + data = await self._read_json(resp) + self._check_success(query, resp.status, resp.reason, data) - # The key for the requested source is absent when there are no results. - groups = data.get("data") if isinstance(data.get("data"), dict) else {} - hits = groups.get(params.search_type) or [] + hits = self._extract_hits(query, data, params.search_type) items = [self._to_item(hit, query, self.max_content_chars) for hit in hits if isinstance(hit, dict) and hit.get("url")] return items[: params.max_results_per_query] + def _log_failure(self, query: str, error: BaseException) -> None: + logger.warning("Firecrawl query '%s' failed: %s", query, self._redact(str(error) or repr(error))) + + def _collect(self, queries: List[str], grouped: list) -> List[FirecrawlSearchResultItem]: + results: List[FirecrawlSearchResultItem] = [] + errors = [] + for query, group in zip(queries, grouped): + if isinstance(group, BaseException): + errors.append(group) + self._log_failure(query, group) + continue + results.extend(group) + + # A bad key or exhausted credits fails every query; raise instead of returning an empty list. + if errors and len(errors) == len(grouped): + raise errors[0] + return results + async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: - if not self.api_key: + if not self._api_key: raise ValueError( "Firecrawl API key is missing. Set FirecrawlSearchToolConfig.api_key or the FIRECRAWL_API_KEY " "environment variable." ) - headers = {"Authorization": f"Bearer {self.api_key}", "Content-Type": "application/json"} + headers = {"Authorization": f"Bearer {self._api_key}", "Content-Type": "application/json"} timeout = aiohttp.ClientTimeout(total=self.timeout) async with aiohttp.ClientSession(headers=headers, timeout=timeout) as session: tasks = [self._fetch(session, q, params) for q in params.queries] grouped = await asyncio.gather(*tasks, return_exceptions=True) - # A bad key or exhausted credits fails every query; raise instead of returning an empty list. - failures = [group for group in grouped if isinstance(group, Exception)] - if failures and len(failures) == len(grouped): - raise failures[0] - - results: List[FirecrawlSearchResultItem] = [] - for query, group in zip(params.queries, grouped): - if isinstance(group, Exception): - logger.warning("Firecrawl query '%s' failed: %s", query, str(group) or repr(group)) - continue - results.extend(group) - return FirecrawlSearchToolOutputSchema(results=results) + return FirecrawlSearchToolOutputSchema(results=self._collect(params.queries, grouped)) def run(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: with ThreadPoolExecutor() as executor: From a64d126974bf32ec6ba3395750daca94d13066e3 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 13:40:58 -0300 Subject: [PATCH 03/15] Wrap aiohttp transport errors so the key never reaches a raised repr --- .../tests/test_firecrawl_search.py | 19 +++++++++++++++++++ .../firecrawl_search/tool/firecrawl_search.py | 14 +++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index f61e2a70..dba29bd6 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -318,6 +318,25 @@ def respond(body): await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) +@pytest.mark.asyncio +async def test_http_transport_error_does_not_expose_key(caplog): + """aiohttp's own errors (here a redirect loop) carry the request headers; the tool must not re-raise them as is.""" + + def respond(body): + return web.Response(status=307, headers={"Location": "/v2/search"}) + + async with fake_firecrawl(respond) as (base_url, seen): + with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): + with pytest.raises(Exception) as raised: + await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + assert len(seen) > 1 + assert "Firecrawl search failed for 'q': TooManyRedirects" in str(raised.value) + assert raised.value.__cause__ is None + for text in (str(raised.value), repr(raised.value), caplog.text): + assert DUMMY_KEY not in text + + @pytest.mark.asyncio async def test_http_dummy_key_kept_out_of_repr_errors_and_logs(caplog): """Even when the provider echoes the key back, it never reaches repr, raised errors, or logs.""" diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 11683d18..efdeed5f 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -2,7 +2,7 @@ import logging import os from concurrent.futures import ThreadPoolExecutor -from typing import List, Literal, Optional +from typing import List, Literal, Optional, Union import aiohttp from pydantic import Field, SecretStr @@ -135,7 +135,7 @@ def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dic def _redact(self, text: str) -> str: return text.replace(self._api_key, "[redacted]") if self._api_key else text - def _error(self, query: str, status: int, detail: str) -> Exception: + def _error(self, query: str, status: Union[int, str], detail: Optional[str]) -> Exception: return Exception(self._redact(f"Firecrawl search failed for '{query}': {status} {detail}")) @staticmethod @@ -164,9 +164,13 @@ async def _fetch( query: str, params: FirecrawlSearchToolInputSchema, ) -> List[FirecrawlSearchResultItem]: - async with session.post(f"{self.base_url}/search", json=self._build_body(query, params)) as resp: - data = await self._read_json(resp) - self._check_success(query, resp.status, resp.reason, data) + try: + async with session.post(f"{self.base_url}/search", json=self._build_body(query, params)) as resp: + data = await self._read_json(resp) + self._check_success(query, resp.status, resp.reason, data) + except aiohttp.ClientError as error: + # aiohttp errors such as TooManyRedirects carry the request headers, Authorization included, in their repr. + raise self._error(query, type(error).__name__, str(error)) from None hits = self._extract_hits(query, data, params.search_type) items = [self._to_item(hit, query, self.max_content_chars) for hit in hits if isinstance(hit, dict) and hit.get("url")] From 02336056967b39bb1362e0bf17a465f4b2adac35 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:03:31 -0300 Subject: [PATCH 04/15] Tighten response validation, batch results and key redaction; test at the aiohttp transport --- atomic-forge/tools/firecrawl_search/README.md | 9 +- .../tests/test_firecrawl_search.py | 486 ++++++++++-------- .../firecrawl_search/tool/firecrawl_search.py | 147 ++++-- 3 files changed, 360 insertions(+), 282 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 1fbacb90..af7120bc 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -15,7 +15,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec 2. Or copy the `tool/` folder directly into your project. ## Configuration -- `api_key` (str): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. Stored as a secret and kept out of reprs, raised errors, and logs. +- `api_key` (`SecretStr`, a plain string is accepted): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. It is only sent in the `Authorization` header and is redacted from raised errors and log messages. - `base_url` (str): API base URL (default `https://api.firecrawl.dev/v2`). - `location` (str, optional): location to localise results, e.g. `Germany`. - `time_range` (str, optional): `day`, `week`, `month`, or `year`. Only applies to web results. @@ -25,16 +25,17 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec ## Input & Output Structure ### Input Schema -- `queries` (list[str]): search queries to run. Operators such as `site:`, `"exact phrase"`, `-term`, and `filetype:pdf` are supported. +- `queries` (list[str], at least one): search queries to run. Operators such as `site:`, `"exact phrase"`, `-term`, and `filetype:pdf` are supported. - `search_type` (str): `web` or `news`. Default `web`. - `max_results_per_query` (int): 1-100 (default 5, kept low because each result can carry a full page with `include_content`). - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -A list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a usable URL are skipped, and badly typed optional fields are left empty. +- `failed_queries`: queries that failed while others returned results. The reason for each is logged as a warning. ## Usage -If every query fails (for example a wrong API key), `run` raises the error. If only some fail, the others are returned and the failures are logged. +If no query returns results and at least one failed (for example a wrong API key, no credits, or a malformed response), `run` raises the first error instead of returning an empty list. Redirects are not followed; they are reported as errors. ```python from tool.firecrawl_search import FirecrawlSearchTool, FirecrawlSearchToolConfig, FirecrawlSearchToolInputSchema diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index dba29bd6..b7fae4f0 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -1,13 +1,17 @@ +import asyncio +import copy import logging import os +import socket import sys +import traceback from contextlib import asynccontextmanager -from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest from aiohttp import web from aiohttp.test_utils import TestServer +from pydantic import ValidationError sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -20,6 +24,8 @@ ) +DUMMY_KEY = "fc-dummy0secret0key0123456789" + WEB_HIT = { "url": "https://www.python.org/", "title": "Welcome to Python.org", @@ -38,14 +44,16 @@ CONTENT_HIT = { "url": "https://example.com/guide", + "title": "Agent guide", "description": "A guide to agents.", "position": 3, "markdown": "# Agent guide\n\nBuild agents step by step.", - "metadata": {"title": "Agent guide | Example", "description": "Metadata description", "statusCode": 200}, + "metadata": {"title": "Agent guide | Example", "statusCode": 200}, } -DUMMY_KEY = "fc-dummy-secret-0123456789" +def ok(source: str, hits: list) -> web.Response: + return web.json_response({"success": True, "data": {source: hits}}) @asynccontextmanager @@ -56,7 +64,8 @@ async def fake_firecrawl(respond): async def handler(request): body = await request.json() seen.append({"headers": dict(request.headers), "body": body}) - return respond(body) + response = respond(body) + return await response if asyncio.iscoroutine(response) else response app = web.Application() app.router.add_post("/v2/search", handler) @@ -68,327 +77,360 @@ async def handler(request): await server.close() -def _http_tool(base_url: str, **config) -> FirecrawlSearchTool: +def http_tool(base_url: str, **config) -> FirecrawlSearchTool: return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY, base_url=base_url, **config)) -def _mock_session(status: int, payload: dict, reason: str = "OK") -> MagicMock: - session = MagicMock() - response = SimpleNamespace(status=status, reason=reason, json=AsyncMock(return_value=payload)) - session.post.return_value.__aenter__.return_value = response - return session +async def search(respond, queries=("q",), **params): + """Run one search against a fake server and return (output, requests seen).""" + async with fake_firecrawl(respond) as (base_url, seen): + out = await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=list(queries), **params)) + return out, seen + +async def search_error(respond, queries=("q",), **params) -> Exception: + with pytest.raises(Exception) as raised: + await search(respond, queries, **params) + return raised.value -@pytest.fixture -def tool(): - return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="test-key")) +def leak_surfaces(error: BaseException) -> str: + """Every text an error can surface through, including the locals of the tool's frames in its traceback.""" + stack = traceback.TracebackException.from_exception(error, capture_locals=True).stack + tool_frames = [ + repr(frame.locals) for frame in stack if frame.filename.endswith(os.path.join("tool", "firecrawl_search.py")) + ] + assert tool_frames, "expected the traceback to pass through the tool" + return "\n".join([str(error), repr(error), repr(error.__context__), repr(error.__cause__), *tool_frames]) + +################## +# Result parsing # +################## def test_to_item_web(): item = FirecrawlSearchTool._to_item(WEB_HIT, "python") - assert isinstance(item, FirecrawlSearchResultItem) - assert item.query == "python" - assert item.title == "Welcome to Python.org" - assert item.url == "https://www.python.org/" - assert item.description.startswith("Python is a programming language") - assert item.position == 1 - assert item.published is None - assert item.content is None - - -def test_to_item_news(): - """News results carry a snippet and a relative date instead of a description.""" + assert item == FirecrawlSearchResultItem( + query="python", + title="Welcome to Python.org", + url="https://www.python.org/", + description="Python is a programming language that lets you work quickly.", + position=1, + ) + + +def test_to_item_news_uses_snippet_and_date(): item = FirecrawlSearchTool._to_item(NEWS_HIT, "python") - assert item.description == "The Python core team shipped 3.14 today." - assert item.published == "2 days ago" - assert item.position == 2 + assert (item.description, item.published, item.position) == ("The Python core team shipped 3.14 today.", "2 days ago", 2) + + +def test_to_item_content_and_truncation(): + assert FirecrawlSearchTool._to_item(CONTENT_HIT, "q").content == CONTENT_HIT["markdown"] + assert FirecrawlSearchTool._to_item(CONTENT_HIT, "q", max_content_chars=7).content == "# Agent" -def test_to_item_with_content(): - """With scrapeOptions each result carries markdown and page metadata.""" - item = FirecrawlSearchTool._to_item(CONTENT_HIT, "agents") - assert item.title == "Agent guide | Example" - assert item.description == "A guide to agents." - assert item.content.startswith("# Agent guide") +@pytest.mark.parametrize( + "field, value, attribute, expected", + [ + ("title", {"nested": True}, "title", "https://example.com/a"), + ("title", 5, "title", "https://example.com/a"), + ("description", ["a", "b"], "description", None), + ("position", "N/A", "position", None), + ("position", 1.5, "position", None), + ("position", True, "position", None), + ("position", -3, "position", None), + ("date", 20250101, "published", None), + ("markdown", 42, "content", None), + ("markdown", {}, "content", None), + ], +) +def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): + """One badly typed field must not discard the result or raise a raw ValidationError.""" + item = FirecrawlSearchTool._to_item({"url": "https://example.com/a", field: value}, "q") + assert getattr(item, attribute) == expected -def test_to_item_falls_back_to_url_for_title(): - item = FirecrawlSearchTool._to_item({"url": "https://example.com/untitled"}, "q") - assert item.title == "https://example.com/untitled" - assert item.description is None +@pytest.mark.parametrize("hit", [1, "s", None, [], {"title": "no url"}, {"url": ""}, {"url": 123}]) +def test_to_item_without_usable_url_is_none(hit): + assert FirecrawlSearchTool._to_item(hit, "q") is None -def test_build_body_defaults(tool): - body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"])) - assert body == {"query": "python", "limit": 5, "sources": ["web"]} +################ +# Request body # +################ +def test_build_body_defaults(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k")) + assert tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"])) == { + "query": "python", + "limit": 5, + "sources": ["web"], + } def test_build_body_with_options(): tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", location="Germany", time_range="week")) params = FirecrawlSearchToolInputSchema(queries=["python"], max_results_per_query=3, include_content=True) - - body = tool._build_body("python", params) - - assert body["sources"] == ["web"] - assert body["limit"] == 3 - assert body["location"] == "Germany" - assert body["tbs"] == "qdr:w" - assert body["scrapeOptions"] == {"formats": ["markdown"], "onlyMainContent": True} + assert tool._build_body("python", params) == { + "query": "python", + "limit": 3, + "sources": ["web"], + "location": "Germany", + "tbs": "qdr:w", + "scrapeOptions": {"formats": ["markdown"], "onlyMainContent": True}, + } def test_build_body_skips_time_range_for_news(): """The API's tbs filter only applies to web results.""" tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", time_range="day")) - body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"], search_type="news")) + assert body == {"query": "python", "limit": 5, "sources": ["news"]} - assert body["sources"] == ["news"] - assert "tbs" not in body - -def test_to_item_truncates_content_and_ignores_bad_metadata(): - hit = {**CONTENT_HIT, "metadata": ["not", "a", "dict"]} - - item = FirecrawlSearchTool._to_item(hit, "agents", max_content_chars=7) - - assert item.content == "# Agent" - assert item.title == "https://example.com/guide" +def test_empty_queries_rejected(): + with pytest.raises(ValidationError): + FirecrawlSearchToolInputSchema(queries=[]) +################################### +# Round trips over real transport # +################################### @pytest.mark.asyncio -async def test_fetch_builds_request(tool): - session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT]}}) - params = FirecrawlSearchToolInputSchema(queries=["python"]) +async def test_web_round_trip(): + out, seen = await search(lambda body: ok("web", [CONTENT_HIT]), queries=["agents"], include_content=True) - items = await tool._fetch(session, "python", params) - - assert len(items) == 1 - assert items[0].url == "https://www.python.org/" - call_args = session.post.call_args - assert call_args[0][0] == "https://api.firecrawl.dev/v2/search" - assert call_args[1]["json"]["query"] == "python" + assert out == FirecrawlSearchToolOutputSchema(results=[FirecrawlSearchTool._to_item(CONTENT_HIT, "agents")]) + assert seen[0]["headers"]["Authorization"] == f"Bearer {DUMMY_KEY}" + assert seen[0]["body"] == { + "query": "agents", + "limit": 5, + "sources": ["web"], + "scrapeOptions": {"formats": ["markdown"], "onlyMainContent": True}, + } @pytest.mark.asyncio -async def test_fetch_reads_news_results(tool): - session = _mock_session(200, {"success": True, "data": {"news": [NEWS_HIT] * 4}}) - params = FirecrawlSearchToolInputSchema(queries=["python"], search_type="news", max_results_per_query=2) - - items = await tool._fetch(session, "python", params) +async def test_news_round_trip_reads_news_key_only(): + def respond(body): + return web.json_response({"success": True, "data": {"web": [WEB_HIT], "news": [NEWS_HIT]}}) - assert len(items) == 2 - assert items[0].published == "2 days ago" + out, _ = await search(respond, search_type="news") + assert [r.url for r in out.results] == [NEWS_HIT["url"]] @pytest.mark.asyncio -async def test_fetch_handles_missing_source_key(tool): - """The API omits the source key entirely when there are no results.""" - session = _mock_session(200, {"success": True, "data": {}}) - - items = await tool._fetch(session, "nothing", FirecrawlSearchToolInputSchema(queries=["nothing"])) - - assert items == [] +async def test_missing_source_key_is_an_empty_search(): + """The API omits the source key when there are no results; that is a real empty result.""" + out, _ = await search(lambda body: web.json_response({"success": True, "data": {}})) + assert out == FirecrawlSearchToolOutputSchema(results=[], failed_queries=[]) @pytest.mark.asyncio -async def test_fetch_skips_hits_without_url(tool): - session = _mock_session(200, {"success": True, "data": {"web": [WEB_HIT, {"title": "no url"}]}}) - - items = await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) - - assert [item.url for item in items] == ["https://www.python.org/"] +async def test_results_trimmed_to_max_results(): + hits = [{"url": f"https://example.com/{i}"} for i in range(5)] + out, _ = await search(lambda body: ok("web", hits), max_results_per_query=2) + assert [r.url for r in out.results] == ["https://example.com/0", "https://example.com/1"] @pytest.mark.asyncio -async def test_fetch_raises_on_http_error(tool): - session = _mock_session(401, {"success": False, "error": "Unauthorized: Invalid token"}, reason="Unauthorized") - - with pytest.raises(Exception, match="Firecrawl search failed for 'python': 401 Unauthorized: Invalid token"): - await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) +async def test_invalid_hits_skipped_valid_ones_kept(): + hits = [1, "s", None, {"title": "x"}, {"url": ""}, WEB_HIT, {"url": "https://example.com/b", "position": "N/A"}] + out, _ = await search(lambda body: ok("web", hits)) + assert [(r.url, r.position) for r in out.results] == [("https://www.python.org/", 1), ("https://example.com/b", None)] @pytest.mark.asyncio -async def test_fetch_raises_when_success_is_false(tool): - session = _mock_session(200, {"success": False}) - - with pytest.raises(Exception, match="Firecrawl search failed for 'python': 200 OK"): - await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) +async def test_all_hits_invalid_raises(): + error = await search_error(lambda body: ok("web", [1, None, {"title": "x"}, {"url": ""}])) + assert str(error) == "Firecrawl search failed for 'q': 200 none of the 4 'web' results had a usable url" +####################### +# Response validation # +####################### @pytest.mark.asyncio -async def test_fetch_reports_status_for_non_json_body(tool): - session = MagicMock() - response = SimpleNamespace(status=502, reason="Bad Gateway", json=AsyncMock(side_effect=ValueError("not json"))) - session.post.return_value.__aenter__.return_value = response - - with pytest.raises(Exception, match="Firecrawl search failed for 'python': 502 Bad Gateway"): - await tool._fetch(session, "python", FirecrawlSearchToolInputSchema(queries=["python"])) +@pytest.mark.parametrize( + "payload", + [ + {"success": True, "data": None}, + {"success": True}, + {"success": True, "data": {"web": {"url": "https://example.com/not-a-list"}}}, + {"success": True, "data": {"web": None}}, + {"success": True, "data": [WEB_HIT]}, + ], +) +async def test_malformed_success_response_raises(payload): + error = await search_error(lambda body: web.json_response(payload)) + assert str(error) == "Firecrawl search failed for 'q': 200 unexpected response shape for 'web' results" @pytest.mark.asyncio -async def test_fetch_applies_max_content_chars(): - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", max_content_chars=5)) - session = _mock_session(200, {"success": True, "data": {"web": [CONTENT_HIT, "not a dict"]}}) - - items = await tool._fetch(session, "agents", FirecrawlSearchToolInputSchema(queries=["agents"], include_content=True)) - - assert [item.content for item in items] == ["# Age"] +@pytest.mark.parametrize("payload", [{"success": "false"}, {"success": "true"}, {"success": 1}, {}]) +async def test_success_must_be_literally_true(payload): + error = await search_error(lambda body: web.json_response({**payload, "data": {"web": [WEB_HIT]}})) + assert str(error) == "Firecrawl search failed for 'q': 200 OK" @pytest.mark.asyncio -async def test_run_async_aggregates_results(tool): - async def fake_fetch(self, session, query, params): - return [FirecrawlSearchTool._to_item(WEB_HIT, query), FirecrawlSearchTool._to_item(NEWS_HIT, query)] - - with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): - out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q1", "q2"])) +@pytest.mark.parametrize("status, reason", [(201, "Created"), (500, "Internal Server Error")]) +async def test_non_200_with_success_true_raises(status, reason): + def respond(body): + return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}, status=status) - assert isinstance(out, FirecrawlSearchToolOutputSchema) - assert len(out.results) == 4 - assert {r.query for r in out.results} == {"q1", "q2"} + error = await search_error(respond) + assert str(error) == f"Firecrawl search failed for 'q': {status} {reason}" @pytest.mark.asyncio -async def test_run_async_skips_failing_query(tool): - async def fake_fetch(self, session, query, params): - if query == "bad": - raise Exception("rate limited") - return [FirecrawlSearchTool._to_item(WEB_HIT, query)] +async def test_provider_error_message_is_reported(): + error = await search_error(lambda body: web.json_response({"success": False, "error": "Payment required"}, status=402)) + assert str(error) == "Firecrawl search failed for 'q': 402 Payment required" - with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): - out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad", "good"])) - assert len(out.results) == 1 - assert out.results[0].query == "good" +@pytest.mark.asyncio +@pytest.mark.parametrize( + "response, expected", + [ + (lambda: web.Response(status=502, text="Bad Gateway", content_type="text/html"), "502 Bad Gateway"), + (lambda: web.Response(status=200, text=""), "200 OK"), + (lambda: web.Response(status=204), "204 No Content"), + ], +) +async def test_non_json_bodies_raise_with_status(response, expected): + error = await search_error(lambda body: response()) + assert str(error) == f"Firecrawl search failed for 'q': {expected}" @pytest.mark.asyncio -async def test_run_async_raises_when_every_query_fails(tool): - """A bad key or exhausted credits should surface as an error, not as an empty result.""" - - async def fake_fetch(self, session, query, params): - raise Exception("Firecrawl search failed for 'q': 401 Unauthorized: Invalid token") +async def test_redirect_is_not_followed(): + """A redirect, even to another host, is reported as an error; the query is never re-sent elsewhere.""" + async with fake_firecrawl(lambda body: ok("web", [WEB_HIT])) as (other_url, other_seen): + error = await search_error(lambda body: web.Response(status=307, headers={"Location": f"{other_url}/search"})) - with patch.object(FirecrawlSearchTool, "_fetch", fake_fetch): - with pytest.raises(Exception, match="401"): - await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q1", "q2"])) + assert str(error) == "Firecrawl search failed for 'q': 307 Temporary Redirect" + assert other_seen == [] +################### +# Batch semantics # +################### @pytest.mark.asyncio -async def test_http_round_trip(): +async def test_partial_failure_reports_failed_queries(caplog): def respond(body): - return web.json_response( - {"success": True, "data": {"web": [{**CONTENT_HIT, "url": f"https://example.com/{body['query']}"}]}} - ) + if body["query"] == "bad": + return web.json_response({"success": False, "error": "Rate limit exceeded"}, status=429) + return ok("web", [WEB_HIT]) - async with fake_firecrawl(respond) as (base_url, seen): - out = await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["agents"], include_content=True)) + with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): + out, _ = await search(respond, queries=["bad", "good"]) - assert [r.url for r in out.results] == ["https://example.com/agents"] - assert out.results[0].content.startswith("# Agent guide") - assert seen[0]["headers"]["Authorization"] == f"Bearer {DUMMY_KEY}" - assert seen[0]["body"]["scrapeOptions"] == {"formats": ["markdown"], "onlyMainContent": True} + assert [r.query for r in out.results] == ["good"] + assert out.failed_queries == ["bad"] + assert "Firecrawl query 'bad' failed: Firecrawl search failed for 'bad': 429 Rate limit exceeded" in caplog.text @pytest.mark.asyncio @pytest.mark.parametrize( - "payload", + "bad, expected", [ - {"success": True, "data": None}, - {"success": True}, - {"success": True, "data": {"web": {"url": "https://example.com/not-a-list"}}}, - {"success": True, "data": {"web": None}}, - {"success": True, "data": [WEB_HIT]}, + ({"success": True, "data": None}, "200 unexpected response shape for 'web' results"), + ({"success": False, "error": "Rate limit exceeded"}, "200 Rate limit exceeded"), ], ) -async def test_http_malformed_success_response_raises(payload): - """A 200 with success=true but no usable result list is an error, not an empty result.""" - async with fake_firecrawl(lambda body: web.json_response(payload)) as (base_url, _): - with pytest.raises(Exception, match="unexpected response shape for 'web' results"): - await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) - +async def test_empty_plus_failed_query_raises(bad, expected): + """No results anywhere plus a failure must not look like an empty search.""" -@pytest.mark.asyncio -async def test_http_non_json_error_page_reports_status(): def respond(body): - return web.Response(status=502, text="Bad Gateway", content_type="text/html") + return web.json_response({"success": True, "data": {}} if body["query"] == "empty" else bad) - async with fake_firecrawl(respond) as (base_url, _): - with pytest.raises(Exception, match="Firecrawl search failed for 'q': 502 Bad Gateway"): - await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + error = await search_error(respond, queries=["empty", "bad"]) + assert str(error) == f"Firecrawl search failed for 'bad': {expected}" @pytest.mark.asyncio -async def test_http_transport_error_does_not_expose_key(caplog): - """aiohttp's own errors (here a redirect loop) carry the request headers; the tool must not re-raise them as is.""" - +async def test_every_query_failing_raises_first_error(): def respond(body): - return web.Response(status=307, headers={"Location": "/v2/search"}) - - async with fake_firecrawl(respond) as (base_url, seen): - with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): - with pytest.raises(Exception) as raised: - await _http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + return web.json_response({"success": False, "error": "Unauthorized"}, status=401) - assert len(seen) > 1 - assert "Firecrawl search failed for 'q': TooManyRedirects" in str(raised.value) - assert raised.value.__cause__ is None - for text in (str(raised.value), repr(raised.value), caplog.text): - assert DUMMY_KEY not in text + error = await search_error(respond, queries=["a", "b"]) + assert str(error) == "Firecrawl search failed for 'a': 401 Unauthorized" +#################### +# Transport errors # +#################### @pytest.mark.asyncio -async def test_http_dummy_key_kept_out_of_repr_errors_and_logs(caplog): - """Even when the provider echoes the key back, it never reaches repr, raised errors, or logs.""" +async def test_connection_error_is_wrapped_without_context(): + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + tool = http_tool(f"http://127.0.0.1:{port}/v2") - def respond(body): - if body["query"] == "bad": - return web.json_response({"success": False, "error": f"Invalid token {DUMMY_KEY}"}, status=401) - return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}) + with pytest.raises(Exception) as raised: + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ClientConnectorError") + assert raised.value.__context__ is None and raised.value.__cause__ is None + assert DUMMY_KEY not in leak_surfaces(raised.value) - async with fake_firecrawl(respond) as (base_url, _): - tool = _http_tool(base_url) - with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): - out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad", "good"])) + +@pytest.mark.asyncio +async def test_timeout_is_wrapped_with_query(): + async def slow(body): + await asyncio.sleep(1) + return ok("web", [WEB_HIT]) + + async with fake_firecrawl(slow) as (base_url, _): + tool = http_tool(base_url) + tool.timeout = 0.05 with pytest.raises(Exception) as raised: - await tool.run_async(FirecrawlSearchToolInputSchema(queries=["bad"])) + await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) - assert [r.query for r in out.results] == ["good"] - assert "401 Invalid token [redacted]" in caplog.text - assert "401 Invalid token [redacted]" in str(raised.value) - for text in (caplog.text, str(raised.value), repr(raised.value), repr(tool), repr(tool.config), str(tool.config)): - assert DUMMY_KEY not in text + assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ") + assert "Timeout" in str(raised.value) + assert raised.value.__context__ is None +################ +# Key handling # +################ @pytest.mark.asyncio -async def test_run_async_sends_bearer_header(tool): - captured = {} +@pytest.mark.parametrize( + "echo", + [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, "fc-someotherkey12345"], +) +async def test_echoed_key_kept_out_of_errors_and_logs(caplog, echo): + """Even when the provider echoes a key back, it never reaches raised errors, tracebacks or logs.""" - class FakeSession: - def __init__(self, headers=None, timeout=None): - captured["headers"] = headers + def respond(body): + if body["query"] == "bad": + return web.json_response({"success": False, "error": echo}, status=401) + return ok("web", [WEB_HIT]) - async def __aenter__(self): - return self + with caplog.at_level(logging.DEBUG): + out, _ = await search(respond, queries=["bad", "good"]) + error = await search_error(respond, queries=["bad"]) - async def __aexit__(self, *args): - return False + assert out.failed_queries == ["bad"] + assert "[redacted]" in str(error) and "[redacted]" in caplog.text + for text in (caplog.text, leak_surfaces(error)): + assert DUMMY_KEY not in text and DUMMY_KEY.upper() not in text and "fc-someotherkey12345" not in text - async def fake_fetch(self, session, query, params): - return [] - with ( - patch("tool.firecrawl_search.aiohttp.ClientSession", FakeSession), - patch.object(FirecrawlSearchTool, "_fetch", fake_fetch), - ): - await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) +def test_key_kept_out_of_tool_and_config_state(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY)) + surfaces = [repr(tool), repr(tool.config), str(tool.config), tool.config.model_dump_json(), repr(vars(tool))] + surfaces.append(repr(vars(copy.copy(tool)))) + assert all(DUMMY_KEY not in text for text in surfaces) + assert tool._api_key.get_secret_value() == DUMMY_KEY + - assert captured["headers"]["Authorization"] == "Bearer test-key" - assert captured["headers"]["Content-Type"] == "application/json" +def test_short_key_does_not_mangle_messages(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="a")) + assert tool._redact("Firecrawl search failed") == "Firecrawl search failed" @pytest.mark.asyncio -async def test_run_async_requires_api_key(monkeypatch): +async def test_missing_key_raises(monkeypatch): monkeypatch.delenv("FIRECRAWL_API_KEY", raising=False) tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) @@ -399,14 +441,14 @@ async def test_run_async_requires_api_key(monkeypatch): def test_api_key_falls_back_to_environment(monkeypatch): monkeypatch.setenv("FIRECRAWL_API_KEY", "env-key") tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) - assert tool._api_key == "env-key" + assert tool._api_key.get_secret_value() == "env-key" -def test_run_invokes_run_async(tool): +def test_run_invokes_run_async(): + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k")) sentinel = FirecrawlSearchToolOutputSchema(results=[]) with patch.object(FirecrawlSearchTool, "run_async", AsyncMock(return_value=sentinel)): - out = tool.run(FirecrawlSearchToolInputSchema(queries=["x"])) - assert out is sentinel + assert tool.run(FirecrawlSearchToolInputSchema(queries=["x"])) is sentinel if __name__ == "__main__": diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index efdeed5f..c91e57c9 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -1,11 +1,12 @@ import asyncio import logging import os +import re from concurrent.futures import ThreadPoolExecutor -from typing import List, Literal, Optional, Union +from typing import Any, List, Literal, Optional import aiohttp -from pydantic import Field, SecretStr +from pydantic import Field, SecretStr, field_validator from atomic_agents import BaseIOSchema, BaseTool, BaseToolConfig @@ -25,6 +26,7 @@ class FirecrawlSearchToolInputSchema(BaseIOSchema): queries: List[str] = Field( ..., + min_length=1, description='Search queries to run. Supports operators such as site:, "exact phrase", -term, and filetype:pdf.', ) search_type: Literal["web", "news"] = Field( @@ -52,11 +54,20 @@ class FirecrawlSearchResultItem(BaseIOSchema): published: Optional[str] = Field(None, description="Publication date string (news only).") content: Optional[str] = Field(None, description="Page content as Markdown (only when include_content is set).") + @field_validator("position", mode="before") + @classmethod + def _positive_rank_or_none(cls, value: Any) -> Optional[int]: + # Ranks come from the provider; anything other than a positive int (bool, float, "N/A") is dropped. + return value if type(value) is int and value > 0 else None + class FirecrawlSearchToolOutputSchema(BaseIOSchema): """Output of the Firecrawl search tool.""" results: List[FirecrawlSearchResultItem] = Field(..., description="Matching results across all queries.") + failed_queries: List[str] = Field( + default_factory=list, description="Queries that failed while others succeeded. Details are logged." + ) ################# @@ -89,10 +100,14 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear """Tool for searching the web and news via the Firecrawl Search API.""" TIME_RANGES = {"day": "qdr:d", "week": "qdr:w", "month": "qdr:m", "year": "qdr:y"} + # Anything shaped like a Firecrawl key is redacted from error and log text, plus the configured key itself + # when it is long enough that replacing it cannot mangle ordinary words. + KEY_PATTERN = re.compile(r"fc-[A-Za-z0-9]{8,}") + MIN_REDACTED_KEY_LENGTH = 8 def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) - self._api_key = config.api_key.get_secret_value() or os.getenv("FIRECRAWL_API_KEY", "") + self._api_key = SecretStr(config.api_key.get_secret_value() or os.getenv("FIRECRAWL_API_KEY", "")) self.base_url = config.base_url.rstrip("/") self.location = config.location self.time_range = config.time_range @@ -100,30 +115,29 @@ def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig self.timeout = config.timeout @staticmethod - def _first(*values): - return next((value for value in values if value), None) + def _first_text(*values: Any) -> Optional[str]: + """Return the first non-empty string; result fields are provider data and may have any type.""" + return next((value for value in values if isinstance(value, str) and value), None) @classmethod - def _to_item(cls, hit: dict, query: str, max_content_chars: Optional[int] = None) -> FirecrawlSearchResultItem: - metadata = hit.get("metadata") - metadata = metadata if isinstance(metadata, dict) else {} - content = hit.get("markdown") + def _to_item(cls, hit: Any, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: + """Normalise one result, or return None when it has no usable URL.""" + url = cls._first_text(hit.get("url")) if isinstance(hit, dict) else None + if url is None: + return None + content = cls._first_text(hit.get("markdown")) return FirecrawlSearchResultItem( query=query, - title=cls._first(hit.get("title"), metadata.get("title"), hit["url"]), - url=hit["url"], - description=cls._first(hit.get("description"), hit.get("snippet"), metadata.get("description")), + title=cls._first_text(hit.get("title"), url), + url=url, + description=cls._first_text(hit.get("description"), hit.get("snippet")), position=hit.get("position"), - published=hit.get("date"), - content=content[:max_content_chars] if content else None, + published=cls._first_text(hit.get("date")), + content=content and content[:max_content_chars], ) def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dict: - body = { - "query": query, - "limit": params.max_results_per_query, - "sources": [params.search_type], - } + body = {"query": query, "limit": params.max_results_per_query, "sources": [params.search_type]} if self.location: body["location"] = self.location if self.time_range and params.search_type == "web": @@ -133,29 +147,54 @@ def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dic return body def _redact(self, text: str) -> str: - return text.replace(self._api_key, "[redacted]") if self._api_key else text + key = self._api_key.get_secret_value() + if len(key) >= self.MIN_REDACTED_KEY_LENGTH: + text = re.sub(re.escape(key), "[redacted]", text, flags=re.IGNORECASE) + return self.KEY_PATTERN.sub("[redacted]", text) - def _error(self, query: str, status: Union[int, str], detail: Optional[str]) -> Exception: - return Exception(self._redact(f"Firecrawl search failed for '{query}': {status} {detail}")) + def _error(self, query: str, detail: str) -> Exception: + return Exception(self._redact(f"Firecrawl search failed for '{query}': {detail}")) @staticmethod - async def _read_json(resp: aiohttp.ClientResponse): + async def _read_json(resp: aiohttp.ClientResponse) -> Any: try: return await resp.json(content_type=None) except ValueError: return None - def _check_success(self, query: str, status: int, reason: Optional[str], data) -> None: + def _failure(self, status: int, reason: Optional[str], data: Any) -> Optional[str]: + """Return why a response is not a successful search (only a 200 whose body says success is true is), + or None when it is.""" payload = data if isinstance(data, dict) else {} - if status != 200 or not payload.get("success"): - raise self._error(query, status, payload.get("error") or reason) + if status == 200 and payload.get("success") is True: + return None + return self._redact(f"{status} {payload.get('error') or reason}") - def _extract_hits(self, query: str, data: dict, search_type: str) -> list: + async def _post(self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema) -> dict: + """Send one search request and return the successful response body. + + Errors are raised outside the except block, so the original aiohttp error (whose repr carries the request + headers) is not attached as context, and the provider body is dropped first, so a traceback that captures + locals cannot show a key the provider echoed back.""" + try: + async with session.post( + f"{self.base_url}/search", json=self._build_body(query, params), allow_redirects=False + ) as resp: + payload = await self._read_json(resp) + failure = self._failure(resp.status, resp.reason, payload) + except (aiohttp.ClientError, asyncio.TimeoutError) as error: + payload, failure = None, f"request failed: {type(error).__name__} {error}" + if failure: + payload = None + raise self._error(query, failure) + return payload + + def _extract_hits(self, query: str, payload: dict, search_type: str) -> list: # The key for the requested source is absent when there are no results. - groups = data.get("data") + groups = payload.get("data") hits = groups.get(search_type, []) if isinstance(groups, dict) else None if not isinstance(hits, list): - raise self._error(query, 200, f"unexpected response shape for '{search_type}' results") + raise self._error(query, f"200 unexpected response shape for '{search_type}' results") return hits async def _fetch( @@ -164,50 +203,46 @@ async def _fetch( query: str, params: FirecrawlSearchToolInputSchema, ) -> List[FirecrawlSearchResultItem]: - try: - async with session.post(f"{self.base_url}/search", json=self._build_body(query, params)) as resp: - data = await self._read_json(resp) - self._check_success(query, resp.status, resp.reason, data) - except aiohttp.ClientError as error: - # aiohttp errors such as TooManyRedirects carry the request headers, Authorization included, in their repr. - raise self._error(query, type(error).__name__, str(error)) from None - - hits = self._extract_hits(query, data, params.search_type) - items = [self._to_item(hit, query, self.max_content_chars) for hit in hits if isinstance(hit, dict) and hit.get("url")] + payload = await self._post(session, query, params) + hits = self._extract_hits(query, payload, params.search_type) + items = list(filter(None, (self._to_item(hit, query, self.max_content_chars) for hit in hits))) + if hits and not items: + raise self._error(query, f"200 none of the {len(hits)} '{params.search_type}' results had a usable url") return items[: params.max_results_per_query] - def _log_failure(self, query: str, error: BaseException) -> None: - logger.warning("Firecrawl query '%s' failed: %s", query, self._redact(str(error) or repr(error))) - - def _collect(self, queries: List[str], grouped: list) -> List[FirecrawlSearchResultItem]: + def _collect(self, queries: List[str], grouped: list) -> FirecrawlSearchToolOutputSchema: + """Merge per-query results. Raise when no query produced results and at least one failed, so a bad key, + exhausted credits or a malformed response never looks like an empty search.""" results: List[FirecrawlSearchResultItem] = [] - errors = [] + failed_queries: List[str] = [] + errors: List[Exception] = [] for query, group in zip(queries, grouped): - if isinstance(group, BaseException): + if isinstance(group, Exception): + failed_queries.append(query) errors.append(group) - self._log_failure(query, group) + logger.warning("Firecrawl query '%s' failed: %s", query, self._redact(str(group))) continue results.extend(group) - # A bad key or exhausted credits fails every query; raise instead of returning an empty list. - if errors and len(errors) == len(grouped): + if errors and not results: raise errors[0] - return results + return FirecrawlSearchToolOutputSchema(results=results, failed_queries=failed_queries) async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: - if not self._api_key: + if not self._api_key.get_secret_value(): raise ValueError( "Firecrawl API key is missing. Set FirecrawlSearchToolConfig.api_key or the FIRECRAWL_API_KEY " "environment variable." ) - headers = {"Authorization": f"Bearer {self._api_key}", "Content-Type": "application/json"} - timeout = aiohttp.ClientTimeout(total=self.timeout) - async with aiohttp.ClientSession(headers=headers, timeout=timeout) as session: - tasks = [self._fetch(session, q, params) for q in params.queries] - grouped = await asyncio.gather(*tasks, return_exceptions=True) + # The Authorization header is built inline so no local variable holds the key. + async with aiohttp.ClientSession( + headers={"Authorization": f"Bearer {self._api_key.get_secret_value()}", "Content-Type": "application/json"}, + timeout=aiohttp.ClientTimeout(total=self.timeout), + ) as session: + grouped = await asyncio.gather(*(self._fetch(session, q, params) for q in params.queries), return_exceptions=True) - return FirecrawlSearchToolOutputSchema(results=self._collect(params.queries, grouped)) + return self._collect(params.queries, grouped) def run(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: with ThreadPoolExecutor() as executor: From 1589b24b8bd573f9c47970174ce49a16875a2307 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:15:47 -0300 Subject: [PATCH 05/15] Report failure reasons, wrap invalid URLs, require http(s) result URLs, dedupe queries --- atomic-forge/tools/firecrawl_search/README.md | 6 +- .../tests/test_firecrawl_search.py | 77 +++++++++++++++---- .../firecrawl_search/tool/firecrawl_search.py | 70 +++++++++++------ 3 files changed, 111 insertions(+), 42 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index af7120bc..674cf918 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -25,14 +25,14 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec ## Input & Output Structure ### Input Schema -- `queries` (list[str], at least one): search queries to run. Operators such as `site:`, `"exact phrase"`, `-term`, and `filetype:pdf` are supported. +- `queries` (list[str], at least one): search queries to run. Duplicates are sent once. Operators such as `site:`, `"exact phrase"`, `-term`, and `filetype:pdf` are supported. - `search_type` (str): `web` or `news`. Default `web`. - `max_results_per_query` (int): 1-100 (default 5, kept low because each result can carry a full page with `include_content`). - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a usable URL are skipped, and badly typed optional fields are left empty. -- `failed_queries`: queries that failed while others returned results. The reason for each is logged as a warning. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without an http(s) URL are skipped, and badly typed optional fields are left empty. +- `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message, with any key redacted). They are also logged as warnings. ## Usage If no query returns results and at least one failed (for example a wrong API key, no credits, or a malformed response), `run` raises the first error instead of returning an empty list. Redirects are not followed; they are reported as errors. diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index b7fae4f0..801866b5 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -16,6 +16,7 @@ sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) from tool.firecrawl_search import ( # noqa: E402 + FirecrawlSearchFailure, FirecrawlSearchResultItem, FirecrawlSearchTool, FirecrawlSearchToolConfig, @@ -25,6 +26,7 @@ DUMMY_KEY = "fc-dummy0secret0key0123456789" +OTHER_KEY = "fc-0123456789abcdef0123456789abcdef" WEB_HIT = { "url": "https://www.python.org/", @@ -138,6 +140,7 @@ def test_to_item_content_and_truncation(): ("position", 1.5, "position", None), ("position", True, "position", None), ("position", -3, "position", None), + ("position", 0, "position", None), ("date", 20250101, "published", None), ("markdown", 42, "content", None), ("markdown", {}, "content", None), @@ -149,7 +152,22 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): assert getattr(item, attribute) == expected -@pytest.mark.parametrize("hit", [1, "s", None, [], {"title": "no url"}, {"url": ""}, {"url": 123}]) +@pytest.mark.parametrize( + "hit", + [ + 1, + "s", + None, + [], + {"title": "no url"}, + {"url": ""}, + {"url": 123}, + {"url": ["https://example.com"]}, + {"url": " https://example.com"}, + {"url": "javascript:alert(1)"}, + {"url": "ftp://example.com/file"}, + ], +) def test_to_item_without_usable_url_is_none(hit): assert FirecrawlSearchTool._to_item(hit, "q") is None @@ -221,7 +239,7 @@ def respond(body): async def test_missing_source_key_is_an_empty_search(): """The API omits the source key when there are no results; that is a real empty result.""" out, _ = await search(lambda body: web.json_response({"success": True, "data": {}})) - assert out == FirecrawlSearchToolOutputSchema(results=[], failed_queries=[]) + assert out == FirecrawlSearchToolOutputSchema(results=[], failures=[]) @pytest.mark.asyncio @@ -244,6 +262,13 @@ async def test_all_hits_invalid_raises(): assert str(error) == "Firecrawl search failed for 'q': 200 none of the 4 'web' results had a usable url" +@pytest.mark.asyncio +async def test_duplicate_queries_sent_once(): + out, seen = await search(lambda body: ok("web", [WEB_HIT]), queries=["q", "q"]) + assert len(seen) == 1 + assert [r.query for r in out.results] == ["q"] + + ####################### # Response validation # ####################### @@ -267,7 +292,7 @@ async def test_malformed_success_response_raises(payload): @pytest.mark.parametrize("payload", [{"success": "false"}, {"success": "true"}, {"success": 1}, {}]) async def test_success_must_be_literally_true(payload): error = await search_error(lambda body: web.json_response({**payload, "data": {"web": [WEB_HIT]}})) - assert str(error) == "Firecrawl search failed for 'q': 200 OK" + assert str(error) == "Firecrawl search failed for 'q': 200 response did not report success" @pytest.mark.asyncio @@ -291,7 +316,7 @@ async def test_provider_error_message_is_reported(): "response, expected", [ (lambda: web.Response(status=502, text="Bad Gateway", content_type="text/html"), "502 Bad Gateway"), - (lambda: web.Response(status=200, text=""), "200 OK"), + (lambda: web.Response(status=200, text=""), "200 response did not report success"), (lambda: web.Response(status=204), "204 No Content"), ], ) @@ -323,9 +348,10 @@ def respond(body): with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): out, _ = await search(respond, queries=["bad", "good"]) + reason = "Firecrawl search failed for 'bad': 429 Rate limit exceeded" assert [r.query for r in out.results] == ["good"] - assert out.failed_queries == ["bad"] - assert "Firecrawl query 'bad' failed: Firecrawl search failed for 'bad': 429 Rate limit exceeded" in caplog.text + assert out.failures == [FirecrawlSearchFailure(query="bad", error=reason)] + assert f"Firecrawl query 'bad' failed: {reason}" in caplog.text @pytest.mark.asyncio @@ -385,9 +411,30 @@ async def slow(body): with pytest.raises(Exception) as raised: await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + assert str(raised.value) == "Firecrawl search failed for 'q': request failed: TimeoutError after 0.05s" + assert raised.value.__context__ is None + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "base_url", + [ + "http://exa\nmple.com/v2", + f"http://{'a' * 70}.example.com/v2", + f"{DUMMY_KEY}/v2", # aiohttp's InvalidUrlClientError echoes the URL, so its text must be redacted too + ], +) +async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url): + """aiohttp raises ValueError/UnicodeError for such URLs from a frame whose locals hold the request headers.""" + with pytest.raises(Exception) as raised: + await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ") - assert "Timeout" in str(raised.value) assert raised.value.__context__ is None + stack = traceback.TracebackException.from_exception(raised.value, capture_locals=True).stack + non_test_frames = [repr(frame.locals) for frame in stack if frame.filename != __file__] + assert non_test_frames + assert DUMMY_KEY not in str(raised.value) + repr(raised.value) + "".join(non_test_frames) ################ @@ -396,7 +443,7 @@ async def slow(body): @pytest.mark.asyncio @pytest.mark.parametrize( "echo", - [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, "fc-someotherkey12345"], + [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, OTHER_KEY, OTHER_KEY.upper()], ) async def test_echoed_key_kept_out_of_errors_and_logs(caplog, echo): """Even when the provider echoes a key back, it never reaches raised errors, tracebacks or logs.""" @@ -410,10 +457,10 @@ def respond(body): out, _ = await search(respond, queries=["bad", "good"]) error = await search_error(respond, queries=["bad"]) - assert out.failed_queries == ["bad"] - assert "[redacted]" in str(error) and "[redacted]" in caplog.text - for text in (caplog.text, leak_surfaces(error)): - assert DUMMY_KEY not in text and DUMMY_KEY.upper() not in text and "fc-someotherkey12345" not in text + assert [failure.query for failure in out.failures] == ["bad"] + assert "[redacted]" in str(error) and "[redacted]" in caplog.text and "[redacted]" in out.failures[0].error + for text in (caplog.text, leak_surfaces(error), repr(out)): + assert all(key not in text for key in (DUMMY_KEY, DUMMY_KEY.upper(), OTHER_KEY, OTHER_KEY.upper())) def test_key_kept_out_of_tool_and_config_state(): @@ -424,9 +471,11 @@ def test_key_kept_out_of_tool_and_config_state(): assert tool._api_key.get_secret_value() == DUMMY_KEY -def test_short_key_does_not_mangle_messages(): +def test_redaction_does_not_mangle_ordinary_text(): + """A one-letter key is not redacted (it would hit every 'a'), and fc- words that are not keys are left alone.""" tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="a")) - assert tool._redact("Firecrawl search failed") == "Firecrawl search failed" + text = "Firecrawl search failed for 'fc-barcelona-2024'" + assert tool._redact(text) == text @pytest.mark.asyncio diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index c91e57c9..4a5de9ef 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -3,7 +3,7 @@ import os import re from concurrent.futures import ThreadPoolExecutor -from typing import Any, List, Literal, Optional +from typing import Any, List, Literal, Optional, Tuple import aiohttp from pydantic import Field, SecretStr, field_validator @@ -61,12 +61,19 @@ def _positive_rank_or_none(cls, value: Any) -> Optional[int]: return value if type(value) is int and value > 0 else None +class FirecrawlSearchFailure(BaseIOSchema): + """A query that failed while other queries returned results.""" + + query: str = Field(..., description="The query that failed.") + error: str = Field(..., description="Why it failed, e.g. an HTTP status and the API's error message.") + + class FirecrawlSearchToolOutputSchema(BaseIOSchema): """Output of the Firecrawl search tool.""" results: List[FirecrawlSearchResultItem] = Field(..., description="Matching results across all queries.") - failed_queries: List[str] = Field( - default_factory=list, description="Queries that failed while others succeeded. Details are logged." + failures: List[FirecrawlSearchFailure] = Field( + default_factory=list, description="Queries that failed while others returned results, with the reason." ) @@ -100,9 +107,9 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear """Tool for searching the web and news via the Firecrawl Search API.""" TIME_RANGES = {"day": "qdr:d", "week": "qdr:w", "month": "qdr:m", "year": "qdr:y"} - # Anything shaped like a Firecrawl key is redacted from error and log text, plus the configured key itself + # Anything in the Firecrawl key format is redacted from error and log text, plus the configured key itself # when it is long enough that replacing it cannot mangle ordinary words. - KEY_PATTERN = re.compile(r"fc-[A-Za-z0-9]{8,}") + KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): @@ -121,9 +128,9 @@ def _first_text(*values: Any) -> Optional[str]: @classmethod def _to_item(cls, hit: Any, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: - """Normalise one result, or return None when it has no usable URL.""" - url = cls._first_text(hit.get("url")) if isinstance(hit, dict) else None - if url is None: + """Normalise one result, or return None when it has no http(s) URL.""" + url = hit.get("url") if isinstance(hit, dict) else None + if not str(url).startswith(("http://", "https://")): return None content = cls._first_text(hit.get("markdown")) return FirecrawlSearchResultItem( @@ -152,8 +159,9 @@ def _redact(self, text: str) -> str: text = re.sub(re.escape(key), "[redacted]", text, flags=re.IGNORECASE) return self.KEY_PATTERN.sub("[redacted]", text) - def _error(self, query: str, detail: str) -> Exception: - return Exception(self._redact(f"Firecrawl search failed for '{query}': {detail}")) + @staticmethod + def _error(query: str, detail: str) -> Exception: + return Exception(f"Firecrawl search failed for '{query}': {detail}") @staticmethod async def _read_json(resp: aiohttp.ClientResponse) -> Any: @@ -164,26 +172,36 @@ async def _read_json(resp: aiohttp.ClientResponse) -> Any: def _failure(self, status: int, reason: Optional[str], data: Any) -> Optional[str]: """Return why a response is not a successful search (only a 200 whose body says success is true is), - or None when it is.""" + or None when it is. Provider text is redacted here, where it enters the tool.""" payload = data if isinstance(data, dict) else {} if status == 200 and payload.get("success") is True: return None return self._redact(f"{status} {payload.get('error') or reason}") - async def _post(self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema) -> dict: - """Send one search request and return the successful response body. + async def _exchange( + self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema + ) -> Tuple[Any, Optional[str]]: + """Send one search request and return the parsed body plus why it failed (None on success). - Errors are raised outside the except block, so the original aiohttp error (whose repr carries the request - headers) is not attached as context, and the provider body is dropped first, so a traceback that captures - locals cannot show a key the provider echoed back.""" + Transport errors, timeouts and invalid URLs are turned into a failure reason here instead of propagating, + because aiohttp's own frames and errors hold the request headers, Authorization included.""" try: async with session.post( f"{self.base_url}/search", json=self._build_body(query, params), allow_redirects=False ) as resp: payload = await self._read_json(resp) - failure = self._failure(resp.status, resp.reason, payload) - except (aiohttp.ClientError, asyncio.TimeoutError) as error: - payload, failure = None, f"request failed: {type(error).__name__} {error}" + reason = resp.reason if resp.status != 200 else "response did not report success" + return payload, self._failure(resp.status, reason, payload) + except (aiohttp.ClientError, asyncio.TimeoutError, ValueError) as error: + return None, self._redact(f"request failed: {type(error).__name__} {str(error) or f'after {self.timeout:g}s'}") + + async def _post(self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema) -> dict: + """Return the body of a successful search response, or raise. + + The raise happens in this frame, which holds neither the response nor an aiohttp error, and the body of a + failed response is dropped first, so a traceback that captures locals cannot show a key echoed back in an + error response.""" + payload, failure = await self._exchange(session, query, params) if failure: payload = None raise self._error(query, failure) @@ -214,19 +232,19 @@ def _collect(self, queries: List[str], grouped: list) -> FirecrawlSearchToolOutp """Merge per-query results. Raise when no query produced results and at least one failed, so a bad key, exhausted credits or a malformed response never looks like an empty search.""" results: List[FirecrawlSearchResultItem] = [] - failed_queries: List[str] = [] + failures: List[FirecrawlSearchFailure] = [] errors: List[Exception] = [] for query, group in zip(queries, grouped): if isinstance(group, Exception): - failed_queries.append(query) errors.append(group) - logger.warning("Firecrawl query '%s' failed: %s", query, self._redact(str(group))) + failures.append(FirecrawlSearchFailure(query=query, error=str(group))) + logger.warning("Firecrawl query '%s' failed: %s", query, failures[-1].error) continue results.extend(group) if errors and not results: raise errors[0] - return FirecrawlSearchToolOutputSchema(results=results, failed_queries=failed_queries) + return FirecrawlSearchToolOutputSchema(results=results, failures=failures) async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: if not self._api_key.get_secret_value(): @@ -235,14 +253,16 @@ async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSe "environment variable." ) + # Duplicate queries would be billed twice and return the same results. + queries = list(dict.fromkeys(params.queries)) # The Authorization header is built inline so no local variable holds the key. async with aiohttp.ClientSession( headers={"Authorization": f"Bearer {self._api_key.get_secret_value()}", "Content-Type": "application/json"}, timeout=aiohttp.ClientTimeout(total=self.timeout), ) as session: - grouped = await asyncio.gather(*(self._fetch(session, q, params) for q in params.queries), return_exceptions=True) + grouped = await asyncio.gather(*(self._fetch(session, q, params) for q in queries), return_exceptions=True) - return self._collect(params.queries, grouped) + return self._collect(queries, grouped) def run(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutputSchema: with ThreadPoolExecutor() as executor: From 7a1bcff0192917218f64f8666eba90ca44c2dab6 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:20:10 -0300 Subject: [PATCH 06/15] Require an http(s) URL with a host for results; cover key-pattern and URL edge cases --- .../firecrawl_search/tests/test_firecrawl_search.py | 9 ++++++++- .../tools/firecrawl_search/tool/firecrawl_search.py | 4 +++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 801866b5..69f53a42 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -152,6 +152,10 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): assert getattr(item, attribute) == expected +def test_to_item_accepts_uppercase_scheme(): + assert FirecrawlSearchTool._to_item({"url": "HTTPS://Example.com/a"}, "q").url == "HTTPS://Example.com/a" + + @pytest.mark.parametrize( "hit", [ @@ -166,6 +170,9 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): {"url": " https://example.com"}, {"url": "javascript:alert(1)"}, {"url": "ftp://example.com/file"}, + {"url": "httpx://example.com"}, + {"url": "https://"}, + {"url": "http:///path-without-host"}, ], ) def test_to_item_without_usable_url_is_none(hit): @@ -474,7 +481,7 @@ def test_key_kept_out_of_tool_and_config_state(): def test_redaction_does_not_mangle_ordinary_text(): """A one-letter key is not redacted (it would hit every 'a'), and fc- words that are not keys are left alone.""" tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="a")) - text = "Firecrawl search failed for 'fc-barcelona-2024'" + text = "Firecrawl search failed for 'fc-barcelona-2024' and 'fc-0123abcd'" assert tool._redact(text) == text diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 4a5de9ef..e0a98b00 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -111,6 +111,8 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear # when it is long enough that replacing it cannot mangle ordinary words. KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 + # A result is only kept when its URL is http(s) with a host. + RESULT_URL_PATTERN = re.compile(r"https?://[^\s/?#]+", re.IGNORECASE) def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) @@ -130,7 +132,7 @@ def _first_text(*values: Any) -> Optional[str]: def _to_item(cls, hit: Any, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: """Normalise one result, or return None when it has no http(s) URL.""" url = hit.get("url") if isinstance(hit, dict) else None - if not str(url).startswith(("http://", "https://")): + if not cls.RESULT_URL_PATTERN.match(str(url)): return None content = cls._first_text(hit.get("markdown")) return FirecrawlSearchResultItem( From cff6915fc6c6957367bf01a7aa0e021c3646a534 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 10:01:23 -0300 Subject: [PATCH 07/15] Keep rejected bodies out of raising frames, require a real host in result URLs, redact query text in diagnostics --- atomic-forge/tools/firecrawl_search/README.md | 4 +- .../tests/test_firecrawl_search.py | 292 ++++++++++++------ .../firecrawl_search/tool/firecrawl_search.py | 110 ++++--- 3 files changed, 264 insertions(+), 142 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 674cf918..5edd2e0e 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -31,8 +31,8 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without an http(s) URL are skipped, and badly typed optional fields are left empty. -- `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message, with any key redacted). They are also logged as warnings. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (one with a host and no whitespace) are skipped, and badly typed optional fields are left empty. +- `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). A key appearing in either is redacted. They are also logged as warnings. ## Usage If no query returns results and at least one failed (for example a wrong API key, no credits, or a malformed response), `run` raises the first error instead of returning an empty list. Redirects are not followed; they are reported as errors. diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 69f53a42..9b3098d8 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -6,6 +6,7 @@ import sys import traceback from contextlib import asynccontextmanager +from typing import AsyncIterator, Awaitable, Callable, Dict, List, Sequence, Tuple, Union from unittest.mock import AsyncMock, patch import pytest @@ -24,9 +25,14 @@ FirecrawlSearchToolOutputSchema, ) +Body = Dict[str, object] +Responder = Callable[[Body], Union[web.Response, Awaitable[web.Response]]] -DUMMY_KEY = "fc-dummy0secret0key0123456789" -OTHER_KEY = "fc-0123456789abcdef0123456789abcdef" +DUMMY_KEY = "fc-dummy0secret0key0123456789" # a configured key in a custom format +CANONICAL_KEY = "fc-0123456789abcdef0123456789abcdef" # a key in Firecrawl's own format +KEYS = (DUMMY_KEY, DUMMY_KEY.upper(), CANONICAL_KEY, CANONICAL_KEY.upper()) +TOOL_FILE = os.path.join("tool", "firecrawl_search.py") +LOOPBACK_HOSTS = {"127.0.0.1", "::1", "localhost"} WEB_HIT = { "url": "https://www.python.org/", @@ -54,16 +60,60 @@ } -def ok(source: str, hits: list) -> web.Response: - return web.json_response({"success": True, "data": {source: hits}}) +@pytest.fixture(autouse=True) +def offline(monkeypatch): + """Keep the suite offline: any attempt to resolve a host other than the local test server fails the test.""" + attempts: List[str] = [] + real_getaddrinfo = socket.getaddrinfo + def loopback_only(host, *args, **kwargs): + if host not in LOOPBACK_HOSTS: + attempts.append(str(host)) + raise socket.gaierror("network access is disabled in these tests") + return real_getaddrinfo(host, *args, **kwargs) + monkeypatch.setattr(socket, "getaddrinfo", loopback_only) + yield + assert attempts == [] + + +##################### +# Response builders # +##################### +def respond_json(payload: object, status: int = 200) -> Responder: + def respond(body: Body) -> web.Response: + return web.json_response(payload, status=status) + + return respond + + +def respond_hits(source: str, hits: Sequence[object]) -> Responder: + return respond_json({"success": True, "data": {source: list(hits)}}) + + +def respond_raw(**response) -> Responder: + def respond(body: Body) -> web.Response: + return web.Response(**response) + + return respond + + +def respond_per_query(bad_query: str, bad: Responder, good: Responder) -> Responder: + def respond(body: Body) -> Union[web.Response, Awaitable[web.Response]]: + return bad(body) if body["query"] == bad_query else good(body) + + return respond + + +################ +# Test harness # +################ @asynccontextmanager -async def fake_firecrawl(respond): +async def fake_firecrawl(respond: Responder) -> AsyncIterator[Tuple[str, List[Body]]]: """Serve POST /v2/search on a local aiohttp server; `respond(body)` builds each response.""" - seen = [] + seen: List[Body] = [] - async def handler(request): + async def handler(request: web.Request) -> web.Response: body = await request.json() seen.append({"headers": dict(request.headers), "body": body}) response = respond(body) @@ -71,7 +121,7 @@ async def handler(request): app = web.Application() app.router.add_post("/v2/search", handler) - server = TestServer(app) + server = TestServer(app, host="127.0.0.1") await server.start_server() try: yield str(server.make_url("/v2")), seen @@ -79,33 +129,52 @@ async def handler(request): await server.close() -def http_tool(base_url: str, **config) -> FirecrawlSearchTool: - return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY, base_url=base_url, **config)) +def http_tool(base_url: str, api_key: str = DUMMY_KEY, **config) -> FirecrawlSearchTool: + return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=api_key, base_url=base_url, **config)) -async def search(respond, queries=("q",), **params): +async def search( + respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY, **params +) -> Tuple[FirecrawlSearchToolOutputSchema, List[Body]]: """Run one search against a fake server and return (output, requests seen).""" async with fake_firecrawl(respond) as (base_url, seen): - out = await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=list(queries), **params)) + tool = http_tool(base_url, api_key) + out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=list(queries), **params)) return out, seen -async def search_error(respond, queries=("q",), **params) -> Exception: +async def search_error(respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY, **params) -> Exception: with pytest.raises(Exception) as raised: - await search(respond, queries, **params) + await search(respond, queries, api_key, **params) return raised.value -def leak_surfaces(error: BaseException) -> str: - """Every text an error can surface through, including the locals of the tool's frames in its traceback.""" +def frame_locals(error: BaseException, keep: Callable[[str], bool]) -> List[str]: stack = traceback.TracebackException.from_exception(error, capture_locals=True).stack - tool_frames = [ - repr(frame.locals) for frame in stack if frame.filename.endswith(os.path.join("tool", "firecrawl_search.py")) - ] + return [repr(frame.locals) for frame in stack if keep(frame.filename)] + + +def is_tool_frame(filename: str) -> bool: + return filename.endswith(TOOL_FILE) + + +def is_outside_this_test_file(filename: str) -> bool: + """Library and tool frames; this file's own frames hold the test's inputs, which may contain the key.""" + return filename != __file__ + + +def error_surfaces(error: BaseException) -> str: + """Every text an error surfaces: str, repr, its chain, and the locals of every tool frame in its traceback.""" + tool_frames = frame_locals(error, is_tool_frame) assert tool_frames, "expected the traceback to pass through the tool" return "\n".join([str(error), repr(error), repr(error.__context__), repr(error.__cause__), *tool_frames]) +def assert_no_key(*texts: str) -> None: + for key in KEYS: + assert not any(key in text for text in texts), key + + ################## # Result parsing # ################## @@ -152,8 +221,18 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): assert getattr(item, attribute) == expected -def test_to_item_accepts_uppercase_scheme(): - assert FirecrawlSearchTool._to_item({"url": "HTTPS://Example.com/a"}, "q").url == "HTTPS://Example.com/a" +@pytest.mark.parametrize( + "url", + [ + "HTTPS://Example.com/a", + "http://example.com", + "https://example.com:8443/a?b=c#d", + "http://[::1]:8080/x", + "https://user@example.com/", + ], +) +def test_to_item_keeps_valid_http_urls(url): + assert FirecrawlSearchTool._to_item({"url": url}, "q").url == url @pytest.mark.parametrize( @@ -167,12 +246,19 @@ def test_to_item_accepts_uppercase_scheme(): {"url": ""}, {"url": 123}, {"url": ["https://example.com"]}, - {"url": " https://example.com"}, {"url": "javascript:alert(1)"}, {"url": "ftp://example.com/file"}, {"url": "httpx://example.com"}, {"url": "https://"}, {"url": "http:///path-without-host"}, + {"url": "https://@/a"}, + {"url": "https://:443/a"}, + {"url": "https://user@:443/a"}, + {"url": " https://example.com"}, + {"url": "https://example.com/a b"}, + {"url": "https://exa mple.com/a"}, + {"url": "https://example.com/a\n"}, + {"url": "https://example.com\t/a"}, ], ) def test_to_item_without_usable_url_is_none(hit): @@ -221,7 +307,7 @@ def test_empty_queries_rejected(): ################################### @pytest.mark.asyncio async def test_web_round_trip(): - out, seen = await search(lambda body: ok("web", [CONTENT_HIT]), queries=["agents"], include_content=True) + out, seen = await search(respond_hits("web", [CONTENT_HIT]), queries=["agents"], include_content=True) assert out == FirecrawlSearchToolOutputSchema(results=[FirecrawlSearchTool._to_item(CONTENT_HIT, "agents")]) assert seen[0]["headers"]["Authorization"] == f"Bearer {DUMMY_KEY}" @@ -235,43 +321,40 @@ async def test_web_round_trip(): @pytest.mark.asyncio async def test_news_round_trip_reads_news_key_only(): - def respond(body): - return web.json_response({"success": True, "data": {"web": [WEB_HIT], "news": [NEWS_HIT]}}) - - out, _ = await search(respond, search_type="news") + out, _ = await search(respond_json({"success": True, "data": {"web": [WEB_HIT], "news": [NEWS_HIT]}}), search_type="news") assert [r.url for r in out.results] == [NEWS_HIT["url"]] @pytest.mark.asyncio async def test_missing_source_key_is_an_empty_search(): """The API omits the source key when there are no results; that is a real empty result.""" - out, _ = await search(lambda body: web.json_response({"success": True, "data": {}})) + out, _ = await search(respond_json({"success": True, "data": {}})) assert out == FirecrawlSearchToolOutputSchema(results=[], failures=[]) @pytest.mark.asyncio async def test_results_trimmed_to_max_results(): hits = [{"url": f"https://example.com/{i}"} for i in range(5)] - out, _ = await search(lambda body: ok("web", hits), max_results_per_query=2) + out, _ = await search(respond_hits("web", hits), max_results_per_query=2) assert [r.url for r in out.results] == ["https://example.com/0", "https://example.com/1"] @pytest.mark.asyncio async def test_invalid_hits_skipped_valid_ones_kept(): - hits = [1, "s", None, {"title": "x"}, {"url": ""}, WEB_HIT, {"url": "https://example.com/b", "position": "N/A"}] - out, _ = await search(lambda body: ok("web", hits)) + hits = [1, "s", None, {"title": "x"}, {"url": "https://@/a"}, WEB_HIT, {"url": "https://example.com/b", "position": "N/A"}] + out, _ = await search(respond_hits("web", hits)) assert [(r.url, r.position) for r in out.results] == [("https://www.python.org/", 1), ("https://example.com/b", None)] @pytest.mark.asyncio async def test_all_hits_invalid_raises(): - error = await search_error(lambda body: ok("web", [1, None, {"title": "x"}, {"url": ""}])) + error = await search_error(respond_hits("web", [1, None, {"title": "x"}, {"url": "https://:443/a"}])) assert str(error) == "Firecrawl search failed for 'q': 200 none of the 4 'web' results had a usable url" @pytest.mark.asyncio async def test_duplicate_queries_sent_once(): - out, seen = await search(lambda body: ok("web", [WEB_HIT]), queries=["q", "q"]) + out, seen = await search(respond_hits("web", [WEB_HIT]), queries=["q", "q"]) assert len(seen) == 1 assert [r.query for r in out.results] == ["q"] @@ -291,30 +374,27 @@ async def test_duplicate_queries_sent_once(): ], ) async def test_malformed_success_response_raises(payload): - error = await search_error(lambda body: web.json_response(payload)) + error = await search_error(respond_json(payload)) assert str(error) == "Firecrawl search failed for 'q': 200 unexpected response shape for 'web' results" @pytest.mark.asyncio @pytest.mark.parametrize("payload", [{"success": "false"}, {"success": "true"}, {"success": 1}, {}]) async def test_success_must_be_literally_true(payload): - error = await search_error(lambda body: web.json_response({**payload, "data": {"web": [WEB_HIT]}})) + error = await search_error(respond_json({**payload, "data": {"web": [WEB_HIT]}})) assert str(error) == "Firecrawl search failed for 'q': 200 response did not report success" @pytest.mark.asyncio @pytest.mark.parametrize("status, reason", [(201, "Created"), (500, "Internal Server Error")]) async def test_non_200_with_success_true_raises(status, reason): - def respond(body): - return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}, status=status) - - error = await search_error(respond) + error = await search_error(respond_json({"success": True, "data": {"web": [WEB_HIT]}}, status=status)) assert str(error) == f"Firecrawl search failed for 'q': {status} {reason}" @pytest.mark.asyncio async def test_provider_error_message_is_reported(): - error = await search_error(lambda body: web.json_response({"success": False, "error": "Payment required"}, status=402)) + error = await search_error(respond_json({"success": False, "error": "Payment required"}, status=402)) assert str(error) == "Firecrawl search failed for 'q': 402 Payment required" @@ -322,21 +402,21 @@ async def test_provider_error_message_is_reported(): @pytest.mark.parametrize( "response, expected", [ - (lambda: web.Response(status=502, text="Bad Gateway", content_type="text/html"), "502 Bad Gateway"), - (lambda: web.Response(status=200, text=""), "200 response did not report success"), - (lambda: web.Response(status=204), "204 No Content"), + ({"status": 502, "text": "Bad Gateway", "content_type": "text/html"}, "502 Bad Gateway"), + ({"status": 200, "text": ""}, "200 response did not report success"), + ({"status": 204}, "204 No Content"), ], ) async def test_non_json_bodies_raise_with_status(response, expected): - error = await search_error(lambda body: response()) + error = await search_error(respond_raw(**response)) assert str(error) == f"Firecrawl search failed for 'q': {expected}" @pytest.mark.asyncio async def test_redirect_is_not_followed(): - """A redirect, even to another host, is reported as an error; the query is never re-sent elsewhere.""" - async with fake_firecrawl(lambda body: ok("web", [WEB_HIT])) as (other_url, other_seen): - error = await search_error(lambda body: web.Response(status=307, headers={"Location": f"{other_url}/search"})) + """A redirect, even to another server, is reported as an error; the query is never re-sent elsewhere.""" + async with fake_firecrawl(respond_hits("web", [WEB_HIT])) as (other_url, other_seen): + error = await search_error(respond_raw(status=307, headers={"Location": f"{other_url}/search"})) assert str(error) == "Firecrawl search failed for 'q': 307 Temporary Redirect" assert other_seen == [] @@ -346,19 +426,15 @@ async def test_redirect_is_not_followed(): # Batch semantics # ################### @pytest.mark.asyncio -async def test_partial_failure_reports_failed_queries(caplog): - def respond(body): - if body["query"] == "bad": - return web.json_response({"success": False, "error": "Rate limit exceeded"}, status=429) - return ok("web", [WEB_HIT]) - +async def test_partial_failure_reports_failures(caplog): + rate_limited = respond_json({"success": False, "error": "Rate limit exceeded"}, status=429) with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): - out, _ = await search(respond, queries=["bad", "good"]) + out, _ = await search(respond_per_query("bad", rate_limited, respond_hits("web", [WEB_HIT])), queries=["bad", "good"]) reason = "Firecrawl search failed for 'bad': 429 Rate limit exceeded" assert [r.query for r in out.results] == ["good"] assert out.failures == [FirecrawlSearchFailure(query="bad", error=reason)] - assert f"Firecrawl query 'bad' failed: {reason}" in caplog.text + assert caplog.messages == [reason] @pytest.mark.asyncio @@ -371,20 +447,14 @@ def respond(body): ) async def test_empty_plus_failed_query_raises(bad, expected): """No results anywhere plus a failure must not look like an empty search.""" - - def respond(body): - return web.json_response({"success": True, "data": {}} if body["query"] == "empty" else bad) - + respond = respond_per_query("bad", respond_json(bad), respond_json({"success": True, "data": {}})) error = await search_error(respond, queries=["empty", "bad"]) assert str(error) == f"Firecrawl search failed for 'bad': {expected}" @pytest.mark.asyncio async def test_every_query_failing_raises_first_error(): - def respond(body): - return web.json_response({"success": False, "error": "Unauthorized"}, status=401) - - error = await search_error(respond, queries=["a", "b"]) + error = await search_error(respond_json({"success": False, "error": "Unauthorized"}, status=401), queries=["a", "b"]) assert str(error) == "Firecrawl search failed for 'a': 401 Unauthorized" @@ -396,21 +466,20 @@ async def test_connection_error_is_wrapped_without_context(): with socket.socket() as sock: sock.bind(("127.0.0.1", 0)) port = sock.getsockname()[1] - tool = http_tool(f"http://127.0.0.1:{port}/v2") with pytest.raises(Exception) as raised: - await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + await http_tool(f"http://127.0.0.1:{port}/v2").run_async(FirecrawlSearchToolInputSchema(queries=["q"])) assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ClientConnectorError") assert raised.value.__context__ is None and raised.value.__cause__ is None - assert DUMMY_KEY not in leak_surfaces(raised.value) + assert_no_key(error_surfaces(raised.value)) @pytest.mark.asyncio async def test_timeout_is_wrapped_with_query(): - async def slow(body): + async def slow(body: Body) -> web.Response: await asyncio.sleep(1) - return ok("web", [WEB_HIT]) + return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}) async with fake_firecrawl(slow) as (base_url, _): tool = http_tool(base_url) @@ -424,24 +493,22 @@ async def slow(body): @pytest.mark.asyncio @pytest.mark.parametrize( - "base_url", + "base_url, error_type", [ - "http://exa\nmple.com/v2", - f"http://{'a' * 70}.example.com/v2", - f"{DUMMY_KEY}/v2", # aiohttp's InvalidUrlClientError echoes the URL, so its text must be redacted too + (f"{DUMMY_KEY}/v2", "InvalidUrlClientError"), # the error echoes the URL, so its text must be redacted + ("http://[::1/v2", "InvalidUrlClientError"), + ("http://127.0.0.1:99999/v2", "InvalidUrlClientError"), + ("http://user:pw@127.0.0.1:1/v2", "ValueError"), ], ) -async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url): - """aiohttp raises ValueError/UnicodeError for such URLs from a frame whose locals hold the request headers.""" +async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url, error_type): + """aiohttp raises for such URLs from frames whose locals hold the request headers; none of that may surface.""" with pytest.raises(Exception) as raised: await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) - assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ") + assert str(raised.value).startswith(f"Firecrawl search failed for 'q': request failed: {error_type} ") assert raised.value.__context__ is None - stack = traceback.TracebackException.from_exception(raised.value, capture_locals=True).stack - non_test_frames = [repr(frame.locals) for frame in stack if frame.filename != __file__] - assert non_test_frames - assert DUMMY_KEY not in str(raised.value) + repr(raised.value) + "".join(non_test_frames) + assert_no_key(str(raised.value), repr(raised.value), *frame_locals(raised.value, is_outside_this_test_file)) ################ @@ -449,32 +516,67 @@ async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url): ################ @pytest.mark.asyncio @pytest.mark.parametrize( - "echo", - [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, OTHER_KEY, OTHER_KEY.upper()], + "echo", [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, CANONICAL_KEY, CANONICAL_KEY.upper()] ) async def test_echoed_key_kept_out_of_errors_and_logs(caplog, echo): - """Even when the provider echoes a key back, it never reaches raised errors, tracebacks or logs.""" - - def respond(body): - if body["query"] == "bad": - return web.json_response({"success": False, "error": echo}, status=401) - return ok("web", [WEB_HIT]) - + """Even when the provider echoes a key back, it never reaches raised errors, tracebacks, logs or failures.""" + respond = respond_per_query( + "bad", respond_json({"success": False, "error": echo}, status=401), respond_hits("web", [WEB_HIT]) + ) with caplog.at_level(logging.DEBUG): out, _ = await search(respond, queries=["bad", "good"]) error = await search_error(respond, queries=["bad"]) - assert [failure.query for failure in out.failures] == ["bad"] - assert "[redacted]" in str(error) and "[redacted]" in caplog.text and "[redacted]" in out.failures[0].error - for text in (caplog.text, leak_surfaces(error), repr(out)): - assert all(key not in text for key in (DUMMY_KEY, DUMMY_KEY.upper(), OTHER_KEY, OTHER_KEY.upper())) + assert out.failures == [FirecrawlSearchFailure(query="bad", error=str(error))] + assert "[redacted]" in str(error) + assert_no_key(caplog.text, error_surfaces(error), repr(out)) + + +def malformed_body_echoing(key: str) -> Body: + return {"success": True, "data": None, "error": {"token": key}} + + +def invalid_hits_echoing(key: str) -> Body: + return {"success": True, "data": {"web": [{"url": "https://@/a", "title": key}, {"description": key}]}} + + +@pytest.mark.asyncio +@pytest.mark.parametrize("api_key", [DUMMY_KEY, CANONICAL_KEY]) +@pytest.mark.parametrize("build_body", [malformed_body_echoing, invalid_hits_echoing]) +async def test_rejected_success_body_not_kept_in_traceback_locals(api_key, build_body): + """A malformed or all-invalid success body is dropped before the raise, so captured locals cannot show a key.""" + error = await search_error(respond_json(build_body(api_key)), api_key=api_key) + assert str(error).startswith("Firecrawl search failed for 'q': 200 ") + assert_no_key(error_surfaces(error)) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("api_key", [DUMMY_KEY, CANONICAL_KEY]) +async def test_key_in_query_text_is_redacted_in_diagnostics(caplog, api_key): + """A query that contains the key (for example pasted by mistake) must not leak it through errors or logs.""" + query = f"why does {api_key} fail" + respond = respond_per_query( + query, respond_json({"success": False, "error": "Unauthorized"}, status=401), respond_hits("web", [WEB_HIT]) + ) + with caplog.at_level(logging.DEBUG): + out, _ = await search(respond, queries=[query, "good"], api_key=api_key) + error = await search_error(respond, queries=[query], api_key=api_key) + + assert out.failures == [FirecrawlSearchFailure(query="why does [redacted] fail", error=str(error))] + assert str(error) == "Firecrawl search failed for 'why does [redacted] fail': 401 Unauthorized" + assert_no_key(caplog.text, repr(out.failures), str(error), repr(error)) def test_key_kept_out_of_tool_and_config_state(): tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY)) - surfaces = [repr(tool), repr(tool.config), str(tool.config), tool.config.model_dump_json(), repr(vars(tool))] - surfaces.append(repr(vars(copy.copy(tool)))) - assert all(DUMMY_KEY not in text for text in surfaces) + assert_no_key( + repr(tool), + repr(tool.config), + str(tool.config), + tool.config.model_dump_json(), + repr(vars(tool)), + repr(vars(copy.copy(tool))), + ) assert tool._api_key.get_secret_value() == DUMMY_KEY diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index e0a98b00..16bea80e 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -3,7 +3,7 @@ import os import re from concurrent.futures import ThreadPoolExecutor -from typing import Any, List, Literal, Optional, Tuple +from typing import Dict, List, Literal, Optional, Tuple, Union, cast import aiohttp from pydantic import Field, SecretStr, field_validator @@ -56,7 +56,7 @@ class FirecrawlSearchResultItem(BaseIOSchema): @field_validator("position", mode="before") @classmethod - def _positive_rank_or_none(cls, value: Any) -> Optional[int]: + def _positive_rank_or_none(cls, value: object) -> Optional[int]: # Ranks come from the provider; anything other than a positive int (bool, float, "N/A") is dropped. return value if type(value) is int and value > 0 else None @@ -106,13 +106,15 @@ class FirecrawlSearchToolConfig(BaseToolConfig): class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSearchToolOutputSchema]): """Tool for searching the web and news via the Firecrawl Search API.""" - TIME_RANGES = {"day": "qdr:d", "week": "qdr:w", "month": "qdr:m", "year": "qdr:y"} + TIME_RANGES: Dict[str, str] = {"day": "qdr:d", "week": "qdr:w", "month": "qdr:m", "year": "qdr:y"} # Anything in the Firecrawl key format is redacted from error and log text, plus the configured key itself # when it is long enough that replacing it cannot mangle ordinary words. KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 - # A result is only kept when its URL is http(s) with a host. - RESULT_URL_PATTERN = re.compile(r"https?://[^\s/?#]+", re.IGNORECASE) + # A result is kept only when its whole URL is http(s) with a host and contains no whitespace. + RESULT_URL_PATTERN = re.compile( + r"https?://(?:[^\s/?#@]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::\d*)?(?:[/?#]\S*)?", re.IGNORECASE + ) def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) @@ -124,29 +126,30 @@ def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig self.timeout = config.timeout @staticmethod - def _first_text(*values: Any) -> Optional[str]: + def _first_text(*values: object) -> Optional[str]: """Return the first non-empty string; result fields are provider data and may have any type.""" return next((value for value in values if isinstance(value, str) and value), None) @classmethod - def _to_item(cls, hit: Any, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: - """Normalise one result, or return None when it has no http(s) URL.""" - url = hit.get("url") if isinstance(hit, dict) else None - if not cls.RESULT_URL_PATTERN.match(str(url)): + def _to_item(cls, hit: object, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: + """Normalise one result, or return None when it has no usable http(s) URL.""" + fields: Dict[str, object] = hit if isinstance(hit, dict) else {} + url = str(fields.get("url")) + if not cls.RESULT_URL_PATTERN.fullmatch(url): return None - content = cls._first_text(hit.get("markdown")) + content = cls._first_text(fields.get("markdown")) return FirecrawlSearchResultItem( query=query, - title=cls._first_text(hit.get("title"), url), + title=cls._first_text(fields.get("title"), url), url=url, - description=cls._first_text(hit.get("description"), hit.get("snippet")), - position=hit.get("position"), - published=cls._first_text(hit.get("date")), - content=content and content[:max_content_chars], + description=cls._first_text(fields.get("description"), fields.get("snippet")), + position=fields.get("position"), + published=cls._first_text(fields.get("date")), + content=content[:max_content_chars] if content else None, ) - def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> dict: - body = {"query": query, "limit": params.max_results_per_query, "sources": [params.search_type]} + def _build_body(self, query: str, params: FirecrawlSearchToolInputSchema) -> Dict[str, object]: + body: Dict[str, object] = {"query": query, "limit": params.max_results_per_query, "sources": [params.search_type]} if self.location: body["location"] = self.location if self.time_range and params.search_type == "web": @@ -161,28 +164,28 @@ def _redact(self, text: str) -> str: text = re.sub(re.escape(key), "[redacted]", text, flags=re.IGNORECASE) return self.KEY_PATTERN.sub("[redacted]", text) - @staticmethod - def _error(query: str, detail: str) -> Exception: - return Exception(f"Firecrawl search failed for '{query}': {detail}") + def _error(self, query: str, detail: str) -> Exception: + # Redacted after the query is interpolated, since the query text can contain a key too. + return Exception(self._redact(f"Firecrawl search failed for '{query}': {detail}")) @staticmethod - async def _read_json(resp: aiohttp.ClientResponse) -> Any: + async def _read_json(resp: aiohttp.ClientResponse) -> object: try: return await resp.json(content_type=None) except ValueError: return None - def _failure(self, status: int, reason: Optional[str], data: Any) -> Optional[str]: + def _failure(self, status: int, reason: Optional[str], data: object) -> Optional[str]: """Return why a response is not a successful search (only a 200 whose body says success is true is), or None when it is. Provider text is redacted here, where it enters the tool.""" - payload = data if isinstance(data, dict) else {} + payload: Dict[str, object] = data if isinstance(data, dict) else {} if status == 200 and payload.get("success") is True: return None return self._redact(f"{status} {payload.get('error') or reason}") async def _exchange( self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema - ) -> Tuple[Any, Optional[str]]: + ) -> Tuple[object, Optional[str]]: """Send one search request and return the parsed body plus why it failed (None on success). Transport errors, timeouts and invalid URLs are turned into a failure reason here instead of propagating, @@ -197,25 +200,40 @@ async def _exchange( except (aiohttp.ClientError, asyncio.TimeoutError, ValueError) as error: return None, self._redact(f"request failed: {type(error).__name__} {str(error) or f'after {self.timeout:g}s'}") - async def _post(self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema) -> dict: + async def _post( + self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema + ) -> Dict[str, object]: """Return the body of a successful search response, or raise. The raise happens in this frame, which holds neither the response nor an aiohttp error, and the body of a - failed response is dropped first, so a traceback that captures locals cannot show a key echoed back in an - error response.""" + failed response is dropped first, so a traceback that captures locals cannot show a key echoed back.""" payload, failure = await self._exchange(session, query, params) if failure: payload = None raise self._error(query, failure) - return payload + # _failure only returns None for a dict body that reports success. + return cast(Dict[str, object], payload) - def _extract_hits(self, query: str, payload: dict, search_type: str) -> list: - # The key for the requested source is absent when there are no results. + @staticmethod + def _extract_hits(payload: Dict[str, object], search_type: str) -> Optional[List[object]]: + """Return the result list for the requested source, or None when the response has an unexpected shape. + The API leaves the source key out when there are no results.""" groups = payload.get("data") hits = groups.get(search_type, []) if isinstance(groups, dict) else None - if not isinstance(hits, list): - raise self._error(query, f"200 unexpected response shape for '{search_type}' results") - return hits + return hits if isinstance(hits, list) else None + + def _parse( + self, payload: Dict[str, object], query: str, search_type: str + ) -> Tuple[List[FirecrawlSearchResultItem], Optional[str]]: + """Turn a successful response body into result items, or say why it cannot be used. It never raises, so + the rejected body is not left in the locals of a raising frame.""" + hits = self._extract_hits(payload, search_type) + if hits is None: + return [], f"200 unexpected response shape for '{search_type}' results" + items = list(filter(None, (self._to_item(hit, query, self.max_content_chars) for hit in hits))) + if hits and not items: + return [], f"200 none of the {len(hits)} '{search_type}' results had a usable url" + return items, None async def _fetch( self, @@ -223,24 +241,24 @@ async def _fetch( query: str, params: FirecrawlSearchToolInputSchema, ) -> List[FirecrawlSearchResultItem]: - payload = await self._post(session, query, params) - hits = self._extract_hits(query, payload, params.search_type) - items = list(filter(None, (self._to_item(hit, query, self.max_content_chars) for hit in hits))) - if hits and not items: - raise self._error(query, f"200 none of the {len(hits)} '{params.search_type}' results had a usable url") + items, problem = self._parse(await self._post(session, query, params), query, params.search_type) + if problem: + raise self._error(query, problem) return items[: params.max_results_per_query] - def _collect(self, queries: List[str], grouped: list) -> FirecrawlSearchToolOutputSchema: + def _collect( + self, queries: List[str], grouped: List[Union[List[FirecrawlSearchResultItem], BaseException]] + ) -> FirecrawlSearchToolOutputSchema: """Merge per-query results. Raise when no query produced results and at least one failed, so a bad key, exhausted credits or a malformed response never looks like an empty search.""" results: List[FirecrawlSearchResultItem] = [] failures: List[FirecrawlSearchFailure] = [] - errors: List[Exception] = [] + errors: List[BaseException] = [] for query, group in zip(queries, grouped): - if isinstance(group, Exception): + if isinstance(group, BaseException): errors.append(group) - failures.append(FirecrawlSearchFailure(query=query, error=str(group))) - logger.warning("Firecrawl query '%s' failed: %s", query, failures[-1].error) + failures.append(FirecrawlSearchFailure(query=self._redact(query), error=self._redact(str(group)))) + logger.warning("%s", failures[-1].error) continue results.extend(group) @@ -262,7 +280,9 @@ async def run_async(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSe headers={"Authorization": f"Bearer {self._api_key.get_secret_value()}", "Content-Type": "application/json"}, timeout=aiohttp.ClientTimeout(total=self.timeout), ) as session: - grouped = await asyncio.gather(*(self._fetch(session, q, params) for q in queries), return_exceptions=True) + grouped: List[Union[List[FirecrawlSearchResultItem], BaseException]] = await asyncio.gather( + *(self._fetch(session, q, params) for q in queries), return_exceptions=True + ) return self._collect(queries, grouped) From 07d9f8cea3c52a0c7cc865faab2c06e048204876 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 10:16:21 -0300 Subject: [PATCH 08/15] Validate result URL hosts and ports, wrap over-deep JSON, return body-or-reason from the request boundary, block non-loopback connects in tests --- atomic-forge/tools/firecrawl_search/README.md | 6 +- .../tests/test_firecrawl_search.py | 84 ++++++++++++++++--- .../firecrawl_search/tool/firecrawl_search.py | 62 ++++++++------ 3 files changed, 114 insertions(+), 38 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 5edd2e0e..c58eb9b9 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -15,7 +15,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec 2. Or copy the `tool/` folder directly into your project. ## Configuration -- `api_key` (`SecretStr`, a plain string is accepted): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. It is only sent in the `Authorization` header and is redacted from raised errors and log messages. +- `api_key` (`SecretStr`, a plain string is accepted): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. It is only sent in the `Authorization` header. Raised errors, log messages and `failures` redact the configured key (when it is at least 8 characters) and anything in Firecrawl's `fc-` key format. - `base_url` (str): API base URL (default `https://api.firecrawl.dev/v2`). - `location` (str, optional): location to localise results, e.g. `Germany`. - `time_range` (str, optional): `day`, `week`, `month`, or `year`. Only applies to web results. @@ -31,8 +31,8 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (one with a host and no whitespace) are skipped, and badly typed optional fields are left empty. -- `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). A key appearing in either is redacted. They are also logged as warnings. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a real host, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. +- `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). Keys are redacted from both as described for `api_key`. They are also logged as warnings. ## Usage If no query returns results and at least one failed (for example a wrong API key, no credits, or a malformed response), `run` raises the first error instead of returning an empty list. Redirects are not followed; they are reported as errors. diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 9b3098d8..f92463be 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -6,7 +6,7 @@ import sys import traceback from contextlib import asynccontextmanager -from typing import AsyncIterator, Awaitable, Callable, Dict, List, Sequence, Tuple, Union +from typing import AsyncIterator, Awaitable, Callable, Dict, Iterator, List, Sequence, Tuple, TypedDict, Union from unittest.mock import AsyncMock, patch import pytest @@ -28,6 +28,14 @@ Body = Dict[str, object] Responder = Callable[[Body], Union[web.Response, Awaitable[web.Response]]] + +class Seen(TypedDict): + """One request as the fake server received it.""" + + headers: Dict[str, str] + body: Body + + DUMMY_KEY = "fc-dummy0secret0key0123456789" # a configured key in a custom format CANONICAL_KEY = "fc-0123456789abcdef0123456789abcdef" # a key in Firecrawl's own format KEYS = (DUMMY_KEY, DUMMY_KEY.upper(), CANONICAL_KEY, CANONICAL_KEY.upper()) @@ -61,19 +69,28 @@ @pytest.fixture(autouse=True) -def offline(monkeypatch): - """Keep the suite offline: any attempt to resolve a host other than the local test server fails the test.""" +def offline(monkeypatch) -> Iterator[List[str]]: + """Keep the suite offline. Resolving a host other than loopback raises gaierror, and connecting to a + non-loopback address raises OSError; both are recorded, and the test fails unless it expected them.""" attempts: List[str] = [] real_getaddrinfo = socket.getaddrinfo + real_connect = socket.socket.connect - def loopback_only(host, *args, **kwargs): + def resolve_loopback_only(host, *args, **kwargs): if host not in LOOPBACK_HOSTS: attempts.append(str(host)) raise socket.gaierror("network access is disabled in these tests") return real_getaddrinfo(host, *args, **kwargs) - monkeypatch.setattr(socket, "getaddrinfo", loopback_only) - yield + def connect_loopback_only(sock, address): + if isinstance(address, tuple) and address[0] not in LOOPBACK_HOSTS: + attempts.append(str(address[0])) + raise OSError("network access is disabled in these tests") + return real_connect(sock, address) + + monkeypatch.setattr(socket, "getaddrinfo", resolve_loopback_only) + monkeypatch.setattr(socket.socket, "connect", connect_loopback_only) + yield attempts assert attempts == [] @@ -109,13 +126,13 @@ def respond(body: Body) -> Union[web.Response, Awaitable[web.Response]]: # Test harness # ################ @asynccontextmanager -async def fake_firecrawl(respond: Responder) -> AsyncIterator[Tuple[str, List[Body]]]: +async def fake_firecrawl(respond: Responder) -> AsyncIterator[Tuple[str, List[Seen]]]: """Serve POST /v2/search on a local aiohttp server; `respond(body)` builds each response.""" - seen: List[Body] = [] + seen: List[Seen] = [] async def handler(request: web.Request) -> web.Response: body = await request.json() - seen.append({"headers": dict(request.headers), "body": body}) + seen.append(Seen(headers=dict(request.headers), body=body)) response = respond(body) return await response if asyncio.iscoroutine(response) else response @@ -135,7 +152,7 @@ def http_tool(base_url: str, api_key: str = DUMMY_KEY, **config) -> FirecrawlSea async def search( respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY, **params -) -> Tuple[FirecrawlSearchToolOutputSchema, List[Body]]: +) -> Tuple[FirecrawlSearchToolOutputSchema, List[Seen]]: """Run one search against a fake server and return (output, requests seen).""" async with fake_firecrawl(respond) as (base_url, seen): tool = http_tool(base_url, api_key) @@ -229,6 +246,10 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): "https://example.com:8443/a?b=c#d", "http://[::1]:8080/x", "https://user@example.com/", + "http://192.0.2.1:8080/x", + "https://müller.de/straße", + "https://xn--mller-kva.de/", + "https://example.com/%C3%BC%20x.pdf", ], ) def test_to_item_keeps_valid_http_urls(url): @@ -259,6 +280,22 @@ def test_to_item_keeps_valid_http_urls(url): {"url": "https://exa mple.com/a"}, {"url": "https://example.com/a\n"}, {"url": "https://example.com\t/a"}, + {"url": "https://./"}, + {"url": "https://-/"}, + {"url": "https://%/"}, + {"url": "https://_x.com/"}, + {"url": "https://[:]/"}, + {"url": "https://[zz]/"}, + {"url": "https://a\x00b.com/"}, + {"url": "https://example.com/a\x7f"}, + {"url": "https://exa\u200bmple.com/"}, + {"url": "https://example.com/\xa0"}, + {"url": "https://example.com:abc/"}, + {"url": "https://example.com:99999/"}, + {"url": "https://example.com:65536/"}, + {"url": "https://example.com:0/"}, + {"url": "https://example.com:\u0664\u0664\u0663/"}, + {"url": "https://a b@example.com/"}, ], ) def test_to_item_without_usable_url_is_none(hit): @@ -412,6 +449,14 @@ async def test_non_json_bodies_raise_with_status(response, expected): assert str(error) == f"Firecrawl search failed for 'q': {expected}" +@pytest.mark.asyncio +async def test_deeply_nested_json_is_a_wrapped_failure(): + """JSON too deep for the parser must fail like any other bad body, not escape with the response in scope.""" + error = await search_error(respond_raw(status=200, text="[" * 200_000, content_type="application/json")) + assert str(error) == "Firecrawl search failed for 'q': 200 response did not report success" + assert_no_key(error_surfaces(error)) + + @pytest.mark.asyncio async def test_redirect_is_not_followed(): """A redirect, even to another server, is reported as an error; the query is never re-sent elsewhere.""" @@ -511,6 +556,25 @@ async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url, assert_no_key(str(raised.value), repr(raised.value), *frame_locals(raised.value, is_outside_this_test_file)) +@pytest.mark.asyncio +@pytest.mark.parametrize( + "base_url, blocked_host", + [ + ("http://exa\nmple.com/v2", "example.com"), # yarl drops the newline, so this resolves example.com + (f"http://{'a' * 70}.example.com/v2", f"{'a' * 70}.example.com"), + ], +) +async def test_unresolvable_base_url_is_wrapped(offline, base_url, blocked_host): + """A DNS failure is reported as a request failure; the offline guard answers the lookup, so nothing leaves.""" + with pytest.raises(Exception) as raised: + await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + + assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ClientConnectorDNSError ") + assert_no_key(str(raised.value), repr(raised.value), *frame_locals(raised.value, is_outside_this_test_file)) + assert offline == [blocked_host] + offline.clear() + + ################ # Key handling # ################ diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 16bea80e..32da97ee 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -3,7 +3,8 @@ import os import re from concurrent.futures import ThreadPoolExecutor -from typing import Dict, List, Literal, Optional, Tuple, Union, cast +from typing import Dict, List, Literal, Optional, Tuple, Union +from urllib.parse import urlsplit import aiohttp from pydantic import Field, SecretStr, field_validator @@ -111,9 +112,10 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear # when it is long enough that replacing it cannot mangle ordinary words. KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 - # A result is kept only when its whole URL is http(s) with a host and contains no whitespace. + # Shape of a usable result URL: http(s), optional userinfo, a host that starts with a letter or digit (or an + # IPv6 literal), an ASCII port, and no whitespace anywhere. urlsplit then validates the port range and IPv6. RESULT_URL_PATTERN = re.compile( - r"https?://(?:[^\s/?#@]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::\d*)?(?:[/?#]\S*)?", re.IGNORECASE + r"https?://(?:[^\s/?#@]*@)?(?:[^\W_][^\s/?#@:\[\]]*|\[[0-9a-f:.]+\])(?::[0-9]*)?(?:[/?#]\S*)?", re.IGNORECASE ) def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): @@ -130,20 +132,31 @@ def _first_text(*values: object) -> Optional[str]: """Return the first non-empty string; result fields are provider data and may have any type.""" return next((value for value in values if isinstance(value, str) and value), None) + @classmethod + def _usable_url(cls, url: str) -> bool: + """True for a printable http(s) URL with a real host and an in-range, non-zero port.""" + if not (url.isprintable() and cls.RESULT_URL_PATTERN.fullmatch(url)): + return False + try: + # urlsplit raises ValueError for an out-of-range port or an invalid IPv6 literal. + return urlsplit(url).port != 0 + except ValueError: + return False + @classmethod def _to_item(cls, hit: object, query: str, max_content_chars: Optional[int] = None) -> Optional[FirecrawlSearchResultItem]: """Normalise one result, or return None when it has no usable http(s) URL.""" fields: Dict[str, object] = hit if isinstance(hit, dict) else {} url = str(fields.get("url")) - if not cls.RESULT_URL_PATTERN.fullmatch(url): + if not cls._usable_url(url): return None content = cls._first_text(fields.get("markdown")) return FirecrawlSearchResultItem( query=query, - title=cls._first_text(fields.get("title"), url), + title=cls._first_text(fields.get("title")) or url, url=url, description=cls._first_text(fields.get("description"), fields.get("snippet")), - position=fields.get("position"), + position=FirecrawlSearchResultItem._positive_rank_or_none(fields.get("position")), published=cls._first_text(fields.get("date")), content=content[:max_content_chars] if content else None, ) @@ -172,21 +185,22 @@ def _error(self, query: str, detail: str) -> Exception: async def _read_json(resp: aiohttp.ClientResponse) -> object: try: return await resp.json(content_type=None) - except ValueError: + except (ValueError, RecursionError): + # Invalid JSON, or JSON nested too deeply for the parser. return None - def _failure(self, status: int, reason: Optional[str], data: object) -> Optional[str]: - """Return why a response is not a successful search (only a 200 whose body says success is true is), - or None when it is. Provider text is redacted here, where it enters the tool.""" + def _check_response(self, status: int, reason: Optional[str], data: object) -> Union[Dict[str, object], str]: + """Return the body of a successful search (a 200 whose body says success is true), or why the response + is not one. Provider text is redacted here, where it enters the tool.""" payload: Dict[str, object] = data if isinstance(data, dict) else {} if status == 200 and payload.get("success") is True: - return None + return payload return self._redact(f"{status} {payload.get('error') or reason}") async def _exchange( self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema - ) -> Tuple[object, Optional[str]]: - """Send one search request and return the parsed body plus why it failed (None on success). + ) -> Union[Dict[str, object], str]: + """Send one search request and return the body of a successful response, or why it failed. Transport errors, timeouts and invalid URLs are turned into a failure reason here instead of propagating, because aiohttp's own frames and errors hold the request headers, Authorization included.""" @@ -196,23 +210,21 @@ async def _exchange( ) as resp: payload = await self._read_json(resp) reason = resp.reason if resp.status != 200 else "response did not report success" - return payload, self._failure(resp.status, reason, payload) + return self._check_response(resp.status, reason, payload) except (aiohttp.ClientError, asyncio.TimeoutError, ValueError) as error: - return None, self._redact(f"request failed: {type(error).__name__} {str(error) or f'after {self.timeout:g}s'}") + return self._redact(f"request failed: {type(error).__name__} {str(error) or f'after {self.timeout:g}s'}") async def _post( self, session: aiohttp.ClientSession, query: str, params: FirecrawlSearchToolInputSchema ) -> Dict[str, object]: """Return the body of a successful search response, or raise. - The raise happens in this frame, which holds neither the response nor an aiohttp error, and the body of a - failed response is dropped first, so a traceback that captures locals cannot show a key echoed back.""" - payload, failure = await self._exchange(session, query, params) - if failure: - payload = None - raise self._error(query, failure) - # _failure only returns None for a dict body that reports success. - return cast(Dict[str, object], payload) + The raise happens in this frame, which only ever holds the redacted failure reason: no response, no aiohttp + error and no failed body, so a traceback that captures locals cannot show a key echoed back.""" + outcome = await self._exchange(session, query, params) + if isinstance(outcome, str): + raise self._error(query, outcome) + return outcome @staticmethod def _extract_hits(payload: Dict[str, object], search_type: str) -> Optional[List[object]]: @@ -300,9 +312,9 @@ def run(self, params: FirecrawlSearchToolInputSchema) -> FirecrawlSearchToolOutp load_dotenv() console = Console() - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=os.getenv("FIRECRAWL_API_KEY", ""))) + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr(os.getenv("FIRECRAWL_API_KEY", "")))) - examples = [ + examples: List[Tuple[Literal["web", "news"], bool, str]] = [ ("web", False, "atomic agents framework"), ("news", False, "AI agents"), ("web", True, "atomic agents framework"), From ab23d310cbc920170bfec68f8dcaf8a2fdc9e67e Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 10:21:44 -0300 Subject: [PATCH 09/15] Reject backslashes before the path in result URLs; cover IPv6 literals; guard connect_ex in tests --- .../tests/test_firecrawl_search.py | 18 +++++++++++++++++- .../firecrawl_search/tool/firecrawl_search.py | 5 +++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index f92463be..cd9070ae 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -1,5 +1,6 @@ import asyncio import copy +import errno import logging import os import socket @@ -75,6 +76,7 @@ def offline(monkeypatch) -> Iterator[List[str]]: attempts: List[str] = [] real_getaddrinfo = socket.getaddrinfo real_connect = socket.socket.connect + real_connect_ex = socket.socket.connect_ex def resolve_loopback_only(host, *args, **kwargs): if host not in LOOPBACK_HOSTS: @@ -82,14 +84,23 @@ def resolve_loopback_only(host, *args, **kwargs): raise socket.gaierror("network access is disabled in these tests") return real_getaddrinfo(host, *args, **kwargs) - def connect_loopback_only(sock, address): + def blocked(address) -> bool: if isinstance(address, tuple) and address[0] not in LOOPBACK_HOSTS: attempts.append(str(address[0])) + return True + return False + + def connect_loopback_only(sock, address): + if blocked(address): raise OSError("network access is disabled in these tests") return real_connect(sock, address) + def connect_ex_loopback_only(sock, address): + return errno.ECONNREFUSED if blocked(address) else real_connect_ex(sock, address) + monkeypatch.setattr(socket, "getaddrinfo", resolve_loopback_only) monkeypatch.setattr(socket.socket, "connect", connect_loopback_only) + monkeypatch.setattr(socket.socket, "connect_ex", connect_ex_loopback_only) yield attempts assert attempts == [] @@ -250,6 +261,7 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): "https://müller.de/straße", "https://xn--mller-kva.de/", "https://example.com/%C3%BC%20x.pdf", + "https://example.com/a\\b", ], ) def test_to_item_keeps_valid_http_urls(url): @@ -296,6 +308,10 @@ def test_to_item_keeps_valid_http_urls(url): {"url": "https://example.com:0/"}, {"url": "https://example.com:\u0664\u0664\u0663/"}, {"url": "https://a b@example.com/"}, + {"url": "https://[v1.x]/"}, + {"url": "https://[::1%25eth0]/"}, + {"url": "https://a.com\\@evil.com/"}, + {"url": "https://evil.com\\.example.com/"}, ], ) def test_to_item_without_usable_url_is_none(hit): diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 32da97ee..0f9d22f5 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -113,9 +113,10 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 # Shape of a usable result URL: http(s), optional userinfo, a host that starts with a letter or digit (or an - # IPv6 literal), an ASCII port, and no whitespace anywhere. urlsplit then validates the port range and IPv6. + # IPv6 literal), an ASCII port, no whitespace anywhere, and no backslash before the path (parsers disagree on + # what host "a.com\@b.com" means). urlsplit then validates the port range and the IPv6 literal. RESULT_URL_PATTERN = re.compile( - r"https?://(?:[^\s/?#@]*@)?(?:[^\W_][^\s/?#@:\[\]]*|\[[0-9a-f:.]+\])(?::[0-9]*)?(?:[/?#]\S*)?", re.IGNORECASE + r"https?://(?:[^\s/?#@\\]*@)?(?:[^\W_][^\s/?#@:\[\]\\]*|\[[0-9a-f:.]+\])(?::[0-9]*)?(?:[/?#]\S*)?", re.IGNORECASE ) def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): From 22a6bcf9699664f2b5bd1439a099153c5c417762 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:01:55 -0300 Subject: [PATCH 10/15] Validate result host names; state the traceback contract for caller inputs; type the tests --- atomic-forge/tools/firecrawl_search/README.md | 4 +- .../tests/test_firecrawl_search.py | 269 ++++++++++++------ .../firecrawl_search/tool/firecrawl_search.py | 35 ++- 3 files changed, 201 insertions(+), 107 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index c58eb9b9..11d8710e 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -15,7 +15,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec 2. Or copy the `tool/` folder directly into your project. ## Configuration -- `api_key` (`SecretStr`, a plain string is accepted): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. It is only sent in the `Authorization` header. Raised errors, log messages and `failures` redact the configured key (when it is at least 8 characters) and anything in Firecrawl's `fc-` key format. +- `api_key` (`SecretStr`, a plain string is accepted): Firecrawl API key. Falls back to the `FIRECRAWL_API_KEY` environment variable when empty. It is only sent in the `Authorization` header. Raised errors, log messages and `failures` redact the configured key (when it is at least 8 characters) and anything in Firecrawl's `fc-` key format. In tracebacks that capture local variables, the key does not appear in the tool's own state or in anything derived from a response or a transport error; the inputs you pass in, such as a query, appear there as given, the same as in your own code's frames. - `base_url` (str): API base URL (default `https://api.firecrawl.dev/v2`). - `location` (str, optional): location to localise results, e.g. `Germany`. - `time_range` (str, optional): `day`, `week`, `month`, or `year`. Only applies to web results. @@ -31,7 +31,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a real host, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name or IPv6 address, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. - `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). Keys are redacted from both as described for `api_key`. They are also logged as warnings. ## Usage diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index cd9070ae..6314df23 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -7,13 +7,26 @@ import sys import traceback from contextlib import asynccontextmanager -from typing import AsyncIterator, Awaitable, Callable, Dict, Iterator, List, Sequence, Tuple, TypedDict, Union +from typing import ( + AsyncIterator, + Awaitable, + Callable, + Dict, + Iterator, + List, + Literal, + Optional, + Sequence, + Tuple, + TypedDict, + Union, +) from unittest.mock import AsyncMock, patch import pytest from aiohttp import web from aiohttp.test_utils import TestServer -from pydantic import ValidationError +from pydantic import SecretStr, ValidationError sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -30,6 +43,16 @@ Responder = Callable[[Body], Union[web.Response, Awaitable[web.Response]]] +Address = Union[Tuple[object, ...], str, bytes] +AddrInfo = Tuple[ + socket.AddressFamily, + socket.SocketKind, + int, + str, + Union[Tuple[str, int], Tuple[str, int, int, int], Tuple[int, bytes]], +] + + class Seen(TypedDict): """One request as the fake server received it.""" @@ -42,6 +65,9 @@ class Seen(TypedDict): KEYS = (DUMMY_KEY, DUMMY_KEY.upper(), CANONICAL_KEY, CANONICAL_KEY.upper()) TOOL_FILE = os.path.join("tool", "firecrawl_search.py") LOOPBACK_HOSTS = {"127.0.0.1", "::1", "localhost"} +# Inputs the caller passes in. They are in the tool's frames as given (as they are in the caller's own frames), +# so the traceback-locals checks for a key that came in through the query skip these names. +CALLER_INPUT_LOCALS = {"query", "queries", "params"} WEB_HIT = { "url": "https://www.python.org/", @@ -70,7 +96,7 @@ class Seen(TypedDict): @pytest.fixture(autouse=True) -def offline(monkeypatch) -> Iterator[List[str]]: +def offline(monkeypatch: pytest.MonkeyPatch) -> Iterator[List[str]]: """Keep the suite offline. Resolving a host other than loopback raises gaierror, and connecting to a non-loopback address raises OSError; both are recorded, and the test fails unless it expected them.""" attempts: List[str] = [] @@ -78,24 +104,26 @@ def offline(monkeypatch) -> Iterator[List[str]]: real_connect = socket.socket.connect real_connect_ex = socket.socket.connect_ex - def resolve_loopback_only(host, *args, **kwargs): + def resolve_loopback_only( + host: Union[bytes, str, None], port: Union[bytes, str, int, None], *args: int, **kwargs: int + ) -> Sequence[AddrInfo]: if host not in LOOPBACK_HOSTS: attempts.append(str(host)) raise socket.gaierror("network access is disabled in these tests") - return real_getaddrinfo(host, *args, **kwargs) + return real_getaddrinfo(host, port, *args, **kwargs) - def blocked(address) -> bool: + def blocked(address: Address) -> bool: if isinstance(address, tuple) and address[0] not in LOOPBACK_HOSTS: attempts.append(str(address[0])) return True return False - def connect_loopback_only(sock, address): + def connect_loopback_only(sock: socket.socket, address: Address) -> None: if blocked(address): raise OSError("network access is disabled in these tests") - return real_connect(sock, address) + real_connect(sock, address) - def connect_ex_loopback_only(sock, address): + def connect_ex_loopback_only(sock: socket.socket, address: Address) -> int: return errno.ECONNREFUSED if blocked(address) else real_connect_ex(sock, address) monkeypatch.setattr(socket, "getaddrinfo", resolve_loopback_only) @@ -119,9 +147,12 @@ def respond_hits(source: str, hits: Sequence[object]) -> Responder: return respond_json({"success": True, "data": {source: list(hits)}}) -def respond_raw(**response) -> Responder: +def respond_raw( + status: int, text: Optional[str] = None, content_type: Optional[str] = None, location: Optional[str] = None +) -> Responder: def respond(body: Body) -> web.Response: - return web.Response(**response) + headers = {"Location": location} if location else None + return web.Response(status=status, text=text, content_type=content_type, headers=headers) return respond @@ -145,7 +176,9 @@ async def handler(request: web.Request) -> web.Response: body = await request.json() seen.append(Seen(headers=dict(request.headers), body=body)) response = respond(body) - return await response if asyncio.iscoroutine(response) else response + if isinstance(response, web.Response): + return response + return await response app = web.Application() app.router.add_post("/v2/search", handler) @@ -157,29 +190,60 @@ async def handler(request: web.Request) -> web.Response: await server.close() -def http_tool(base_url: str, api_key: str = DUMMY_KEY, **config) -> FirecrawlSearchTool: - return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=api_key, base_url=base_url, **config)) +def http_tool(base_url: str, api_key: str = DUMMY_KEY) -> FirecrawlSearchTool: + return FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr(api_key), base_url=base_url)) + + +def search_input( + queries: Sequence[str], + search_type: Literal["web", "news"] = "web", + max_results_per_query: int = 5, + include_content: bool = False, +) -> FirecrawlSearchToolInputSchema: + return FirecrawlSearchToolInputSchema( + queries=list(queries), + search_type=search_type, + max_results_per_query=max_results_per_query, + include_content=include_content, + ) async def search( - respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY, **params + respond: Responder, + queries: Sequence[str] = ("q",), + api_key: str = DUMMY_KEY, + search_type: Literal["web", "news"] = "web", + max_results_per_query: int = 5, + include_content: bool = False, ) -> Tuple[FirecrawlSearchToolOutputSchema, List[Seen]]: """Run one search against a fake server and return (output, requests seen).""" + params = search_input(queries, search_type, max_results_per_query, include_content) async with fake_firecrawl(respond) as (base_url, seen): - tool = http_tool(base_url, api_key) - out = await tool.run_async(FirecrawlSearchToolInputSchema(queries=list(queries), **params)) + out = await http_tool(base_url, api_key).run_async(params) return out, seen -async def search_error(respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY, **params) -> Exception: +async def search_error(respond: Responder, queries: Sequence[str] = ("q",), api_key: str = DUMMY_KEY) -> BaseException: with pytest.raises(Exception) as raised: - await search(respond, queries, api_key, **params) + await search(respond, queries, api_key) return raised.value -def frame_locals(error: BaseException, keep: Callable[[str], bool]) -> List[str]: +def kept_item(hit: object, query: str = "q", max_content_chars: Optional[int] = None) -> FirecrawlSearchResultItem: + """_to_item for a hit the test expects to be kept.""" + result = FirecrawlSearchTool._to_item(hit, query, max_content_chars) + assert result is not None + return result + + +def without_names(names: Dict[str, str], skip_names: Sequence[str]) -> Dict[str, str]: + return {name: value for name, value in names.items() if name not in skip_names} + + +def frame_locals(error: BaseException, keep: Callable[[str], bool], skip_names: Sequence[str] = ()) -> List[str]: + """The captured locals of every frame in the error's traceback whose file passes `keep`.""" stack = traceback.TracebackException.from_exception(error, capture_locals=True).stack - return [repr(frame.locals) for frame in stack if keep(frame.filename)] + return [repr(without_names(frame.locals or {}, skip_names)) for frame in stack if keep(frame.filename)] def is_tool_frame(filename: str) -> bool: @@ -206,8 +270,8 @@ def assert_no_key(*texts: str) -> None: ################## # Result parsing # ################## -def test_to_item_web(): - item = FirecrawlSearchTool._to_item(WEB_HIT, "python") +def test_to_item_web() -> None: + item = kept_item(WEB_HIT, "python") assert item == FirecrawlSearchResultItem( query="python", title="Welcome to Python.org", @@ -217,14 +281,14 @@ def test_to_item_web(): ) -def test_to_item_news_uses_snippet_and_date(): - item = FirecrawlSearchTool._to_item(NEWS_HIT, "python") +def test_to_item_news_uses_snippet_and_date() -> None: + item = kept_item(NEWS_HIT, "python") assert (item.description, item.published, item.position) == ("The Python core team shipped 3.14 today.", "2 days ago", 2) -def test_to_item_content_and_truncation(): - assert FirecrawlSearchTool._to_item(CONTENT_HIT, "q").content == CONTENT_HIT["markdown"] - assert FirecrawlSearchTool._to_item(CONTENT_HIT, "q", max_content_chars=7).content == "# Agent" +def test_to_item_content_and_truncation() -> None: + assert kept_item(CONTENT_HIT, "q").content == CONTENT_HIT["markdown"] + assert kept_item(CONTENT_HIT, "q", max_content_chars=7).content == "# Agent" @pytest.mark.parametrize( @@ -243,10 +307,10 @@ def test_to_item_content_and_truncation(): ("markdown", {}, "content", None), ], ) -def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): +def test_to_item_drops_badly_typed_fields(field: str, value: object, attribute: str, expected: object) -> None: """One badly typed field must not discard the result or raise a raw ValidationError.""" - item = FirecrawlSearchTool._to_item({"url": "https://example.com/a", field: value}, "q") - assert getattr(item, attribute) == expected + kept = kept_item({"url": "https://example.com/a", field: value}) + assert getattr(kept, attribute) == expected @pytest.mark.parametrize( @@ -262,10 +326,13 @@ def test_to_item_drops_badly_typed_fields(field, value, attribute, expected): "https://xn--mller-kva.de/", "https://example.com/%C3%BC%20x.pdf", "https://example.com/a\\b", + "https://example.com./", + "https://my_host.example.com/", + f"https://{'a' * 63}.com/", ], ) -def test_to_item_keeps_valid_http_urls(url): - assert FirecrawlSearchTool._to_item({"url": url}, "q").url == url +def test_to_item_keeps_valid_http_urls(url: str) -> None: + assert kept_item({"url": url}).url == url @pytest.mark.parametrize( @@ -312,17 +379,25 @@ def test_to_item_keeps_valid_http_urls(url): {"url": "https://[::1%25eth0]/"}, {"url": "https://a.com\\@evil.com/"}, {"url": "https://evil.com\\.example.com/"}, + {"url": "https://ev\\il.com/"}, + {"url": "https://example..com/a"}, + {"url": "https://a%.com/a"}, + {"url": "https://a%41b.com/a"}, + {"url": "https://.example.com/"}, + {"url": "https://-a.com/"}, + {"url": "https://a-.com/"}, + {"url": f"https://{'a' * 64}.com/"}, ], ) -def test_to_item_without_usable_url_is_none(hit): +def test_to_item_without_usable_url_is_none(hit: object) -> None: assert FirecrawlSearchTool._to_item(hit, "q") is None ################ # Request body # ################ -def test_build_body_defaults(): - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k")) +def test_build_body_defaults() -> None: + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr("k"))) assert tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"])) == { "query": "python", "limit": 5, @@ -330,8 +405,8 @@ def test_build_body_defaults(): } -def test_build_body_with_options(): - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", location="Germany", time_range="week")) +def test_build_body_with_options() -> None: + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr("k"), location="Germany", time_range="week")) params = FirecrawlSearchToolInputSchema(queries=["python"], max_results_per_query=3, include_content=True) assert tool._build_body("python", params) == { "query": "python", @@ -343,14 +418,14 @@ def test_build_body_with_options(): } -def test_build_body_skips_time_range_for_news(): +def test_build_body_skips_time_range_for_news() -> None: """The API's tbs filter only applies to web results.""" - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k", time_range="day")) + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr("k"), time_range="day")) body = tool._build_body("python", FirecrawlSearchToolInputSchema(queries=["python"], search_type="news")) assert body == {"query": "python", "limit": 5, "sources": ["news"]} -def test_empty_queries_rejected(): +def test_empty_queries_rejected() -> None: with pytest.raises(ValidationError): FirecrawlSearchToolInputSchema(queries=[]) @@ -359,10 +434,10 @@ def test_empty_queries_rejected(): # Round trips over real transport # ################################### @pytest.mark.asyncio -async def test_web_round_trip(): +async def test_web_round_trip() -> None: out, seen = await search(respond_hits("web", [CONTENT_HIT]), queries=["agents"], include_content=True) - assert out == FirecrawlSearchToolOutputSchema(results=[FirecrawlSearchTool._to_item(CONTENT_HIT, "agents")]) + assert out == FirecrawlSearchToolOutputSchema(results=[kept_item(CONTENT_HIT, "agents")]) assert seen[0]["headers"]["Authorization"] == f"Bearer {DUMMY_KEY}" assert seen[0]["body"] == { "query": "agents", @@ -373,40 +448,40 @@ async def test_web_round_trip(): @pytest.mark.asyncio -async def test_news_round_trip_reads_news_key_only(): +async def test_news_round_trip_reads_news_key_only() -> None: out, _ = await search(respond_json({"success": True, "data": {"web": [WEB_HIT], "news": [NEWS_HIT]}}), search_type="news") assert [r.url for r in out.results] == [NEWS_HIT["url"]] @pytest.mark.asyncio -async def test_missing_source_key_is_an_empty_search(): +async def test_missing_source_key_is_an_empty_search() -> None: """The API omits the source key when there are no results; that is a real empty result.""" out, _ = await search(respond_json({"success": True, "data": {}})) assert out == FirecrawlSearchToolOutputSchema(results=[], failures=[]) @pytest.mark.asyncio -async def test_results_trimmed_to_max_results(): +async def test_results_trimmed_to_max_results() -> None: hits = [{"url": f"https://example.com/{i}"} for i in range(5)] out, _ = await search(respond_hits("web", hits), max_results_per_query=2) assert [r.url for r in out.results] == ["https://example.com/0", "https://example.com/1"] @pytest.mark.asyncio -async def test_invalid_hits_skipped_valid_ones_kept(): +async def test_invalid_hits_skipped_valid_ones_kept() -> None: hits = [1, "s", None, {"title": "x"}, {"url": "https://@/a"}, WEB_HIT, {"url": "https://example.com/b", "position": "N/A"}] out, _ = await search(respond_hits("web", hits)) assert [(r.url, r.position) for r in out.results] == [("https://www.python.org/", 1), ("https://example.com/b", None)] @pytest.mark.asyncio -async def test_all_hits_invalid_raises(): +async def test_all_hits_invalid_raises() -> None: error = await search_error(respond_hits("web", [1, None, {"title": "x"}, {"url": "https://:443/a"}])) assert str(error) == "Firecrawl search failed for 'q': 200 none of the 4 'web' results had a usable url" @pytest.mark.asyncio -async def test_duplicate_queries_sent_once(): +async def test_duplicate_queries_sent_once() -> None: out, seen = await search(respond_hits("web", [WEB_HIT]), queries=["q", "q"]) assert len(seen) == 1 assert [r.query for r in out.results] == ["q"] @@ -426,58 +501,60 @@ async def test_duplicate_queries_sent_once(): {"success": True, "data": [WEB_HIT]}, ], ) -async def test_malformed_success_response_raises(payload): +async def test_malformed_success_response_raises(payload: Body) -> None: error = await search_error(respond_json(payload)) assert str(error) == "Firecrawl search failed for 'q': 200 unexpected response shape for 'web' results" @pytest.mark.asyncio @pytest.mark.parametrize("payload", [{"success": "false"}, {"success": "true"}, {"success": 1}, {}]) -async def test_success_must_be_literally_true(payload): +async def test_success_must_be_literally_true(payload: Body) -> None: error = await search_error(respond_json({**payload, "data": {"web": [WEB_HIT]}})) assert str(error) == "Firecrawl search failed for 'q': 200 response did not report success" @pytest.mark.asyncio @pytest.mark.parametrize("status, reason", [(201, "Created"), (500, "Internal Server Error")]) -async def test_non_200_with_success_true_raises(status, reason): +async def test_non_200_with_success_true_raises(status: int, reason: str) -> None: error = await search_error(respond_json({"success": True, "data": {"web": [WEB_HIT]}}, status=status)) assert str(error) == f"Firecrawl search failed for 'q': {status} {reason}" @pytest.mark.asyncio -async def test_provider_error_message_is_reported(): +async def test_provider_error_message_is_reported() -> None: error = await search_error(respond_json({"success": False, "error": "Payment required"}, status=402)) assert str(error) == "Firecrawl search failed for 'q': 402 Payment required" @pytest.mark.asyncio @pytest.mark.parametrize( - "response, expected", + "status, text, content_type, expected", [ - ({"status": 502, "text": "Bad Gateway", "content_type": "text/html"}, "502 Bad Gateway"), - ({"status": 200, "text": ""}, "200 response did not report success"), - ({"status": 204}, "204 No Content"), + (502, "Bad Gateway", "text/html", "502 Bad Gateway"), + (200, "", None, "200 response did not report success"), + (204, None, None, "204 No Content"), ], ) -async def test_non_json_bodies_raise_with_status(response, expected): - error = await search_error(respond_raw(**response)) +async def test_non_json_bodies_raise_with_status( + status: int, text: Optional[str], content_type: Optional[str], expected: str +) -> None: + error = await search_error(respond_raw(status, text, content_type)) assert str(error) == f"Firecrawl search failed for 'q': {expected}" @pytest.mark.asyncio -async def test_deeply_nested_json_is_a_wrapped_failure(): +async def test_deeply_nested_json_is_a_wrapped_failure() -> None: """JSON too deep for the parser must fail like any other bad body, not escape with the response in scope.""" - error = await search_error(respond_raw(status=200, text="[" * 200_000, content_type="application/json")) + error = await search_error(respond_raw(200, "[" * 200_000, "application/json")) assert str(error) == "Firecrawl search failed for 'q': 200 response did not report success" assert_no_key(error_surfaces(error)) @pytest.mark.asyncio -async def test_redirect_is_not_followed(): +async def test_redirect_is_not_followed() -> None: """A redirect, even to another server, is reported as an error; the query is never re-sent elsewhere.""" async with fake_firecrawl(respond_hits("web", [WEB_HIT])) as (other_url, other_seen): - error = await search_error(respond_raw(status=307, headers={"Location": f"{other_url}/search"})) + error = await search_error(respond_raw(307, location=f"{other_url}/search")) assert str(error) == "Firecrawl search failed for 'q': 307 Temporary Redirect" assert other_seen == [] @@ -487,7 +564,7 @@ async def test_redirect_is_not_followed(): # Batch semantics # ################### @pytest.mark.asyncio -async def test_partial_failure_reports_failures(caplog): +async def test_partial_failure_reports_failures(caplog: pytest.LogCaptureFixture) -> None: rate_limited = respond_json({"success": False, "error": "Rate limit exceeded"}, status=429) with caplog.at_level(logging.WARNING, logger="tool.firecrawl_search"): out, _ = await search(respond_per_query("bad", rate_limited, respond_hits("web", [WEB_HIT])), queries=["bad", "good"]) @@ -506,7 +583,7 @@ async def test_partial_failure_reports_failures(caplog): ({"success": False, "error": "Rate limit exceeded"}, "200 Rate limit exceeded"), ], ) -async def test_empty_plus_failed_query_raises(bad, expected): +async def test_empty_plus_failed_query_raises(bad: Body, expected: str) -> None: """No results anywhere plus a failure must not look like an empty search.""" respond = respond_per_query("bad", respond_json(bad), respond_json({"success": True, "data": {}})) error = await search_error(respond, queries=["empty", "bad"]) @@ -514,7 +591,7 @@ async def test_empty_plus_failed_query_raises(bad, expected): @pytest.mark.asyncio -async def test_every_query_failing_raises_first_error(): +async def test_every_query_failing_raises_first_error() -> None: error = await search_error(respond_json({"success": False, "error": "Unauthorized"}, status=401), queries=["a", "b"]) assert str(error) == "Firecrawl search failed for 'a': 401 Unauthorized" @@ -523,7 +600,7 @@ async def test_every_query_failing_raises_first_error(): # Transport errors # #################### @pytest.mark.asyncio -async def test_connection_error_is_wrapped_without_context(): +async def test_connection_error_is_wrapped_without_context() -> None: with socket.socket() as sock: sock.bind(("127.0.0.1", 0)) port = sock.getsockname()[1] @@ -537,7 +614,7 @@ async def test_connection_error_is_wrapped_without_context(): @pytest.mark.asyncio -async def test_timeout_is_wrapped_with_query(): +async def test_timeout_is_wrapped_with_query() -> None: async def slow(body: Body) -> web.Response: await asyncio.sleep(1) return web.json_response({"success": True, "data": {"web": [WEB_HIT]}}) @@ -562,7 +639,7 @@ async def slow(body: Body) -> web.Response: ("http://user:pw@127.0.0.1:1/v2", "ValueError"), ], ) -async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url, error_type): +async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url: str, error_type: str) -> None: """aiohttp raises for such URLs from frames whose locals hold the request headers; none of that may surface.""" with pytest.raises(Exception) as raised: await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) @@ -573,22 +650,23 @@ async def test_invalid_base_url_is_wrapped_and_no_frame_holds_the_key(base_url, @pytest.mark.asyncio -@pytest.mark.parametrize( - "base_url, blocked_host", - [ - ("http://exa\nmple.com/v2", "example.com"), # yarl drops the newline, so this resolves example.com - (f"http://{'a' * 70}.example.com/v2", f"{'a' * 70}.example.com"), - ], -) -async def test_unresolvable_base_url_is_wrapped(offline, base_url, blocked_host): - """A DNS failure is reported as a request failure; the offline guard answers the lookup, so nothing leaves.""" +@pytest.mark.parametrize("base_url", ["http://localhost:9/v2", "http://local\nhost:9/v2"]) # yarl drops the newline +async def test_unresolvable_base_url_is_wrapped(monkeypatch: pytest.MonkeyPatch, base_url: str) -> None: + """A DNS failure is reported as a request failure. The lookup is for localhost and is answered by a stub that + fails it, so no external host name is involved.""" + lookups: List[str] = [] + + def unresolvable(host: Union[bytes, str, None], *args: object, **kwargs: object) -> Sequence[AddrInfo]: + lookups.append(str(host)) + raise socket.gaierror("name resolution failed") + + monkeypatch.setattr(socket, "getaddrinfo", unresolvable) with pytest.raises(Exception) as raised: - await http_tool(base_url).run_async(FirecrawlSearchToolInputSchema(queries=["q"])) + await http_tool(base_url).run_async(search_input(["q"])) assert str(raised.value).startswith("Firecrawl search failed for 'q': request failed: ClientConnectorDNSError ") assert_no_key(str(raised.value), repr(raised.value), *frame_locals(raised.value, is_outside_this_test_file)) - assert offline == [blocked_host] - offline.clear() + assert lookups == ["localhost"] ################ @@ -598,7 +676,7 @@ async def test_unresolvable_base_url_is_wrapped(offline, base_url, blocked_host) @pytest.mark.parametrize( "echo", [DUMMY_KEY, f"Bearer {DUMMY_KEY}", DUMMY_KEY.upper(), {"token": DUMMY_KEY}, CANONICAL_KEY, CANONICAL_KEY.upper()] ) -async def test_echoed_key_kept_out_of_errors_and_logs(caplog, echo): +async def test_echoed_key_kept_out_of_errors_and_logs(caplog: pytest.LogCaptureFixture, echo: object) -> None: """Even when the provider echoes a key back, it never reaches raised errors, tracebacks, logs or failures.""" respond = respond_per_query( "bad", respond_json({"success": False, "error": echo}, status=401), respond_hits("web", [WEB_HIT]) @@ -623,7 +701,7 @@ def invalid_hits_echoing(key: str) -> Body: @pytest.mark.asyncio @pytest.mark.parametrize("api_key", [DUMMY_KEY, CANONICAL_KEY]) @pytest.mark.parametrize("build_body", [malformed_body_echoing, invalid_hits_echoing]) -async def test_rejected_success_body_not_kept_in_traceback_locals(api_key, build_body): +async def test_rejected_success_body_not_kept_in_traceback_locals(api_key: str, build_body: Callable[[str], Body]) -> None: """A malformed or all-invalid success body is dropped before the raise, so captured locals cannot show a key.""" error = await search_error(respond_json(build_body(api_key)), api_key=api_key) assert str(error).startswith("Firecrawl search failed for 'q': 200 ") @@ -632,8 +710,9 @@ async def test_rejected_success_body_not_kept_in_traceback_locals(api_key, build @pytest.mark.asyncio @pytest.mark.parametrize("api_key", [DUMMY_KEY, CANONICAL_KEY]) -async def test_key_in_query_text_is_redacted_in_diagnostics(caplog, api_key): - """A query that contains the key (for example pasted by mistake) must not leak it through errors or logs.""" +async def test_key_in_query_text_is_redacted_in_diagnostics(caplog: pytest.LogCaptureFixture, api_key: str) -> None: + """A query that contains the key (for example pasted by mistake) must not leak it through errors, logs, + failures, or any tool-frame local other than the caller's own inputs.""" query = f"why does {api_key} fail" respond = respond_per_query( query, respond_json({"success": False, "error": "Unauthorized"}, status=401), respond_hits("web", [WEB_HIT]) @@ -644,11 +723,13 @@ async def test_key_in_query_text_is_redacted_in_diagnostics(caplog, api_key): assert out.failures == [FirecrawlSearchFailure(query="why does [redacted] fail", error=str(error))] assert str(error) == "Firecrawl search failed for 'why does [redacted] fail': 401 Unauthorized" - assert_no_key(caplog.text, repr(out.failures), str(error), repr(error)) + assert_no_key(caplog.text, repr(out.failures), str(error), repr(error), repr(error.__context__)) + # Traceback locals: the caller's own inputs appear as given; nothing the tool derives from them may hold the key. + assert_no_key(*frame_locals(error, is_tool_frame, skip_names=sorted(CALLER_INPUT_LOCALS))) -def test_key_kept_out_of_tool_and_config_state(): - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=DUMMY_KEY)) +def test_key_kept_out_of_tool_and_config_state() -> None: + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr(DUMMY_KEY))) assert_no_key( repr(tool), repr(tool.config), @@ -660,15 +741,15 @@ def test_key_kept_out_of_tool_and_config_state(): assert tool._api_key.get_secret_value() == DUMMY_KEY -def test_redaction_does_not_mangle_ordinary_text(): +def test_redaction_does_not_mangle_ordinary_text() -> None: """A one-letter key is not redacted (it would hit every 'a'), and fc- words that are not keys are left alone.""" - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="a")) + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr("a"))) text = "Firecrawl search failed for 'fc-barcelona-2024' and 'fc-0123abcd'" assert tool._redact(text) == text @pytest.mark.asyncio -async def test_missing_key_raises(monkeypatch): +async def test_missing_key_raises(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("FIRECRAWL_API_KEY", raising=False) tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) @@ -676,14 +757,14 @@ async def test_missing_key_raises(monkeypatch): await tool.run_async(FirecrawlSearchToolInputSchema(queries=["q"])) -def test_api_key_falls_back_to_environment(monkeypatch): +def test_api_key_falls_back_to_environment(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("FIRECRAWL_API_KEY", "env-key") tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig()) assert tool._api_key.get_secret_value() == "env-key" -def test_run_invokes_run_async(): - tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key="k")) +def test_run_invokes_run_async() -> None: + tool = FirecrawlSearchTool(config=FirecrawlSearchToolConfig(api_key=SecretStr("k"))) sentinel = FirecrawlSearchToolOutputSchema(results=[]) with patch.object(FirecrawlSearchTool, "run_async", AsyncMock(return_value=sentinel)): assert tool.run(FirecrawlSearchToolInputSchema(queries=["x"])) is sentinel diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index 0f9d22f5..c0cab888 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -50,10 +50,10 @@ class FirecrawlSearchResultItem(BaseIOSchema): query: str = Field(..., description="The query that produced this result.") title: str = Field(..., description="Title of the result.") url: str = Field(..., description="URL of the result.") - description: Optional[str] = Field(None, description="Query-relevant excerpt or snippet for the result.") - position: Optional[int] = Field(None, description="Rank of the result.") - published: Optional[str] = Field(None, description="Publication date string (news only).") - content: Optional[str] = Field(None, description="Page content as Markdown (only when include_content is set).") + description: Optional[str] = Field(default=None, description="Query-relevant excerpt or snippet for the result.") + position: Optional[int] = Field(default=None, description="Rank of the result.") + published: Optional[str] = Field(default=None, description="Publication date string (news only).") + content: Optional[str] = Field(default=None, description="Page content as Markdown (only when include_content is set).") @field_validator("position", mode="before") @classmethod @@ -112,13 +112,18 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear # when it is long enough that replacing it cannot mangle ordinary words. KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 - # Shape of a usable result URL: http(s), optional userinfo, a host that starts with a letter or digit (or an - # IPv6 literal), an ASCII port, no whitespace anywhere, and no backslash before the path (parsers disagree on - # what host "a.com\@b.com" means). urlsplit then validates the port range and the IPv6 literal. + # Shape of a usable result URL: http(s), optional userinfo without a backslash (parsers disagree on what host + # "a.com\@b.com" means), a host or IPv6 literal, an optional port, and no whitespace anywhere. urlsplit then + # validates the port and the IPv6 literal, and _valid_host the host name. RESULT_URL_PATTERN = re.compile( - r"https?://(?:[^\s/?#@\\]*@)?(?:[^\W_][^\s/?#@:\[\]\\]*|\[[0-9a-f:.]+\])(?::[0-9]*)?(?:[/?#]\S*)?", re.IGNORECASE + r"https?://(?:[^\s/?#@\\]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::[^\s/?#]*)?(?:[/?#]\S*)?", re.IGNORECASE ) + # One DNS label: letters, digits, hyphens and underscores, starting and ending with a letter or digit. No DNS + # lookup happens; this only rejects host names that cannot be valid, such as "example..com" or "a%.com". + HOST_LABEL = r"[^\W_](?:[\w-]{0,61}[^\W_])?" + HOST_NAME_PATTERN = re.compile(rf"(?:{HOST_LABEL}\.)*{HOST_LABEL}\.?") + def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) self._api_key = SecretStr(config.api_key.get_secret_value() or os.getenv("FIRECRAWL_API_KEY", "")) @@ -133,14 +138,21 @@ def _first_text(*values: object) -> Optional[str]: """Return the first non-empty string; result fields are provider data and may have any type.""" return next((value for value in values if isinstance(value, str) and value), None) + @classmethod + def _valid_host(cls, host: Optional[str]) -> bool: + """True for an IPv6 literal (urlsplit has already validated it) or a host name made of non-empty labels of + letters, digits, hyphens and underscores that start and end with a letter or digit.""" + return host is not None and (":" in host or cls.HOST_NAME_PATTERN.fullmatch(host) is not None) + @classmethod def _usable_url(cls, url: str) -> bool: - """True for a printable http(s) URL with a real host and an in-range, non-zero port.""" + """True for a printable http(s) URL with a valid host and an in-range, non-zero port.""" if not (url.isprintable() and cls.RESULT_URL_PATTERN.fullmatch(url)): return False try: # urlsplit raises ValueError for an out-of-range port or an invalid IPv6 literal. - return urlsplit(url).port != 0 + parts = urlsplit(url) + return parts.port != 0 and cls._valid_host(parts.hostname) except ValueError: return False @@ -221,7 +233,8 @@ async def _post( """Return the body of a successful search response, or raise. The raise happens in this frame, which only ever holds the redacted failure reason: no response, no aiohttp - error and no failed body, so a traceback that captures locals cannot show a key echoed back.""" + error and no failed body, so a traceback that captures locals cannot show a key echoed back. The caller's + own inputs (the query and params) are in these frames as given, as they are in the caller's frames.""" outcome = await self._exchange(session, query, params) if isinstance(outcome, str): raise self._error(query, outcome) From 9bb769e2a3e4b38a709b026b7e31a374b2fe6d3f Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:10:32 -0300 Subject: [PATCH 11/15] Keep @ and backslash out of the port; accept numeric hosts only as IPv4; cap host names at 253 characters --- atomic-forge/tools/firecrawl_search/README.md | 2 +- .../tests/test_firecrawl_search.py | 11 +++++++ .../firecrawl_search/tool/firecrawl_search.py | 33 ++++++++++++++----- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 11d8710e..fbd0ecdb 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -31,7 +31,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name or IPv6 address, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name, IPv4 or IPv6 address, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. - `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). Keys are redacted from both as described for `api_key`. They are also logged as warnings. ## Usage diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 6314df23..62ed9aaf 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -329,6 +329,9 @@ def test_to_item_drops_badly_typed_fields(field: str, value: object, attribute: "https://example.com./", "https://my_host.example.com/", f"https://{'a' * 63}.com/", + f"https://{'a' * 63}.{'b' * 63}.{'c' * 63}.{'d' * 61}/", + "https://1.2.3.4/", + "https://3com.example.123abc/", ], ) def test_to_item_keeps_valid_http_urls(url: str) -> None: @@ -387,6 +390,14 @@ def test_to_item_keeps_valid_http_urls(url: str) -> None: {"url": "https://-a.com/"}, {"url": "https://a-.com/"}, {"url": f"https://{'a' * 64}.com/"}, + {"url": "https://example.com../"}, + {"url": f"https://{('a' * 63 + '.') * 4}com/"}, + {"url": "https://a.com:1\\@b.com/"}, + {"url": "https://a.com:x@/"}, + {"url": "https://256.0.0.1/"}, + {"url": "https://1.2.3/"}, + {"url": "https://999999/"}, + {"url": "https://example.\u0661\u0662\u0663/"}, ], ) def test_to_item_without_usable_url_is_none(hit: object) -> None: diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index c0cab888..a585af49 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -1,4 +1,5 @@ import asyncio +import ipaddress import logging import os import re @@ -112,17 +113,19 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear # when it is long enough that replacing it cannot mangle ordinary words. KEY_PATTERN = re.compile(r"\bfc-[0-9a-f]{32}\b", re.IGNORECASE) MIN_REDACTED_KEY_LENGTH = 8 - # Shape of a usable result URL: http(s), optional userinfo without a backslash (parsers disagree on what host - # "a.com\@b.com" means), a host or IPv6 literal, an optional port, and no whitespace anywhere. urlsplit then - # validates the port and the IPv6 literal, and _valid_host the host name. + # Shape of a usable result URL: http(s), optional userinfo, a host or IPv6 literal, an optional port, and no + # whitespace anywhere. No backslash or extra "@" before the path, since parsers disagree on what host + # "a.com\@b.com" or "a.com:1\@b.com" means. urlsplit then validates the port and the IPv6 literal, and + # _valid_host the host name. RESULT_URL_PATTERN = re.compile( - r"https?://(?:[^\s/?#@\\]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::[^\s/?#]*)?(?:[/?#]\S*)?", re.IGNORECASE + r"https?://(?:[^\s/?#@\\]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::[^\s/?#@\\]*)?(?:[/?#]\S*)?", re.IGNORECASE ) # One DNS label: letters, digits, hyphens and underscores, starting and ending with a letter or digit. No DNS # lookup happens; this only rejects host names that cannot be valid, such as "example..com" or "a%.com". HOST_LABEL = r"[^\W_](?:[\w-]{0,61}[^\W_])?" - HOST_NAME_PATTERN = re.compile(rf"(?:{HOST_LABEL}\.)*{HOST_LABEL}\.?") + HOST_NAME_PATTERN = re.compile(rf"(?:{HOST_LABEL}\.)*{HOST_LABEL}") + MAX_HOST_NAME_LENGTH = 253 def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): super().__init__(config) @@ -140,9 +143,23 @@ def _first_text(*values: object) -> Optional[str]: @classmethod def _valid_host(cls, host: Optional[str]) -> bool: - """True for an IPv6 literal (urlsplit has already validated it) or a host name made of non-empty labels of - letters, digits, hyphens and underscores that start and end with a letter or digit.""" - return host is not None and (":" in host or cls.HOST_NAME_PATTERN.fullmatch(host) is not None) + """True for an IPv6 literal (urlsplit has already validated it), an IPv4 address, or a host name of at most + 253 characters made of labels that start and end with a letter or digit, with one optional trailing dot.""" + name = (host or "").removesuffix(".") + if ":" in name: + return True + if name.rsplit(".", 1)[-1].isdigit(): + # A numeric last label is never a TLD, so only a real IPv4 address is usable (not 256.0.0.1 or 1.2.3). + return cls._is_ipv4(name) + return len(name) <= cls.MAX_HOST_NAME_LENGTH and cls.HOST_NAME_PATTERN.fullmatch(name) is not None + + @staticmethod + def _is_ipv4(name: str) -> bool: + try: + ipaddress.IPv4Address(name) + except ValueError: + return False + return True @classmethod def _usable_url(cls, url: str) -> bool: From 24b09860e0607a1cb7acbcbcf11cf6c09a84772b Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:14:07 -0300 Subject: [PATCH 12/15] Pin the 253-character host name limit in tests --- .../tools/firecrawl_search/tests/test_firecrawl_search.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 62ed9aaf..9086f636 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -392,6 +392,7 @@ def test_to_item_keeps_valid_http_urls(url: str) -> None: {"url": f"https://{'a' * 64}.com/"}, {"url": "https://example.com../"}, {"url": f"https://{('a' * 63 + '.') * 4}com/"}, + {"url": f"https://{'a' * 63}.{'b' * 63}.{'c' * 63}.{'d' * 62}/"}, # 254 characters {"url": "https://a.com:1\\@b.com/"}, {"url": "https://a.com:x@/"}, {"url": "https://256.0.0.1/"}, From 2b21389f35698d98434958cc7b389b3869ac2d84 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Fri, 9 Oct 2026 08:00:58 -0300 Subject: [PATCH 13/15] Check Unicode host names in their IDNA-encoded form --- atomic-forge/tools/firecrawl_search/README.md | 2 +- .../tests/test_firecrawl_search.py | 7 +++++ .../firecrawl_search/tool/firecrawl_search.py | 28 ++++++++++++++----- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index fbd0ecdb..598657fb 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -31,7 +31,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name, IPv4 or IPv6 address, a valid port, no whitespace or control characters) are skipped, and badly typed optional fields are left empty. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name, IPv4 or IPv6 address, a valid port, no whitespace or control characters; Unicode host names are checked in their IDNA-encoded form) are skipped, and badly typed optional fields are left empty. - `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). Keys are redacted from both as described for `api_key`. They are also logged as warnings. ## Usage diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 9086f636..27fadedb 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -331,6 +331,10 @@ def test_to_item_drops_badly_typed_fields(field: str, value: object, attribute: f"https://{'a' * 63}.com/", f"https://{'a' * 63}.{'b' * 63}.{'c' * 63}.{'d' * 61}/", "https://1.2.3.4/", + "https://stra\u00dfe.de/", + "https://\u05e9\u05dc\u05d5\u05dd.co.il/", + "https://\u4f8b\u5b50.\u6d4b\u8bd5/", + f"https://{'\u00e9' * 20}.com/", "https://3com.example.123abc/", ], ) @@ -399,6 +403,9 @@ def test_to_item_keeps_valid_http_urls(url: str) -> None: {"url": "https://1.2.3/"}, {"url": "https://999999/"}, {"url": "https://example.\u0661\u0662\u0663/"}, + {"url": "https://a\u05d0.com/a"}, # mixes left-to-right and right-to-left in one label + {"url": f"https://{'\u00e9' * 63}.com/"}, # 63 characters, but over 63 bytes once IDNA-encoded + {"url": f"https://{('\u00e9' * 20 + '.') * 10}com/"}, # under 253 characters, over 253 once encoded ], ) def test_to_item_without_usable_url_is_none(hit: object) -> None: diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index a585af49..b6954fdf 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -121,10 +121,11 @@ class FirecrawlSearchTool(BaseTool[FirecrawlSearchToolInputSchema, FirecrawlSear r"https?://(?:[^\s/?#@\\]*@)?(?:[^\s/?#@:\[\]]+|\[[0-9a-f:.]+\])(?::[^\s/?#@\\]*)?(?:[/?#]\S*)?", re.IGNORECASE ) - # One DNS label: letters, digits, hyphens and underscores, starting and ending with a letter or digit. No DNS - # lookup happens; this only rejects host names that cannot be valid, such as "example..com" or "a%.com". - HOST_LABEL = r"[^\W_](?:[\w-]{0,61}[^\W_])?" - HOST_NAME_PATTERN = re.compile(rf"(?:{HOST_LABEL}\.)*{HOST_LABEL}") + # One label of a host name in its ASCII (IDNA) form: letters, digits, hyphens and underscores, starting and ending + # with a letter or digit. The IDNA codec enforces the 63-byte label limit. No DNS lookup happens; this only + # rejects host names that cannot be valid, such as "example..com", "a%.com" or "aא.com". + HOST_LABEL = r"[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?" + HOST_NAME_PATTERN = re.compile(rf"(?:{HOST_LABEL}\.)*{HOST_LABEL}", re.IGNORECASE) MAX_HOST_NAME_LENGTH = 253 def __init__(self, config: FirecrawlSearchToolConfig = FirecrawlSearchToolConfig()): @@ -143,15 +144,28 @@ def _first_text(*values: object) -> Optional[str]: @classmethod def _valid_host(cls, host: Optional[str]) -> bool: - """True for an IPv6 literal (urlsplit has already validated it), an IPv4 address, or a host name of at most - 253 characters made of labels that start and end with a letter or digit, with one optional trailing dot.""" + """True for an IPv6 literal (urlsplit has already validated it), an IPv4 address, or a host name whose ASCII + (IDNA) form is at most 253 characters of labels that start and end with a letter or digit, with one optional + trailing dot. Unicode names are checked after encoding, which is what DNS sees.""" name = (host or "").removesuffix(".") if ":" in name: return True if name.rsplit(".", 1)[-1].isdigit(): # A numeric last label is never a TLD, so only a real IPv4 address is usable (not 256.0.0.1 or 1.2.3). return cls._is_ipv4(name) - return len(name) <= cls.MAX_HOST_NAME_LENGTH and cls.HOST_NAME_PATTERN.fullmatch(name) is not None + ascii_name = cls._idna(name) + return ascii_name is not None and cls.HOST_NAME_PATTERN.fullmatch(ascii_name) is not None + + @classmethod + def _idna(cls, name: str) -> Optional[str]: + """The ASCII (IDNA) form of a host name, or None when it is longer than 253 characters or cannot be encoded: + the codec rejects labels that are empty or over 63 bytes once encoded, and Unicode labels that break the IDNA + rules, such as mixing left-to-right and right-to-left scripts.""" + try: + ascii_name = name.encode("idna").decode("ascii") + except UnicodeError: + return None + return ascii_name if len(ascii_name) <= cls.MAX_HOST_NAME_LENGTH else None @staticmethod def _is_ipv4(name: str) -> bool: From ae9c6514c86cb64a3ce09b81596d39dcdb0eee7e Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Fri, 9 Oct 2026 08:10:06 -0300 Subject: [PATCH 14/15] Encode host names before the numeric-label check so fullwidth dots cannot bypass it --- atomic-forge/tools/firecrawl_search/README.md | 2 +- .../tests/test_firecrawl_search.py | 6 +++++ .../firecrawl_search/tool/firecrawl_search.py | 26 ++++++++++++------- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/atomic-forge/tools/firecrawl_search/README.md b/atomic-forge/tools/firecrawl_search/README.md index 598657fb..9910002d 100644 --- a/atomic-forge/tools/firecrawl_search/README.md +++ b/atomic-forge/tools/firecrawl_search/README.md @@ -31,7 +31,7 @@ Searches the web or recent news through the [Firecrawl Search](https://www.firec - `include_content` (bool): also return each result page as Markdown (default `False`). ### Output Schema -- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name, IPv4 or IPv6 address, a valid port, no whitespace or control characters; Unicode host names are checked in their IDNA-encoded form) are skipped, and badly typed optional fields are left empty. +- `results`: a list of `FirecrawlSearchResultItem` items. Each has `query`, `title`, `url`, and optional `description` (a query-relevant excerpt or snippet), `position`, `published` (news), and `content` (Markdown, when `include_content` is set). Results without a well-formed http(s) URL (a valid host name, IPv4 or IPv6 address, a valid port, no whitespace or control characters; Unicode host names are checked in their IDNA 2003 encoded form, as Python's `idna` codec produces it) are skipped, and badly typed optional fields are left empty. - `failures`: queries that failed while others returned results, each with `query` and `error` (the HTTP status and the API's error message). Keys are redacted from both as described for `api_key`. They are also logged as warnings. ## Usage diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 27fadedb..4b634938 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -335,6 +335,9 @@ def test_to_item_drops_badly_typed_fields(field: str, value: object, attribute: "https://\u05e9\u05dc\u05d5\u05dd.co.il/", "https://\u4f8b\u5b50.\u6d4b\u8bd5/", f"https://{'\u00e9' * 20}.com/", + "https://example\u3002com/", + "https://example.com\u3002/", + "https://1\uff0e2\uff0e3\uff0e4/", "https://3com.example.123abc/", ], ) @@ -406,6 +409,9 @@ def test_to_item_keeps_valid_http_urls(url: str) -> None: {"url": "https://a\u05d0.com/a"}, # mixes left-to-right and right-to-left in one label {"url": f"https://{'\u00e9' * 63}.com/"}, # 63 characters, but over 63 bytes once IDNA-encoded {"url": f"https://{('\u00e9' * 20 + '.') * 10}com/"}, # under 253 characters, over 253 once encoded + {"url": "https://256.0.0\uff0e1/"}, # fullwidth dot: encodes to 256.0.0.1 + {"url": "https://1.2\u30023/"}, # ideographic dot: encodes to 1.2.3 + {"url": "https://a\uff61999999/"}, # halfwidth ideographic dot: numeric last label ], ) def test_to_item_without_usable_url_is_none(hit: object) -> None: diff --git a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py index b6954fdf..f811b50f 100644 --- a/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py @@ -147,22 +147,30 @@ def _valid_host(cls, host: Optional[str]) -> bool: """True for an IPv6 literal (urlsplit has already validated it), an IPv4 address, or a host name whose ASCII (IDNA) form is at most 253 characters of labels that start and end with a letter or digit, with one optional trailing dot. Unicode names are checked after encoding, which is what DNS sees.""" - name = (host or "").removesuffix(".") + name = host or "" if ":" in name: return True - if name.rsplit(".", 1)[-1].isdigit(): - # A numeric last label is never a TLD, so only a real IPv4 address is usable (not 256.0.0.1 or 1.2.3). - return cls._is_ipv4(name) ascii_name = cls._idna(name) - return ascii_name is not None and cls.HOST_NAME_PATTERN.fullmatch(ascii_name) is not None + if ascii_name is None: + return False + # A numeric last label is never a TLD, so only a real IPv4 address is usable (not 256.0.0.1 or 1.2.3). Both + # forms are checked: encoding turns dots such as "。" into ".", and leaves digits such as "١٢٣" non-ASCII. + if cls._numeric_last_label(name.removesuffix("."), ascii_name): + return cls._is_ipv4(ascii_name) + return cls.HOST_NAME_PATTERN.fullmatch(ascii_name) is not None + + @staticmethod + def _numeric_last_label(*names: str) -> bool: + return any(name.rsplit(".", 1)[-1].isdigit() for name in names) @classmethod def _idna(cls, name: str) -> Optional[str]: - """The ASCII (IDNA) form of a host name, or None when it is longer than 253 characters or cannot be encoded: - the codec rejects labels that are empty or over 63 bytes once encoded, and Unicode labels that break the IDNA - rules, such as mixing left-to-right and right-to-left scripts.""" + """The ASCII (IDNA 2003, as Python's codec implements it) form of a host name without its trailing dot, or + None when it is longer than 253 characters or cannot be encoded: the codec rejects labels that are empty or + over 63 bytes once encoded, and Unicode labels that break the IDNA rules, such as mixing left-to-right and + right-to-left scripts. Encoding also turns dots such as "。" into ".".""" try: - ascii_name = name.encode("idna").decode("ascii") + ascii_name = name.encode("idna").decode("ascii").removesuffix(".") except UnicodeError: return None return ascii_name if len(ascii_name) <= cls.MAX_HOST_NAME_LENGTH else None From c8bcadecd3b399b988947117e104c51cb9563b31 Mon Sep 17 00:00:00 2001 From: Juampi <102912458+JuampiHernandez@users.noreply.github.com> Date: Fri, 9 Oct 2026 08:12:27 -0300 Subject: [PATCH 15/15] Cover a host whose IDNA form ends in an empty label --- .../tools/firecrawl_search/tests/test_firecrawl_search.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py index 4b634938..e16000e0 100644 --- a/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py +++ b/atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py @@ -412,6 +412,7 @@ def test_to_item_keeps_valid_http_urls(url: str) -> None: {"url": "https://256.0.0\uff0e1/"}, # fullwidth dot: encodes to 256.0.0.1 {"url": "https://1.2\u30023/"}, # ideographic dot: encodes to 1.2.3 {"url": "https://a\uff61999999/"}, # halfwidth ideographic dot: numeric last label + {"url": "https://a\u2488./"}, # "\u2488" encodes to "1.", so this ends in an empty label ], ) def test_to_item_without_usable_url_is_none(hit: object) -> None: