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

AraHaan · 2024-07-04T23:00:48Z

This is because libraries like discord.py needs to use a wss type of url to make a socket to an api service, and it cannot be done outside of aiohttp as it can block the async event loop which can result in undefined behavior.

Test will arrive soon.

What do these changes do?

This fixes the breaking change that breaks the usage of wss style urls inside of api wrappers that requires them in order to start up at all.

Are there changes in behavior for the user?

The only change is that the wss style urls that previously did not throw in 3.9.5 will now still work in 3.10.0 and 4.0.0 when this gets merged. This is because this change special cases wss links.

Is it a substantial burden for the maintainers to support this?

I think this is not a substantial burden as this is both a simple change, and prevents breaking applications that expect this entirely while also allowing them to get the latest updates on both features and bug fixes (and possibly also security fixes) vs pinning them forever to 3.9.5 being the last version they can ever use and them eating a security bug that they can never get fixed.

Related issue number

Fixes #8481

Checklist

Will finish the checklist as I work on this.

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES/ folder
- name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)
- if you don't have an issue number, change it to the pull request
  number after creating the PR
  - .bugfix: A bug fix for something the maintainers deemed an
    improper undesired behavior that got corrected to match
    pre-agreed expectations.
  - .feature: A new behavior, public APIs. That sort of stuff.
  - .deprecation: A declaration of future API removals and breaking
    changes in behavior.
  - .breaking: When something public is removed in a breaking way.
    Could be deprecated in an earlier release.
  - .doc: Notable updates to the documentation structure or build
    process.
  - .packaging: Notes for downstreams about unobvious side effects
    and tooling. Changes in the test invocation considerations and
    runtime assumptions.
  - .contrib: Stuff that affects the contributor experience. e.g.
    Running tests, building the docs, setting up the development
    environment.
  - .misc: Changes that are hard to assign to any of the above
    categories.
- Make sure to use full sentences with correct case and punctuation,
  for example:
```
Fixed issue with non-ascii contents in doctest text files
-- by :user:`contributor-gh-handle`.
```
  Use the past tense or the present tense a non-imperative mood,
  referring to what's changed compared to the last released version
  of this project.

webknjaz · 2024-07-04T23:47:54Z

Ref #6722 (comment)

webknjaz · 2024-07-04T23:49:28Z

This should probably not modify the constant but be in a separate one so the initial connect and redirect might be treated differently..

AraHaan · 2024-07-04T23:56:12Z

Yeah that is true as well.

aiohttp/client.py

AraHaan · 2024-07-05T00:10:56Z

Does this look better?

codecov · 2024-07-05T00:18:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.61%. Comparing base (7bf6ee1) to head (b16e5ae).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8482   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files         107      107           
  Lines       33159    33191   +32     
  Branches     3895     3898    +3     
=======================================
+ Hits        32369    32401   +32     
  Misses        571      571           
  Partials      219      219

Flag	Coverage Δ
CI-GHA	`97.53% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`97.20% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`95.65% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`96.86% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.00% <100.00%> (+<0.01%)`	⬆️
Py-3.10.14	`96.96% <100.00%> (+<0.01%)`	⬆️
Py-3.11.9	`97.18% <100.00%> (+<0.01%)`	⬆️
Py-3.12.4	`97.30% <100.00%> (+<0.01%)`	⬆️
Py-3.8.10	`95.42% <100.00%> (+<0.01%)`	⬆️
Py-3.8.18	`96.85% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`96.99% <100.00%> (-0.01%)`	⬇️
Py-3.9.19	`96.95% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`96.52% <100.00%> (+<0.01%)`	⬆️
VM-macos	`96.86% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`97.20% <100.00%> (+<0.01%)`	⬆️
VM-windows	`95.65% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

webknjaz · 2024-07-05T09:39:12Z

Yeah, I think so. Though, we won't see a proof until the tests are in.

Dreamsorcerer · 2024-07-05T11:16:41Z

Based on the comments we had on that previous PR, this looks like the right change.

Can we get a regression test though, to be sure it doesn't get broken again in future?

AraHaan · 2024-07-05T13:04:20Z

Based on the comments we had on that previous PR, this looks like the right change.

Can we get a regression test though, to be sure it doesn't get broken again in future?

sure, I will need to crab an wss and ws url that does not need any json format query params though as I am not sure if the discord one can be used or not for the test here.

aiohttp/client.py

CHANGES/8481.bugfix.rst

webknjaz · 2024-07-05T13:29:15Z

I am not sure if the discord one can be used or not for the test here.

Make sure that the test does not depend on the internet access.

AraHaan · 2024-07-05T15:21:59Z

uh do I edit test_client_ws.py or test_client_ws_functional.py?

Dreamsorcerer · 2024-07-06T13:38:21Z

uh do I edit test_client_ws.py or test_client_ws_functional.py?

Whichever makes sense for the test you write (comparing to the tests in each file). I think the first tests small details using mocks, while the second is closer to E2E tests against a running application.

This is because libraries like discord.py needs to use a wss type of url to make a socket to an api service, and it cannot be done outside of aiohttp as it can block the async event loop which can result in undefined behavior. Test will arrive soon.

for more information, see https://pre-commit.ci

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

aiohttp/client.py

tests/test_client_session.py

tests/conftest.py

@@ -1,5 +1,7 @@
 # type: ignore
 import asyncio
+import base64
+import hashlib


tests/test_client_session.py

Co-authored-by: Sam Bull <aa6bs0@sambull.org>

patchback · 2024-07-17T13:45:46Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 62173be on top of patchback/backports/3.10/62173bea1e4e3538d46e6e9b94d727d870d86879/pr-8482

Backporting merged PR #8482 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/62173bea1e4e3538d46e6e9b94d727d870d86879/pr-8482 upstream/3.10

Now, cherry-pick PR Fix incorrect rejection of ws:// and wss:// urls #8482 contents into that branch:
```
$ git cherry-pick -x 62173bea1e4e3538d46e6e9b94d727d870d86879
```
If it'll yell at you with something like fatal: Commit 62173bea1e4e3538d46e6e9b94d727d870d86879 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 62173bea1e4e3538d46e6e9b94d727d870d86879
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix incorrect rejection of ws:// and wss:// urls #8482 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/62173bea1e4e3538d46e6e9b94d727d870d86879/pr-8482

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

bdraco · 2024-07-17T13:46:00Z

Thanks @AraHaan

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua> Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit 62173be)

…d wss:// urls (#8511) Co-authored-by: pre-commit-ci[bot] Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: Sam Bull <aa6bs0@sambull.org> Co-authored-by: AraHaan <seandhunt_7@yahoo.com>

webknjaz · 2024-07-17T15:27:00Z

CHANGES/8481.bugfix.rst

@@ -0,0 +1,2 @@
+Fixed the incorrect rejection of ``ws://`` and ``wss://`` urls


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

Sorry I missed the there was more to that request.

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.

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

@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.

webknjaz · 2024-07-17T15:29:21Z

tests/test_client_session.py

+ # if the connection wasn't already closed
+ for c in connections:
+ c.close()
+ del c


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.

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

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

followup to #8482

webknjaz · 2024-07-18T22:32:53Z

CHANGES/8481.bugfix.rst

@@ -0,0 +1,2 @@
+Fixed the incorrect rejection of ``ws://`` and ``wss://`` urls
+-- by :user:` AraHaan`.


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

webknjaz added the backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot label Jul 4, 2024

AraHaan commented Jul 5, 2024

View reviewed changes

aiohttp/client.py Outdated Show resolved Hide resolved

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 5, 2024

bdraco reviewed Jul 5, 2024

View reviewed changes

aiohttp/client.py Outdated Show resolved Hide resolved

webknjaz reviewed Jul 5, 2024

View reviewed changes

CHANGES/8481.bugfix.rst Outdated Show resolved Hide resolved

AraHaan force-pushed the patch-1 branch from b8465bd to 5a27077 Compare July 6, 2024 11:52

AraHaan and others added 6 commits July 9, 2024 13:25

Apply suggestions from code review

e086eff

Update client.py

d58cfc7

Added change log.

858b96a

[pre-commit.ci] auto fixes from pre-commit.com hooks

9e20f5a

for more information, see https://pre-commit.ci

Update CHANGES/8481.bugfix.rst

2409cd1

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

AraHaan force-pushed the patch-1 branch from 5a27077 to 2409cd1 Compare July 9, 2024 17:25

bdraco reviewed Jul 11, 2024

View reviewed changes

aiohttp/client.py Show resolved Hide resolved

bdraco added 2 commits July 11, 2024 15:04

Add coverage for ws/wss

e19da9d

Add test coverage

a11518d

bdraco changed the title ~~Fixed the incorrect rejection of wss:// urls.~~ Fixed the incorrect rejection of wss:// urls Jul 11, 2024

bdraco changed the title ~~Fixed the incorrect rejection of wss:// urls~~ Fixed the incorrect rejection of ws:// and wss:// urls Jul 11, 2024

bdraco changed the title ~~Fixed the incorrect rejection of ws:// and wss:// urls~~ Fix the incorrect rejection of ws:// and wss:// urls Jul 11, 2024

bdraco changed the title ~~Fix the incorrect rejection of ws:// and wss:// urls~~ Fix incorrect rejection of ws:// and wss:// urls Jul 11, 2024

github-advanced-security bot found potential problems Jul 11, 2024

View reviewed changes

lint

be8f71e

Dreamsorcerer reviewed Jul 13, 2024

View reviewed changes

tests/test_client_session.py Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Jul 13, 2024

View reviewed changes

tests/test_client_session.py Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Jul 13, 2024

View reviewed changes

tests/test_client_session.py Outdated Show resolved Hide resolved

Update tests/test_client_session.py

20f8bfd

Co-authored-by: Sam Bull <aa6bs0@sambull.org>

bdraco mentioned this pull request Jul 14, 2024

Shutdown logic: Only wait on handlers #8495

Merged

1 task

bdraco added 3 commits July 14, 2024 21:29

Merge branch 'master' into patch-1

79e9987

tweak testing

68ea65f

Merge branch 'master' into patch-1

b16e5ae

bdraco approved these changes Jul 16, 2024

View reviewed changes

bdraco marked this pull request as ready for review July 16, 2024 00:18

bdraco requested a review from asvetlov as a code owner July 16, 2024 00:18

bdraco merged commit 62173be into aio-libs:master Jul 17, 2024
37 of 38 checks passed

AraHaan deleted the patch-1 branch July 17, 2024 14:12

webknjaz reviewed Jul 17, 2024

View reviewed changes

bdraco added a commit that referenced this pull request Jul 17, 2024

Small cleanup to wss/ws fix

d4e608e

followup to #8482

bdraco mentioned this pull request Jul 17, 2024

Small cleanup to wss/ws fix #8513

Merged

webknjaz added a commit that referenced this pull request Jul 18, 2024

Link the hotfix PR to the #8482 feature PR

e8000e4

webknjaz reviewed Jul 18, 2024

View reviewed changes

patchback bot mentioned this pull request Jul 20, 2024

[PR #8513/bbe90e5d backport][3.10] Small cleanup to wss/ws fix #8519

Merged

bdraco mentioned this pull request Aug 4, 2024

Error while trying to use unix socket, when aiohttp>=3.10 #8600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AraHaan commented Jul 4, 2024 •

edited

Loading

webknjaz commented Jul 4, 2024

webknjaz commented Jul 4, 2024

AraHaan commented Jul 4, 2024

AraHaan commented Jul 5, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

webknjaz commented Jul 5, 2024

Dreamsorcerer commented Jul 5, 2024

AraHaan commented Jul 5, 2024

webknjaz commented Jul 5, 2024

AraHaan commented Jul 5, 2024

Dreamsorcerer commented Jul 6, 2024

patchback bot commented Jul 17, 2024 •

edited

Loading

bdraco commented Jul 17, 2024

webknjaz Jul 17, 2024

bdraco Jul 17, 2024

webknjaz Jul 17, 2024

bdraco Jul 17, 2024

Dreamsorcerer Jul 21, 2024

webknjaz Jul 21, 2024

webknjaz Jul 17, 2024

bdraco Jul 17, 2024

webknjaz Jul 17, 2024

bdraco Jul 17, 2024

webknjaz Jul 18, 2024

		@@ -0,0 +1,2 @@
		Fixed the incorrect rejection of ``ws://`` and ``wss://`` urls

		@@ -0,0 +1,2 @@
		Fixed the incorrect rejection of ``ws://`` and ``wss://`` urls
		-- by :user:` AraHaan`.

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

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

Conversation

AraHaan commented Jul 4, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

webknjaz commented Jul 4, 2024

webknjaz commented Jul 4, 2024

AraHaan commented Jul 4, 2024

AraHaan commented Jul 5, 2024

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

webknjaz commented Jul 5, 2024

Dreamsorcerer commented Jul 5, 2024

AraHaan commented Jul 5, 2024

webknjaz commented Jul 5, 2024

AraHaan commented Jul 5, 2024

Dreamsorcerer commented Jul 6, 2024

patchback bot commented Jul 17, 2024 • edited Loading

Backport to 3.10: 💔 cherry-picking failed — conflicts found

Backporting merged PR #8482 into master

bdraco commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AraHaan commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

patchback bot commented Jul 17, 2024 •

edited

Loading