[pull] master from aio-libs:master

CHANGES/13821.bugfix.rst

@@ -0,0 +1 @@
+Fixed the web server trusting the scheme of an absolute-form request-target -- by :user:`Dreamsorcerer`.

aiohttp/_cparser.pxd

@@ -67,7 +67,8 @@ cdef extern from "llhttp.h":
         HTTP_RESPONSE
 
     enum llhttp_method:
-        HTTP_CONNECT
+        HTTP_CONNECT,
+        HTTP_OPTIONS
 
     void llhttp_settings_init(llhttp_settings_t* settings)
     void llhttp_init(llhttp_t* parser, llhttp_type type,

aiohttp/_http_parser.pyx

@@ -743,12 +743,20 @@ cdef class HttpRequestParser(HttpParser):
             return
         self._path = self._buf.decode('utf-8', 'surrogateescape')
         try:
-            idx3 = len(self._path)
             if self._cparser.method == cparser.HTTP_CONNECT:
                 # authority-form,
                 # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3
-                self._url = URL.build(authority=self._path, encoded=True)
-            elif idx3 > 1 and self._path[0] == '/':
+                try:
+                    self._url = URL.build(authority=self._path, encoded=True)
+                    host = self._url.raw_host
+                except ValueError:
+                    host = None
+                if host is None:
+                    raise InvalidURLError(
+                        self._path.encode(errors="surrogateescape")
+                        .decode("latin1")
+                    )
+            elif self._path[0] == '/':
                 # origin-form,
                 # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.1
                 idx1 = self._path.find("?")
@@ -779,10 +787,23 @@ cdef class HttpRequestParser(HttpParser):
                     fragment=fragment,
                     encoded=True,
                 )
+            elif self._path == '*' and self._cparser.method == cparser.HTTP_OPTIONS:
+                # asterisk-form,
+                self._url = URL(self._path, encoded=True)
             else:
                 # absolute-form for proxy maybe,
                 # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2
-                self._url = URL(self._path, encoded=True)
+                try:
+                    self._url = URL(self._path, encoded=True)
+                    host = self._url.raw_host
+                except ValueError:
+                    host = None
+                # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.1-4
+                if host is None:
+                    raise InvalidURLError(
+                        self._path.encode(errors="surrogateescape")
+                        .decode("latin1")
+                    )
         finally:
             PyByteArray_Resize(self._buf, 0)
 

aiohttp/http_parser.py

@@ -685,7 +685,18 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage:
         if method == "CONNECT":
             # authority-form,
             # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3
-            url = URL.build(authority=path, encoded=True)
+            try:
+                url = URL.build(authority=path, encoded=True)
+                host = url.raw_host
+            except ValueError:
+                # Duplicated so mypy understands that url must be defined below.
+                raise InvalidURLError(
+                    path.encode(errors="surrogateescape").decode("latin1")
+                )
+            if host is None:
+                raise InvalidURLError(
+                    path.encode(errors="surrogateescape").decode("latin1")
+                )
         elif path.startswith("/"):
             # origin-form,
             # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.1
@@ -708,10 +719,16 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage:
         else:
             # absolute-form for proxy maybe,
             # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2
-            url = URL(path, encoded=True)
-            if not url.absolute:
-                # authority-form is only allowed with CONNECT
-                # https://www.rfc-editor.org/info/rfc9112/#section-3.2.3-1
+            try:
+                url = URL(path, encoded=True)
+                host = url.raw_host
+            except ValueError:
+                # Duplicated so mypy understands that url must be defined below.
+                raise InvalidURLError(
+                    path.encode(errors="surrogateescape").decode("latin1")
+                )
+            # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.1-4
+            if host is None:
                 raise InvalidURLError(
                     path.encode(errors="surrogateescape").decode("latin1")
                 )

aiohttp/web_request.py

