Skip to content

Change raise_for_status message to hide password in URL#5221

Open
Overv wants to merge 4 commits intopsf:mainfrom
Overv:fix-5021
Open

Change raise_for_status message to hide password in URL#5221
Overv wants to merge 4 commits intopsf:mainfrom
Overv:fix-5021

Conversation

@Overv
Copy link

@Overv Overv commented Oct 2, 2019

This PR fixes #5021 by replacing a password in the URL with asterisks.

Example:

import requests

r = requests.get("http://foo:secret@httpbin.org/status/401")
r.raise_for_status()

Output:

Traceback (most recent call last):
  File "5021.py", line 4, in <module>
    r.raise_for_status()
  File "***/python-requests/requests/requests/models.py", line 968, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 401 Client Error: UNAUTHORIZED for url: http://foo:***@httpbin.org/status/401

jackyzha0
jackyzha0 previously approved these changes Oct 7, 2019
Copy link

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we need to update in the documentation for this? Other than that, looks good! 👍

barroca
barroca previously approved these changes Oct 22, 2019
Copy link

@barroca barroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add unit test for this change?

@Overv
Copy link
Author

Overv commented Oct 22, 2019

@jackyzha0 @barroca I'll have time to get back to this next weekend.

@barroca
Copy link

barroca commented Oct 23, 2019

@Overv I opened a similar PR for this that might help you to write a test #5244 let me know if you need help.

@Overv
Copy link
Author

Overv commented Oct 28, 2019

I've added a unit test.

@Overv Overv dismissed stale reviews from barroca and jackyzha0 via a508b49 October 28, 2019 20:47
with pytest.raises(requests.exceptions.HTTPError) as e:
r.raise_for_status()

assert 'pass' not in str(e)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 I'd rather assert e is equals with the string with pass obfuscated, and also add a test that there doesn't obfuscate things that wasn't suppose to, but I'm happy to approve it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the test more specific now. How would you implement a test that checks if other things are not obfuscated?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a test like this:

    def test_status_raising_doesnt_hides_other_stuff(self, httpbin):
        host = urlparse(httpbin()).netloc

        r = requests.get('http://shouldntchange@' + host + '/status/404')
        with pytest.raises(requests.exceptions.HTTPError) as e:
            r.raise_for_status()

        host_without_port = host.split(':')[0]

        assert str(e.value) == '404 Client Error: NOT FOUND for url: http://shouldntchange@' + host_without_port + '/status/404'

This is just a validation that the match doesn't put wrong information if the password is not supplied

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding that test actually made me find two bugs!

  • The host in the exception message no longer included the port, which shouldn't happen.
  • Asterisks were included even if there was no password specified.

Thanks for the suggestion.

barroca
barroca previously approved these changes Oct 29, 2019
@nateprewitt nateprewitt changed the base branch from master to main January 3, 2022 15:28
@mpolena
Copy link

mpolena commented Sep 28, 2022

Running into this same issue for requests with client_id and client_secret parameters in the url. Those values would get exposed despite the proposed fix here. Seems like a more general solution is needed.

@ZenithClown
Copy link

Running into this same issue for requests with access_key and apikey which exposes the API key.

ZenithClown added a commit to sharkutilities/forexrates that referenced this pull request Mar 20, 2025
- reference issue psf/requests#5221
- unless this issue is not resolved, may have to do an alternate approach
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.

raise_for_status leaks HTTP basic auth

6 participants