Skip to content

fix(security): browser redirect SSRF bypass + explicit URL scheme whitelist - #3046

Closed
0xbyt4 wants to merge 2 commits into
NousResearch:mainfrom
0xbyt4:fix/browser-redirect-ssrf-bypass
Closed

fix(security): browser redirect SSRF bypass + explicit URL scheme whitelist#3046
0xbyt4 wants to merge 2 commits into
NousResearch:mainfrom
0xbyt4:fix/browser-redirect-ssrf-bypass

Conversation

@0xbyt4

@0xbyt4 0xbyt4 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Two related SSRF hardening fixes for the browser and URL safety modules:

1. Post-navigation redirect validation (CRITICAL)

browser_navigate() validated the initial URL with is_safe_url() but Chromium follows HTTP redirects internally. An attacker-controlled URL like https://evil.com that 302-redirects to http://169.254.169.254/latest/meta-data/ bypassed the pre-flight SSRF check.

Now the final URL returned by Chromium is re-validated after navigation. If the redirect target is a private/internal address, the browser session is closed and an error is returned.

web_tools and vision_tools already had redirect protection (httpx event hooks). The browser tool was the only one missing it because agent-browser is a subprocess — CDP doesn't expose redirect events, so post-navigation validation is the correct approach.

2. Explicit URL scheme whitelist (HIGH)

is_safe_url() in url_safety.py blocked javascript:, data:, file:// URLs by accident — they had no hostname, so the hostname check returned False. This was fragile — if the hostname check logic ever changed, these dangerous schemes could slip through.

Added explicit parsed.scheme not in ("http", "https") check before hostname resolution. Now unsupported schemes are rejected with a clear log message.

Test plan

  • Scheme validation: javascript, data, file, ftp all blocked; http/https allowed
  • SSRF: localhost, 169.254.169.254, 192.168.x.x all blocked
  • tests/tools/test_url_safety.py — all passed
  • tests/tools/test_website_policy.py — 22/22 passed
  • tests/tools/test_browser_cdp_override.py — 4/4 passed
  • tests/tools/test_vision_tools.py — all passed
  • Total: 120/120 passed

0xbyt4 added 2 commits March 26, 2026 00:43
…it scheme whitelist

Two related SSRF hardening fixes:

1. browser_navigate: After Chromium follows redirects internally, re-validate
   the final URL with is_safe_url(). A public URL that 302-redirects to
   http://169.254.169.254/ (cloud metadata) was not caught because the
   pre-flight check only validated the initial URL. Session is closed if
   the redirect target is blocked.

2. url_safety: Add explicit scheme whitelist (http/https only). Previously
   javascript:, data:, file:// URLs were blocked by accident (no hostname
   parsed → returned False). Now they are explicitly rejected with a log
   message, preventing future regressions if the hostname check changes.
Ensures this PR works independently without requiring PR NousResearch#3041
(pre-navigation SSRF check) to be merged first.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P0 Critical — data loss, security, crash loop tool/browser Browser automation (CDP, Playwright) labels May 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Addresses the redirect SSRF gap identified in #7342. Prior merged work: #3058 (basic SSRF) and #4198 (configurable allow_private_urls).

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

Addresses the redirect SSRF gap identified in #7342. Prior merged work: #3058 (basic SSRF) and #4198 (configurable allow_private_urls).

@egilewski

Copy link
Copy Markdown
Contributor

Recommendation: close this PR as superseded.

I verified this against current upstream/main at 689ef5e23. The two hardening changes proposed here are already covered by newer upstream code:

  • tools/url_safety.py now explicitly rejects non-HTTP(S) schemes before hostname resolution.
  • tools/browser_tool.py now re-validates the final URL after browser navigation for cloud browser paths, and it also has a stronger always-blocked cloud-metadata floor for IMDS endpoints, including hybrid sidecar routing.

I also attempted a local diagnostic rebase of this PR onto current upstream. It conflicts exactly in tools/browser_tool.py and tools/url_safety.py, because the current code has newer logic in those same areas. Applying this PR as-is would not be an improvement: the post-redirect block here is broader than current upstream and does not preserve the current local/hybrid routing distinctions.

What can still be salvaged:

  • The threat writeup remains useful historical context for security(browser): DNS pinning for SSRF protection #7342.
  • A narrow test-only follow-up could add explicit javascript:, data:, and file:// URL-scheme cases. Current upstream already has scheme enforcement and some tests, but those exact examples from this PR body are still useful regression coverage.
  • The post-redirect validation idea has already landed upstream in a better integrated form, so there is no code from this PR that I would recommend rebasing directly.

Verification I ran on current upstream:

scripts/run_tests.sh tests/tools/test_browser_ssrf_local.py tests/tools/test_url_safety.py -- -k "redirect or scheme or unsupported or always_blocked or safe_url"

Result: 14 passed, 0 failed.

Based on that, I recommend closing this PR as superseded and opening a fresh small test PR only if the extra explicit scheme cases are desired.

@egilewski

Copy link
Copy Markdown
Contributor

Recommendation: close this PR as superseded.

I reviewed PR head a7ccfffe6ccdfbf0571d5fb36c8b9430a8f8c9e0 against current upstream/main as published by GitHub at 214b7e070f9ebc456bd0d9f1d768d78b76c706de.

Validation:

  • Current main already covers the PR's two stated behaviors. In the exact current-main source snapshot, env -i ... PYTHONPATH=<main> /home/mac/hermes-agent/.venv/bin/python -m pytest tests/tools/test_url_safety.py tests/tools/test_browser_ssrf_local.py -q passed: 132 passed.
  • A semantic probe on current main confirmed javascript:, data:, file://, and ftp:// are rejected, and a redirect to http://169.254.169.254/latest/meta-data/ returns Blocked: redirect landed on a cloud metadata endpoint after navigating the session to about:blank.
  • The same PR-head probe confirms this PR blocks the basic redirect case, but with older behavior: it calls browser close and lacks current main's always-blocked metadata floor, local/hybrid routing distinctions, and fail-closed _is_safe_url import fallback.
  • git apply --check of the PR patch against current-main snapshot failed in all three changed hunks (tools/browser_tool.py and tools/url_safety.py), so the PR is not currently mergeable as-is.
  • CodeRabbit could not be run on a current-main-applied diff because the patch no longer applies. I ran it on a temporary two-commit repo reconstructed from the PR's original base/head snapshots; coderabbit doctor passed connectivity/auth but still reported the known /home/mac/.coderabbit storage failure, and coderabbit review --plain --base HEAD~1 --type committed completed with 2 findings. The major finding was that this PR only validates redirects and does not add the initial unsafe-URL check; current upstream already has the pre-navigation safety path.

The linked issue #7342 is now narrowed to DNS-pinning / DNS-rebinding / connection-level TOCTOU. This PR's redirect and scheme changes are already superseded by newer upstream code and do not address that remaining scope.

Signed: GPT-5.5-xhigh in Codex

@teknium1

Copy link
Copy Markdown
Contributor

This looks implemented on current main by newer security hardening. Automated hermes-sweeper review.

Evidence:

  • tools/url_safety.py:329 explicitly rejects URL schemes outside http and https before hostname resolution; git log identifies this as 6af994232 fix(url-safety): allow only http and https schemes.
  • tools/browser_tool.py:2402 reads the browser-returned final URL after navigation, and tools/browser_tool.py:2425 blocks redirects whose final URL fails _is_safe_url, navigating away to about:blank before returning the error.
  • tools/browser_tool.py:2413 also applies the stronger always-blocked cloud metadata floor for redirected final URLs; git log identifies the browser-side hardening as 0214858ef fix(browser): enforce cloud-metadata SSRF floor in hybrid routing (#16234) (#21228).
  • tests/tools/test_browser_ssrf_local.py:222 covers redirect-to-private blocking, and tests/tools/test_browser_ssrf_local.py:297 covers redirect-to-IMDS blocking even through the hybrid sidecar path.
  • Targeted verification on current main passed: scripts/run_tests.sh tests/tools/test_browser_ssrf_local.py tests/tools/test_url_safety.py -- -k "redirect or scheme or unsupported or always_blocked or safe_url" returned 14 tests passed, 0 failed.

The prior discussion noting that this PR was superseded by newer upstream logic matches the current source. Thanks to @0xbyt4 for the original hardening work and threat writeup; the same behaviors are now covered on main in the integrated implementation.

@teknium1 teknium1 closed this Jun 10, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P0 Critical — data loss, security, crash loop sweeper:implemented-on-main Sweeper: behavior already present on current main tool/browser Browser automation (CDP, Playwright) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants