-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
url: improve url.parse() compliance with WHATWG URL #45011
Conversation
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).
Although this could be considered a semver-major breaking change, I'd argue this is a bugfix as the changed behavior would be very much for edge cases only and the current behavior seems not-intuitive and wrong. It is difficult to imagine a legitimate use case that would depend on it. Out of caution, though, we should run CITGM. |
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.
💯
@nodejs/url |
code > 127; | ||
|
||
// Invalid host character | ||
const isValid = (code !== CHAR_FORWARD_SLASH && |
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 think this is now too lax. It allows code points like U+005E (^), or U+007C (|) forbidden by the URL Standard.
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.
Before and after this change url.parse()
returns the same result for foo.com^bar.com
and foo.com|bar.com
. (I agree, though, that it would be better if it threw errors in those cases like it does for WHATWG URL, but that's probably a subsequent change. Small changes to url.parse()
will make it easier to back out if we mess up and break something in the ecosystem.)
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.
This is done in #45046.
CITGM results look good to me |
Landed in a8225dd |
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss). PR-URL: #45011 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss). PR-URL: #45011 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Could this be the cause of #45514? |
This comment was marked as spam.
This comment was marked as spam.
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss). PR-URL: #45011 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss). PR-URL: #45011 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API. Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss). PR-URL: #45011 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Make the url.parse() hostname parsing closer to that of WHATWG URL parsing. This mitigates for cases where hostname spoofing becomes possible if your code checks the hostname using one API but the library you use to send the request uses the other API.
Concerns about hostname-spoofing were raised and presented in excellent detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).