bpo-36338: Reject hostname with [ at position > 0 #14896
Conversation
Before:
>>> urlparse('http://good.com[malicious.com]/aoeu').hostname
'malicious.com'
After:
>>> urlparse('http://good.com[malicious.com]/aoeu')
ValueError: Invalid IPv6 URL
mangrisano
left a comment
mangrisano
left a comment
There was a problem hiding this comment.
LGTM. Thank you for providing the test as well.
|
Any time ! Will try to keep on to have always one one patch at the time, focusing on security issues at first ;) |
|
Thanks for the kind words, looking forward to review prior to starting on another ticket ;) |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Additional checks are very incomplete. IMHO the urllib.parser is a weak implementation of RFC 2396 and RFC 2732.
For example, I don't think such URLs are valid according to the RFCs:
>>> urlparse('http://google.com::::80/')
ParseResult(scheme='http', netloc='google.com::::80', path='/', params='', query='', fragment='')
>>> urlparse('http://[::1]/')
ParseResult(scheme='http', netloc='[::1]', path='/', params='', query='', fragment='')
>>> urlparse('http://[[::1]]/')
ParseResult(scheme='http', netloc='[[::1]]', path='/', params='', query='', fragment='')
>>> urlparse('http://[::1][]/')
ParseResult(scheme='http', netloc='[::1][]', path='/', params='', query='', fragment='')
IMHO the code should be rewritten to better respect the RFCs.
| (']' in netloc and '[' not in netloc)): | ||
| (']' in netloc and '[' not in netloc) or | ||
| ('[' in netloc and netloc.index('[') != 0)): | ||
| raise ValueError("Invalid IPv6 URL") |
There was a problem hiding this comment.
_splitnetloc() is called 2 times and the same code to validate the IPv6 address is duplicated, whereas you only fix one place. IMHO it would be better to move the check into _splitnetloc().
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I proposed a stricter change: PR #16780. |
6 participants
Before:
After:
https://bugs.python.org/issue36338