Repository navigation
fix(llm): cap in-process PDF downloads and correct the trust_urls docs - #1677
Conversation
encode() read a URL body with a single unbounded read(), so a large or never-ending response was held fully in memory. It now reads at most the limit plus one byte and raises a ValueError past it. The limit is attachment_max_mb on Attachment and UserMessage (32 MB by default), next to attachment_timeout. The modalities page said image URLs cause no in-process network I/O. That holds for Railtracks, but litellm downloads some image URLs itself (http:// URLs for Anthropic; http:// and type-less https:// URLs for Gemini), and trust_urls does not control that. The docs now say so. Closes #1646
Gemini models on the Google AI Studio path download every image URL, not only those whose type is unclear, so a per-provider list goes stale. State the rule instead.
A negative limit made the capped read unbounded and inf overflowed int(). Also narrow the docs claim about Railtracks' own requests: with trust_urls=True, an image URL without a recognized extension gets a HEAD probe.
Scope notes for reviewersThis PR is a stopgap: it caps the PDF download Railtracks makes and corrects the docs. Below are the questions reviewers are likely to raise about URL fetching, and where each one is handled. Changes since the PR was opened
Known gaps, deferred to a follow-up under #13471. litellm still downloads some image URLs in our process, whatever
2. Railtracks' own fetch (HEAD probe and PDF download) has a size cap but no SSRF guard. SSRF (server-side request forgery) here means a user-supplied URL making our process reach internal addresses such as Follow-up plan: two PRs, guarded fetcher first.
PR A is worth landing on its own, since it also hardens today's HEAD probe and PDF download. Details for PR B that reviewers may ask about:
Out of scope for now: moving the PDF download from construction to send time so PDFs and images share one path (this changes when the error is raised), and downloading several images in parallel. |
Reading with the full cap as the size reserved that much memory up front when the server sent no Content-Length, and a large cap raised MemoryError. The body is now read in 64 KB chunks up to the cap. A body that ends before its Content-Length raises instead of being encoded. encode() validates max_mb itself, through a validator Attachment shares.
|
Thanks, all five points held up. I reproduced both "should fix" items against a local socket before changing anything: a 13-byte body with no Fixed in 468af8c:
Tests: |
A chunked PDF download that ends before its final chunk raised a raw http.client.IncompleteRead, unlike the ValueError raised for a body short of its Content-Length. Both now raise ValueError, chained to the cause. The HTTPError message uses repr(e).
|
@Amir-R25 Sorry, noticed a new edge with the method throwing http.client.IncompleteRead. Patched with 5 lines but need approval again |
Summary
Closes #1646. Part of #1347.
Download cap. With
trust_urls=True, a PDF URL is downloaded in-process byencode(), which used a single unboundedread(). A large or never-ending response was held fully in memory.encode()now reads the body in 64 KB chunks and raises aValueErroras soon as it passes the limit, so memory use stays near the actual body size whatever the limit is. A body that ends before its declaredContent-Lengthalso raises, so a cut-off PDF is not sent to the provider. The limit is a newattachment_max_mbargument onAttachmentandUserMessage(32 MB by default), passed through the same way asattachment_timeout. It must be positive and finite; other values raise aValueError, checked inencode()and again when theAttachmentis built.Docs. The modalities page said image URLs cause no in-process network I/O. That is true for Railtracks, but litellm 1.102.1 (the pinned version) downloads some image URLs itself before calling the provider, and
trust_urlsdoes not control that:http://image URLs (https://URLs are passed through).GeminiLLMuses): every image URL,http://andhttps://.The page now says this in a warning box that names no providers, since the list depends on the litellm version. It also documents
attachment_max_mb, and thetrust_urlsdocstring notes it does not cover litellm's downloads. Thepdf_timeoutdocs snippet shows the new argument.Type of change
Checklist
ruff check . && ruff format .)pytest tests)Notes
Tests:
test_encoding.pycovers a body over the limit (raises), a body exactly at the limit (encoded), that every read passes a chunk size, that local files are not limited, and that 0, -1, inf and NaN are rejected before any request. Three tests parse raw HTTP through a realhttp.client.HTTPResponseover aBufferedReader, as a socket would: a body with noContent-Lengthunder a 1,000,000 MB limit (on the previous read this raisedMemoryError), a body cut short of itsContent-Length(raises), and a complete one (encoded).test_message.pyextends the existing threading test soattachment_max_mbreachesencode(), and rejects the same invalid values onAttachment. Unit and integration suites pass locally, except the two MCP server tests whose fixture needspip, which my uv environment does not have.mkdocs build --strictpasses, and mypy ondocs/scripts/multimodal.pypasses (and flags the new argument againstmain, so the check is real).Behaviour change: a
trust_urls=TruePDF URL larger than 32 MB used to be downloaded and sent; it now raises unlessattachment_max_mbis raised. 32 MB is already at or above most providers' request limits once the file is base64-encoded.Corner cases left out, and why (the full fix is the single fetch policy planned under #1347):
urllib, including to internal hosts. That is part of the SSRF surface, along with no blocking of private or link-local addresses.Content-Lengthheader is not checked up front. The bounded read already limits the download to the cap, so a header check would only save that much traffic.Merge note: this PR and #1676 (from #1645) both add a docstring line right after
attachment_timeoutinAttachment.__init__, so whichever merges second needs a trivial rebase of that one hunk. Nothing else overlaps.