-
-
Notifications
You must be signed in to change notification settings - Fork 172
🐛 Raises meaningful exception when IPv6 URL is malformed #1512
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.
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
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #1512 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project check has failed because the head coverage (98.06%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1512 +/- ##
=======================================
Coverage 99.37% 99.37%
=======================================
Files 30 30
Lines 6068 6079 +11
Branches 265 265
=======================================
+ Hits 6030 6041 +11
Misses 35 35
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e3af387
to
5001d56
Compare
tests/test_url.py
Outdated
with pytest.raises(ValueError, match="An IPv4 address cannot be in brackets"): | ||
URL("http://[127.0.0.1]/") | ||
@pytest.mark.parametrize( | ||
("url"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("url"), | |
"url", |
note: we agreed with Mael that he'll add a change note later on, while traveling |
5001d56
to
f89d97b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaelPic thanks for your contribution during the PyCon US Sprints last week!
Would you me able to make the following cosmetic changes? Or do you need us to pick up the PR and complete it instead?
CHANGES/1512.bugfix.rst
Outdated
It fixes the issue where exception `IndexError` was raised in the | ||
following conditions: empty IPv6 URL, brackets in reverse order. | ||
|
||
-- by :user:` MaelPic`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- by :user:` MaelPic`. | |
-- by :user:`MaelPic` |
CHANGES/1512.bugfix.rst
Outdated
It fixes the issue where exception `IndexError` was raised in the | ||
following conditions: empty IPv6 URL, brackets in reverse order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fixes the issue where exception `IndexError` was raised in the | |
following conditions: empty IPv6 URL, brackets in reverse order. | |
These fixes the issue where exception :exc:`IndexError` was | |
leaking from the internal code because of not being handled and | |
transformed into a user-facing error. The problem was happening | |
under the following conditions: empty IPv6 URL, brackets in | |
reverse order. |
CHANGES/1512.bugfix.rst
Outdated
Aligned the type of exception raised for improper IPv6 URL values, | ||
which should be `ValueError`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned the type of exception raised for improper IPv6 URL values, | |
which should be `ValueError`. | |
Started raising a :exc:`ValueError` exception raised for corrupted | |
IPv6 URL values. |
tests/test_url.py
Outdated
"url", | ||
[ | ||
"http://[]/", # Empty IPv6 URL | ||
"http://[1]/", # No semicolon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(you probably meant “no colon” in there)
tests/test_url.py
Outdated
[ | ||
"http://[]/", # Empty IPv6 URL | ||
"http://[1]/", # No semicolon | ||
"http://[127.0.0.1]/", # IPv4 inside brackets | ||
"http://]1dec:0:0:0::1[/", # Brackets in reversed order | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having code comments, it's best to set explicit param ids. They will show up in the test report, and will make it easier to both reason about the testing semantics, plus can be used to select and run one of the test params with pytest
:
[ | |
"http://[]/", # Empty IPv6 URL | |
"http://[1]/", # No semicolon | |
"http://[127.0.0.1]/", # IPv4 inside brackets | |
"http://]1dec:0:0:0::1[/", # Brackets in reversed order | |
], | |
( | |
"http://[]/", | |
"http://[1]/", | |
"http://[127.0.0.1]/", | |
"http://]1dec:0:0:0::1[/", | |
), | |
ids=( | |
"empty-IPv6-like-URL", | |
"no-colons-in-IPv6", | |
"IPv4-inside-brackets", | |
"brackets-in-reversed-order", | |
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it is better
yarl/_parse.py
Outdated
@@ -69,11 +69,11 @@ def split_url(url: str) -> SplitURLType: | |||
# Valid bracketed hosts are defined in | |||
# https://www.rfc-editor.org/rfc/rfc3986#page-49 | |||
# https://url.spec.whatwg.org/ | |||
if bracketed_host[0] == "v": | |||
if bracketed_host.startswith("v"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if bracketed_host.startswith("v"): | |
if bracketed_host and bracketed_host[0] == "v": |
This is faster since there is no function call
…ersed Problem: If brackets are reversed or content inside brackets is empty, `bracketed_host` will be an empty string. In consequence, reaching the first element of `bracketed_host` returns an `IndexError`. Solution: Use `startswith` because it does not raise anything if the string is empty and then it fallbacks into the following check verifying the presence of `:`. Also, the raised message was made more generic but still keeps its meaning.
f89d97b
to
63ba8c5
Compare
What do these changes do?
For IPv6 URL, if brackets are set in reversed order (closing bracket before open bracket), the raised exception is now
ValueError("Invalid IPv6 URL")
, which is more convenient that the reported one (IndexError: string index out of range
).Are there changes in behavior for the user?
If url contains brackets in opposite order:
Previous behavior: exception
IndexError
is raisedNew behavior: exception
ValueError
is raisedRelated issue number
Fixes #1485
Checklist