Skip to content

gh-105704: Disallow IPv6 URLs with invalid prefix/suffix #111261

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

Closed
bcail wants to merge 5 commits into from
Closed

gh-105704: Disallow IPv6 URLs with invalid prefix/suffix #111261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23e23d9
gh-105704: Disallow IPv6 URLs with invalid prefix/suffix
bcail Oct 13, 2023
f75c6a5
Add checking for `bytes` URLs
bcail Oct 24, 2023
9cb076e
update to not require attribute access
bcail Nov 26, 2024
735c797
use _ for unused var
bcail Nov 26, 2024
4374ca4
add blurb
bcail Nov 26, 2024
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
Next Next commit
gh-105704: Disallow IPv6 URLs with invalid prefix/suffix
  • Loading branch information
@bcail
bcail committed Nov 22, 2024
commit 23e23d9aa20bda640860030108045584aa7db7ff
33 changes: 23 additions & 10 deletions Lib/test/test_urlparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1402,16 +1402,29 @@ def test_issue14072(self):
self.assertEqual(p2.path, '+31641044153')

def test_invalid_bracketed_hosts(self):

@orsenthil orsenthil Nov 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This refactoring, personally makes it hard to review. Could you please share the description, which existing test cases behavior are changing now (if any)? If you don't get to it, I will try to find out too. (But this can be considered for future refactors of test code).

@bcail bcail Dec 2, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure - here are the differences in the tests:

  • urlsplit and urlparse are now tested (not just urlsplit)
  • strings and bytes are now tested (not just strings)
  • four new test cases are added (the first ten are the same as they were before)

If I need to split the refactoring into a separate PR, I can do that.

self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[192.0.2.146]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[important.com:8000]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[v123r.IP]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[v12ae]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[v.IP]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[v123.]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[v]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[0439:23af::2309::fae7:1234]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@[0439:23af:2309::fae7:1234:2342:438e:192.0.2.146]/Path?Query')
self.assertRaises(ValueError, urllib.parse.urlsplit, 'Scheme://user@]v6a.ip[/Path')
cases = [
'Scheme://user@[192.0.2.146]/Path?Query',
'Scheme://user@[important.com:8000]/Path?Query',
'Scheme://user@[v123r.IP]/Path?Query',
'Scheme://user@[v12ae]/Path?Query',
'Scheme://user@[v.IP]/Path?Query',
'Scheme://user@[v123.]/Path?Query',
'Scheme://user@[v]/Path?Query',
'Scheme://user@[0439:23af::2309::fae7:1234]/Path?Query',
'Scheme://user@[0439:23af:2309::fae7:1234:2342:438e:192.0.2.146]/Path?Query',
'Scheme://user@]v6a.ip[/Path',
'Scheme://user@[v6a.ip/path?query',
'Scheme://user@v6a.ip]/path?query',
'Scheme://user@prefix.[v6a.ip]/path?query',
'Scheme://user@[v6a.ip].suffix/path?query',
]

for case in cases:
with self.subTest(case=case):
with self.assertRaises(ValueError):
urllib.parse.urlsplit(case).hostname

@gpshead gpshead Nov 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously urlsplit raised the ValueError. This PR as is changes that and only raises it upon hostname or port attribute access.

We need to continue raising the ValueError at urlsplit time. Existing code must be assumed to expect that split time validation behavior rather than at attribute access time.

@bcail bcail Nov 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks, @gpshead. I updated the code to do the check at urlsplit time.

with self.assertRaises(ValueError):
urllib.parse.urlparse(case).hostname

def test_splitting_bracketed_hosts(self):
p1 = urllib.parse.urlsplit('scheme://user@[v6a.ip]/path?query')
Expand Down
12 changes: 7 additions & 5 deletions Lib/urllib/parse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,15 @@ def _userinfo(self):
def _hostinfo(self):
netloc = self.netloc
_, _, hostinfo = netloc.rpartition('@')
_, have_open_br, bracketed = hostinfo.partition('[')
bracket_prefix, have_open_br, bracketed = hostinfo.partition('[')
if have_open_br:
if bracket_prefix:
raise ValueError('Invalid IPv6 URL')
hostname, _, port = bracketed.partition(']')
_, _, port = port.partition(':')
_check_bracketed_host(hostname)
bracket_suffix, _, port = port.partition(':')
if bracket_suffix:
raise ValueError('Invalid IPv6 URL')
else:
hostname, _, port = hostinfo.partition(':')
if not port:
Expand Down Expand Up @@ -504,9 +509,6 @@ def _urlsplit(url, scheme=None, allow_fragments=True):
if (('[' in netloc and ']' not in netloc) or
(']' in netloc and '[' not in netloc)):
raise ValueError("Invalid IPv6 URL")
if '[' in netloc and ']' in netloc:

@gpshead gpshead Nov 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be kept and updated to error appropriately as callers of urlsplit and urlparse expect the exception at parsing time. Not later on when accessing attributes.

bracketed_host = netloc.partition('[')[2].partition(']')[0]
_check_bracketed_host(bracketed_host)
if allow_fragments and '#' in url:
url, fragment = url.split('#', 1)
if '?' in url:
Expand Down