url: treat special characters in hostnames more strictly in url.parse() #45046
+43
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
needs-ci
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
lpinca
reviewed
Oct 18, 2022
lib/url.js
Outdated
| this.host = rest.slice(start, nonHost); | ||
| // WHATWG URL removes tabs, newlines, and carriage returns. Let's do that too. |
There was a problem hiding this comment.
It does it everywhere, not only on the host component. I'm not sure if url.parse() should do the same and how many breaking changes are tolerable.
There was a problem hiding this comment.
It does it everywhere, not only on the host component. I'm not sure if
url.parse()should do the same and how many breaking changes are tolerable.
I agree in principle, but I think narrowly scoping to hostname to start (and maybe even stopping there) makes a lot of sense as hostname spoofing is a bigger security concern than path or scheme spoofing. (Or at least that's my assumption. If anyone else thinks that's incorrect, I'm open to being persuaded.)
This comment was marked as resolved.
This comment was marked as resolved.
BridgeAR
approved these changes
Oct 18, 2022
|
@Trott Can we run a benchmark to see the performance impact of this pull request? |
Sure. But I think I'm OK if |
jasnell
approved these changes
Oct 18, 2022
There was a problem hiding this comment.
when this goes out in a release, let's be sure to include details on the performance impact of these changes. The perf regression is a key reason why these kinds of changes weren't made before. What we're essentially saying is that we are no longer committing to maintaining the same perf on url.parse().
anonrig
approved these changes
Oct 18, 2022
15 tasks
Trott
added
commit-queue-squash
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
|
To unbreak one of the benchmark URL, we use toAscii()/punycode when we find % in a domain, like WHATWG URL. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Here are all 55 of the three-star/highest-confidence results from the benchmark run. 😱 |
|
Benchmark with
|
This was referenced Oct 20, 2022
Trott
force-pushed
the
stricter
branch
4 times, most recently
from
769ffa2
to
105b71a
Compare
Throw if ^, |, and some other special characters are in the hostname, similar to WHATWG URL. Use punycode/toAscii when % appears in a hostname, like WHATWG URL.
|
Benchmark on latest changes: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1206/ |
watilde
approved these changes
Nov 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Throw if ^, |, and some other special characters are in the hostname, similar to WHATWG URL.