@@ -194,16 +194,21 @@ def __init__(
         self._cache: dict[str, Any] = {}
         url = message.url
         if url.absolute:
+            if scheme is None and url.scheme:
+                # Absolute URL is peer-controlled, use the protocol.
+                scheme = "https" if protocol.ssl_context else "http"
             if scheme is not None:
+                # Authority-form (CONNECT) has no scheme: leave the target
+                # unchanged unless clone(scheme=...) overrides it.
                 url = url.with_scheme(scheme)
+                self._cache["scheme"] = scheme
             if host is not None:
                 url = url.with_host(host)
             # absolute URL is given,
             # override auto-calculating url, host, and scheme
             # all other properties should be good
             self._cache["url"] = url
             self._cache["host"] = url.host
-            self._cache["scheme"] = url.scheme
             self._rel_url = url.relative()
         else:
             self._rel_url = url

tests/test_http_parser.py

@@ -1181,6 +1181,59 @@ def test_url_authority_form_only_connect(parser: HttpRequestParser) -> None:
         parser.feed_data(b"GET www.google.com:443 HTTP/1.1\r\nHost: a\r\n\r\n")
 
 
+@pytest.mark.parametrize(
+    "target",
+    (
+        b"https:///protected",
+        b"https:////protected",
+        b"https://:80/protected",
+        b"https://user@/protected",
+    ),
+    ids=("empty-host", "empty-host-extra-slash", "port-only", "userinfo-only"),
+)
+def test_url_absolute_form_empty_host_rejected(
+    parser: HttpRequestParser, target: bytes
+) -> None:
+    # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.2-4
+    with pytest.raises(http_exceptions.InvalidURLError):
+        parser.feed_data(b"GET " + target + b" HTTP/1.1\r\nHost: a\r\n\r\n")
+
+
+def test_url_absolute_form_invalid_port_rejected(parser: HttpRequestParser) -> None:
+    # yarl raises ValueError for an out-of-range port; that must surface as
+    # a 400, not escape the parser as a bare ValueError.
+    with pytest.raises(http_exceptions.InvalidURLError):
+        parser.feed_data(b"GET http://example.com:65536/x HTTP/1.1\r\nHost: a\r\n\r\n")
+
+
+def test_url_connect_invalid_port_rejected(parser: HttpRequestParser) -> None:
+    with pytest.raises(http_exceptions.InvalidURLError):
+        parser.feed_data(b"CONNECT example.com:65536 HTTP/1.1\r\nHost: a\r\n\r\n")
+
+
+def test_url_connect_empty_host_rejected(parser: HttpRequestParser) -> None:
+    with pytest.raises(http_exceptions.InvalidURLError):
+        parser.feed_data(b"CONNECT :80 HTTP/1.1\r\nHost: a\r\n\r\n")
+
+
+def test_url_origin_form_bare_slash(parser: HttpRequestParser) -> None:
+    messages, upgrade, tail = parser.feed_data(b"GET / HTTP/1.1\r\nHost: a\r\n\r\n")
+    assert messages[0][0].url == URL("/")
+
+
+def test_url_asterisk_form_options(parser: HttpRequestParser) -> None:
+    # https://www.rfc-editor.org/rfc/rfc9112#section-3.2.4
+    messages, upgrade, tail = parser.feed_data(b"OPTIONS * HTTP/1.1\r\nHost: a\r\n\r\n")
+    assert messages[0][0].url == URL("*")
+
+
+def test_url_asterisk_form_only_options(parser: HttpRequestParser) -> None:
+    # asterisk-form is only valid for OPTIONS; for other methods "*" is
+    # neither origin-form nor a valid absolute-form target.
+    with pytest.raises(http_exceptions.InvalidURLError):
+        parser.feed_data(b"GET * HTTP/1.1\r\nHost: a\r\n\r\n")
+
+
 def test_headers_old_websocket_key1(parser: HttpRequestParser) -> None:
     text = b"GET /test HTTP/1.1\r\nHost: a\r\nSEC-WEBSOCKET-KEY1: line\r\n\r\n"
 

tests/test_web_functional.py

@@ -2343,6 +2343,48 @@ async def handler(request: web.Request) -> web.Response:
     assert max(drain_reads) < decompressed_size
 
 
+async def test_absolute_form_target_does_not_spoof_scheme(
+    aiohttp_server: AiohttpServer,
+) -> None:
+    """A plaintext absolute-form target with https must not look secure."""
+
+    async def handler(request: web.Request) -> web.Response:
+        return web.json_response(
+            {
+                "scheme": request.scheme,
+                "secure": request.secure,
+                "url": str(request.url),
+                "host": request.host,
+            }
+        )
+
+    app = web.Application()
+    app.router.add_get("/probe", handler)
+    server = await aiohttp_server(app)
+
+    reader, writer = await asyncio.open_connection(server.host, server.port)
+    try:
+        writer.write(
+            b"GET https://trusted.example/probe HTTP/1.1\r\n"
+            b"Host: trusted.example\r\n"
+            b"Connection: close\r\n\r\n"
+        )
+        await writer.drain()
+        raw = await asyncio.wait_for(reader.read(), 5)
+    finally:
+        writer.close()
+        with suppress(ConnectionResetError, BrokenPipeError):
+            await writer.wait_closed()
+
+    assert raw.startswith(b"HTTP/1.1 200 ")
+    assert json.loads(raw.split(b"\r\n\r\n", 1)[1]) == {
+        "scheme": "http",
+        "secure": False,
+        "url": "http://trusted.example/probe",
+        "host": "trusted.example",
+    }
+
+
 async def test_app_max_client_size(aiohttp_client: AiohttpClient) -> None:
     async def handler(request: web.Request) -> NoReturn:
         await request.post()

tests/test_web_request.py

@@ -15,12 +15,14 @@
 from yarl import URL
 
 from aiohttp import ETag, HttpVersion, web
+from aiohttp.abc import AbstractStreamWriter
 from aiohttp.base_protocol import BaseProtocol
 from aiohttp.helpers import DEFAULT_CHUNK_SIZE, HeadersDictProxy
 from aiohttp.http_exceptions import BadHttpMessage, LineTooLong
 from aiohttp.http_parser import RawRequestMessage
 from aiohttp.streams import StreamReader
 from aiohttp.test_utils import make_mocked_request
+from aiohttp.web_protocol import RequestHandler
 from aiohttp.web_request import _FORWARDED_PAIR_RE
 
 
@@ -29,6 +31,19 @@ def protocol() -> mock.Mock:
     return mock.Mock(_reading_paused=False)
 
 
+def make_base_request(
+    message: RawRequestMessage, protocol: RequestHandler[web.BaseRequest]
+) -> web.BaseRequest:
+    return web.BaseRequest(
+        message,
+        mock.create_autospec(StreamReader, spec_set=True, instance=True),
+        protocol,
+        mock.create_autospec(AbstractStreamWriter, spec_set=True, instance=True),
+        mock.create_autospec(asyncio.Task, spec_set=True, instance=True),
+        mock.create_autospec(asyncio.AbstractEventLoop, spec_set=True, instance=True),
+    )
+
+
 def test_base_ctor() -> None:
     message = RawRequestMessage(
         "GET",
@@ -43,13 +58,11 @@ def test_base_ctor() -> None:
         URL("/path/to?a=1&b=2"),
     )
 
-    protocol = mock.Mock()
+    protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True)
     protocol.ssl_context = None
     protocol.peername = None
     protocol.sockname = ("127.0.0.1", 80)
