From caaeb755087b931b3ecc530c850c1380f1209796 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 22 Mar 2026 02:40:41 +0000 Subject: [PATCH] =?UTF-8?q?=E2=8F=BA=20Here's=20a=20summary=20of=20the=20H?= =?UTF-8?q?TTP=20client=20migration:?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completed: - Created NativeSimpleHttpClient with a HomeServer-compatible constructor that accepts both hs and explicit params - Made session creation lazy (deferred to first use) to avoid "no running event loop" during homeserver init - Updated get_file to return dict[bytes, list[bytes]] headers for backward compatibility - Added SimpleHttpClient = NativeSimpleHttpClient alias for drop-in replacement - Wired into server.py: get_simple_http_client(), get_proxied_http_client(), get_proxied_blocklisted_http_client() all return NativeSimpleHttpClient - Updated all imports: url_previewer.py, appservice/api.py, identity.py, module_api/__init__.py, matrixfederationclient.py - Fixed handlers/oidc.py to use aiohttp response API (.status, .reason, .read(), plain dict headers) - Updated test mock tests/test_utils/oidc.py to use plain dict headers and NativeFakeResponse - Created NativeFakeResponse test utility class Still using Twisted HTTP client: - ReplicationClient — routes to worker instances via TCP/UNIX sockets - matrixfederationclient.py — federation agent with SRV resolution, well-known lookup, TLS verification - connectproxyclient.py — CONNECT proxy protocol implementation - Various test files using FakeResponse (old Twisted version) --- synapse/appservice/api.py | 3 +- synapse/handlers/identity.py | 2 +- synapse/handlers/oidc.py | 34 +++----- synapse/http/matrixfederationclient.py | 2 +- synapse/http/native_client.py | 113 ++++++++++++++++--------- synapse/media/url_previewer.py | 2 +- synapse/module_api/__init__.py | 2 +- synapse/server.py | 13 +-- tests/test_utils/__init__.py | 32 +++++++ tests/test_utils/oidc.py | 42 ++++----- 10 files changed, 146 insertions(+), 99 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 2bbf77a352..f3c5270bc9 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -41,7 +41,8 @@ from synapse.appservice import ( ) from synapse.events import EventBase from synapse.events.utils import SerializeEventConfig, serialize_event -from synapse.http.client import SimpleHttpClient, is_unknown_endpoint +from synapse.http.client import is_unknown_endpoint +from synapse.http.native_client import SimpleHttpClient from synapse.logging import opentracing from synapse.metrics import SERVER_NAME_LABEL from synapse.types import DeviceListUpdates, JsonDict, JsonMapping, ThirdPartyInstanceID diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 1596c55570..85b983ab97 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -36,7 +36,7 @@ from synapse.api.errors import ( ) from synapse.api.ratelimiting import Ratelimiter from synapse.http import RequestTimedOutError -from synapse.http.client import SimpleHttpClient +from synapse.http.native_client import SimpleHttpClient from synapse.http.site import SynapseRequest from synapse.types import JsonDict, Requester from synapse.util.hash import sha256_and_url_safe_base64 diff --git a/synapse/handlers/oidc.py b/synapse/handlers/oidc.py index a6189c4508..b073fbd968 100644 --- a/synapse/handlers/oidc.py +++ b/synapse/handlers/oidc.py @@ -49,12 +49,6 @@ from pymacaroons.exceptions import ( MacaroonInvalidSignatureException, ) -try: - from twisted.web.client import readBody - from twisted.web.http_headers import Headers -except ImportError: - pass - from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.config.oidc import OidcProviderClientSecretJwtKey, OidcProviderConfig @@ -62,7 +56,6 @@ from synapse.handlers.sso import MappingException, UserAttributes from synapse.http.server import finish_request from synapse.http.servlet import parse_string from synapse.http.site import SynapseRequest -from synapse.logging.context import make_deferred_yieldable from synapse.module_api import ModuleApi from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart from synapse.util.caches.cached_call import RetryOnExceptionCachedCall @@ -760,7 +753,7 @@ class OidcProvider: token_endpoint = metadata.get("token_endpoint") raw_headers: dict[str, str] = { "Content-Type": "application/x-www-form-urlencoded", - "User-Agent": self._http_client.user_agent.decode("ascii"), + "User-Agent": self._http_client.user_agent if isinstance(self._http_client.user_agent, str) else self._http_client.user_agent.decode("ascii"), "Accept": "application/json", } @@ -777,7 +770,7 @@ class OidcProvider: uri, raw_headers, body = self._client_auth.prepare( method="POST", uri=token_endpoint, headers=raw_headers, body=body ) - headers = Headers({k: [v] for (k, v) in raw_headers.items()}) + headers = {k: v for (k, v) in raw_headers.items()} # Do the actual request # We're not using the SimpleHttpClient util methods as we don't want to @@ -791,12 +784,12 @@ class OidcProvider: # This is used in multiple error messages below status = "{code} {phrase}".format( - code=response.code, phrase=response.phrase.decode("utf-8") + code=response.status, phrase=response.reason or "" ) - resp_body = await make_deferred_yieldable(readBody(response)) + resp_body = await response.read() - if response.code >= 500: + if response.status >= 500: # In case of a server error, we should first try to decode the body # and check for an error field. If not, we respond with a generic # error message. @@ -825,7 +818,7 @@ class OidcProvider: # In case the authorization server responded with an error field, # it should be a 4xx code. If not, warn about it but don't do # anything special and report the original error message. - if response.code < 400: + if response.status < 400: logger.debug( "Invalid response from the authorization server: " 'responded with a "%s" ' @@ -840,7 +833,7 @@ class OidcProvider: # Now, this should not be an error. According to RFC6749 sec 5.1, it # should be a 200 code. We're a bit more flexible than that, and will # only throw on a 4xx code. - if response.code >= 400: + if response.status >= 400: description = ( 'Authorization server responded with a "{status}" error ' 'but did not include an "error" field in its response.'.format( @@ -870,18 +863,15 @@ class OidcProvider: resp = await self._http_client.request( "GET", metadata["userinfo_endpoint"], - headers=Headers( - {"Authorization": ["Bearer {}".format(token["access_token"])]} - ), + headers={"Authorization": "Bearer {}".format(token["access_token"])}, ) - body = await readBody(resp) + body = await resp.read() - content_type_headers = resp.headers.getRawHeaders("Content-Type") - assert content_type_headers + content_type = resp.headers.get("Content-Type", "") # We use `startswith` because the header value can contain the `charset` parameter - # even if it is useless, and Twisted doesn't take care of that for us. - if content_type_headers[0].startswith("application/jwt"): + # even if it is useless. + if content_type.startswith("application/jwt"): alg_values = metadata.get( "id_token_signing_alg_values_supported", ["RS256"] ) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 2ea58696b7..ef37ef475c 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -74,12 +74,12 @@ from synapse.http.client import ( BlocklistingAgentWrapper, BodyExceededMaxSize, ByteWriteable, - SimpleHttpClient, _make_scheduler, encode_query_args, read_body_with_max_size, read_multipart_response, ) +from synapse.http.native_client import SimpleHttpClient from synapse.http.connectproxyclient import BearerProxyCredentials from synapse.http.federation.matrix_federation_agent import MatrixFederationAgent from synapse.http.proxyagent import ProxyAgent diff --git a/synapse/http/native_client.py b/synapse/http/native_client.py index b667e17341..16434dfde1 100644 --- a/synapse/http/native_client.py +++ b/synapse/http/native_client.py @@ -14,11 +14,8 @@ """asyncio-native HTTP client using aiohttp. -Phase 4 of the Twisted → asyncio migration. Provides NativeSimpleHttpClient -as a replacement for SimpleHttpClient, using aiohttp.ClientSession instead -of treq + Twisted Agent. - -This module is unused until later phases switch callers to use it. +Provides NativeSimpleHttpClient as a replacement for SimpleHttpClient, using +aiohttp.ClientSession instead of treq + Twisted Agent. """ import asyncio @@ -28,6 +25,7 @@ import urllib.parse from http import HTTPStatus from io import BytesIO from typing import ( + TYPE_CHECKING, Any, BinaryIO, Callable, @@ -47,6 +45,9 @@ from synapse.http.client import ( redact_uri, ) +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) # Reuse metrics from the existing client module where possible @@ -141,57 +142,82 @@ class NativeSimpleHttpClient: def __init__( self, - user_agent: str, + hs_or_user_agent: "HomeServer | str", ip_allowlist: IPSet | None = None, ip_blocklist: IPSet | None = None, + use_proxy: bool = False, + # Explicit params (used when hs_or_user_agent is a string) proxy_url: str | None = None, ssl_context: ssl.SSLContext | None = None, max_connections: int = 100, connection_timeout: float = 15.0, request_timeout: float = _DEFAULT_REQUEST_TIMEOUT, + # Extra kwargs accepted for BaseHttpClient compat (ignored) + **kwargs: Any, ) -> None: """ - Args: - user_agent: User-Agent header value. - ip_allowlist: IP addresses to allow even if in blocklist. - ip_blocklist: IP addresses to disallow. - proxy_url: HTTP proxy URL (e.g., "http://proxy:8080"). - ssl_context: SSL context for TLS connections. - max_connections: Max persistent connections per host. - connection_timeout: Timeout for establishing connections. - request_timeout: Default timeout for request headers. + Can be constructed with either a HomeServer or explicit params. + + HomeServer mode (matches SimpleHttpClient API): + NativeSimpleHttpClient(hs, ip_allowlist=..., ip_blocklist=..., use_proxy=...) + + Explicit mode: + NativeSimpleHttpClient(user_agent, ip_blocklist=..., proxy_url=..., ...) """ + if isinstance(hs_or_user_agent, str): + # Explicit mode + user_agent = hs_or_user_agent + else: + # HomeServer mode + hs = hs_or_user_agent + ua = hs.version_string + user_agent = ua.decode("ascii") if isinstance(ua, bytes) else ua + max_connections = max(int(100 * hs.config.caches.global_factor), 5) + if use_proxy: + proxy_config = hs.config.server.proxy_config + if proxy_config: + # Prefer HTTPS proxy, fall back to HTTP proxy + proxy_url = proxy_config.https_proxy or proxy_config.http_proxy + self.user_agent = user_agent self._ip_allowlist = ip_allowlist self._ip_blocklist = ip_blocklist or IPSet() self._proxy_url = proxy_url self._request_timeout = request_timeout + self._ssl_context = ssl_context + self._max_connections = max_connections + self._connection_timeout = connection_timeout + self._session: aiohttp.ClientSession | None = None - # Build connector with optional IP blocklisting resolver - resolver = None - if ip_blocklist: - resolver = _BlocklistingResolver(ip_allowlist, ip_blocklist) + def _get_session(self) -> aiohttp.ClientSession: + """Lazily create the aiohttp session on first use.""" + if self._session is None: + resolver = None + if self._ip_blocklist: + resolver = _BlocklistingResolver(self._ip_allowlist, self._ip_blocklist) - connector = aiohttp.TCPConnector( - limit_per_host=max_connections, - resolver=resolver, - ssl=ssl_context or False, - ) + connector = aiohttp.TCPConnector( + limit_per_host=self._max_connections, + resolver=resolver, + ssl=self._ssl_context or False, + ) - timeout = aiohttp.ClientTimeout( - sock_connect=connection_timeout, - total=None, # We manage total timeout per-request - ) + timeout = aiohttp.ClientTimeout( + sock_connect=self._connection_timeout, + total=None, # We manage total timeout per-request + ) - self._session = aiohttp.ClientSession( - connector=connector, - timeout=timeout, - headers={"User-Agent": user_agent}, - ) + self._session = aiohttp.ClientSession( + connector=connector, + timeout=timeout, + headers={"User-Agent": self.user_agent}, + ) + return self._session async def close(self) -> None: """Close the underlying aiohttp session.""" - await self._session.close() + if self._session is not None: + await self._session.close() async def request( self, @@ -230,8 +256,9 @@ class NativeSimpleHttpClient: logger.debug("Sending request %s %s", method, redact_uri(uri)) try: + session = self._get_session() response = await asyncio.wait_for( - self._session.request( + session.request( method, uri, data=data, @@ -470,7 +497,7 @@ class NativeSimpleHttpClient: max_size: int | None = None, headers: RawHeaders | None = None, is_allowed_content_type: Callable[[str], bool] | None = None, - ) -> tuple[int, dict[str, list[str]], str, int]: + ) -> tuple[int, dict[bytes, list[bytes]], str, int]: """Download a file from a URL, streaming to output_stream. Args: @@ -482,6 +509,7 @@ class NativeSimpleHttpClient: Returns: Tuple of (length, response_headers, final_url, status_code). + Headers are bytes-keyed for compatibility with existing callers. Raises: SynapseError: On non-2xx response, oversized body, or timeout. @@ -489,9 +517,10 @@ class NativeSimpleHttpClient: h = self._build_headers(headers) response = await self.request("GET", url, headers=h) - resp_headers: dict[str, list[str]] = {} + # Build bytes-keyed headers for backward compatibility + resp_headers: dict[bytes, list[bytes]] = {} for key, value in response.headers.items(): - resp_headers.setdefault(key, []).append(value) + resp_headers.setdefault(key.encode("ascii"), []).append(value.encode("utf-8")) if response.status > 299: logger.warning("Got %d when downloading %s", response.status, url) @@ -501,8 +530,8 @@ class NativeSimpleHttpClient: Codes.UNKNOWN, ) - if is_allowed_content_type and "Content-Type" in resp_headers: - content_type = resp_headers["Content-Type"][0] + if is_allowed_content_type and b"Content-Type" in resp_headers: + content_type = resp_headers[b"Content-Type"][0].decode("ascii") if not is_allowed_content_type(content_type): raise SynapseError( HTTPStatus.BAD_GATEWAY, @@ -563,3 +592,7 @@ class NativeSimpleHttpClient: for v in raw_values: h[key_str] = v.decode("ascii") if isinstance(v, bytes) else str(v) return h + + +# Alias for drop-in replacement of synapse.http.client.SimpleHttpClient +SimpleHttpClient = NativeSimpleHttpClient diff --git a/synapse/media/url_previewer.py b/synapse/media/url_previewer.py index 8a6d1d7d34..989472d262 100644 --- a/synapse/media/url_previewer.py +++ b/synapse/media/url_previewer.py @@ -41,7 +41,7 @@ except ImportError: pass from synapse.api.errors import Codes, SynapseError -from synapse.http.client import SimpleHttpClient +from synapse.http.native_client import SimpleHttpClient from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.media._base import FileInfo, get_filename_from_headers from synapse.media.media_storage import MediaStorage, SHA256TransparentIOWriter diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index c79aefc6a9..707b3e8934 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -68,7 +68,7 @@ from synapse.handlers.auth import ( AuthHandler, ) from synapse.handlers.push_rules import RuleSpec, check_actions -from synapse.http.client import SimpleHttpClient +from synapse.http.native_client import SimpleHttpClient from synapse.http.server import ( DirectServeHtmlResource, DirectServeJsonResource, diff --git a/synapse/server.py b/synapse/server.py index b89ca02ad5..8b1fedd75c 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -147,6 +147,7 @@ from synapse.http.client import ( ReplicationClient, SimpleHttpClient, ) +from synapse.http.native_client import NativeSimpleHttpClient from synapse.http.matrixfederationclient import MatrixFederationHttpClient from synapse.logging.context import PreserveLoggingContext from synapse.media.media_repository import MediaRepository @@ -765,26 +766,26 @@ class HomeServer(metaclass=abc.ABCMeta): return RegularPolicyForHTTPS() @cache_in_self - def get_simple_http_client(self) -> SimpleHttpClient: + def get_simple_http_client(self) -> NativeSimpleHttpClient: """ An HTTP client with no special configuration. """ - return SimpleHttpClient(self) + return NativeSimpleHttpClient(self) @cache_in_self - def get_proxied_http_client(self) -> SimpleHttpClient: + def get_proxied_http_client(self) -> NativeSimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies. """ - return SimpleHttpClient(self, use_proxy=True) + return NativeSimpleHttpClient(self, use_proxy=True) @cache_in_self - def get_proxied_blocklisted_http_client(self) -> SimpleHttpClient: + def get_proxied_blocklisted_http_client(self) -> NativeSimpleHttpClient: """ An HTTP client that uses configured HTTP(S) proxies and blocks IPs based on the configured IP ranges. """ - return SimpleHttpClient( + return NativeSimpleHttpClient( self, ip_allowlist=self.config.server.ip_range_allowlist, ip_blocklist=self.config.server.ip_range_blocklist, diff --git a/tests/test_utils/__init__.py b/tests/test_utils/__init__.py index 9f4d1f0aec..03c8505e74 100644 --- a/tests/test_utils/__init__.py +++ b/tests/test_utils/__init__.py @@ -155,6 +155,38 @@ class FakeResponse: # type: ignore[misc] return cls(code=code, body=body, headers=headers) +@attr.s(slots=True, frozen=True, auto_attribs=True) +class NativeFakeResponse: + """A fake aiohttp.ClientResponse-compatible object for tests. + + Provides the subset of aiohttp.ClientResponse API used by + NativeSimpleHttpClient and its callers. + """ + + # HTTP status code + status: int = 200 + + # body of the response + body: bytes = b"" + + # response headers (dict-like) + headers: dict[str, str] = attr.Factory(dict) + + @property + def reason(self) -> str: + code_phrase = RESPONSES.get(self.status, b"Unknown Status") + return code_phrase.decode("ascii") if isinstance(code_phrase, bytes) else str(code_phrase) + + async def read(self) -> bytes: + return self.body + + @classmethod + def json(cls, *, code: int = 200, payload: JsonSerializable) -> "NativeFakeResponse": + headers = {"Content-Type": "application/json"} + body = json.dumps(payload).encode("utf-8") + return cls(status=code, body=body, headers=headers) + + # A small image used in some tests. # # Resolution: 1×1, MIME type: image/png, Extension: png, Size: 67 B diff --git a/tests/test_utils/oidc.py b/tests/test_utils/oidc.py index 96a34ecfd4..2aaf5d3c97 100644 --- a/tests/test_utils/oidc.py +++ b/tests/test_utils/oidc.py @@ -29,17 +29,11 @@ from urllib.parse import parse_qs import attr -try: - from twisted.web.http_headers import Headers - from twisted.web.iweb import IResponse -except ImportError: - pass - from synapse.server import HomeServer from synapse.util.clock import Clock from synapse.util.stringutils import random_string -from tests.test_utils import FakeResponse +from tests.test_utils import NativeFakeResponse as FakeResponse @attr.s(slots=True, frozen=True, auto_attribs=True) @@ -281,7 +275,7 @@ class FakeOidcServer: token: If True, makes the token endpoint return a 500 error. userinfo: If True, makes the userinfo endpoint return a 500 error. """ - buggy = FakeResponse(code=500, body=b"Internal server error") + buggy = FakeResponse(status=500, body=b"Internal server error") patches = {} if jwks: @@ -300,26 +294,22 @@ class FakeOidcServer: method: str, uri: str, data: bytes | None = None, - headers: Headers | None = None, - ) -> IResponse: + headers: dict[str, str] | None = None, + ) -> FakeResponse: """The override of the SimpleHttpClient#request() method""" access_token: str | None = None if headers is None: - headers = Headers() + headers = {} # Try to find the access token in the headers if any - auth_headers = headers.getRawHeaders(b"Authorization") - if auth_headers: - parts = auth_headers[0].split(b" ") - if parts[0] == b"Bearer" and len(parts) == 2: - access_token = parts[1].decode("ascii") + auth_header = headers.get("Authorization", "") + if auth_header.startswith("Bearer "): + access_token = auth_header[len("Bearer "):] if method == "POST": # If the method is POST, assume it has an url-encoded body - if data is None or headers.getRawHeaders(b"Content-Type") != [ - b"application/x-www-form-urlencoded" - ]: + if data is None or headers.get("Content-Type") != "application/x-www-form-urlencoded": return FakeResponse.json(code=400, payload={"error": "invalid_request"}) params = parse_qs(data.decode("utf-8")) @@ -338,28 +328,28 @@ class FakeOidcServer: elif uri == self.userinfo_endpoint: return self.get_userinfo_handler(access_token=access_token) - return FakeResponse(code=404, body=b"404 not found") + return FakeResponse(status=404, body=b"404 not found") # Request handlers - def _get_jwks_handler(self) -> IResponse: + def _get_jwks_handler(self) -> FakeResponse: """Handles requests to the JWKS URI.""" return FakeResponse.json(payload=self.get_jwks()) - def _get_metadata_handler(self) -> IResponse: + def _get_metadata_handler(self) -> FakeResponse: """Handles requests to the OIDC well-known document.""" return FakeResponse.json(payload=self.get_metadata()) - def _get_userinfo_handler(self, access_token: str | None) -> IResponse: + def _get_userinfo_handler(self, access_token: str | None) -> FakeResponse: """Handles requests to the userinfo endpoint.""" if access_token is None: - return FakeResponse(code=401) + return FakeResponse(status=401) user_info = self.get_userinfo(access_token) if user_info is None: - return FakeResponse(code=401) + return FakeResponse(status=401) return FakeResponse.json(payload=user_info) - def _post_token_handler(self, params: dict[str, list[str]]) -> IResponse: + def _post_token_handler(self, params: dict[str, list[str]]) -> FakeResponse: """Handles requests to the token endpoint.""" code = params.get("code", [])