From a8a97c358feee4ee546670691f6f72e8812f9d3a Mon Sep 17 00:00:00 2001 From: heathley Date: Wed, 1 Jul 2026 02:22:29 -0700 Subject: [PATCH] fix(matrix): block unsafe image redirects per-hop Matrix outbound image downloads validated only the final URL after following redirects, so a public URL that 302-redirects to loopback / private-network / cloud-metadata endpoints had already connected to the unsafe hop before the check ran. Re-validate every redirect hop before following it: - aiohttp path resolves redirects manually with allow_redirects=False, validating each Location via is_safe_url (aiohttp can't use the httpx response event hook). - httpx fallback installs the shared _ssrf_redirect_guard event hook. Regression tests cover per-hop blocking of an unsafe redirect, following a safe redirect chain, and httpx guard wiring. --- plugins/platforms/matrix/adapter.py | 64 ++++++++---- tests/gateway/test_matrix.py | 157 +++++++++++++++++++++++++--- 2 files changed, 185 insertions(+), 36 deletions(-) diff --git a/plugins/platforms/matrix/adapter.py b/plugins/platforms/matrix/adapter.py index adb880b4d..68dfda67c 100644 --- a/plugins/platforms/matrix/adapter.py +++ b/plugins/platforms/matrix/adapter.py @@ -57,7 +57,7 @@ import mimetypes import os import re import time -from urllib.parse import urlsplit, urlunsplit +from urllib.parse import urljoin, urlsplit, urlunsplit from dataclasses import dataclass, field from html import escape as _html_escape @@ -128,6 +128,7 @@ from gateway.platforms.base import ( SendResult, resolve_proxy_url, proxy_kwargs_for_aiohttp, + _ssrf_redirect_guard, ) from gateway.platforms.helpers import ThreadParticipationTracker @@ -1766,36 +1767,57 @@ class MatrixAdapter(BasePlatformAdapter): fname = url.rsplit("/", 1)[-1].split("?")[0] or "image.png" + def _safe_redirect_target(current_url: str, location: str) -> str: + """Resolve a redirect Location and re-validate it against SSRF policy. + + A public-looking URL can 302-redirect the gateway toward loopback, + private-network, or cloud-metadata endpoints. Validating only the + final URL is insufficient because the connection to the unsafe hop + has already been made. Re-check every hop before following it. + """ + next_url = urljoin(current_url, location) + if not is_safe_url(next_url): + raise ValueError("blocked unsafe redirect URL") + return next_url + try: import aiohttp as _aiohttp _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(self._proxy_url) async with _aiohttp.ClientSession(**_sess_kw) as http: - async with http.get( - url, - timeout=_aiohttp.ClientTimeout(total=30), - allow_redirects=True, - **_req_kw, - ) as resp: - resp.raise_for_status() - if not is_safe_url(str(resp.url)): - raise ValueError("blocked unsafe redirect URL") - _check_content_length(resp.headers) - parts: list[bytes] = [] - total = 0 - async for chunk in resp.content.iter_chunked(65536): - total = _append_chunk(parts, total, bytes(chunk)) - ct = _check_image_content_type( - getattr(resp, "content_type", None) - or resp.headers.get("content-type", "application/octet-stream") - ) - return b"".join(parts), ct, fname + fetch_url = url + for _ in range(20): + async with http.get( + fetch_url, + timeout=_aiohttp.ClientTimeout(total=30), + allow_redirects=False, + **_req_kw, + ) as resp: + if resp.status in {301, 302, 303, 307, 308}: + location = resp.headers.get("Location") + if not location: + raise ValueError("redirect missing Location") + fetch_url = _safe_redirect_target(fetch_url, location) + continue + resp.raise_for_status() + _check_content_length(resp.headers) + parts: list[bytes] = [] + total = 0 + async for chunk in resp.content.iter_chunked(65536): + total = _append_chunk(parts, total, bytes(chunk)) + ct = _check_image_content_type( + getattr(resp, "content_type", None) + or resp.headers.get("content-type", "application/octet-stream") + ) + return b"".join(parts), ct, fname + raise ValueError("too many redirects") except ImportError: import httpx _httpx_kw: dict = {} if self._proxy_url: _httpx_kw["proxy"] = self._proxy_url + _httpx_kw["event_hooks"] = {"response": [_ssrf_redirect_guard]} async with httpx.AsyncClient(**_httpx_kw) as http: async with http.stream( "GET", @@ -1804,8 +1826,6 @@ class MatrixAdapter(BasePlatformAdapter): timeout=30, ) as resp: resp.raise_for_status() - if not is_safe_url(str(resp.url)): - raise ValueError("blocked unsafe redirect URL") _check_content_length(resp.headers) parts: list[bytes] = [] total = 0 diff --git a/tests/gateway/test_matrix.py b/tests/gateway/test_matrix.py index 7481e66b3..06e0cbae0 100644 --- a/tests/gateway/test_matrix.py +++ b/tests/gateway/test_matrix.py @@ -3385,6 +3385,7 @@ class TestMatrixImageOnlyMediaNormalization: class _Response: url = "https://example.com/image.png" + status = 200 headers = {"Content-Length": "11"} content_type = "image/png" content = _Content() @@ -3434,6 +3435,7 @@ class TestMatrixImageOnlyMediaNormalization: class _Response: url = "https://example.com/image.png" + status = 200 headers = {} content_type = "image/png" content = _Content() @@ -3472,15 +3474,82 @@ class TestMatrixImageOnlyMediaNormalization: @pytest.mark.asyncio async def test_external_media_download_rejects_unsafe_redirect(self, monkeypatch): + """A 302 to a private/loopback target must be blocked per-hop, before + the redirect is followed (not only re-checked on the final URL).""" + import aiohttp + import tools.url_safety as url_safety + + class _RedirectResponse: + status = 302 + headers = {"Location": "http://127.0.0.1/private.png"} + content_type = "image/png" + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args): + return None + + def raise_for_status(self): + return None + + class _Session: + def __init__(self): + self.requested = [] + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args): + return None + + def get(self, url, *_args, **_kwargs): + self.requested.append(url) + return _RedirectResponse() + + session = _Session() + monkeypatch.setattr(aiohttp, "ClientSession", lambda **_kwargs: session) + monkeypatch.setattr( + url_safety, + "is_safe_url", + lambda candidate, **_kwargs: str(candidate) == "https://example.com/image.png", + ) + + with pytest.raises(ValueError, match="unsafe redirect"): + await self.adapter._download_external_media_with_cap( + "https://example.com/image.png" + ) + + # Only the initial public URL was fetched — the loopback hop was never + # followed because it was rejected before the next GET. + assert session.requested == ["https://example.com/image.png"] + + @pytest.mark.asyncio + async def test_external_media_download_follows_safe_redirect(self, monkeypatch): + """A redirect to another allowed URL is followed and its body returned.""" import aiohttp import tools.url_safety as url_safety class _Content: async def iter_chunked(self, _size): - yield b"ok" + yield b"imgbytes" - class _Response: - url = "http://127.0.0.1/private.png" + class _RedirectResponse: + status = 302 + headers = {"Location": "https://cdn.example.com/final.png"} + content_type = "image/png" + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args): + return None + + def raise_for_status(self): + return None + + class _OkResponse: + status = 200 headers = {} content_type = "image/png" content = _Content() @@ -3495,26 +3564,85 @@ class TestMatrixImageOnlyMediaNormalization: return None class _Session: + def __init__(self): + self.requested = [] + async def __aenter__(self): return self async def __aexit__(self, *_args): return None - def get(self, *_args, **_kwargs): - return _Response() + def get(self, url, *_args, **_kwargs): + self.requested.append(url) + return _RedirectResponse() if len(self.requested) == 1 else _OkResponse() - monkeypatch.setattr(aiohttp, "ClientSession", lambda **_kwargs: _Session()) - monkeypatch.setattr( - url_safety, - "is_safe_url", - lambda candidate, **_kwargs: str(candidate) == "https://example.com/image.png", + session = _Session() + monkeypatch.setattr(aiohttp, "ClientSession", lambda **_kwargs: session) + monkeypatch.setattr(url_safety, "is_safe_url", lambda *_args, **_kwargs: True) + + data, ct, _fname = await self.adapter._download_external_media_with_cap( + "https://example.com/image.png" ) - with pytest.raises(ValueError, match="unsafe redirect"): - await self.adapter._download_external_media_with_cap( - "https://example.com/image.png" - ) + assert data == b"imgbytes" + assert ct == "image/png" + assert session.requested == [ + "https://example.com/image.png", + "https://cdn.example.com/final.png", + ] + + @pytest.mark.asyncio + async def test_external_media_download_httpx_installs_redirect_guard(self, monkeypatch): + """The httpx fallback re-checks redirect targets via the shared guard.""" + import tools.url_safety as url_safety + from gateway.platforms.base import _ssrf_redirect_guard + + clients = [] + + class _Content: + async def iter_chunked(self, _size): + yield b"ok" + + class _Response: + headers = {"content-type": "image/png"} + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args): + return None + + def raise_for_status(self): + return None + + async def aiter_bytes(self): + yield b"ok" + + class _Client: + def __init__(self, **kwargs): + self.kwargs = kwargs + clients.append(self) + + async def __aenter__(self): + return self + + async def __aexit__(self, *_args): + return None + + def stream(self, *_args, **_kwargs): + return _Response() + + monkeypatch.setattr(url_safety, "is_safe_url", lambda *_args, **_kwargs: True) + with patch.dict(sys.modules, {"aiohttp": None}): + with patch("httpx.AsyncClient", _Client): + data, ct, _fname = await self.adapter._download_external_media_with_cap( + "https://example.com/image.png" + ) + + assert data == b"ok" + assert ct == "image/png" + assert clients[0].kwargs["event_hooks"]["response"] == [_ssrf_redirect_guard] @pytest.mark.asyncio async def test_external_media_download_rejects_unsafe_initial_url(self): @@ -3534,6 +3662,7 @@ class TestMatrixImageOnlyMediaNormalization: class _Response: url = "https://example.com/image.png" + status = 200 headers = {} content_type = "text/html" content = _Content()