-
Notifications
You must be signed in to change notification settings - Fork 3k
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 username/password from log message #5339
Conversation
…Looking in indexes" log messages
@@ -63,7 +63,7 @@ def export(self, location): | |||
"""Export the svn repository at the url to the destination location""" | |||
url, rev = self.get_url_rev() | |||
rev_options = get_rev_options(self, url, rev) | |||
url = self.remove_auth_from_url(url) | |||
url = remove_auth_from_url(url) |
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 removes the auth information that will later be used in the command 👎
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.
It's the behavior it had prior to this patch; I think that's fine.
@@ -84,7 +84,7 @@ def update(self, dest, rev_options): | |||
def obtain(self, dest): | |||
url, rev = self.get_url_rev() | |||
rev_options = get_rev_options(self, url, rev) | |||
url = self.remove_auth_from_url(url) | |||
url = remove_auth_from_url(url) |
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.
dito here
Needs a NEWS fragment. |
Thanks for this PR @jzafran! Could you update this PR by adding a news entry as per https://pip.pypa.io/en/latest/development/#adding-a-news-entry? |
@pradyunsg all set; I added a news entry |
tests/unit/test_utils.py
Outdated
@@ -624,3 +625,29 @@ def test_call_subprocess_works_okay_when_just_given_nothing(): | |||
def test_call_subprocess_closes_stdin(): | |||
with pytest.raises(InstallationError): | |||
call_subprocess([sys.executable, '-c', 'input()']) | |||
|
|||
|
|||
def test_remove_auth_from_url(): |
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.
Could you use pytest's "parameterized tests" here? (Also, maybe remove the svn naming of the URLs)
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.
Done
@@ -139,3 +139,18 @@ def test_secure_origin(location, trusted, expected): | |||
logger = MockLogger() | |||
finder._validate_secure_origin(logger, location) | |||
assert logger.called == expected | |||
|
|||
|
|||
def test_get_formatted_locations_basic_auth(): |
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.
Not for this PR but get_formatted_locations
needs tests.
Thanks for this @jzafran! ^.^ |
Sweet! |
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. |
Sanitize the output of the "Looking in indexes:" log message (added in #4483) to remove the username/password for basic authentication if configured.
Currently, when using a basic authentication-protected index/repository, the username/password is printed to stdout during a
pip install
:With this change, the username and password is removed from the log statement:
Fixes #5249