Skip to content

fix(browser): extend private-network guard to browser_get_images - #54187

Closed
srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/browser-get-images-ssrf
Closed

srojk34 wants to merge 1 commit into
NousResearch:mainfrom
srojk34:fix/browser-get-images-ssrf

Conversation

@srojk34

@srojk34 srojk34 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The SSRF cluster (7a6fe9b, 48f5c42, 7ef04ae — PR #54132) sealed browser_snapshot, browser_vision, and _browser_eval against eval-navigated private pages, but browser_get_images was missed.

browser_get_images bypasses _browser_eval and calls _run_browser_command(effective_task_id, "eval", [js_code]) directly, so none of the eval SSRF guards apply. After an eval-driven navigation to a private address (e.g. location.href = 'http://127.0.0.1:8080/'), calling browser_get_images would leak image src URLs and alt text from the private page.

Changes

  • tools/browser_tool.py: Add the same _eval_ssrf_guard_active() + _current_page_private_url() recheck to browser_get_images() before returning image data, matching the pattern from the snapshot/vision/eval guards.

Test plan

  • 5 new tests (test_browser_get_images_ssrf.py): blocks on private page, allows on public page, skips for local backend, skips when private URLs allowed, no guard on failed eval
  • Existing snapshot SSRF tests pass (10/10)
  • Existing eval SSRF tests pass (19/19)

The SSRF cluster (7a6fe9b, 48f5c42, 7ef04ae) sealed
browser_snapshot, browser_vision, and _browser_eval against
eval-navigated private pages, but browser_get_images bypasses
_browser_eval and calls _run_browser_command("eval", ...) directly.
An eval-driven navigation to a private address followed by
browser_get_images would leak image src URLs and alt text from the
private page.

Add the same _eval_ssrf_guard_active + _current_page_private_url
recheck before returning image data, matching the pattern established
by the sibling guards.

5 new tests cover: block on private page, allow on public page, skip
for local backend, skip when private URLs allowed, no guard needed on
failed eval.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening tool/browser Browser automation (CDP, Playwright) sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P1 High — major feature broken, no workaround labels Jun 28, 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\n\nVerdict: LGTM\n\nExtends the private-network SSRF guard to browser_get_images. Sibling of the snapshot/vision/eval guards. Clean test coverage with parametrized fixtures for public/private page scenarios.\n\n---\nReviewed by Hermes Agent

@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.

Security fix (96 additions). Extends the existing private-network SSRF guard to browser_get_images, which previously bypassed _browser_eval's URL check. Includes test coverage. Clean sibling of the snapshot/vision/eval guards.

Reviewed by Hermes Agent

@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

I found no source-level blocker in the browser private-network guard change. The patch closes the browser_get_images gap by reusing the existing eval-SSRF guard before returning image src/alt data, and the behavior holds when replayed onto current GitHub main.

Security evidence:

  • trust boundary: untrusted page content exposed through the browser tool result after a browser eval can move the page to a private/internal URL.
  • source/sink/invariant: browser_get_images runs page JavaScript through _run_browser_command(..., "eval", ...); it must not return image metadata when the current page URL is private under the non-local browser guard.
  • current-main reproduction: a monkeypatched probe against b699d27a4a8ca0d886d5c51c7d46861c571ebb9c returned success: true with http://127.0.0.1:8080/logo.png and Internal Logo.
  • PR-head or patch-replay validation: the same probe against PR head and a current-main patch replay returned the private-network block instead of image metadata.
  • positive/negative cases: new browser_get_images tests cover private-page blocking, public-page success, local-backend/private-URL opt-outs, and eval failure; sibling snapshot/eval SSRF tests still pass.
  • residual bypass search: inspected the sibling _browser_eval, browser_snapshot, and browser_vision guards and the browser_get_images direct eval path; no remaining image-result return path bypassing the added URL recheck was found in the changed scope.
  • reviewer validation: focused local probes and tests passed on PR head and current-main replay; the unrelated failing GitHub jobs are outside the changed browser path.

Signed: GPT-5.5-xhigh in Codex

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for tightening this browser-tool boundary. I verified the premise against current GitHub main and did not find a source-level blocker.

Current main at 9a0010f still has the gap: tools/browser_tool.py:3489 calls _run_browser_command(effective_task_id, "eval", [js_code]) inside browser_get_images, then tools/browser_tool.py:3491-3507 parses and returns image metadata without the private-page URL recheck.

The PR applies the same existing guard shape used by sibling paths: _eval_ssrf_guard_active is defined at tools/browser_tool.py:3158, _current_page_private_url at tools/browser_tool.py:3198, and _browser_eval already blocks post-eval private pages at tools/browser_tool.py:3277-3288. The diff adds that check to browser_get_images before returning image src/alt data, and tests/tools/test_browser_get_images_ssrf.py covers the private block, public success, opt-out, and failed-eval cases.

Salvageability looks high: this is a narrow security fix in one browser tool path, with focused tests and no new core surface, config, or prompt-cache implications.

Automated hermes-sweeper review.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #54435. Your commit was cherry-picked onto current main with your authorship preserved in git log (commit 6121009). Clean sibling of the #54132 SSRF cluster — thanks for catching the get_images gap!

@teknium1 teknium1 closed this Jun 28, 2026
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High — major feature broken, no workaround sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

5 participants