Repository: aio-libs/aiohttp
Pull request: Fix connecting to npipe://, tcp://, and unix:// urls
<!-- Thank you for your contribution! -->

## What do these changes do?

Fixes a regression in 3.10.x connecting to `npipe://`, `tcp://`, and `unix://` urls

A future PR should remove `allowed_protocol_schema_set` in `BaseConnector` as its only there for backwards compat

## Are there changes in behavior for the user?
no

## Is it a substantial burden for the maintainers to support this?
no

## Related issue number
 fixes #8600


Problem statement:
Error while trying to use unix socket, when `aiohttp`>=3.10
## Long story short

<!-- Please describe your problem and why the fix is important. -->

* Expected behaviour: Communicating via unix socket should work.
* Actual behaviour: Error. Its traceback:
```
Traceback (most recent call last):
  File "/tmp/aiodocker_fail/example.py", line 12, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 654, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/example.py", line 8, in main
    await docker.version()
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 190, in version
    data = await self._query_json("version")
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 319, in _query_json
    async with self._query(
  File "/usr/lib/python3.11/contextlib.py", line 210, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 231, in _query
    yield await self._do_query(
          ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 257, in _do_query
    await self._check_version()
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 209, in _check_version
    ver = await self._query_json("version", versioned_api=False)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 319, in _query_json
    async with self._query(
  File "/usr/lib/python3.11/contextlib.py", line 210, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 231, in _query
    yield await self._do_query(
          ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiodocker/docker.py", line 267, in _do_query
    response = await self.session.request(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/aiodocker_fail/venv/lib/python3.11/site-packages/aiohttp/client.py", line 510, in _request
    raise NonHttpUrlClientError(url)
aiohttp.client_exceptions.NonHttpUrlClientError: unix://localhost/version
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7f5ede16bf10>
```
## How to reproduce

<!-- Please describe steps to reproduce the issue.
     If you have a script that does that please include it here within
     markdown code markup -->

In fresh virtual environment run `pip install aiodocker`, the following script fails:

```python
#!/usr/bin/env python3
from aiodocker import Docker
import asyncio

async def main():
    docker = Docker()
    await docker.version()

if __name__ == "__main__":
    asyncio.run(main())
```
## Your environment

Just the latest version of aiodocker with combination of 3.10 aiohttp.

My complete `requirements.txt` of the environment:
```txt
aiodocker==0.22.2
aiohappyeyeballs==2.3.4
aiohttp==3.10.0
aiosignal==1.3.1
attrs==24.1.0
frozenlist==1.4.1
idna==3.7
multidict==6.0.5
yarl==1.9.4
```

<!-- Describe the environment you have that lead to your issue.
     This includes project version, OS, proxy server and other bits that
     are related to your case. -->
Current fix for me is the usage of `aiohttp==3.9.0`

Hints from issue discussion:
Likely similar to https://github.com/aio-libs/aiohttp/pull/8482
+1
I actually didn't realise the client can make unix socket requests.

Looking at the connectors, there is UnixConnector and also NamedPipeConnector. It is also possible to create custom connectors...

Maybe we need the connector to define what a valid URL looks like and then the client should use that information?
That sounds like a better design to me.  The downside is it might technically be a breaking change for anyone subclassing the BaseConnector as they would have to add this now.
Maybe allow anything in the BaseConnector for 3.10, and have it abstract in v4.
Same problem here with monitor_docker component using aiodocker. The aiohttp 3.10.x broke the unix socket and the tcp socket.

Following worked in aiohttp 3.9.5:
tcp://IP:2375
unix://localhost

Both are broken now, for the same reason. The tcp can be fixed by replacing it with http.
@Dreamsorcerer 
> UnixConnector

It looks like the aiodocker is already doing that, I see in the code:

connector = aiohttp.UnixConnector(docker_host[UNIX_PRE_LEN:])
and also a pipe one:
connector = aiohttp.NamedPipeConnector(docker_host[WIN_PRE_LEN:].replace("/", "\\"))
> It looks like the aiodocker is already doing that, I see in the code:

We're aware of that. The point is that currently the client just tries a whitelist of protocols, but really the whitelist should depend which connector is being used (http:// won't work if UnixConnector is used, for example).

I guess tcp:// should be added to the list? I'm not aware of this being a recognised URL protocol though, so not sure why that is in use..
I don't think `tcp://` is needed since that is always expected to have a higher level `http`,`https`,`ws`,or `wss`.
> I don't think `tcp://` is needed since that is always expected to have a higher level `http`,`https`,`ws`,or `wss`.

The linked issue suggests there are multiple people doing this in homeassistant though? https://github.com/ualex73/monitor_docker/issues/157#issuecomment-2269311789

Commit message: Update aiohttp/client.py

Patch to review:
diff --git a/CHANGES/8632.bugfix.rst b/CHANGES/8632.bugfix.rst
new file mode 100644
index 00000000000..c6da81d7ab3
--- /dev/null
+++ b/CHANGES/8632.bugfix.rst
@@ -0,0 +1 @@
+Fixed connecting to ``npipe://``, ``tcp://``, and ``unix://`` urls -- by :user:`bdraco`.
diff --git a/aiohttp/client.py b/aiohttp/client.py
index 3256fa82bb2..299201a24c7 100644
--- a/aiohttp/client.py
+++ b/aiohttp/client.py
@@ -80,7 +80,13 @@
     ClientWebSocketResponse,
     ClientWSTimeout,
 )
-from .connector import BaseConnector, NamedPipeConnector, TCPConnector, UnixConnector
+from .connector import (
+    HTTP_AND_EMPTY_SCHEMA_SET,
+    BaseConnector,
+    NamedPipeConnector,
+    TCPConnector,
+    UnixConnector,
+)
 from .cookiejar import CookieJar
 from .helpers import (
     _SENTINEL,
@@ -210,9 +216,6 @@ class ClientTimeout:
 
 # https://www.rfc-editor.org/rfc/rfc9110#section-9.2.2
 IDEMPOTENT_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE"})
-HTTP_SCHEMA_SET = frozenset({"http", "https", ""})
-WS_SCHEMA_SET = frozenset({"ws", "wss"})
-ALLOWED_PROTOCOL_SCHEMA_SET = HTTP_SCHEMA_SET | WS_SCHEMA_SET
 
 _RetType = TypeVar("_RetType")
 _CharsetResolver = Callable[[ClientResponse, bytes], str]
@@ -466,7 +469,8 @@ async def _request(
         except ValueError as e:
             raise InvalidUrlClientError(str_or_url) from e
 
-        if url.scheme not in ALLOWED_PROTOCOL_SCHEMA_SET:
+        assert self._connector is not None
+        if url.scheme not in self._connector.allowed_protocol_schema_set:
             raise NonHttpUrlClientError(url)
 
         skip_headers = set(self._skip_auto_headers)
@@ -597,7 +601,6 @@ async def _request(
                             real_timeout.connect,
                             ceil_threshold=real_timeout.ceil_threshold,
                         ):
-                            assert self._connector is not None
                             conn = await self._connector.connect(
                                 req, traces=traces, timeout=real_timeout
                             )
@@ -693,7 +696,7 @@ async def _request(
                             ) from e
 
                         scheme = parsed_redirect_url.scheme
-                        if scheme not in HTTP_SCHEMA_SET:
+                        if scheme not in HTTP_AND_EMPTY_SCHEMA_SET:
                             resp.close()
                             raise NonHttpUrlRedirectClientError(r_url)
                         elif not scheme:
diff --git a/aiohttp/connector.py b/aiohttp/connector.py
index fdb76c3e931..41c6eaad34d 100644
--- a/aiohttp/connector.py
+++ b/aiohttp/connector.py
@@ -9,6 +9,7 @@
 import warnings
 from collections import defaultdict, deque
 from contextlib import suppress
+from functools import cached_property
 from http import HTTPStatus
 from http.cookies import SimpleCookie
 from itertools import cycle, islice
@@ -21,6 +22,7 @@
     Callable,
     DefaultDict,
     Dict,
+    FrozenSet,
     Iterator,
     List,
     Literal,
@@ -64,6 +66,17 @@
     SSLContext = object  # type: ignore[misc,assignment]
 
 
+EMPTY_SCHEMA_SET = frozenset({""})
+HTTP_SCHEMA_SET = frozenset({"http", "https"})
+WS_SCHEMA_SET = frozenset({"ws", "wss"})
+TCP_PROTOCOL_SCHEMA_SET = frozenset({"tcp"})
+UNIX_PROTOCOL_SCHEMA_SET = frozenset({"unix"})
+NAMED_PIPE_PROTOCOL_SCHEMA_SET = frozenset({"npipe"})
+
+HTTP_AND_EMPTY_SCHEMA_SET = HTTP_SCHEMA_SET | EMPTY_SCHEMA_SET
+HIGH_LEVEL_SCHEMA_SET = HTTP_AND_EMPTY_SCHEMA_SET | WS_SCHEMA_SET
+
+
 __all__ = ("BaseConnector", "TCPConnector", "UnixConnector", "NamedPipeConnector")
 
 
@@ -244,6 +257,14 @@ def __init__(
         self._cleanup_closed_transports: List[Optional[asyncio.Transport]] = []
         self._cleanup_closed()
 
+    @cached_property
+    def allowed_protocol_schema_set(self) -> FrozenSet[str]:
+        """Return allowed protocol schema set.
+
+        By default we allow all protocols for backwards compatibility.
+        """
+        return HIGH_LEVEL_SCHEMA_SET
+
     def __del__(self, _warnings: Any = warnings) -> None:
         if self._closed:
             return
@@ -791,6 +812,11 @@ def _close_immediately(self) -> List["asyncio.Future[None]"]:
             ev.cancel()
         return super()._close_immediately()
 
+    @cached_property
+    def allowed_protocol_schema_set(self) -> FrozenSet[str]:
+        """Return allowed protocol schema set."""
+        return super().allowed_protocol_schema_set | TCP_PROTOCOL_SCHEMA_SET
+
     @property
     def family(self) -> int:
         """Socket family like AF_INET."""
@@ -1358,6 +1384,11 @@ def __init__(
         )
         self._path = path
 
+    @cached_property
+    def allowed_protocol_schema_set(self) -> FrozenSet[str]:
+        """Return allowed protocol schema set."""
+        return super().allowed_protocol_schema_set | UNIX_PROTOCOL_SCHEMA_SET
+
     @property
     def path(self) -> str:
         """Path to unix socket."""
@@ -1418,6 +1449,11 @@ def __init__(
             )
         self._path = path
 
+    @cached_property
+    def allowed_protocol_schema_set(self) -> FrozenSet[str]:
+        """Return allowed protocol schema set."""
+        return super().allowed_protocol_schema_set | NAMED_PIPE_PROTOCOL_SCHEMA_SET
+
     @property
     def path(self) -> str:
         """Path to the named pipe."""
diff --git a/tests/test_client_session.py b/tests/test_client_session.py
index f7506c880d8..c2524c92a6a 100644
--- a/tests/test_client_session.py
+++ b/tests/test_client_session.py
@@ -521,7 +521,9 @@ async def create_connection(
                     c.__del__()
 
 
-@pytest.mark.parametrize("protocol", ["http", "https", "ws", "wss"])
+@pytest.mark.parametrize(
+    "protocol", ["http", "https", "npipe", "ws", "wss", "unix", "tcp"]
+)
 async def test_ws_connect_allowed_protocols(
     create_session: Callable[..., Awaitable[ClientSession]],
     create_mocked_conn: Callable[[], ResponseHandler],
@@ -536,15 +538,17 @@ async def test_ws_connect_allowed_protocols(
         hdrs.CONNECTION: "upgrade",
         hdrs.SEC_WEBSOCKET_ACCEPT: ws_key,
     }
-    resp.url = URL(f"{protocol}://example.com")
+    resp.url = URL(f"{protocol}://example")
     resp.cookies = SimpleCookie()
     resp.start = mock.AsyncMock()
 
     req = mock.create_autospec(aiohttp.ClientRequest, spec_set=True)
     req_factory = mock.Mock(return_value=req)
     req.send = mock.AsyncMock(return_value=resp)
+    # BaseConnector allows all protocols by default
+    connector = BaseConnector()
 
-    session = await create_session(request_class=req_factory)
+    session = await create_session(connector=connector, request_class=req_factory)
 
     connections = []
     assert session._connector is not None
@@ -569,7 +573,7 @@ async def create_connection(
         "aiohttp.client.os"
     ) as m_os:
         m_os.urandom.return_value = key_data
-        await session.ws_connect(f"{protocol}://example.com")
+        await session.ws_connect(f"{protocol}://example")
 
     # normally called during garbage collection.  triggers an exception
     # if the connection wasn't already closed
diff --git a/tests/test_connector.py b/tests/test_connector.py
index 2087e0d0cbd..335e2a1ebc0 100644
--- a/tests/test_connector.py
+++ b/tests/test_connector.py
@@ -1636,6 +1636,11 @@ async def test_tcp_connector_ctor(loop: asyncio.AbstractEventLoop) -> None:
     assert conn.family == 0
 
 
+async def test_tcp_connector_allowed_protocols(loop: asyncio.AbstractEventLoop) -> None:
+    conn = aiohttp.TCPConnector()
+    assert conn.allowed_protocol_schema_set == {"", "tcp", "http", "https", "ws", "wss"}
+
+
 async def test_invalid_ssl_param() -> None:
     with pytest.raises(TypeError):
         aiohttp.TCPConnector(ssl=object())  # type: ignore[arg-type]
@@ -1819,6 +1824,19 @@ async def test_ctor_with_default_loop(loop: asyncio.AbstractEventLoop) -> None:
     assert loop is conn._loop
 
 
+async def test_base_connector_allows_high_level_protocols(
+    loop: asyncio.AbstractEventLoop,
+) -> None:
+    conn = aiohttp.BaseConnector()
+    assert conn.allowed_protocol_schema_set == {
+        "",
+        "http",
+        "https",
+        "ws",
+        "wss",
+    }
+
+
 async def test_connect_with_limit(
     loop: asyncio.AbstractEventLoop, key: ConnectionKey
 ) -> None:
@@ -2621,6 +2639,14 @@ async def handler(request: web.Request) -> web.Response:
 
     connector = aiohttp.UnixConnector(unix_sockname)
     assert unix_sockname == connector.path
+    assert connector.allowed_protocol_schema_set == {
+        "",
+        "http",
+        "https",
+        "ws",
+        "wss",
+        "unix",
+    }
 
     session = ClientSession(connector=connector)
     r = await session.get(url)
@@ -2648,6 +2674,14 @@ async def handler(request: web.Request) -> web.Response:
 
     connector = aiohttp.NamedPipeConnector(pipe_name)
     assert pipe_name == connector.path
+    assert connector.allowed_protocol_schema_set == {
+        "",
+        "http",
+        "https",
+        "ws",
+        "wss",
+        "npipe",
+    }
 
     session = ClientSession(connector=connector)
     r = await session.get(url)


Code review comments:
diff_hunk: @@ -791,6 +812,11 @@ def _close_immediately(self) -> List["asyncio.Future[None]"]:
             ev.cancel()
         return super()._close_immediately()
 
+    @cached_property
+    def allowed_protocol_schema_set(self) -> FrozenSet[str]:
+        """Return allowed protocol schema set."""
+        return super().allowed_protocol_schema_set | TCP_PROTOCOL_SCHEMA_SET
+
path: aiohttp/connector.py
text: @user1:
Personally, I'd have kept it to a simple:
```
allowed_protocol_schema_set = HIGH_LEVEL_SCHEMA_SET.union("tcp")
```

I'm not convinced on the single use globals or cached_property.

@author:
It did end up more complex than needed.

adjusted in 374821b2f847e250e7f2e69874401ad6e80c8c1a