Fix the URL quoting in _clean_link() for IPv6 addresses #6245

nicolasbock · 2019-02-07T19:53:41Z

In cases where the base_url is a [] protected IPv6 address, the
_clean_link() function converts [ to %5B and ] to %5D, which
renders the base_url invalid. For example:

	Starting new HTTP connection (1): fd00:0:0:236::100:8181
	http://fd00:0:0:236::100:8181 "GET /os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt HTTP/1.1" 200 None
	Setting setuptools==40.6.3 (from -c http://[fd00:0:0:236::100]:8181/os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt (line 204)) extras to: ()
	Looking in indexes: http://[fd00:0:0:236::100]:8181/simple
	Collecting setuptools==40.6.3 (from -c http://[fd00:0:0:236::100]:8181/os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt (line 204))
	  1 location(s) to search for versions of setuptools:
	  * http://[fd00:0:0:236::100]:8181/simple/setuptools/
	  Getting page http://[fd00:0:0:236::100]:8181/simple/setuptools/
	  http://fd00:0:0:236::100:8181 "GET /simple/setuptools/ HTTP/1.1" 200 376
	  Analyzing links from page http://[fd00:0:0:236::100]:8181/simple/setuptools/
	    _package_versions: link = http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00:0:0:
	236::100]:8181/simple/setuptools/)
	    _link_package_versions: link = http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00
	:0:0:236::100]:8181/simple/setuptools/)
	    Found link http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00:0:0:236::100]:8181/
	simple/setuptools/), version: 40.6.3
	  Using version 40.6.3 (newest of versions: 40.6.3)
        Could not install packages due to an EnvironmentError.
        InvalidURL: Failed to parse: %5bfd00:0:0:236::100%5d:8181

This change moves the base_url outside the _clean_link call,
leaving it unaltered.

Signed-off-by: Nicolas Bock [email protected]

Fixes: #6285

src/pip/_internal/index.py

uranusjr · 2019-02-14T14:20:49Z

Replacing hand-craft URL escaping with stdlib calls is almost always the right thing to do. +1 to this with one small request in implementation.

nicolasbock · 2019-02-20T18:00:41Z

Do you want me to do anything else for this PR @uranusjr ?

uranusjr · 2019-02-21T00:40:25Z

Do you know why CI is failing? It would be most ideal if we can get a green tick. Also I think this change needs to appear in the changlog, instead of being classified as trivial.

nicolasbock · 2019-02-21T18:26:16Z

I updated the news item. I will have a look at the CI failure. I don't have Windows so hopefully I can get something out of the console output 😃

pradyunsg · 2019-03-12T12:34:23Z

Hey @nicolasbock! Did you manage to take a look at the CI failure?

nicolasbock · 2019-03-12T13:04:42Z

@pradyunsg Yes, but I haven't been able to make a lot of progress on it. Since I don't have access to Windows I can't reproduce the issue to troubleshoot. Do you have a suggestion as to how to go about this?

cjerdonek

Some comments.

tests/unit/test_url.py

cjerdonek · 2019-03-23T06:10:50Z

tests/unit/test_url.py

+           _clean_path)
+
+
+def test_ipv4_clean_link():


Rather than making a separate test for each test case, it would be better to use @pytest.mark.parametrize(). Then each test case can simply be an additional argument to parametrize().

Done. I am still using two extra variables, _path and _clean_path. Is that appropriate? Or should I fold that into the arguments for the decorator?

src/pip/_internal/index.py

cjerdonek · 2019-03-23T06:25:16Z

Also, I'm not completely sure, but maybe the Windows failure is in part because the PR causes the : after the drive letter to be quoted. From the CI output--

Collecting simple
  1 location(s) to search for versions of simple:
  * file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/
   file: URL is directory, getting file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
  Getting page file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
  Analyzing links from page file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
    Found link file:///T%3A/pytest-1/popen-gw2/test_file_index_url_quoting0/data/packages/simple-1.0.tar.gz#md5=4bdf78ebb7911f215c1972cf71b378f0 (from file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html), version: 1.0
  Using version 1.0 (newest of versions: 1.0)

nicolasbock · 2019-04-01T22:38:29Z

