Repository navigation
fix(llm): reject URL attachments whose extension names a non-image type - #1676
Conversation
A URL attachment with an extension outside the supported list, such as .mp3, .mp4 or .docx, was sent to the provider as an image_url. Attachment now raises a ValueError when stdlib mimetypes maps the extension to an audio, video, text or Office document type. Extension-less URLs and other types still pass through as images, as documented, so dynamic image handlers (.php, .ashx, .aspx) keep working. Closes #1645
|
PDFs look fine to me. One edge case worth knowing about: I don't think this blocks the PR, since it's rare and the workaround is easy. If you want to cover it, you could let the probe decide when |
Detailed! Thanks! Rather than letting the probe decide, I've added a short list of server-script extensions (.php, .asp, .aspx, .ashx, .jsp, .cgi, .pl, .py) that skip the check and behave exactly as on main. That covers /img.pl with trust_urls=False and download.pl with trust_urls=True, and .mp3 still raises without a request. These files are so standard I do not expect them to have a negative impact. |
mimetypes can map .pl, .py and other script extensions to text/*, which rejected CGI-style image and PDF endpoints. Those extensions now skip the lookup and keep the pass-through and HEAD-probe behaviour. Also complete the Attachment Raises docstring.
#1677) ## 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 - [x] Bug fix - [ ] Feature - [ ] Breaking change - [x] Docs - [ ] Refactor / chore / build / tests ## Checklist - [x] Lint & format pass (`ruff check . && ruff format .`) - [x] Tests added/updated and pass locally (`pytest tests`) - [x] 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 #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.
…l-attachments # Conflicts: # packages/railtracks/src/railtracks/llm/message.py
Amir-R25
left a comment
Amir-R25
left a comment
There was a problem hiding this comment.
The extension-less and server-script handling looks right to me. The main issue is in the first comment: some of the types this means to reject (.docx, .xlsx, .m4a, ...) are only rejected on machines that have a system MIME file.
Amir-R25
left a comment
Amir-R25
left a comment
There was a problem hiding this comment.
Thanks Guan, posted some review comments, none are blocking, leave it to you decide which to take on :)
…e decide Replace the host-dependent mimetypes lookup with a fixed set of audio, video, text and Office extensions, so .docx, .xlsx and .m4a are rejected on every Python version and host. With trust_urls=True the HEAD probe now runs first, so thumbnail endpoints such as clip.mp4?video-thumbnail=auto that serve an image are accepted again. Document the behaviour and the ValueError on UserMessage.
|
Wonder! Thank for the review @Amir-R25 |
Summary
Closes #1645. Part of #1347.
A URL attachment whose extension is not in the supported list was sent to the provider as an
image_url, sohttps://example.com/call.mp3reached the model as an "image".Attachment._init_urlnow raises aValueErrorwhen the URL's extension is in a fixed set of audio, video, text and Office document extensions (_NON_IMAGE_EXTENSIONS, e.g..mp3,.mp4,.txt,.docx). The error matches the existing one for local files ("Unsupported attachment format: ...").With
trust_urls=True, the HEAD probe runs first and decides: a URL such asclip.mp4?video-thumbnail=autothat servesimage/jpegis accepted, and one that reports any other type, or whose probe fails, raises.Unchanged on purpose:
Attachmentdocstring documents andexamples/tutorials/pv_xray_agent.pyrelies on..php,.aspx,.pl, ...) are not in the set, so/img.pl?id=3and adownload.plthat serves a PDF behave exactly as onmain.A fixed set is used instead of stdlib
mimetypes(suggested in the issue) because themimetypestable depends on the host: on Python 3.10 to 3.12 without a system MIME file (slim containers),.docx,.xlsx,.m4a,.flac,.oggand.mkvare unknown, and other libraries can change the table at runtime withmimetypes.add_type.Type of change
Checklist
ruff check . && ruff format .)pytest tests)Notes
Tests (
test_message.py): non-image URLs raise (.mp3,.mp4,.wav,.docx,.xlsx,.m4a,.txt, uppercase, with a query string); extension-less,.php,.ashx,.pland.svgURLs still pass; withtrust_urls=Truea non-image extension is accepted when the probe reports an image and raises when it reportsaudio/mpegor fails;UserMessagesurfaces the error.UserMessage'strust_urlsdocstring and the modalities docs page describe the new behaviour.Corner cases left out, and why:
.zip,.json,.xml,.rtf,.odt, ...) still pass as images, as onmain. Including them risks breaking dynamic image endpoints; classifying them properly needs an explicitmime_type=onAttachment, planned under [Epic] Rich media support #1347..svg,.bmp,.tiff,.heic,.avif) still pass through without amime_type. Whether a provider accepts them is a per-provider question, planned as allowlists under [Epic] Rich media support #1347.trust_urls=True, an extension-less URL that the probe identifies asaudio/*orvideo/*is still sent as an image, as onmain.trust_urls=False: a URL whose path ends in, say,.mp4but serves an image now raises. The workaround istrust_urls=True(the probe decides) or passing the bytes directly.