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.

Sign up for GitHub

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

Redact URL to hide password #6295

Merged
merged 10 commits into from
Mar 1, 2019
Merged

Redact URL to hide password #6295

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
00b872f
Redact URL to hide password
expobrain Feb 23, 2019
34b123d
Use already existent utility function ot hide password
expobrain Feb 24, 2019
5c232bc
Updated unit test
expobrain Feb 24, 2019
ddb7320
Pass index url straight to the command
expobrain Feb 24, 2019
70dee95
Moved test into unit
expobrain Feb 25, 2019
c33535b
Use caplog instead of mock
expobrain Feb 27, 2019
1214941
Added NEWS file
expobrain Feb 27, 2019
5bfb47c
Improved unit test
expobrain Feb 28, 2019
137a7ab
Better unit test comment
expobrain Mar 1, 2019
b37ad3f
Better NEWS files content
expobrain Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pip/_internal/index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def _get_html_response(url, session):
if _is_url_like_archive(url):
_ensure_html_response(url, session=session)

logger.debug('Getting page %s', url)
logger.debug('Getting page %s', redact_password_from_url(url))

resp = session.get(
url,
Expand Down
18 changes: 18 additions & 0 deletions tests/functional/test_install_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,24 @@ def test_command_line_options_override_env_vars(script, virtualenv):
assert "Getting page https://download.zope.org/ppix" in result.stdout


def test_no_password_in_debug_message(script, virtualenv):
"""
Test that password in the URL is not logged
"""
password = "my_password"

script.environ['PIP_INDEX_URL'] = (
Copy link
Member

Choose a reason for hiding this comment

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

Why the environ options instead of passing '--index-url', 'https://user:{}@example.com/simple/'.format(password) to script.pip ?

Copy link
Member

Choose a reason for hiding this comment

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

@xavfernandez Can you confirm if this test hits the network as written? It would be a lot better IMO if it didn't as I think such cases should be reduced to a minimum.

@expobrain It might be better to write a unit test (so in the unit directory rather than functional) of _get_html_response() to avoid hitting the network. You can look at test_get_legacy_build_wheel_path__no_names() and nearby functions for examples of using the caplog fixture (since the code wouldn't be running in a separate subprocess).

Copy link
Member

@cjerdonek cjerdonek Feb 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

PS - it looks like there are already a number of unit tests of that function (search for test_get_html_response_), and you can follow the examples there of mocking the session response.

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm if this test hits the network as written? It would be a lot better IMO if it didn't as I think such cases should be reduced to a minimum.

I confirm it should (I was actually thinking about suggesting to add the network marker to the test) and I don't see a way around that for a functional test.
An unit test could indeed also remove the need for network.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, here's an example of a test failing because of flakiness due to installing INITools in the test: https://dev.azure.com/pypa/pip/_build/results?buildId=5274&view=logs&jobId=c897e773-a04c-5d18-b461-2c6584d92c5a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjerdonek I've moved the test into unit, please have a look at it

'https://user:{}@example.com/simple/'.format(password)
)
result = script.pip('install', '-vvv', 'INITools', expect_error=True)

assert password not in result.stdout
assert (
"Getting page https://user:***@example.com/simple/initools"
in result.stdout
)
Copy link
Member

@cjerdonek cjerdonek Feb 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In addition, you might as well assert that my_password is not in the output. (And I would assign my_password to a variable so that that portion of the test doesn't pass because of a typo.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done



@pytest.mark.network
def test_env_vars_override_config_file(script, virtualenv):
"""
Expand Down