Also, I'm not completely sure, but maybe the Windows failure is in part because the PR causes the : after the drive letter to be quoted. From the CI output--

Collecting simple
  1 location(s) to search for versions of simple:
  * file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/
   file: URL is directory, getting file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
  Getting page file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
  Analyzing links from page file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html
    Found link file:///T%3A/pytest-1/popen-gw2/test_file_index_url_quoting0/data/packages/simple-1.0.tar.gz#md5=4bdf78ebb7911f215c1972cf71b378f0 (from file:///T:/pytest-1/popen-gw2/test_file_index_url_quoting0/data/indexes/in%20dex/simple/index.html), version: 1.0
  Using version 1.0 (newest of versions: 1.0)

That's a good point. I will have a look into how to parse a Windows path. Presumably / is equivalent to \?

nicolasbock · 2019-04-01T22:41:32Z

I added a Windowsish path to the test. Note that at this point this is expected to fail.

nicolasbock · 2019-04-02T17:45:52Z

@cjerdonek I think I got it now. The test is passing for me.

However, since there is no check whether pip is running on a Windows host, and I am simply assuming that anything that looks like a drive letter is a drive letter, there might be cases where it is desirable to keep the : in the "drive letter" quoted. For example, as far as I know, on Linux the following URI is legal: /C:/a/b/c where C: is not a drive letter obviously. Whether not quoting the : can be an issue I don't know.

src/pip/_internal/index.py

tests/unit/test_index.py

cjerdonek · 2019-04-04T12:16:20Z

@nicolasbock That looks good. That's what I meant.

For the few test cases where the test expectation will be platform-dependent, the way I would do it is break those few cases off into test_clean_link_windows() and test_clean_link_non_windows(), and then use an appropriate pytest condition so that the test will be skipped if the platform is windows (or not), etc. I'm sure you can find some existing examples in the test suite.

src/pip/_internal/index.py

nicolasbock · 2019-04-04T12:48:24Z

@nicolasbock That looks good. That's what I meant.

For the few test cases where the test expectation will be platform-dependent, the way I would do it is break those few cases off into test_clean_link_windows() and test_clean_link_non_windows(), and then use an appropriate pytest condition so that the test will be skipped if the platform is windows (or not), etc. I'm sure you can find some existing examples in the test suite.

Thanks for the hint @cjerdonek . I have split the one test into two as you suggested. Let's see if I got the test conditions for Windows right this time 😄.

cjerdonek · 2019-04-04T13:01:42Z

Looking through the code base, the pattern I see being used is this--

@pytest.mark.skipif("sys.platform != 'win32'")

(or == in place of !=)

nicolasbock · 2019-04-04T13:47:26Z

Looking through the code base, the pattern I see being used is this--
@pytest.mark.skipif("sys.platform != 'win32'")
(or == in place of !=)

Isn't there a win64 also?

uranusjr · 2019-04-04T13:48:42Z

No, 64-bit Windows also identifies itself as win32.

nicolasbock · 2019-04-04T16:35:54Z

Thanks @uranusjr . I adjusted the skip expressions @cjerdonek .

nicolasbock · 2019-04-05T17:05:01Z

@uranusjr , @pradyunsg , @cjerdonek , Any chance for another review?

@pfmoore Could you check this PR on Windows again to make sure I got this right?

Thanks all!

cjerdonek

Some wording suggestions on the comments. Otherwise looks good!

news/6285.bugfix

src/pip/_internal/index.py

tests/unit/test_index.py

When the `base_url` is a `[]` protected IPv6 address, the `_clean_link()` function converts `[` to `%5B` and `]` to `%5D`, which renders the `base_url` invalid. For example: ``` Starting new HTTP connection (1): fd00:0:0:236::100:8181 http://fd00:0:0:236::100:8181 "GET /os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt HTTP/1.1" 200 None Setting setuptools==40.6.3 (from -c http://[fd00:0:0:236::100]:8181/os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt (line 204)) extras to: () Looking in indexes: http://[fd00:0:0:236::100]:8181/simple Collecting setuptools==40.6.3 (from -c http://[fd00:0:0:236::100]:8181/os-releases/19.0.0.0b1/opensuse_leap-42.3-x86_64/requirements_absolute_requirements.txt (line 204)) 1 location(s) to search for versions of setuptools: * http://[fd00:0:0:236::100]:8181/simple/setuptools/ Getting page http://[fd00:0:0:236::100]:8181/simple/setuptools/ http://fd00:0:0:236::100:8181 "GET /simple/setuptools/ HTTP/1.1" 200 376 Analyzing links from page http://[fd00:0:0:236::100]:8181/simple/setuptools/ _package_versions: link = http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00:0:0: 236::100]:8181/simple/setuptools/) _link_package_versions: link = http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00 :0:0:236::100]:8181/simple/setuptools/) Found link http://%5bfd00:0:0:236::100%5d:8181/packages/opensuse_leap-42.3-x86_64/setuptools/setuptools-40.6.3-py2.py3-none-any.whl#md5=389d3cd088d7afec3a1133b1d8e15df0 (from http://[fd00:0:0:236::100]:8181/ simple/setuptools/), version: 40.6.3 Using version 40.6.3 (newest of versions: 40.6.3) Could not install packages due to an EnvironmentError. InvalidURL: Failed to parse: %5bfd00:0:0:236::100%5d:8181 ``` This change uses the vendored `urllib` library to split the host part off of the url before URL quoting only the path part. Fixes: pypa#6285 Signed-off-by: Nicolas Bock <[email protected]>

nicolasbock · 2019-04-07T11:57:58Z

Thanks for the comments @cjerdonek ! I pushed another iteration incorporating your suggestions.

cjerdonek

Looks great! Thanks for all your work and persistence on this. :) Before merging this I'll give @pfmoore another few days or so to take a look if he wants.

pfmoore · 2019-04-08T06:55:54Z

I'm OK with it - I'm not going to have time to have another look in the next few days, so go for it. And thanks @nicolasbock for all the work on this!

cjerdonek · 2019-04-08T07:09:35Z

Okay, thanks, @pfmoore!

cjerdonek · 2019-04-08T07:10:16Z

Thanks again, @nicolasbock!

nicolasbock · 2019-04-08T10:39:18Z

Thanks for you all your helpful reviews!

pradyunsg · 2019-04-11T10:27:28Z

This is nice! Thanks @nicolasbock for doing this and @cjerdonek, @pfmoore and @uranusjr for the reviews and inputs here. :D

lock · 2019-05-28T05:07:17Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

nicolasbock force-pushed the ipv6_uri branch 5 times, most recently from 2fe0cec to 15f17db Compare February 12, 2019 13:09

uranusjr reviewed Feb 14, 2019

View reviewed changes

src/pip/_internal/index.py Outdated Show resolved Hide resolved

nicolasbock force-pushed the ipv6_uri branch from 15f17db to 2719971 Compare February 14, 2019 18:20

nicolasbock force-pushed the ipv6_uri branch 2 times, most recently from cda40bf to 910e3e9 Compare February 21, 2019 18:25

pradyunsg added type: bugfix S: needs triage Issues/PRs that need to be triaged labels Mar 11, 2019

cjerdonek suggested changes Mar 23, 2019

View reviewed changes

cjerdonek removed the S: needs triage Issues/PRs that need to be triaged label Mar 23, 2019

nicolasbock force-pushed the ipv6_uri branch 2 times, most recently from a310119 to 9520258 Compare April 1, 2019 22:37

nicolasbock force-pushed the ipv6_uri branch from 9520258 to 64ef718 Compare April 2, 2019 17:40

uranusjr reviewed Apr 3, 2019

View reviewed changes

src/pip/_internal/index.py Outdated Show resolved Hide resolved

nicolasbock force-pushed the ipv6_uri branch from 64ef718 to fa249b5 Compare April 3, 2019 03:26

uranusjr reviewed Apr 3, 2019

View reviewed changes

src/pip/_internal/index.py Outdated Show resolved Hide resolved

cjerdonek reviewed Apr 3, 2019

View reviewed changes

tests/unit/test_index.py Outdated Show resolved Hide resolved

cjerdonek reviewed Apr 4, 2019

View reviewed changes

src/pip/_internal/index.py Outdated Show resolved Hide resolved

cjerdonek reviewed Apr 4, 2019

View reviewed changes

src/pip/_internal/index.py Show resolved Hide resolved

nicolasbock force-pushed the ipv6_uri branch from af1f5d9 to e1b6f22 Compare April 4, 2019 12:47

nicolasbock force-pushed the ipv6_uri branch from e1b6f22 to 14ba085 Compare April 4, 2019 16:35

nicolasbock force-pushed the ipv6_uri branch from 14ba085 to 0fc88a0 Compare April 4, 2019 16:44

cjerdonek reviewed Apr 7, 2019

View reviewed changes

nicolasbock force-pushed the ipv6_uri branch from 0fc88a0 to 913757c Compare April 7, 2019 11:57

cjerdonek approved these changes Apr 8, 2019

View reviewed changes

cjerdonek changed the title ~~Do not clean base_url~~ Fix the URL quoting in _clean_link() for IPv6 addresses Apr 8, 2019

cjerdonek added the C: finder PackageFinder and index related code label Apr 8, 2019

cjerdonek merged commit 54b6a91 into pypa:master Apr 8, 2019

nicolasbock deleted the ipv6_uri branch April 8, 2019 10:39

cjerdonek mentioned this pull request Apr 24, 2019

Pip install from internal pypi server containing git links fails with pypi 19.1 #6437

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019

lock bot locked as resolved and limited conversation to collaborators May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the URL quoting in _clean_link() for IPv6 addresses #6245

Fix the URL quoting in _clean_link() for IPv6 addresses #6245

nicolasbock commented Feb 7, 2019 •

edited

Loading

uranusjr commented Feb 14, 2019

nicolasbock commented Feb 20, 2019

uranusjr commented Feb 21, 2019

nicolasbock commented Feb 21, 2019

pradyunsg commented Mar 12, 2019

nicolasbock commented Mar 12, 2019

cjerdonek left a comment

cjerdonek Mar 23, 2019

nicolasbock Apr 1, 2019

cjerdonek commented Mar 23, 2019 •

edited

Loading

nicolasbock commented Apr 1, 2019

nicolasbock commented Apr 1, 2019

nicolasbock commented Apr 2, 2019 •

edited

Loading

cjerdonek commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

cjerdonek commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

uranusjr commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

nicolasbock commented Apr 5, 2019

cjerdonek left a comment

nicolasbock commented Apr 7, 2019

cjerdonek left a comment

pfmoore commented Apr 8, 2019

cjerdonek commented Apr 8, 2019

cjerdonek commented Apr 8, 2019

nicolasbock commented Apr 8, 2019

pradyunsg commented Apr 11, 2019 •

edited

Loading

lock bot commented May 28, 2019

Fix the URL quoting in _clean_link() for IPv6 addresses #6245

Fix the URL quoting in _clean_link() for IPv6 addresses #6245

Conversation

nicolasbock commented Feb 7, 2019 • edited Loading

uranusjr commented Feb 14, 2019

nicolasbock commented Feb 20, 2019

uranusjr commented Feb 21, 2019

nicolasbock commented Feb 21, 2019

pradyunsg commented Mar 12, 2019

nicolasbock commented Mar 12, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

cjerdonek Mar 23, 2019

Choose a reason for hiding this comment

nicolasbock Apr 1, 2019

Choose a reason for hiding this comment

cjerdonek commented Mar 23, 2019 • edited Loading

nicolasbock commented Apr 1, 2019

nicolasbock commented Apr 1, 2019

nicolasbock commented Apr 2, 2019 • edited Loading

cjerdonek commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

cjerdonek commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

uranusjr commented Apr 4, 2019

nicolasbock commented Apr 4, 2019

nicolasbock commented Apr 5, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

nicolasbock commented Apr 7, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

pfmoore commented Apr 8, 2019

cjerdonek commented Apr 8, 2019

cjerdonek commented Apr 8, 2019

nicolasbock commented Apr 8, 2019

pradyunsg commented Apr 11, 2019 • edited Loading

lock bot commented May 28, 2019

nicolasbock commented Feb 7, 2019 •

edited

Loading

cjerdonek commented Mar 23, 2019 •

edited

Loading

nicolasbock commented Apr 2, 2019 •

edited

Loading

pradyunsg commented Apr 11, 2019 •

edited

Loading