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 parsing of verify_ssl=false in urls with authentication in Pipfile #2656

Merged
merged 3 commits into from
Aug 1, 2018
Merged

Fix parsing of verify_ssl=false in urls with authentication in Pipfile #2656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0969a9e
Properly handle the first source having user/pass
Enzime Jul 27, 2018
fd5b4ba
Add news entry for pr #2656
techalchemy Aug 1, 2018
8cb2d99
Merge branch 'master' into fix/basic-auth-in-repo-URLs
techalchemy Aug 1, 2018
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
1 change: 1 addition & 0 deletions news/2656.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a bug which sometimes caused pipenv to parse the ``trusted_host`` argument to pip incorrectly when parsing source URLs which specify ``verify_ssl = false``.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bug wasn't a result of verify_ssl being false it was because the source contained a username and password.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to rewrite the fragment if you feel it is incorrect!

Copy link
Member

Choose a reason for hiding this comment

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

I mean, while the bug may not be caused by verify_ssl being false (I didn't say that it was caused by setting verify_ssl = false, only that it sometimes occurred when parsing URLs which specified this option), it can certainly only apply to URLs that specify verify_ssl = false as I mention in the news fragment, wouldn't you say?

Fixed a bug which sometimes caused pipenv to parse the trusted_host argument to pip incorrectly

This is the nature of the bug, no?

parsing source URLs which specify verify_ssl = false.

This is a condition under which the bug occurs, right? I didn't want to get super detailed, as it's most likely an edge case that most users won't encounter -- there is a bug parsing the trusted host argument sometimes if you specify the argument via your pipfile. There are other ways to specify trusted hosts, but this is the only one that is handled by pipenv directly, so it seems relevant to caveat this by highlighting the specific context without delving into the low level, 'if you first type x, then y, then z, then put a thing here, etc'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I got totally confused about the nature of the bug, after taking a look at the code again and remembering what the bug is. I totally agree with what you wrote.

2 changes: 1 addition & 1 deletion pipenv/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def prepare_pip_source_args(sources, pip_args=None):
# Trust the host if it's not verified.
if not sources[0].get("verify_ssl", True):
pip_args.extend(
["--trusted-host", urlparse(sources[0]["url"]).netloc.split(":")[0]]
["--trusted-host", urlparse(sources[0]["url"]).hostname]
)
# Add additional sources as extra indexes.
if len(sources) > 1:
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,20 @@ def test_nix_normalize_drive(self, input_path, expected):
"https://user:password@custom.example.com/simple",
],
),
(
[
{
"url": "https://user:password@custom.example.com/simple",
"verify_ssl": False,
},
],
[
"-i",
"https://user:password@custom.example.com/simple",
"--trusted-host",
"custom.example.com",
],
),
],
)
def test_prepare_pip_source_args(self, sources, expected_args):
Expand Down