Redact URL to hide password by expobrain · Pull Request #6295 · pypa/pip (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits10 Checks0 Files changed

Conversation

This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

When the verbose flag is used in pip install and in ~/.pip/pip.conf you setup the access to you private registry with basic auth, i.e. extra-index-url = https://username:pwd!@pypi.example.com/simple/, when using pip install -v the password is printed in clear to stdout.

This PR redact the URL to hide original password.

@@ -155,7 +166,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', _redacted_url(url))

Choose a reason for hiding this comment

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

You should use the redact_password_from_url() function which has this purpose.

Choose a reason for hiding this comment

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

Good to know!, Fixed in the next commit

Choose a reason for hiding this comment

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

A couple more comments.

"Getting page https://user:@example.com/simple/initools"
in result.stdout
)
virtualenv.clear()

Choose a reason for hiding this comment

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

Is this needed? If so, should it be in a finally block?

Choose a reason for hiding this comment

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

I used test_command_line_options_override_env_vars as a template and I assumed that you need to cleanup manually. Removed.

assert (
"Getting page https://user:@example.com/simple/initools"
in result.stdout
)

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

Choose a reason for hiding this comment

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

Done

"""
password = "my_password"

script.environ['PIP_INDEX_URL'] = (

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 ?

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

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.

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.

Choose a reason for hiding this comment

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

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

Can I use this PR for the news file or better create a new issue?

Thanks for asking. You can use this PR.

@@ -118,6 +118,29 @@ def test_get_html_response_no_head(url):
]


@mock.patch("pip._internal.index.logger")
def test_get_html_response_dont_log_clear_text_password(m_logger):

Choose a reason for hiding this comment

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

You shouldn't need to mock logging. I'll repeat my suggestion from before:

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

Choose a reason for hiding this comment

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

I forgot about it, and fixed in the next commit

Choose a reason for hiding this comment

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

Looking a lot better now. Thanks for sticking it out! Just a couple more minor comments..

def test_get_html_response_dont_log_clear_text_password(caplog):
"""
`_get_html_response()` shouldn't send a HEAD request if the URL does not
look like an archive, only the GET request that retrieves data.

Choose a reason for hiding this comment

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

Looks like this docstring was copied without updating.


caplog.set_level(logging.DEBUG)

resp = _get_html_response(url_template.format(password), session=session)

Choose a reason for hiding this comment

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

Instead of using url_template, it would be simpler and more direct just to pass https://user:my_password@example.com/simple/.

record = caplog.records[0]
assert record.levelname == 'DEBUG'
assert record.message.splitlines() == [
"Getting page {}".format(url_template.format("****")),

Choose a reason for hiding this comment

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

Again, you can just check that actual string instead of using nested format strings, so "Getting page https://user:****@example.com/simple/"... (It's generally better to be more explicit in test code.)

look like an archive, only the GET request that retrieves data.
"""
password = "my_password"
url_template = "https://user:{}@example.com/simple/"

Choose a reason for hiding this comment

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

You won't need these two variables (see below).

Choose a reason for hiding this comment

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

A couple wording comments.

@@ -0,0 +1 @@
Redacts the URL of the extra index when `pip -v` to hide the user's password

Choose a reason for hiding this comment

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

Since the URL isn't being removed but only the password, it would be better to say something like, "Redact the password from the extra index URL when using pip -v." Also, you want to use double backticks before and after "pip -v" rather than single, and end the sentence with a period.