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.

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

Support including port part in trusted-host #6909

Merged
merged 2 commits into from
Aug 25, 2019

Conversation

frostming
Copy link
Contributor

Closes #6886
Closes #6710

This PR proposes official support for including port part in trusted-host option, for both HTTP and HTTPS indexes. So:

--trusted-host example.com trusts all these indexes:

  • http://example.com
  • http://example.com:8080
  • https://example.com
  • https://example.com:8080

While --trusted-host example.com:8080 only trusts the indexes with port 8080 exactly:

  • http://example.com:8080
  • https://example.com:8080

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.

Thanks! A couple comments. Also, the command-line help should be updated to reflect the change.

src/pip/_internal/download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Show resolved Hide resolved
@frostming frostming force-pushed the feature/trusted-host-port branch 6 times, most recently from ef3ad9a to dbf4120 Compare August 23, 2019 09:26
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.

Thanks for the updates. Looking a lot better! Some more comments.

src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.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
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
@frostming frostming force-pushed the feature/trusted-host-port branch 3 times, most recently from c5dc2e4 to 7f184ee Compare August 24, 2019 00:51
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.

Thanks for your changes. One more comment that I can see. Almost there!

src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Member

Oh, and there is still the comment from before re: updating the command-line help to say that host-port pair is also acceptable. That code is in cmdoptions.py.

@cjerdonek
Copy link
Member

I came across another thought: what if the index URL is HTTPS and contains auth part? ...

@frostming By the way, regarding the comment quoted above, would you mind filing that as a separate issue so we don't lose track of it? It would be much appreciated..

@cjerdonek
Copy link
Member

FYI, I made some minor formatting tweaks in preparation for merging.

@cjerdonek cjerdonek added the type: enhancement Improvements to functionality label Aug 25, 2019
@cjerdonek cjerdonek merged commit 8ac2214 into pypa:master Aug 25, 2019
@cjerdonek
Copy link
Member

Thanks again, @frostming! 🎉

@frostming frostming deleted the feature/trusted-host-port branch August 27, 2019 01:18
@frostming
Copy link
Contributor Author

@cjerdonek Thanks for that, I created #6931 to track the remaining issue.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support (or don't support) including a port number in --trusted-host
2 participants