-    req = web.BaseRequest(
-        message, mock.Mock(), protocol, mock.Mock(), mock.Mock(), mock.Mock()
-    )
+    req = make_base_request(message, protocol)
 
     assert "GET" == req.method
     assert HttpVersion(1, 1) == req.version
@@ -214,10 +227,26 @@ def test_non_ascii_raw_path() -> None:
 
 def test_absolute_url() -> None:
     req = make_mocked_request("GET", "https://example.com/path/to?a=1")
+    assert req.url == URL("http://example.com/path/to?a=1")
+    # The scheme of an absolute-form target is peer-controlled and must not
+    # override the transport-derived scheme.
+    assert req.scheme == "http"
+    assert not req.secure
+    assert req.host == "example.com"
+    assert req.rel_url == URL.build(path="/path/to", query={"a": "1"})
+
+
+def test_absolute_url_with_tls_transport() -> None:
+    sslcontext = ssl.create_default_context()
+    req = make_mocked_request(
+        "GET", "http://example.com/path/to?a=1", sslcontext=sslcontext
+    )
     assert req.url == URL("https://example.com/path/to?a=1")
+    # Over a TLS transport the effective scheme is https even when the
+    # absolute-form target claims plain http.
     assert req.scheme == "https"
+    assert req.secure
     assert req.host == "example.com"
-    assert req.rel_url == URL.build(path="/path/to", query={"a": "1"})
 
 
 def test_absolute_form_raw_path() -> None:
@@ -244,23 +273,54 @@ def test_connect_authority_form_raw_path() -> None:
         False,
         URL.build(authority="example.com:443", encoded=True),
     )
-    protocol = mock.Mock()
+    protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True)
     protocol.ssl_context = None
     protocol.peername = None
     protocol.sockname = ("127.0.0.1", 80)
-    req = web.BaseRequest(
-        message, mock.Mock(), protocol, mock.Mock(), mock.Mock(), mock.Mock()
-    )
+    req = make_base_request(message, protocol)
     assert req._message.url.absolute
     assert req.raw_path == "example.com:443"
 
 
+@pytest.mark.parametrize("secure", (False, True))
+def test_connect_authority_form_url_untouched(secure: bool) -> None:
+    # A CONNECT target has no scheme; the transport scheme must not be glued
+    # onto request.url (yarl would also elide a default port, e.g.
+    # "https://example.com:443" serializes without the ":443").
+    message = RawRequestMessage(
+        "CONNECT",
+        "example.com:443",
+        HttpVersion(1, 1),
+        HeadersDictProxy(CIMultiDict()),
+        (),
+        False,
+        None,
+        False,
+        False,
+        URL.build(authority="example.com:443", encoded=True),
+    )
+    protocol = mock.create_autospec(RequestHandler, spec_set=True, instance=True)
+    protocol.ssl_context = ssl.create_default_context() if secure else None
+    protocol.peername = None
+    protocol.sockname = ("127.0.0.1", 8080)
+    req = make_base_request(message, protocol)
+    assert str(req.url) == "//example.com:443"
+    assert req.url.scheme == ""
+    assert req.url.port == 443
+    assert req.host == "example.com"
+    assert req.scheme == ("https" if secure else "http")
+    assert req.secure is secure
+
+
 def test_clone_absolute_scheme() -> None:
     req = make_mocked_request("GET", "https://example.com/path/to?a=1")
-    assert req.scheme == "https"
-    req2 = req.clone(scheme="http")
-    assert req2.scheme == "http"
-    assert req2.url.scheme == "http"
+    assert req.scheme == "http"
+    req2 = req.clone(scheme="https")
+    assert req2.scheme == "https"
+    assert req2.url.scheme == "https"
+    req3 = req2.clone(scheme="http")
+    assert req3.scheme == "http"
+    assert req3.url.scheme == "http"
 
 
 def test_clone_absolute_host() -> None: