Skip to content
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

Catch up to recent changes in whatwg/url #33315

Closed
domenic opened this issue May 8, 2020 · 10 comments
Closed

Catch up to recent changes in whatwg/url #33315

domenic opened this issue May 8, 2020 · 10 comments
Labels
url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@domenic
Copy link
Contributor

domenic commented May 8, 2020

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

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 whatwg/url@d589670 and whatwg/url@7ae1c69.

Finally I'll mention that whatwg/url#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
Copy link
Member

jasnell commented May 8, 2020

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.

/cc @nodejs/url

@jasnell jasnell added url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 8, 2020
@domenic
Copy link
Contributor Author

domenic commented May 8, 2020

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

@jasnell
Copy link
Member

jasnell commented May 8, 2020

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
Copy link
Member

targos commented May 9, 2020

I'm doing the gopher change.

PR: #33325

@yashLadha
Copy link
Contributor

yashLadha commented May 9, 2020

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

node/lib/url.js

Lines 311 to 312 in 8607f9e

case CHAR_LEFT_ANGLE_BRACKET:
case CHAR_RIGHT_ANGLE_BRACKET:

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

@yashLadha
Copy link
Contributor

yashLadha commented May 9, 2020

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.

args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));

A check can be placed at:

return _domainToASCII(`${domain}`);
on the return value

@annevk
Copy link

annevk commented May 9, 2020

whatwg/url#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
Copy link
Member

Trott commented May 10, 2020

(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
Copy link
Member

jasnell commented May 10, 2020

For additional issues, separate issues are better.

yashLadha added a commit to yashLadha/node that referenced this issue May 21, 2020
As per the recent changes in whatwg/url spec. lt and gt are also added
in the list of forbidden hostCodePoint list.

Ref: whatwg/url#459
Ref: nodejs#33315
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 23, 2020
As per the recent changes in whatwg/url spec. lt and gt are also added
in the list of forbidden hostCodePoint list.

PR-URL: nodejs#33328
Refs: whatwg/url#459
Refs: nodejs#33315
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos added a commit to targos/node that referenced this issue May 29, 2020
TimothyGu added a commit to whatwg/url that referenced this issue Jun 4, 2020
Per the discussion in nodejs/node#33315,
Node.js would like to have bugs filed whenever there is a normative spec
change. Make sure this is done by adding an item to the PR template.
targos added a commit that referenced this issue Jun 6, 2020
Refs: #33315
Refs: whatwg/url@d589670
Refs: whatwg/url@7ae1c69

PR-URL: #33325
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@watilde
Copy link
Member

watilde commented Sep 16, 2020

Closing as resolved by #33770. Thank you.

@watilde watilde closed this as completed Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests

7 participants