Three review items addressed:
* **Veria (Medium): SSRF via redirect.** ``fetch_validated_image_bytes``
was calling ``validate_url(url)`` once and then fetching with the
default httpx client, so a 3xx to an internal IP would have been
followed unvalidated. Switched to ``async_safe_get`` (the existing
SSRF primitive used elsewhere in the codebase) which walks each
redirect hop, re-validates, and rejects redirects to blocked
networks. Default ``litellm.user_url_validation`` is True so
protection is on out of the box.
* **Greptile (P2): SVG can embed JS.** Removed ``image/svg+xml`` from
the allowed-Content-Type set. The hardcoded response media type
(``image/jpeg`` / ``image/x-icon``) means a real SVG body wouldn't
render as SVG anyway in modern browsers — the allowlist entry was
giving up XSS surface for no actual SVG-rendering benefit. If real
SVG support is wanted later, that's a deliberate feature PR with CSP
/ nosniff bundled.
* **Greptile (P2): cache-write OSError drops validated bytes.** When
the upstream fetch succeeded but ``open(cache_path, "wb")`` raised
(read-only assets dir), the bytes were discarded and the default
logo was served — a silent regression for that deployment. Now
serve the validated bytes inline via ``Response(...)`` as a fallback
before falling back to default.
Tests:
- Replaced low-level mocks of ``validate_url`` with mocks of
``async_safe_get`` directly, exercising the helper's contract
rather than the SSRF primitive's internals.
- New ``test_rejects_svg_content_type`` confirms SVG is blocked.
- ``test_get_image_cache_logic`` fixture now sets
``mock_response.is_redirect = False`` so ``async_safe_get`` doesn't
treat the Mock's truthy attribute as a redirect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three CI failures from the previous push, all addressed:
* ``lint`` (mypy): ``async_client.get(url, **request_kwargs)`` confused
mypy because ``AsyncHTTPHandler.get``'s second positional arg is typed
``bool | None``. Switched to an explicit branch:
``await async_client.get(rewritten_url, headers={"host": host_header})``
for the HTTP-rewritten case, plain ``get(rewritten_url)`` otherwise.
* ``proxy-infra`` /
``test_get_image_custom_local_logo_bypasses_cache``: the existing
test set ``UI_LOGO_PATH=/app/custom_logo.jpg`` with no
``LITELLM_ASSETS_PATH``, asserting the path was served verbatim. That
was the LFI behaviour the new path-containment guard closes. Updated
the test to set ``LITELLM_ASSETS_PATH=/app`` so the path is inside an
allowed root, and patched the helper's ``realpath`` / ``isfile`` to
go along with the mocked filesystem. Test intent (bypass cache when
``UI_LOGO_PATH`` is local) is preserved.
* ``auth-and-jwt`` / ``test_get_image_cache_logic``: existing test
built a ``Mock`` response without ``headers``, so the new
Content-Type check tripped on ``Mock().split(";")[0]``. Two fixes:
1. Set ``mock_response.headers = {"content-type": "image/jpeg"}``
on the test (matches the real upstream contract — a logo CDN
always sets a Content-Type).
2. Make ``fetch_validated_image_bytes`` defensive: if the
Content-Type header is missing or non-string, treat as non-image
and fall back to default. Closes a subtle hole — pre-fix, an
upstream that omits Content-Type entirely would have served
arbitrary bytes under the ``image/jpeg`` wrapper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
user