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

URLs that infinitely redirect to an authenticated Location will raise a ValueError instead of TooManyRedirects #9436

Closed
1 task done
PLPeeters opened this issue Oct 9, 2024 · 2 comments · Fixed by #9443
Assignees
Labels

Comments

@PLPeeters
Copy link
Contributor

PLPeeters commented Oct 9, 2024

Describe the bug

I encountered a URL that seems to always respond with this, even for subsequent authenticated calls:

HTTP/1.1 301 Moved Permanently
Server: nginx/1.18.0 (Ubuntu)
Date: Wed, 09 Oct 2024 07:12:26 GMT
Content-Type: text/html; charset=UTF-8
Connection: keep-alive
X-redirected: 1
Location: http://bourospan@itheka.gr

This then leads to a ValueError once the redirect is handled, instead of a TooManyRedirects error (which is what both Chrome and requests throw for this).

To Reproduce

import aiohttp
import asyncio


async def main():
    async with aiohttp.ClientSession() as session:
        await session.get('http://itheka.gr', allow_redirects=True)


if __name__ == "__main__":
    asyncio.run(main())

Expected behavior

It should raise a TooManyRedirectsError instead. A ValueError gives the impression the user did something wrong, while this is out of their control.

Logs/tracebacks

>>> import aiohttp
>>> import asyncio
>>> 
>>> 
>>> async def main():
...     async with aiohttp.ClientSession() as session:
...         await session.get('http://itheka.gr', allow_redirects=True)
... 
>>> 
>>> if __name__ == "__main__":
...     asyncio.run(main())
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/opt/homebrew/Cellar/python@3.12/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "<stdin>", line 3, in main
  File "/Users/peetersp/Documents/Swan/website-processors/venv/lib/python3.12/site-packages/aiohttp/client.py", line 591, in _request
    raise ValueError(
ValueError: Cannot combine AUTH argument with credentials encoded in URL

Python Version

$ python --version
Python 3.12.4

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.10.9
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /Users/peetersp/Documents/Swan/website-processors/venv/lib/python3.12/site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, yarl
Required-by:

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /Users/peetersp/Documents/Swan/website-processors/venv/lib/python3.12/site-packages
Requires: 
Required-by: aiohttp, yarl

propcache Version

$ python -m pip show propcache
WARNING: Package(s) not found: propcache

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.12.1
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache-2.0
Location: /Users/peetersp/Documents/Swan/website-processors/venv/lib/python3.12/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

Ubuntu, macOS

Related component

Client

Additional context

The root cause lies here:

aiohttp/aiohttp/client.py

Lines 558 to 562 in c4a3f27

if auth and auth_from_url:
raise ValueError(
"Cannot combine AUTH argument with "
"credentials encoded in URL"
)

I believe this could be fixed in one of two ways:

  1. Checking whether auth and auth_from_url are different before raising
  2. Only checking this for the initial URL and skipping the check for redirects

Option 2 is probably best, as the problem at hand is that URL-based auth can be out of the user's control, so it should only raise a ValueError if it is indeed the user that made a mistake, which can only really be the case for the initial URL in the redirect chain.

I'll happily PR this once I get the go-ahead for the method to use.

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@PLPeeters PLPeeters added the bug label Oct 9, 2024
@Dreamsorcerer
Copy link
Member

I don't think that check should be happening on redirects, only on the original url parameter. So, option 2.
I'm also wondering if we should be reusing that auth in a redirect at all, but that's a separate issue...

@Dreamsorcerer
Copy link
Member

It probably makes sense to just move that check to somewhere before the loop.

PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 9, 2024
PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 11, 2024
PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 11, 2024
PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 11, 2024
PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 14, 2024
PLPeeters pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 18, 2024
bdraco pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 18, 2024
bdraco pushed a commit to PLPeeters/aiohttp that referenced this issue Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants