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

Remove user credentials in URLs when converting to a string #3513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zanderxyz
Copy link

@zanderxyz zanderxyz commented Feb 25, 2025

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

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly. [I did not make any documentation changes as I do not think any are needed]

@zanderxyz zanderxyz force-pushed the mask-credentials branch 3 times, most recently from 60f4499 to fb53495 Compare February 25, 2025 09:59
@zanderxyz zanderxyz changed the title Mask passwords in URLs when converting to a string Remove passwords in URLs when converting to a string Feb 25, 2025
@zanderxyz zanderxyz changed the title Remove passwords in URLs when converting to a string Remove user credentials in URLs when converting to a string Feb 25, 2025
)

return f"{self.__class__.__name__}({url!r})"
return f"{self.__class__.__name__}({str(self._uri_reference)!r})"
Copy link
Author

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.

@karpetrosyan
Copy link
Member

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.

@zanderxyz
Copy link
Author

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants