-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Auth provided in URL skipped if user field is blank #6494
Comments
I know that cURL doesn't do post-processing of the user input and can send invalid queries if you ask it to so it's a not very good reference. Also, it looks like this may be a corner case, RFC-wise:
Plus, there are mentions of deprecations in various browsers too: https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#access_using_credentials_in_the_url. This basically means that the username is mandatory and the password is optional but not vice versa. I've seen some services (like GitHub) allowing the use of tokens in the username field to work around this. So technically, I'd say that aiohttp is RFC-compliant. But maybe if there are other libs (that are not |
Whilst working on a patch I noticed there seems to be two places the url.user is checked for basic auth - I've addressed both hopefully in PR #6495: |
Responding to your comments, I don't think the use of credentials in URLs is going to go away anytime soon, deprecated by the RFC or not, and I'm not sure that discussion of deprecation of the wider feature is relevant to handling this edge case. The rest of the quotation "Applications should not render as clear text any data..." to me at least seems to be talking about the "URL bar" in the web browser, and in mouseover on hyperlinks ie. we hide the password entered from the user so it would not show on screenshots etc. This seems a sensible precaution - but not really relevant here? I think that for web browsers with a human operator, removing HTTP Basic Auth and auth in URLs is the way forward - they are used rarely and from a usability point of view the 'authentication failed' workflow is quite poor. It's better to move the auth token into the path or query parts, by sending a long one-time URL and deal with expiry within a successful HTTP web page response that the user will understand and trust. Specifically dealing with a http client library driving an API, the use of credentials embedded into a URL is totally common place, as is the case here. We may as well support the edge cases and be compliant with both observed behavior in other libraries, and required behavior by some services. Of course the developer can work around this by cracking the URL first, reassembling it without the authentication, and passing in our own explicit |
I must note that another side of this precaution concerns logging — anything included in the URL (authority or query-string) can potentially leak to the logs in multiple proxies (it's not uncommon that there's at least one, and each place needs to be sanitized). So no, it's not only a user-facing representation.
Agreed. |
Describe the bug
Some services publish a URL like:
You can see the user field is an empty length string. When a request is made using aiohttp, the credentials are ignored and the server responds with a 403. This differs from requests and also curl, which parse this URL as having a zero-length username and a valid password.
To Reproduce
The request is sent without credentials
Expected behavior
Credentials should be sent
Logs/tracebacks
Python Version
aiohttp Version
$ python -m pip show aiohttp
multidict Version
yarl Version
OS
macOS
Related component
Client
Additional context
In
asyncio/helpers.py
specifically invokingstrip_auth_from_url
callsBasicAuth.from_url
line, which contains:The underlying cause appears to be that yarl returns
url.user=None
andurl.password=mypassword
. You can see thaturl.password
is not checked in this case. I will open a PR with suggested fix and test.Code of Conduct
The text was updated successfully, but these errors were encountered: