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

ValueError when redirecting to URL with incorrect schema #3315

Closed
berislavlopac opened this issue Oct 3, 2018 · 4 comments · Fixed by #6722
Closed

ValueError when redirecting to URL with incorrect schema #3315

berislavlopac opened this issue Oct 3, 2018 · 4 comments · Fixed by #6722

Comments

@berislavlopac
Copy link

Long story short

When the Client handles redirection, if the redirected URL contains an incorrect schema, it throws a ValueError:

if scheme not in ('http', 'https', ''):

While this is technically correct, it is more sensible to expect some kind of more specific error.

Expected behaviour

I would expect the Client to throw an aiohttp-specific error, preferably InvalidURL.

Actual behaviour

The Client throws a generic ValueError, which is difficult to specifically handle.

Steps to reproduce

Create a redirection URL, e.g. using an URL shortener like https://tinyurl.com/, and have it redirect to any URL with an incorrect schema, for example file://foo/bar. Then, call it with aiohttp

import aiohttp, asyncio

async def get_url(url):
    async with aiohttp.ClientSession() as s, s.get(url) as resp:
        print(await resp.text())

asyncio.get_event_loop().run_until_complete(get_url('https://tinyurl.com/y9rf58n8'))

Output:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/asyncio/base_events.py", line 468, in run_until_complete
    return future.result()
  File "<stdin>", line 2, in get_url
  File "/home/berislav/.virtualenvs/foobar/lib/python3.6/site-packages/aiohttp/client.py", line 855, in __aenter__
    self._resp = await self._coro
  File "/home/berislav/.virtualenvs/foobar/lib/python3.6/site-packages/aiohttp/client.py", line 456, in _request
    'Can redirect only to http or https')
ValueError: Can redirect only to http or https

Your environment

Python 3.6.6, and the installed packages:

Package       Version
------------- -------
aiohttp       3.4.4  
async-timeout 3.0.0  
attrs         18.2.0 
chardet       3.0.4  
idna          2.7    
idna-ssl      1.1.0  
multidict     4.4.2  
pip           18.0   
setuptools    40.4.3 
wheel         0.32.0 
yarl          1.2.6 
@aio-libs-bot
Copy link

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #3149 (Raise "TooManyRedirects" when redirect to new url with different host), #499 (Url GET parameters are lost when redirecting), #824 (URL with #), #1996 ([Client] Incorrect handling of '..' in url path.), and #56 (Redirect loop).

@webknjaz
Copy link
Member

webknjaz commented Oct 3, 2018

@berislavlopac I'd like to emphasize that it is not an invalid/incorrect schema, it's just non-HTTP.
Please refer to my earlier explanation: #2479 (comment).

I however agree with you that there must be a better exception retaining all context information needed.

Ref: #2507
Ref: #2335

@berislavlopac
Copy link
Author

Yes, that was basically my comment that we need something more informative and easier to manage upstream than ValueError. Seeing that this is essentially a duplicate of #2507 I'll close it.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
webknjaz added a commit that referenced this issue Feb 13, 2024
This patch introduces 5 granular user-facing exceptions that may occur
when HTTP requests are made:
* `InvalidUrlClientError`
* `RedirectClientError`
* `NonHttpUrlClientError`
* `InvalidUrlRedirectClientError`
* `NonHttpUrlRedirectClientError`

Previously `ValueError` or `InvalidURL` was raised and screening out was
complicated (a valid URL that redirects to invalid one raised the same error
as an invalid URL).

Ref: #6722 (comment)

PR #6722

Resolves #2507
Resolves #2630
Resolves #3315

Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
setla added a commit to setla/aiohttp that referenced this issue Feb 14, 2024
This patch introduces 5 granular user-facing exceptions that may occur
when HTTP requests are made:
* `InvalidUrlClientError`
* `RedirectClientError`
* `NonHttpUrlClientError`
* `InvalidUrlRedirectClientError`
* `NonHttpUrlRedirectClientError`

Previously `ValueError` or `InvalidURL` was raised and screening out was
complicated (a valid URL that redirects to invalid one raised the same error
as an invalid URL).

Ref: aio-libs#6722 (comment)

PR aio-libs#6722

Resolves aio-libs#2507
Resolves aio-libs#2630
Resolves aio-libs#3315

Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
(cherry picked from commit fb465e1)
webknjaz pushed a commit that referenced this issue Feb 14, 2024
…rchy in the HTTP client (#8158)

**This is a backport of PR #6722 as merged into master
(fb465e1).**

This patch introduces 5 granular user-facing exceptions that may occur
when HTTP requests are made:
* `InvalidUrlClientError`
* `RedirectClientError`
* `NonHttpUrlClientError`
* `InvalidUrlRedirectClientError`
* `NonHttpUrlRedirectClientError`

Previously `ValueError` or `InvalidURL` was raised and screening out was
complicated (a valid URL that redirects to invalid one raised the same
error
as an invalid URL).

Ref:
#6722 (comment)

PR #6722

Resolves #2507
Resolves #2630
Resolves #3315

Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
(cherry picked from commit fb465e1)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants