Node: Catch up to recent changes in whatwg/url

[domenic]

The URL Standard has recently made a few minor normative changes which Node.js should probably track:

• Remove U+0000 case in fragment state
• Verify domain is not empty after Unicode ToASCII

It's also possible other not-so-recent changes were missed, e.g. I didn't see anything labeled whatwg-url when I searched for "gopher" on the issue tracker that would correspond to https://github.com/whatwg/url/commit/d589670451a4da9717bad69ed5d5364f93fede33 and https://github.com/whatwg/url/commit/7ae1c691c96f0d82fafa24c33aa1e8df9ffbf2bc.

Finally I'll mention that https://github.com/whatwg/url/pull/459 is likely to land soon (forbid < and > in hosts).

I've suggested to @annevk that he file bugs on Node.js for any such normative changes, but for now I wanted to file this one to help the project track.

[jasnell]

Thank you @domenic ... this is extremely helpful. I'm going to reframe your list into a todo checklist so we can use this as a tracking issue for these items.

• [x] [Remove U+0000 case in fragment state](https://github.com/whatwg/url/commit/3d574017081f6594ce1fa7a5107027c83dbe931f)
• [x] [Verify domain is not empty after Unicode ToASCII](https://github.com/whatwg/url/commit/cceb4356cca233b6dfdaabd888263157b2204e44)
• [x] gopher updates https://github.com/whatwg/url/commit/d589670451a4da9717bad69ed5d5364f93fede33 and https://github.com/whatwg/url/commit/7ae1c691c96f0d82fafa24c33aa1e8df9ffbf2bc (being worked on right now, #33325)
• [x] [Forbid < and > in hosts](https://github.com/whatwg/url/pull/459) (#33328)

/cc @nodejs/url

[domenic]

Note that there might be more; I didn't confirm when was the last time Node's implementation synced with the spec.

[jasnell]

Yeah, we're overdo for an audit on that for sure. I'll try to schedule some time to do it but wouldn't object if another contributor got to it first ;-)

[targos]

I'm doing the gopher change.

PR: https://github.com/nodejs/node/pull/33325

[yashLadha]

@jasnell I think Forbid < and > in hosts is already covered for not allowed hostname check:

https://github.com/nodejs/node/blob/8607f9ec5c2a488f5df7f9eb3e5f5c7dfa0b492a/lib/url.js#L311-L312

However, the check for the same is missing in node_url.cc

[yashLadha]

Can I take the Verify domain is not empty after Unicode ToASCII? @jasnell

I think it is probably related to checking for the empty host value as sometimes the hostname can be invalid.

https://github.com/nodejs/node/blob/8607f9ec5c2a488f5df7f9eb3e5f5c7dfa0b492a/src/node_url.cc#L2277

A check can be placed at: https://github.com/nodejs/node/blob/8607f9ec5c2a488f5df7f9eb3e5f5c7dfa0b492a/lib/internal/url.js#L1257 on the return value

[annevk]

https://github.com/whatwg/url/pull/505 is another one I'd like to make, though it still needs some implementer discussion. (I can start filing new issues from now on as well though, let me know.)

[Trott]

(I can start filing new issues from now on as well though, let me know.)

I would prefer a new issue for each thing rather than a perpetually open tracking issue, but @jasnell or others may feel differently. Either is way is better than not having the issue noted somewhere in the tracker, so although I have a mild preference, I'm happy either way. (And thanks!!!!)

[jasnell]

For additional issues, separate issues are better.

[watilde]

Closing as resolved by https://github.com/nodejs/node/pull/33770. Thank you.