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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Jul 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b4c161d
Fixed the incorrect rejection of wss:// urls.
AraHaan Jul 4, 2024
e086eff
Apply suggestions from code review
AraHaan Jul 5, 2024
d58cfc7
Update client.py
AraHaan Jul 5, 2024
858b96a
Added change log.
AraHaan Jul 5, 2024
9e20f5a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 5, 2024
2409cd1
Update CHANGES/8481.bugfix.rst
AraHaan Jul 5, 2024
e19da9d
Add coverage for ws/wss
bdraco Jul 11, 2024
a11518d
Add test coverage
bdraco Jul 11, 2024
be8f71e
lint
bdraco Jul 11, 2024
20f8bfd
Update tests/test_client_session.py
bdraco Jul 13, 2024
79e9987
Merge branch 'master' into patch-1
bdraco Jul 15, 2024
68ea65f
tweak testing
bdraco Jul 15, 2024
b16e5ae
Merge branch 'master' into patch-1
bdraco Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES/8481.bugfix.rst
View file
Open in desktop
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.

#8513

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.

7 changes: 6 additions & 1 deletion aiohttp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ 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", ""})
# special case ws and wss links when making requests as in some cases in asyncio
# programs any other ways to request them might result in blocking the event loop
# (or worse a deadlock).
# Fixes https://github.com/aio-libs/aiohttp/issues/8481
WS_SCHEMA_SET = frozenset({"ws", "wss"})
bdraco marked this conversation as resolved.
Show resolved Hide resolved

_RetType = TypeVar("_RetType")
_CharsetResolver = Callable[[ClientResponse, bytes], str]
Expand Down Expand Up @@ -452,7 +457,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 HTTP_SCHEMA_SET and url.scheme not in WS_SCHEMA_SET:
bdraco marked this conversation as resolved.
Show resolved Hide resolved
raise NonHttpUrlClientError(url)

skip_headers = set(self._skip_auto_headers)
Expand Down
Loading