Don't coerce numbers to strings for port checking

[benjamingr]

Following discussion from nodejs/node#19595 cc @annevk @nodeav

At the moment URLs convert ports passed as numbers to strings in order to get the port number (specified in https://url.spec.whatwg.org/#port-state ). This is like the parseInt algorithm in that it ignores things after the first dot - which enables the following behavior:

var port = readPort(); // attacker returns 30 ** 30, , which gets coerced to 2.05891132094649e+44
// our validation: don't allow opening a connection to a lower-than 1024 port to the server.
if (typeof port !== 'number' || port < 1024) { 
  return false; 
}
serverUrl.port = port; // port set to 2
download(serverUrl); // user connected to port we didn't mean them to be able to connect to

This is a feature request to validate and ignore passed numbers to the port setter.

[annevk]

So this would mean changing the type from USVString to ([EnforceRange] unsigned short or USVString) although then it would end up throwing.

If we want to ignore out-of-band values something like (double or USVString) is probably the way to go.

Either way, prose will need to be adjusted as well to deal with the fact that it can now receive a number.

[benjamingr]

I am expressing implementer interest for Node.js (not sure if that counts, works that way, or if I get to do that - but I noticed the flag and we are a platform after all).

[domenic]

Why include USVString in the type? I guess for the return value :-/. Unfortunate IDL limitation...

Alternately you could modify the parser to fail to parse such ports instead of gobbling up as much as it can?

[annevk]

I think we also want to keep ToString() for any objects passed to the setter (I haven't verified how IDL deals with number or string though, perhaps it does the wrong thing).

We could also modify the parser or perhaps the setter to terminate early if the value contains a "." or "e" or some such. Maybe that's easier.

[TimothyGu]

I think modifying the parser will be easier, but we have to make sure not to change how the parser works for non-state-overridden usage (i.e., full URL parses).

[benjamingr]

What would it take to move forward on this?

[annevk]

A concrete proposal for what exactly the change ends up looking like (perhaps even in the form of a PR) would be good. Then we'll have to convince at least one browser to implement it to see if it's actually feasible, update and add tests, and then get everyone else along too.

[annevk]

See also https://whatwg.org/working-mode#changes.

[benjamingr]

A concrete proposal for what exactly the change ends up looking like (perhaps even in the form of a PR) would be good. Then we'll have to convince at least one browser to implement it to see if it's actually feasible, update and add tests, and then get everyone else along too.

Some very general questions:

• Would Node qualify as an implementor?
• Is Node a member of WHATWG?

We've been integrating web APIs (like URL) and are looking into integrating more WHATWG APIs where it makes sense to improve browser <-> node compatibility.

[annevk]

Node qualifies as an implementer, but I think in order to make changes on the web we'll continue to need two browsers. I think TC39 is roughly equivalent in that regard. If Node wanted to change something I'd expect that to be taken seriously though, if that's what you mean.

In terms of standards development WHATWG talks about participants: https://whatwg.org/workstream-policy. Node does not appear to be listed in https://github.com/whatwg/participant-data/blob/master/entities.json so they have not signed up at https://participate.whatwg.org/agreement (yet).

The Steering Group does have members, but they don't participate in making standards and mostly serve as an escalation path. As per https://whatwg.org/sg-agreement that's currently a closed group.

Hope that helps, if you want to discuss this further I suggest raising an issue at https://github.com/whatwg/meta/issues as it's a little off-topic here.

[benjamingr]

Thanks a lot! I'll take this up internally in Node first and if I have any more questions I'll open a meta issue.