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

Jump to bottom

Fix inconsistent behavior of trusted host #6709

Merged
merged 2 commits into from
Aug 15, 2019
Merged

Fix inconsistent behavior of trusted host #6709

merged 2 commits into from
Aug 15, 2019

Conversation

frostming
Copy link
Contributor

@frostming frostming commented Jul 14, 2019 •
edited
Loading

Closes #6705

Propose for a hostname without port for the trusted-host option value and deprecate the other manner. Make sure all ports are recognized as insecure sources for the same host.

  • News entry
  • Test cases
  • Deprecation warning

@cjerdonek
Copy link
Member

Can you separate this into two PR's?

  1. Fix the bug so that --trusted-host works without a port in both http and https (https is the only one not working from your report).
  2. Add a deprecation warning if a port is included in --trusted-host.

@cjerdonek
Copy link
Member

Also, I think you should at least do the deprecation warning when the --trusted-hosts argument is being parsed. This happens in cmdoptions.py. You can look at python_version for an example of how argument checking can be done with a callback.

@frostming
Copy link
Contributor Author

Also, I think you should at least do the deprecation warning when the --trusted-hosts argument is being parsed. This happens in cmdoptions.py. You can look at python_version for an example of how argument checking can be done with a callback.

@cjerdonek Oh sure, does the deprecation still work if the value is read from pip.conf?

@cjerdonek
Copy link
Member

Yes, config values go through those option handling functions, too.

@frostming frostming mentioned this pull request Jul 14, 2019
Closed
cjerdonek
cjerdonek reviewed Jul 14, 2019
View reviewed changes
src/pip/_internal/download.py Outdated
@@ -598,6 +598,9 @@ def __init__(self, *args, **kwargs):
def add_insecure_host(self, host):
# type: (str) -> None
self.mount('https://{}/'.format(host), self._insecure_adapter)
if ":" not in host:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you be using some parsing function (e.g. based on urllib_parse.urlparse())? It doesn't seem like this would work e.g. with IPv6 addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried the function but it didn't work for the scheme missing. Is there a more elegant way to do this than supplying the scheme to form a full URL?

Copy link
Member

Choose a reason for hiding this comment

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

Well, a scheme is being added above anyways, so it wouldn’t require doing more than what’s already being done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, things get complicated when counting with IPv6 addresses, need a utility function to reliably construct a URL with IPv6 host(and potentially port).

cjerdonek
cjerdonek reviewed Aug 12, 2019
View reviewed changes
src/pip/_internal/download.py Outdated
@@ -598,6 +598,9 @@ def __init__(self, *args, **kwargs):
def add_insecure_host(self, host):
# type: (str) -> None
self.mount('https://{}/'.format(host), self._insecure_adapter)
if not urllib_parse.urlparse('https://{}/'.format(host)).port:
Copy link
Member

@cjerdonek cjerdonek Aug 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delay. Can you make this line into a unit-testable function? It can be something like does_netloc_have_port(netloc) and be put before split_auth_from_netloc(). It will be easy to test that way, and you can follow the example of how the related functions are tested. It would also be easy to include a couple IPv6 tests among those so people can see what happens to those (as well as other edge cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change as requested.

@pypa-bot
Copy link

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Aug 12, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 12, 2019
cjerdonek
cjerdonek reviewed Aug 13, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

A few comments.

src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
cjerdonek
cjerdonek reviewed Aug 14, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Some additional (final?) comments..

news/6705.bugfix Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Show resolved Hide resolved
tests/unit/test_utils.py Outdated Show resolved Hide resolved
tests/unit/test_utils.py Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Member

@frostming I squashed your commits and made some minor finishing changes, in preparation for merging. Thanks for your patience and for your excellent work on this! 💯

@cjerdonek cjerdonek merged commit c4e45e9 into pypa:master Aug 15, 2019
@cjerdonek
Copy link
Member

Thanks again!

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 14, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjerdonek cjerdonek cjerdonek approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Which is the recommended value of trusted host when the URL contains port
3 participants
@frostming @cjerdonek @pypa-bot