-
-
Notifications
You must be signed in to change notification settings - Fork 961
Remove user credentials in URLs when converting to a string #3513
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
Conversation
60f4499
to
fb53495
Compare
fb53495
to
0441070
Compare
) | ||
|
||
return f"{self.__class__.__name__}({url!r})" | ||
return f"{self.__class__.__name__}({str(self._uri_reference)!r})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems simpler, rather than re-implementing the logic already in _urlparse.py
. The user credentials are sent in a header rather than the URL anyway.
0441070
to
7395692
Compare
Yep, hiding user credentials at the lowest layer and preventing them from being passed higher makes perfect sense. Now it's exposed in URL.str, but not in URL.repr, which is a bit weird. str is supposed to show more user-related data, while repr is more for debugging and development. So, hiding it in repr would make more sense. However, I think preventing it entirely is the most secure way. |
Just to clarify - after the change in this PR, the username & password are not exposed in either str or **repr✱. I do think this is the most secure implementation, and I don't think we really lose anything.. |
Anyone have a view on when this might get merged (and released)? |
Seems to be taking an age to be reviewed/merge so in the meantime, if you find this and don't want to log secrets you could just alter the log level of httpx... import logging
httpx_logger = logging.getLogger("httpx")
httpx_logger.setLevel(logging.WARNING) |
Thank you no. |
Summary
As previously noted in this GitHub discussion, this library by default leaks credentials which are included in URL strings (common for basic authentication). It can also raise exceptions which contain the credentials in the error string if a request fails (see
raise_for_status
).This PR updates the
__str__
method on URLs to remove the user & password details. I believe this is the correct default behaviour for a library like this, as it avoids any risk of leakage. Removing the user & password entirely seems both (a) the safest option, and (b) the simplest implementation. These credentials are passed as headers in reality, and are not technically part of the URL.Checklist