Skip to content

fix(llm): cap in-process PDF downloads and correct the trust_urls docs - #1677

Merged
CoronRing merged 7 commits into
mainfrom
fix/1646-trust-urls-download-cap
Oct 16, 2026
Merged

CoronRing merged 7 commits into
mainfrom
fix/1646-trust-urls-download-cap

Conversation

@CoronRing

@CoronRing CoronRing commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Closes #1646. Part of #1347.

Download cap. With trust_urls=True, a PDF URL is downloaded in-process by encode(), which used a single unbounded read(). A large or never-ending response was held fully in memory. encode() now reads the body in 64 KB chunks and raises a ValueError as 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 declared Content-Length also raises, so a cut-off PDF is not sent to the provider. The limit is a new attachment_max_mb argument on Attachment and UserMessage (32 MB by default), passed through the same way as attachment_timeout. It must be positive and finite; other values raise a ValueError, checked in encode() and again when the Attachment is 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_urls does not control that:

  • Anthropic: http:// image URLs (https:// URLs are passed through).
  • Gemini (Google AI Studio path, which GeminiLLM uses): every image URL, http:// and https://.

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 the trust_urls docstring notes it does not cover litellm's downloads. The pdf_timeout docs snippet shows the new argument.

Type of change

  • Bug fix
  • Feature
  • Breaking change
  • Docs
  • Refactor / chore / build / tests

Checklist

  • Lint & format pass (ruff check . && ruff format .)
  • Tests added/updated and pass locally (pytest tests)
  • Docs updated if user-facing behavior changed
  • Breaking changes include migration notes

Notes

Tests: test_encoding.py covers 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 real http.client.HTTPResponse over a BufferedReader, as a socket would: a body with no Content-Length under a 1,000,000 MB limit (on the previous read this raised MemoryError), a body cut short of its Content-Length (raises), and a complete one (encoded). test_message.py extends the existing threading test so attachment_max_mb reaches encode(), and rejects the same invalid values on Attachment. Unit and integration suites pass locally, except the two MCP server tests whose fixture needs pip, which my uv environment does not have. mkdocs build --strict passes, and mypy on docs/scripts/multimodal.py passes (and flags the new argument against main, so the check is real).

Behaviour change: a trust_urls=True PDF URL larger than 32 MB used to be downloaded and sent; it now raises unless attachment_max_mb is 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):

  • litellm's own downloads are documented, not prevented or capped. Stopping them means routing those URLs through a guarded Railtracks fetcher.
  • Redirects are still followed by urllib, including to internal hosts. That is part of the SSRF surface, along with no blocking of private or link-local addresses.
  • The timeout is per socket operation, not a total deadline. A server that trickles bytes can keep the download alive for a long time, though it can no longer exceed the size cap. The chunked read loop is where a total deadline would go.
  • The cap is on raw bytes; base64 adds about a third, so a file just under the cap can still exceed a provider's request limit. Per-provider limits are planned under [Epic] Rich media support #1347.
  • Local files are not capped: they are the developer's own files, not a remote input.
  • The Content-Length header 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.
  • Error messages include the full URL, as the existing "Failed to encode URL" message already did, so a signed URL's query string can reach logs. Redacting it belongs with the guarded fetcher.

Merge note: this PR and #1676 (from #1645) both add a docstring line right after attachment_timeout in Attachment.__init__, so whichever merges second needs a trivial rebase of that one hunk. Nothing else overlaps.

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

CoronRing commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator Author

Scope notes for reviewers

This 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

  • b77f0410: attachment_max_mb must be positive and finite. A negative value made the capped read unbounded, and inf overflowed int(). It raises a ValueError in Attachment.__init__ and is covered by a parametrized test (0, -1, inf, NaN). The docs sentence about Railtracks' own requests is narrowed: with trust_urls=True, an image URL without a recognized extension gets a HEAD probe.
  • e88e43e3: the warning about litellm's downloads no longer names providers. GeminiLLM uses litellm's Google AI Studio path, which downloads every image URL, not only extension-less ones, so a per-provider list goes stale with litellm versions.

Known gaps, deferred to a follow-up under #1347

1. litellm still downloads some image URLs in our process, whatever trust_urls says. Today that covers every Gemini image URL and Anthropic http:// URLs (litellm 1.102.1). This PR documents it but does not prevent it.

  • Why not configure litellm? Its only switches (user_url_validation, user_url_allowed_hosts, MAX_IMAGE_URL_DOWNLOAD_SIZE_MB) are process-wide globals, while trust_urls is set per message. A library shouldn't change globals that other code in the process relies on.
  • Why not route those URLs through Railtracks' fetch now? Our fetch is not guarded yet (point 2). litellm's safe_get blocks private addresses and re-checks redirects, so moving Gemini downloads to our urllib call would make trust_urls=True less safe than it is today.

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 169.254.169.254, the cloud metadata service. Today the fetch follows redirects anywhere, accepts ftp/ftps, honours HTTP(S)_PROXY, and does not check the resolved address.

Follow-up plan: two PRs, guarded fetcher first.

PR What Estimate
A. Guarded fetcher One fetch for the HEAD probe, the PDF download and later image downloads. http/https only. Default-deny any address that is not ip.is_global, which catches CGNAT, 0.0.0.0, reserved ranges and IPv4-mapped IPv6 (unwrapped explicitly). Azure's 168.63.129.16 is also blocked. Connect to the IP that was checked while verifying TLS against the hostname, which closes DNS rebinding (a second DNS answer pointing somewhere else). Follow redirects by hand with a hop limit, re-checking each hop. Proxies are off for this fetch, or the behaviour is documented. Keeps this PR's size cap. Strips query strings from URLs in error messages and logs, so signed URLs don't leak. Our own code, with litellm's URL-validation test cases copied in as tests 100 to 150 lines plus tests
B. Single fetch policy Each provider declares which image URLs it takes as-is (pass-through by default; Anthropic https only; Gemini and Ollama none). Railtracks downloads the rest through A when trust_urls=True and raises otherwise. The download is cached on the Attachment, so agent loops fetch once instead of every turn about 80 lines plus tests

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:

  • Gemini image URLs under the default trust_urls=False work today because litellm downloads them silently. They get one release of warn_pending_change (railtracks.utils.deprecation, a FutureWarning, which Python shows by default, unlike DeprecationWarning) before they start raising. The warning fires only where litellm would actually download.
  • Drift tests cover every provider, including the pass-through ones (OpenAI, OpenAI-compatible, Hugging Face, Azure, Portkey). Each runs litellm's message conversion with downloads patched out and fails if anything is fetched. A litellm upgrade would most likely break the promise on a pass-through provider, since we never inspect those URLs.
  • The docs promise is "nothing in your process downloads a URL", said in those words. A local OpenAI-compatible server (vLLM, LM Studio) still fetches the URL from the same machine and network, which the docs will state.
  • Saved sessions: UserMessage.encode() writes only the modality, type and URL, so neither Attachment.encoding nor the new cache is persisted. PR B adds a test to keep it that way.

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

Copy link
Copy Markdown
Collaborator Author

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 Content-Length peaked at 32.1 MB under the default cap, a 1,000,000 MB cap raised MemoryError, and a body cut short of its Content-Length was encoded without error where a plain read() raises IncompleteRead.

Fixed in 468af8c:

  • Chunked read. The URL download moved into _download_capped, which reads 64 KB chunks and raises once the total passes the cap, as you suggested. This is also where a total deadline would go later.
  • Truncation. After the loop, a remaining length above 0 raises ValueError: ... ended N bytes short of its Content-Length. The check requires an int, since ftp:// responses have no length and mocked responses return a mock. Chunked responses that are cut short already raise IncompleteRead.
  • Validation in one place. validate_max_mb lives in encoding.py. encode() calls it first, and Attachment calls it too, so an invalid value still fails at construction for attachments that never download.
  • UserMessage docstring now says "Must be positive and finite".
  • URLs in error messages: left as is, consistent with the existing message. Redacting query strings is added to the guarded-fetcher follow-up.

Tests: test_url_read_is_bounded now asserts every read passes a size of at most one chunk. Three new tests parse raw HTTP through a real http.client.HTTPResponse over a BufferedReader, so they exercise the stdlib behaviour you described rather than a mock: no Content-Length under a 1,000,000 MB cap, a truncated body, and a complete one. The max_mb validation test patches urlopen and asserts it is never called. All 7 new or changed cases fail against the previous commit. The PR description is updated to match.

@CoronRing
CoronRing marked this pull request as ready for review October 14, 2026 08:07
Amir-R25
Amir-R25 previously approved these changes Oct 14, 2026
CoronRing and others added 2 commits October 14, 2026 17:44
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).
@CoronRing

Copy link
Copy Markdown
Collaborator Author

@Amir-R25 Sorry, noticed a new edge with the method throwing http.client.IncompleteRead. Patched with 5 lines but need approval again

@CoronRing
CoronRing requested a review from Amir-R25 October 15, 2026 01:17
@CoronRing
CoronRing merged commit 3c44de7 into main Oct 16, 2026
18 checks passed
@CoronRing
CoronRing deleted the fix/1646-trust-urls-download-cap branch October 16, 2026 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trust_urls docs overstate the SSRF guarantee; PDF download has no size cap

2 participants