fix(download): remove the download_queue REST API; block non-public URLs for server-side downloads - #9492
Conversation
…-public URLs The download endpoint accepted an arbitrary http(s) source and a relative destination from any authenticated user. Two problems followed. The destination was resolved against the server process's working directory rather than the download cache, so the caller chose where on the filesystem the download landed. An existing file at that path is unlinked and replaced, not skipped. In a source install started from the root directory that reaches `custom_nodes_dir`, whose top-level packages are imported at startup; in the container image the working directory holds the application's own package and the built frontend. The source URL was fetched with no restriction on where it pointed, so the job could be used to reach loopback services, private-network hosts and the cloud metadata endpoint. Redirects were followed unchecked, so a public URL could bounce the request onto a private address. Changes: - Anchor `dest` under `download_cache_path` and check containment after resolving, so the working directory no longer takes part. - Add `invokeai/app/util/ssrf.py`. A session built by `build_guarded_session()` validates the peer address of every socket before the request is written; checking the connected socket is what makes this hold against DNS rebinding and against host spellings that `requests` percent-decodes and we do not. `validate_download_url()` is a cheap up-front check that keeps the API's errors useful without being relied on alone. - Re-check every redirect hop through a response hook. - Require admin on all download_queue routes. The queue is server-wide: its jobs carry remote URLs and local paths, and cancelling one affects whoever started it. The web client does not use these routes. - Reject Content-Disposition and URL-derived filenames that are not a single safe path component. `..\evil`, `C:evil` and a trailing `..` segment all escaped the destination directory. - Add `allow_private_download_urls` for installs that mirror models on their own network. Warn at startup when a proxy is configured, since address policy belongs to the proxy in that case. Single-user mode is unaffected: `AdminUserOrDefault` resolves to the system administrator when multiuser is off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FindingsHigh: Chain:
The docstring at Minimum viable mitigation inside this PR's scope: reject a To expose this issue, add a test that writes a file into Medium:
Trigger: a caller submits a URL whose hostname is delegated to an unresponsive authoritative nameserver. The resolver blocks for its full retry budget (glibc default 5s x 2 attempts per nameserver) while the event loop is held, so every other in-flight HTTP request and every socket.io event for every user is stalled. Repeat the POST to hold it indefinitely. Note this is reachable without any credentials in the default single-user configuration: The repo already has the idiom for this: To expose this issue, add a test that monkeypatches Medium:
The
Security impact is nil (urllib3 shares the resolver, so it cannot dial these spellings either), but the assertion is a portability defect and the guard does silently degrade. The equivalent router cases pass only incidentally: Medium: the admin-gating of the router does not achieve its stated confidentiality goal, because the same fields are still broadcast to every connected socket. The PR justifies moving
So a non-admin in multiuser mode cannot To expose this issue, add a test that asserts Medium: no test binds
Same gap for the opt-in branch: nothing asserts that To expose this issue, add a test that constructs Low:
Low:
Open Questions
VerificationReviewed PR #9492 ( Ran locally on Windows 10 / Python 3.11 / urllib3 2.7.0 / requests 2.34.2:
Static checks performed:
Residual Risk
|
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
-
invokeai/app/api/routers/download_queue.py:34-38,62: Admin can place an unverified payload in the exact cache path consumed bydownload_and_cache_model; non-admin inference then loads it viatorch.load/torch.jit.load. Test: download a malicious archive todownload_cache_path / slugify(source), then assert cache consumers reject it. -
invokeai/app/api/routers/download_queue.py:106-111: Synchronoussocket.getaddrinfo()runs insideasync def, allowing a slow resolver to stall the entire event loop. Test: delay_resolve, issue POST concurrently with a trivial request, assert the trivial request completes before DNS returns.
Other findings/issues:
-
invokeai/app/api/sockets.py:610-612: Download events still broadcast signed source URLs and absolute filesystem paths to non-admin sockets, defeating the route's stated confidentiality goal. Test: emitDownloadStartedEventin multiuser mode and assert it targetsadmin/authorized rooms. -
invokeai/app/util/ssrf.py:265-267,291-305: WithHTTP_PROXY/ALL_PROXY, proxy-side DNS can resolve attacker-controlled names to private hosts while the socket guard sees only the proxy. Test: route a hostname unresolved locally through a test proxy to loopback and assert the request is blocked. Expected docs:docs/src/content/docs/features/Multi-User Mode/admin-guide.mdxshould state proxy-side enforcement is required. -
invokeai/app/services/download/download_default.py:615-620: A rejected redirect raises from astream=Trueresponse hook without closing the response, leaking connections on repeated attempts. Test: custom adapter returns a 302 with closableraw; assertraw.close()beforeUnsafeDownloadURLExceptionpropagates. -
invokeai/app/util/ssrf.py:169-176: The API returns resolved private IPs in 400 responses, exposing internal DNS data. Test: mock_resolveas10.0.0.7, call the route, and assert the response omits the address.
Suggestions:
-
Consider retaining
invokeai/app/util/ssrf.py:130-166and its regression cases; pure parsing fixes octal, decimal, and hex IPv4 forms independent of platform resolver behavior. -
Consider adding a production-wiring test for
DownloadQueueService()withoutrequests_session; current service tests inject sessions and could miss removal of the guarded adapter.
|
@lstein @Pfannkuchensack Fixed since
|
FindingsHigh: The revision adds
Observed locally, and it is a pure literal-parse decision with no resolver or network involvement, so it reproduces on ubuntu, macOS and windows alike: This is not just a wrong assertion, it is an unresolved spec conflict inside the module. Decide one way: either keep the blanket block and delete the new test, or make Medium: The stated intent is to stop ambient proxies from moving destination resolution out of process. if self.trust_env:
# Set environment's proxies.
...
# Look for requests environment configuration
# and be compatible with cURL.
if verify is True or verify is None:
verify = (
os.environ.get("REQUESTS_CA_BUNDLE")
or os.environ.get("CURL_CA_BUNDLE")
or verify
)The CA-bundle lookup is inside the same guard. Chain:
Fix surgically: keep To expose this issue, add a test that sets Medium: after
Note this also reverses the previous revision's own stated reasoning without a changelog entry: "Setting it to Low:
On glibc the same URLs are blocked, because Exploitability is nil today - urllib3 calls the same To expose this issue, add Low: the new event-loop regression test is wall-clock-based and covers only half the fix.
Two coverage gaps in the same test:
Open Questions
VerificationReviewed the updated head All six findings from the previous round were addressed, and I re-verified each against the code rather than the commit message:
Local run on Windows 10 / Python 3.11 / urllib3 2.7.0 / requests 2.34.2, across Environment note affecting confidence: my first run showed three additional failures in Checked and found benign:
Residual Risk
|
|
@Pfannkuchensack @lstein Fixed since
|
`_api_job` hands the API a relativised `dest`, which pydantic serialises with `str()`. On Windows that yields `models\sd15.safetensors` for a request that submitted `models/sd15.safetensors`, so the value a client gets back depends on the server OS -- and `test_download_accepts_relative_dest` fails on both windows-cpu CI legs. Serialise `dest` and `download_path` with `as_posix()`, matching what the download events in `events_common.py` already do. The JSON schema is unchanged: both fields are still plain strings, so no artifact regeneration is needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review — head
|
JPPhoto
left a comment
There was a problem hiding this comment.
There are no merge blockers!
I found one more thing to address:
invokeai/app/services/download/download_default.py:79-80: An injected ordinaryrequests.Sessionbypasses the socket SSRF guard when private URLs are disabled; only DNS preflight remains. DNS rebinding can therefore reach loopback. Test: injectrequests.Session(), make_resolve()return a public address forlocalhost, and request a local server.- Instead of implicitly trusting injected sessions, require an explicit unsafe-session opt-in or apply the guarded adapter whenever private URLs are disabled.
Smoke-tested removal supersedes the confinement approach for the router itself: the /api/v1/download_queue/* endpoints were a thin remote-control surface over the internal DownloadQueueService with no first-party consumer (the frontend only ever saw them as generated schema types; model install drives the same service in-process), and the enqueue route existed mostly as SSRF/dest-traversal attack surface. Deleting it removes the risk rather than fencing it. Everything protecting downloads the server performs on its own behalf stays: the guarded requests session (socket-peer public-address enforcement), per-connect URL validation and per-redirect-hop re-checks in DownloadQueueService, the server-derived-filename check, the allow_private_download_urls / download_proxy settings with proxy credential redaction, and admin-room scoping of download/model events. DownloadJob remains in the OpenAPI schema via ModelInstallJob.download_parts. schema.ts and openapi.json regenerated with the CI pipelines (typegen + prettier pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…is set Surfaced by adversarial review of the router-removal commit: with both settings configured, the private-opt-in branch built a bare Session() that never received the explicit proxy — downloads went direct (or via ambient *_PROXY, the opposite of the documented policy) with no hint. The proxy is applied per request rather than on the session because request-level proxies are the only kind that take precedence over ambient *_PROXY variables in a plain Session (environment proxies are folded into the request dict with setdefault, and that dict wins the merge against session.proxies). Regression test verified to fail against the previous branch logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Course change per maintainer decision, pushed as 3facee9 + 27e3fe1: the |
Summary
Fixes a reported security issue in
POST /api/v1/download_queue/i/, reproduced againstmainat 45ad17b.In multiuser mode any authenticated non-admin could supply an arbitrary http(s)
sourceand a relativedest. Two things followed:Unconfined destination.
destwas resolved against the server process's working directory instead of the download cache, so the caller chose where the file landed. An existing file at that path is unlinked and replaced, not skipped. The working directory is not arbitrary — in a source install started from the root directory it containscustom_nodes_dir, whose top-level packages are imported at startup; in the container image (WORKDIR ${INVOKEAI_SRC}) it contains theinvokeai/package and the built frontend. So the practical impact is code execution on restart, or replacing frontend assets served to every user.Unrestricted source. The URL was fetched with no restriction on where it resolved, so the job could be used to reach loopback services, private-network hosts and the cloud metadata endpoint. Redirects were also followed unchecked, which the original report did not cover — a public URL returning a 302 to a private address was followed and its body fetched.
Changes
The
/api/v1/download_queue/*router is removed entirely. Earlier revisions of this PR confined it (admin-only, destination anchored under a dedicated directory, URL validation on enqueue); after smoke-testing the removal we are dropping it instead. The router was a thin remote control over the internalDownloadQueueServicewith no first-party consumer — the web client never called it (its only trace was generated schema types), and model install drives the same service in-process through/api/v2/models/install. Deleting the endpoint removes the destination-traversal surface outright instead of fencing it.DownloadJoblegitimately remains in the OpenAPI schema viaModelInstallJob.download_parts;schema.tsandopenapi.jsonare regenerated with the CI pipelines.The SSRF protections stay, because the server still downloads user-supplied URLs on its own behalf (model installs from URLs being the main path):
invokeai/app/util/ssrf.py.build_guarded_session()returns arequests.Sessionwhose adapter validates the peer address of every socket before any request byte is written.validate_download_url()is a cheap up-front check for good error messages.DownloadQueueServicebuilds its own sessions guarded; a caller-supplied session (tests) is left as given.requestsresponse hook, and the URL is re-checked at connect time, not just submit time...\evil,C:evil(drive-relative) and a trailing..URL segment all escaped the destination directory.allow_private_download_urls(defaultfalse) for installs that mirror models on their own network.HTTP_PROXY/HTTPS_PROXY/ALL_PROXY(resolved withtrust_env=False) so an environment proxy cannot route around the socket guard; an explicitdownload_proxysetting is provided for mandatory-egress environments, with a startup warning that address policy then belongs to the proxy. Proxy credentials are redacted from the runtime-config endpoint.download_proxyis honored withallow_private_download_urlsenabled too (applied per request in that mode, the only level ambient variables cannot override).AdminUserOrDefaultsemantics).Why the guard is at the socket layer
Two earlier iterations of this fix validated the URL string, and both were bypassable:
requestsruns URLs throughrequote_uri(), which percent-decodes unreserved characters in the host before connecting. A check onurlsplit().hostnametherefore validates a different string than the one dialled.getaddrinfoindependently.Checking
sock.getpeername()is the only check that cannot be desynchronised from what the client actually does. The up-front check is kept because it gives the API a useful 400 and avoids turning the socket layer into a port-existence oracle, but it is explicitly not load-bearing.Address classification is worth a careful look during review.
not ip.is_globalalone is not safe: for IPv6, CPython definesis_globalasnot is_private, and the IPv6 private list excludes reserved/unallocated space — so NAT64 (64:ff9b::/96), IPv4-compatible (::/8) and4000::/3all reportis_global == True. Converselyis_private/is_reservedalone miss IPv4100.64.0.0/10. The predicate needs all three ofnot is_global,is_reserved,is_multicast, applied to the address and to every IPv4 it can wrap (v4-mapped, 6to4, Teredo, ISATAP) — yielding rather than substituting, so a wrapper with a public payload is still judged as a wrapper.Verification
tests/minustests/backend): 2828 passed, 0 failures, including router, download-service, model-install, SSRF and socket.io suites. Lint (ruff 0.11.2) andtscclean./api/v1/download_queue/*returns 404, remaining routes intact; frontend smoke test of the full app passed on the equivalent standalone removal branch.https://example.comfetch through the guarded session returns 200, and Range/streaming/redirect behaviour is unchanged.Notes for review
download_proxyexplicitly. This is deliberate: an ambient proxy silently defeats the peer-address guard, andno_proxyinteractions made the degraded mode hard to reason about.Known gap, deliberately not fixed here
download_and_cache_modelreturns cache entries unverified and consumers pass them totorch.load/torch.jit.load, so a write inside the cache — by whatever means — can still poison a cached model that any user's node may load. Removing the REST endpoint eliminates the only remote write path this PR knew about, but hash verification in the cache is still worth doing separately.Two adjacent pre-existing items, also out of scope:
external_generation/providers/alibabacloud.py::_download_imagefetches a provider-response URL with no guard, andcustom_nodes.pypassessourcestraight togit clonewithout a scheme check.🤖 Generated with Claude Code