Fix handling of redirects with authentication by PLPeeters · Pull Request #9443 · aio-libs/aiohttp

PLPeeters · 2024-10-09T15:10:01Z

What do these changes do?

They make the client ignore auth clashes that are solely due to redirects, in addition to having redirect authentication take precedence over previously set authentication.

Are there changes in behavior for the user?

Users will no longer get a ValueError if a website suddenly includes multiple authenticated URLs in its redirect chain (see #9436 for an example).

While writing the test, I also wondered what should happen if we already have authentication set and get new authentication in a redirect URL. Mimicking what Chrome seems to be doing in this case, I opted to supersede the auth with that of the redirect. We can of course discuss this and tweak it if needed. I'm also on the fence on whether this warrants a separate PR, so I bundled it for now.

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

Probably not.

Related issue number

Fixes #9436

Checklist

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.

codecov · 2024-10-09T15:19:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (d639a06) to head (b6b3e82).
Report is 629 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9443   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files         105      105           
  Lines       35104    35128   +24     
  Branches     4178     4180    +2     
=======================================
+ Hits        34612    34636   +24     
  Misses        329      329           
  Partials      163      163

Flag	Coverage Δ
CI-GHA	`98.48% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`98.14% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.53% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`97.84% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.71% <100.00%> (-0.01%)`	⬇️
Py-3.10.15	`97.63% <100.00%> (+<0.01%)`	⬆️
Py-3.11.10	`97.71% <100.00%> (+0.01%)`	⬆️
Py-3.11.9	`97.79% <100.00%> (+<0.01%)`	⬆️
Py-3.12.7	`98.20% <100.00%> (+<0.01%)`	⬆️
Py-3.13.0	`98.18% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`97.61% <100.00%> (+<0.01%)`	⬆️
Py-3.9.20	`97.54% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`97.17% <100.00%> (+<0.01%)`	⬆️
VM-macos	`97.84% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`98.14% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.53% <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.

aiohttp/client.py

CHANGES/9436.bugfix.rst

aiohttp/client.py

tests/test_client_functional.py

PLPeeters · 2024-10-18T10:35:52Z

@webknjaz Is anything still required on my end to get this merged?

webknjaz

Is anything still required on my end to get this merged?

All's good on my side. It'd be nice to have a docstring in the test function, but that's minor. I'll let somebody else merge, though.

bdraco · 2024-10-18T18:54:15Z

That proxy test is flakey. I restarted the CI. I'm just about to walk out the door though so will check it when I get back home

bdraco · 2024-10-18T18:54:50Z

If everything passes, I'll throw it on production and make sure there aren't any unexpected side effects as soon as I have some spare cycles

Fixes aio-libs#9436

bdraco · 2024-10-23T05:16:22Z

Testing this now

bdraco · 2024-10-23T05:18:17Z

note for the future docs/client_advanced.rst has a conflict on backport.

bdraco

No unexpected side effects observed, however production has very limited use of auth, but enough that I feel comfortable it won't break most use cases.

bdraco · 2024-10-28T19:12:10Z

Thanks @PLPeeters

patchback · 2024-10-28T19:12:15Z

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

❌ Failed to cleanly apply 06b2398 on top of patchback/backports/3.10/06b23989d9a80da93eddf285960e00a01b078151/pr-9443

Backporting merged PR #9443 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/06b23989d9a80da93eddf285960e00a01b078151/pr-9443 upstream/3.10

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/06b23989d9a80da93eddf285960e00a01b078151/pr-9443

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.

patchback · 2024-10-28T19:12:21Z

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

❌ Failed to cleanly apply 06b2398 on top of patchback/backports/3.11/06b23989d9a80da93eddf285960e00a01b078151/pr-9443

Backporting merged PR #9443 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.11/06b23989d9a80da93eddf285960e00a01b078151/pr-9443 upstream/3.11

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.11/06b23989d9a80da93eddf285960e00a01b078151/pr-9443

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.

Co-authored-by: Pierre-Louis Peeters <[email protected]> (cherry picked from commit 06b2398)

bdraco · 2024-10-28T19:15:31Z

#9570

Co-authored-by: Pierre-Louis Peeters <[email protected]> (cherry picked from commit 06b2398)

bdraco · 2024-10-28T19:36:46Z

#9571

…entication (#9570) Co-authored-by: Pierre-Louis Peeters <[email protected]> Co-authored-by: Pierre-Louis Peeters <[email protected]>

…entication (#9571) Co-authored-by: Pierre-Louis Peeters <[email protected]> Co-authored-by: Pierre-Louis Peeters <[email protected]>

PLPeeters requested review from asvetlov and webknjaz as code owners October 9, 2024 15:10

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

Dreamsorcerer reviewed Oct 9, 2024

View reviewed changes

aiohttp/client.py Show resolved Hide resolved

Dreamsorcerer reviewed Oct 9, 2024

View reviewed changes

aiohttp/client.py Show resolved Hide resolved

Dreamsorcerer requested a review from bdraco October 9, 2024 17:35

bdraco reviewed Oct 10, 2024

View reviewed changes

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

bdraco reviewed Oct 10, 2024

View reviewed changes

aiohttp/client.py Show resolved Hide resolved

PLPeeters force-pushed the bugfix/auth-redirect branch from 9b8f85d to f21bc20 Compare October 11, 2024 08:24

webknjaz reviewed Oct 11, 2024

View reviewed changes