Skip to content

fix(yuanbao): block redirect-based SSRF in download_url - #56927

Open
solyanviktor-star wants to merge 1 commit into
NousResearch:mainfrom
solyanviktor-star:fix/yuanbao-ssrf-redirect-guard
Open

fix(yuanbao): block redirect-based SSRF in download_url#56927
solyanviktor-star wants to merge 1 commit into
NousResearch:mainfrom
solyanviktor-star:fix/yuanbao-ssrf-redirect-guard

Conversation

@solyanviktor-star

Copy link
Copy Markdown
Contributor

What does this PR do?

gateway/platforms/yuanbao_media.py::download_url fetches model-supplied and inbound URLs server-side. It has an is_safe_url() pre-flight, but its per-hop redirect guard was inert:

async def _redirect_guard(response: httpx.Response) -> None:
    if response.is_redirect and response.next_request:   # <- next_request is None here
        ...

Inside an httpx.AsyncClient response event hook, response.next_request is None even for a genuine 302 (it is populated later by the redirect-following machinery). So the guard condition was always falsy and never validated the redirect target. A public URL that passes the pre-flight check can therefore 302-redirect to http://169.254.169.254/ (cloud metadata / IMDS) or any private address, and httpx (follow_redirects=True) follows it — a redirect-based SSRF.

This is the same class that #56160 (merged) fixed across the other httpx download hooks by resolving the hop from the Location header via redirect_target_from_response; yuanbao_media.py was the one path that migration missed. (The broken next_request guard here was introduced by #54470.)

Related Issue

No open issue — found while auditing the httpx redirect guards after #56160. N/A.

Type of Change

  • 🔒 Security fix

Changes Made

  • gateway/platforms/yuanbao_media.py: _redirect_guard now resolves the redirect target with redirect_target_from_response(response) (reads the Location header, urljoin-resolving relative Locations) instead of the always-None response.next_request, then rejects it with is_safe_url. This mirrors gateway/platforms/base.py::_ssrf_redirect_guard.
  • tests/gateway/test_yuanbao_media_ssrf.py: added test_redirect_to_metadata_blocked — drives a 302 whose Location points at 169.254.169.254 with next_request=None (the real in-hook shape) through the registered guard and asserts ValueError. Fails on the old guard, passes on the fix.

How to Test

$ python -m pytest tests/gateway/test_yuanbao_media_ssrf.py -q
6 passed

The new test reproduces the bug: with the previous next_request-based guard, a 302 to the metadata endpoint (with next_request=None, as httpx leaves it inside the hook) is not blocked; with the fix it raises ValueError: Blocked redirect to private/internal address.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(yuanbao): ...)
  • I searched existing PRs/issues to make sure this isn't a duplicate (no open PR/issue for the yuanbao redirect guard; fix(security): close SSRF redirect-guard bypass across all httpx download hooks #56160 fixed the sibling hooks and merged)
  • My PR contains only changes related to this fix
  • I've run the relevant tests (pytest tests/gateway/test_yuanbao_media_ssrf.py -q → 6 passed); the full suite runs in CI
  • I've added a regression test for this fix
  • I've tested on my platform: Windows 10

Documentation & Housekeeping

  • Documentation — N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform impact — N/A (server-side URL fetch; platform-independent)
  • Tool descriptions/schemas — N/A

Screenshots / Logs

See How to Test above.

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 2, 2026
@falkoro

falkoro commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Verified the core claim empirically and it's worse than "frequently None" — as far as I can tell it's always None in this context: in httpx's redirect loop, response event hooks run before response.next_request is assigned, so the existing guard on main never fires at all. Local repro (httpx 0.28.1, Linux, real HTTP server answering 302): the hook observes next_request = None on a genuine redirect. That means main currently follows redirects to private/internal addresses unchecked — this PR fixes a live SSRF bypass, not a flaky edge case.

Also confirmed redirect_target_from_response already exists on main (tools/url_safety.py:479), and ran tests/gateway/test_yuanbao_media_ssrf.py on the branch: 6 passed (Python 3.12).

LGTM — this deserves priority attention given the guard it replaces is dead code.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the missed Yuanbao path. The premise holds on current main: gateway/platforms/yuanbao_media.py:228-234 still guards redirects only through response.next_request, while follow_redirects=True is active at gateway/platforms/yuanbao_media.py:237-240.

The proposed use of redirect_target_from_response matches the shared guard at gateway/platforms/base.py:557-562. That helper explicitly resolves the Location header before falling back to next_request (tools/url_safety.py:479-503), including relative redirects. The new regression test exercises the relevant in-hook shape: a metadata Location with next_request=None.

No blocking correctness or scope issues found. This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
download_url()'s redirect guard keyed on response.next_request, which is
None inside an httpx AsyncClient response event hook even for a genuine
302. The guard therefore never fired, so a public URL that passed the
is_safe_url() pre-flight could 302-redirect to http://169.254.169.254/
(cloud metadata) or any private/internal address and httpx would follow it.

Resolve the hop from the Location header via redirect_target_from_response,
mirroring gateway/platforms/base.py::_ssrf_redirect_guard -- the same fix
one path it missed. Add a regression test driving a 302 -> metadata through
the guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@solyanviktor-star

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to resolve the conflict with 42626da (create_ssrf_safe_async_client DNS pinning) — the import now carries both the pinned client and redirect_target_from_response, and the guarded client is kept.

Honest severity re-assessment after that commit: connect-time IP validation now blocks the dial to a private address even when the response hook never fires, so this is no longer a live SSRF bypass. What this PR still fixes is the application-layer redirect guard itself, which remains dead code on main (response.next_request is None inside the hook — exactly the failure mode documented in redirect_target_from_response's own docstring, mirrored from the merged #56160 fix). With the hook alive again the redirect is rejected cleanly at the hop, before any connect attempt, and the defense stays layered rather than resting on the pinned-dial guard alone. Regression test unchanged: fails on current main, passes with the fix.

@solyanviktor-star
solyanviktor-star force-pushed the fix/yuanbao-ssrf-redirect-guard branch from ced1a84 to 9e48e16 Compare July 25, 2026 13:20
@alt-glitch alt-glitch added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have needs-repro Bug needs reproduction steps and removed P2 Medium — degraded but workaround exists labels Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants