Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect rejection of ws:// and wss:// urls #8482

Merged
merged 13 commits into from
Jul 17, 2024
2 changes: 2 additions & 0 deletions CHANGES/8481.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixed the incorrect rejection of ``ws://`` and ``wss://`` urls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be useful to improve this text with context as requested earlier in #8482 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed the there was more to that request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I only got to take a look again and realized that this description is rather vague. It refers to an incorrect behavior without really getting into what's incorrect about it and where/how it's happening, what's the visible effect for the end-users.

I know this is hard to write and hard to explain, so I'm thinking of ways to lint it better. I was hoping to try integrating https://vale.sh and see if that would help people / give better hints on how to address such things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the "incorrect" behaviour referenced here is not in any release of aiohttp, so probably a changelog entry is unneeded anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dreamsorcerer yep! I actually brought it up in the follow-up and we dropped this note in favor of linking the original one and crediting the contribution to more people, mentioning more PRs/issues.

-- by :user:` AraHaan`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AraHaan @bdraco apparently, there was a typo here — a rogue whitespace after the first backtick.

4 changes: 3 additions & 1 deletion aiohttp/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ 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"})
bdraco marked this conversation as resolved.
Show resolved Hide resolved
ALLOWED_PROTOCOL_SCHEMA_SET = HTTP_SCHEMA_SET | WS_SCHEMA_SET

_RetType = TypeVar("_RetType")
_CharsetResolver = Callable[[ClientResponse, bytes], str]
Expand Down Expand Up @@ -452,7 +454,7 @@ async def _request(
except ValueError as e:
raise InvalidUrlClientError(str_or_url) from e

if url.scheme not in HTTP_SCHEMA_SET:
if url.scheme not in ALLOWED_PROTOCOL_SCHEMA_SET:
raise NonHttpUrlClientError(url)

skip_headers = set(self._skip_auto_headers)
Expand Down
19 changes: 18 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
# type: ignore
import asyncio
import base64
import os
import socket
import ssl
import sys
from hashlib import md5, sha256
from hashlib import md5, sha1, sha256
from pathlib import Path
from tempfile import TemporaryDirectory
from typing import Any, List
Expand All @@ -13,6 +14,7 @@

import pytest

from aiohttp.http import WS_KEY
from aiohttp.test_utils import loop_context

try:
Expand Down Expand Up @@ -218,3 +220,18 @@ def start_connection():
spec_set=True,
) as start_connection_mock:
yield start_connection_mock


@pytest.fixture
def key_data():
return os.urandom(16)


@pytest.fixture
def key(key_data: Any):
return base64.b64encode(key_data)


@pytest.fixture
def ws_key(key: Any):
return base64.b64encode(sha1(key + WS_KEY).digest()).decode()
54 changes: 54 additions & 0 deletions tests/test_client_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,60 @@ async def create_connection(req, traces, timeout):
c.__del__()


@pytest.mark.parametrize("protocol", ["http", "https", "ws", "wss"])
async def test_ws_connect_allowed_protocols(
create_session: Any,
create_mocked_conn: Any,
protocol: str,
ws_key: Any,
key_data: Any,
) -> None:
url = URL(f"{protocol}://example.com")
req = mock.Mock()
bdraco marked this conversation as resolved.
Show resolved Hide resolved
req_factory = mock.Mock(return_value=req)
resp = mock.Mock()
resp.status = 101
resp.headers = {
hdrs.UPGRADE: "websocket",
hdrs.CONNECTION: "upgrade",
hdrs.SEC_WEBSOCKET_ACCEPT: ws_key,
}
resp.url = url
resp.cookies = SimpleCookie()
resp.start = mock.AsyncMock()
req.send = mock.AsyncMock(return_value=resp)
session = await create_session(request_class=req_factory)

connections = []
original_connect = session._connector.connect

async def connect(req, traces, timeout):
conn = await original_connect(req, traces, timeout)
connections.append(conn)
return conn

async def create_connection(req, traces, timeout):
# return self.transport, self.protocol
conn = create_mocked_conn()
return conn
bdraco marked this conversation as resolved.
Show resolved Hide resolved

session._connector.connect = connect
session._connector._create_connection = create_connection
session._connector._release = mock.Mock()
bdraco marked this conversation as resolved.
Show resolved Hide resolved

with mock.patch("aiohttp.client.os") as m_os:
m_os.urandom.return_value = key_data
await session.ws_connect(f"{protocol}://example.com")

# normally called during garbage collection. triggers an exception
# if the connection wasn't already closed
for c in connections:
c.close()
del c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, __del__() is more or less predictable in CPython, but in PyPy it's not really guaranteed to be called at a specific moment in time. Calling gc.collect() might improve it, especially in tests, but might need several invocations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it better to switch it back to __del__? codeql complained about it

Code scanning
/ CodeQL
__del__ is called explicitly
The del special method is called explicitly.
Show more details

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, linters would normally complain and you'd have to ignore. However, I'd probably just stick a few calls to gc.collect(), maybe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


await session.close()


async def test_cookie_jar_usage(loop: Any, aiohttp_client: Any) -> None:
req_url = None

Expand Down
15 changes: 0 additions & 15 deletions tests/test_client_ws.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,6 @@
from aiohttp.test_utils import make_mocked_coro


@pytest.fixture
def key_data():
return os.urandom(16)


@pytest.fixture
def key(key_data: Any):
return base64.b64encode(key_data)


@pytest.fixture
def ws_key(key: Any):
return base64.b64encode(hashlib.sha1(key + WS_KEY).digest()).decode()


async def test_ws_connect(ws_key: Any, loop: Any, key_data: Any) -> None:
resp = mock.Mock()
resp.status = 101
Expand Down
Loading