fix(external): guard AlibabaCloud image downloads - #9525
Conversation
The existing regression test patches `requests.Session.get` to raise, which a plain unguarded Session does just as happily -- so swapping `build_guarded_session()` for `requests.Session()` reopened a full loopback fetch with the whole suite green. These tests connect to a real listener instead, so the socket-level peer check is the only thing that can refuse the download. Covers the http and https paths, an IP literal, a hostname and a percent-encoded host, and asserts that the session `_download_image` actually uses is both guarded and free of ambient proxies -- the latter because a plain Session carrying the guarded adapter still honours `*_PROXY`, which moves destination resolution out of the process and leaves the peer check inspecting the proxy rather than the target. The https test needs no certificate: the peer check runs in `_new_conn()`, before the handshake, so a bare TCP listener is enough to tell a refusal apart from an `SSLError`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcFjqVzCMGwNmzsjf8hrki
lstein
left a comment
There was a problem hiding this comment.
Approving. The fix is correct, and I've pushed one commit (a21df2d) adding socket-level regression tests — details below.
What I verified
I reviewed this adversarially in a worktree, trying to bypass the guard rather than to confirm it. The security core holds up well:
- Every loopback spelling is blocked at the socket, verified against a live listener so the TCP connect actually succeeds and the peer check is what refuses: plain
127.0.0.1,localhost,%6cocalhost, decimal2130706433, octal017700000001, hex0x7f000001,[::1],[::ffff:127.0.0.1], trailing-dot, and credentials-in-URL. UnsafeDownloadURLExceptionpropagates unwrapped. It's aValueError, which sits outside urllib3'surlopenretry catch-tuple and outsideHTTPAdapter.send's translation set, so the newexcept UnsafeDownloadURLExceptionarm is live rather than dead code. Confirmed on requests 2.34.2 / urllib3 2.7.0, on both schemes.- Redirects are re-vetted per hop — I counted
check_addressfiring twice across a single 302. - Public URLs still work;
file://and malformed URLs land asInvalidSchema/MissingSchemaand convert correctly. - Scope is right:
alibabacloudis the only provider that fetches a URL — gemini, openai and seedream all takeb64_json.
Why I pushed tests
The one thing I'd have held the merge on was that test_download_image_rejects_unsafe_provider_url cannot fail if the guard is removed. It patches requests.Session.get to raise, and a plain unguarded Session does that just as happily. Replacing build_guarded_session() with requests.Session() reopened a complete loopback fetch — I confirmed the image was actually retrieved from 127.0.0.1 — with all 46 tests green.
A second adversarial pass over my own tests found a sharper version of the same problem, so the committed tests also cover it: asserting pool_classes_by_scheme["https"] is _GuardedHTTPSConnectionPool only checks a class name. Pointing that pool back at urllib3's stock HTTPSConnection reopens a full HTTPS loopback SSRF while every test in the repo stays green — and DashScope hands back https URLs in practice.
The five new tests drive real connections, and each one fails when the guard is unwired:
| Sabotage | Result |
|---|---|
build_guarded_session() → plain requests.Session() |
5 fail |
stock HTTPAdapter mounted over the guarded one |
5 fail |
_check_socket() made a no-op |
4 fail |
https pool → stock HTTPSConnection |
1 fails |
build_guarded_session returns a plain Session |
1 fails |
merge_environment_settings reverted to stock |
1 fails |
provider hand-rolls Session() + SsrfGuardedAdapter() |
1 fails |
The last three are the ambient-proxy case: a plain Session carrying the guarded adapter still honours *_PROXY, which moves destination resolution out of the process and leaves the peer check inspecting the proxy rather than the target. The test asserts against the session _download_image actually used, not against build_guarded_session being called by name, so hand-rolling an equivalent fails too.
The https test needs no certificate — the peer check runs in _new_conn() before the handshake, so a bare TCP listener distinguishes a refusal from an SSLError. Tests bind whichever loopback family localhost resolves to, so they don't depend on the runner having IPv4-first localhost; they leak no fds or threads, don't touch the network, and are order-independent (155 pass across external_generation + test_ssrf.py + download, unchanged when deselected).
Non-blocking follow-ups
None of these block the merge, but they're worth a look — happy to file issues if you'd prefer:
allow_private_download_urlsisn't honoured here —download_default.py:82respects it, this path doesn't. Verified: with it set totrue, loopback is still refused. Sinceexternal_alibabacloud_base_urlis a supported knob, pointing it at a LAN DashScope-compatible gateway now hard-fails with no opt-out. Either honour it (self._app_configis already available) or note the difference deliberately.- Proxy asymmetry inside the provider — submit (
requests.post) and poll (requests.get) still honour ambient proxies; the download no longer does. On proxied egress, generation succeeds and then the image fetch fails on a direct connect.warn_if_proxied()also isn't called, so nothing is logged to explain it. - No up-front
validate_download_url(), so this is a port-existence oracle. An open internal port yields"DashScope returned an unsafe image URL"; a closed one yields"...Connection refused"— and both reach the user via the node error.ssrf.py's module docstring names exactly this as why the up-front check exists alongside the socket guard. It also means a hostile provider response still completes a TCP connect to any internal address. One line fixes it. - Latent trap:
UnsafeDownloadURLExceptionsubclassesValueError, and_download_imagecontains a bareexcept ValueError: pass. Unreachable today (onlyint(content_length)sits inside it), but it would silently swallow the guard if a future edit moved a call in. _download_imagebuilds a fresh session per image (called per-image at both call sites), so batch requests pay a TLS handshake each.
Separately and outside this PR: test_guarded_session_is_installed_for_both_schemes in tests/app/util/test_ssrf.py has the same identity-only HTTPS blind spot, so the module-level suite still misses the sabotage in row 4 above. Worth closing there too.
Thanks for picking up follow-up item 4 of #9493 — this closes a gap I'd flagged as out of scope during the download-queue work.
Summary
requests.get()pathThe submit and task-poll requests are unchanged; this patch only hardens the follow-up fetch of provider-generated image URLs.
Related Issues / Discussions
Refs #9493, specifically follow-up item 4.
This PR intentionally does not close #9493 because that issue tracks several independent security follow-ups.
QA Instructions
mainbecause_download_image()calls unguardedrequests.get()python -m pytest tests/app/services/external_generation/test_alibabacloud_provider.py -q— 9 passedpython -m pytest tests/app/services/external_generation -q— 46 passedgit diff --checkpassedReviewer focus:
build_guarded_session()intentionally ignores ambient proxy variables so destination resolution remains in-process and the connected peer can be checked. This matches the existing download security boundary.Merge Plan
No special merge steps are required. The patch is limited to the AlibabaCloud provider and its focused tests; it changes no schemas, generated assets, configuration, or release metadata.
Checklist