Skip to content

fix(web): re-check Firecrawl final URLs for SSRF - #35840

Closed
zapabob wants to merge 2 commits into
NousResearch:mainfrom
zapabob:codex/firecrawl-final-url-ssrf-20260531
Closed

fix(web): re-check Firecrawl final URLs for SSRF#35840
zapabob wants to merge 2 commits into
NousResearch:mainfrom
zapabob:codex/firecrawl-final-url-ssrf-20260531

Conversation

@zapabob

@zapabob zapabob commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Re-check Firecrawl metadata.sourceURL final URLs with is_safe_url() before returning scraped content.
  • Preserve the existing website blocklist redirect check after the SSRF guard.
  • Add regression coverage for a Firecrawl redirect/final URL pointing at the cloud metadata endpoint.

Security impact

web_extract already rejects unsafe input URLs before dispatching to a backend. Firecrawl can report a different final URL after scrape/redirect via metadata.sourceURL; this patch blocks that final target before any scraped content is returned to the agent.

Verification

  • uv run --extra dev ruff check plugins/web/firecrawl/provider.py tests/tools/test_website_policy.py
  • uv run --extra dev python -X utf8 -m pytest tests/tools/test_website_policy.py::TestWebToolPolicy::test_web_extract_blocks_firecrawl_unsafe_final_url tests/tools/test_website_policy.py::TestWebToolPolicy::test_web_extract_blocks_redirected_final_url -q --timeout-method=thread
  • uv run --extra dev python -X utf8 -m pytest tests/tools/test_website_policy.py -q --timeout-method=thread
  • uv run --extra dev ruff check .

@zapabob
zapabob marked this pull request as ready for review May 31, 2026 12:19
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround tool/web Web search and extraction comp/plugins Plugin system and bundled plugins labels May 31, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

✅ What's Good

  • Good SSRF defense-in-depth: Firecrawl can report a different final URL after scrape/redirect via metadata.sourceURL. This patch properly re-checks that final URL with is_safe_url() before returning scraped content — closing a real SSRF bypass vector.
  • Clean integration: The new check sits naturally alongside the existing check_website_access redirect check. The continue after the block prevents any content leakage.
  • Well-structured test: The test verifies that the unsafe final URL is blocked, that the website policy check is NOT called for unsafe final URLs, and that scraped content is wiped. The mock setup is clean and focused.
  • No scope creep: 2 files changed, 64 additions, 0 deletions — tightly scoped to the security concern.
  • Existing test fix: The test monkeypatch for is_safe_url was added to the redirected-final-url test to prevent false failures, showing good awareness of test interactions.

Summary

A tightly scoped, well-tested security fix. No issues found.


Reviewed by Hermes Agent

@egilewski

Copy link
Copy Markdown
Contributor

Recommendation: rebase or resolve the current-main conflict before merge. The security fix itself validated in focused checks.

Checked current main 6f6eb871d83415fe2980f3483cc41a435ba22196 against PR head 85714c46cc513ca84364692c3ba9eb8e3fcf1f56. With a mocked Firecrawl scrape returning metadata.sourceURL = "http://169.254.169.254/latest/meta-data/", current main returned "metadata credentials" from that unsafe final URL. The patched provider blocks that final URL before returning content, with content == "" and Blocked: URL targets a private or internal network address.

Validation:

  • python -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_website_policy.py::TestWebToolPolicy::test_web_extract_blocks_firecrawl_unsafe_final_url tests/tools/test_website_policy.py::TestWebToolPolicy::test_web_extract_blocks_redirected_final_url -q --timeout-method=thread -> 2 passed
  • python -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_website_policy.py -q --timeout-method=thread -> 21 passed
  • coderabbit review --plain --base 6f6eb871d83415fe2980f3483cc41a435ba22196 --type uncommitted -> No findings

GitHub currently reports mergeable=CONFLICTING / mergeStateStatus=DIRTY, so this should be refreshed against current main before merging.

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jun 21, 2026
teknium1 added a commit that referenced this pull request Jul 1, 2026
Follow-up for salvaged PR #35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
@teknium1

teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #56143. Your commit was cherry-picked onto current main with your authorship preserved in git log (commit 2e12401). I added one small follow-up commit adapting the new test to the current web_extract_tool signature (main removed the use_llm_processing kwarg and moved the input SSRF gate to async_is_safe_url). Thanks for the security fix.

@teknium1 teknium1 closed this Jul 1, 2026
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
Jasper6439 pushed a commit to Jasper6439/hermes-agent that referenced this pull request Jul 5, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
Follow-up for salvaged PR NousResearch#35840: current main removed the
use_llm_processing kwarg (LLM summarization dropped) and moved the input
SSRF gate to async_is_safe_url. Adjust the new firecrawl-final-url test
to match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P1 High — major feature broken, no workaround sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/web Web search and extraction type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants