◐ Shell
reader mode source ↗
Skip to content

bpo-36338: Reject hostname with [ at position > 0#14896

Closed
jpic wants to merge 1 commit into
python:masterfrom
jpic:bpo-36338
Closed

bpo-36338: Reject hostname with [ at position > 0#14896
jpic wants to merge 1 commit into
python:masterfrom
jpic:bpo-36338

Conversation

@jpic

@jpic jpic commented Jul 21, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Before:

>>> urlparse('http://good.com[malicious.com]/aoeu').hostname
'malicious.com'

After:

>>> urlparse('http://good.com[malicious.com]/aoeu')
ValueError: Invalid IPv6 URL

https://bugs.python.org/issue36338

@jpic
bpo-36338: Reject hostname with [ at position > 0 …
7313bf4 
Before:

    >>> urlparse('http://good.com[malicious.com]/aoeu').hostname
    'malicious.com'

After:

    >>> urlparse('http://good.com[malicious.com]/aoeu')
    ValueError: Invalid IPv6 URL
mangrisano
mangrisano approved these changes Jul 22, 2019
View reviewed changes

@mangrisano mangrisano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM. Thank you for providing the test as well.

@jpic

jpic commented Aug 5, 2019

Copy link
Copy Markdown
Contributor Author

Any time ! Will try to keep on to have always one one patch at the time, focusing on security issues at first ;)
Have fun !

CuriousLearner
CuriousLearner approved these changes Aug 11, 2019
View reviewed changes

@CuriousLearner CuriousLearner left a comment

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.

Hide comment

Looks good to me! 🌮

@jpic

jpic commented Oct 5, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the kind words, looking forward to review prior to starting on another ticket ;)

vstinner
vstinner requested changes Oct 14, 2019
View reviewed changes

@vstinner vstinner left a comment

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.

Hide 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.

@bedevere-bot

bedevere-bot commented Oct 14, 2019

Copy link
Copy Markdown

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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@vstinner

vstinner commented Oct 14, 2019

Copy link
Copy Markdown
Member

I proposed a stricter change: PR #16780.

@jpic jpic closed this Oct 15, 2019
@jpic

jpic commented Oct 15, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for your comments @vstinner, apparently #16780 superseeds this one, nice approach BTW.

@sanebow sanebow mannequin mentioned this pull request Apr 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

@CuriousLearner CuriousLearner CuriousLearner approved these changes

+1 more reviewer

@mangrisano mangrisano mangrisano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jpic @bedevere-bot @vstinner @CuriousLearner @mangrisano @the-knights-who-say-ni