-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
host vs hostname inconsistency #16712
Comments
@sholladay Would it make sense as a first step to add some more documentation about this behavior? |
What's the expected behaviour? |
@maclover7 Perhaps calling out the inconsistency would be nice. I don't find the documentation to be bad, per se, though. It's the API design that should be improved. @PatrickHeneise The expected behavior is that const { URL } = require('url');
const server = require('net').createServer();
const { host, hostname, port } = new URL('http://localhost:3000');
server.listen({ host }, () => {}); // errors
server.listen({ hostname, port }, () => {}); // listens on the wrong hostname |
The Perhaps a good middle ground is to follow what |
@bnoordhuis nothing I am asking for here should be a breaking change, as far as I can tell.
There seems to be a clear path towards whatwg URL compatibility without any breakage changes. |
Whether that can be done in a backwards compatible way depends on whether host+port strings can always be unambiguously distinguished from strings that just look like them. I'm thinking of strings like (I made ^ up on the spot. More realistic examples can be construed with a bit more effort.) |
From what I can tell playing around in the REPL, Node doesn't allow http.createServer().listen({ host : 'abba::1972' })
Error: Invalid listen argument: { host: 'abba::1972' } http.createServer().listen({ host : '::1' })
Error: Invalid listen argument: { host: '::1' } |
That's the other thing you commented on, the mandatory (Touches on another issue: what if |
Oh, right. True enough... the error message is misleading. At any rate, from what I've read, part of the purpose of the URL spec was to clean up the grammar. I imagine they have a good way to distinguish IPv6 and such from hosts with ports. But I confess I don't know the details of how that works. Since Node already has the URL parser implemented, perhaps it should just use that directly and not worry about re-implementing anything? As for conflicting input about the port number, here again I would go for consistency with the existing APIs. I don't think WHATWG provides any functionality that is prone to that, but Node's url.format({ host : 'localhost:4000', port : 5000 });
// 'localhost:4000' It could even do something like |
Using url parser directly won't really work without the other bits to make a valid url... e.g.: const myUrl = new url.URL(url.format({host: 'localhost:3000'}))
myUrl.protocol // "localhost:"
myUrl.pathname // "3000" It would need a dummy protocol (or maybe just http) to pull out hostname/port/host properly. And, even then, it wouldn't work for the current case, passing both const myUrl = new url.URL(url.format({protocol: 'http', host: 'localhost', port: 4000}))
myUrl.host // "localhost"
myUrl.hostname // "localhost"
myUrl.port // "" - port is lost when `host` provided over `hostname` |
I'm not necessarily asking for Node itself to directly use the WHATWG URL parser. That is an implementation detail that may or may not be appropriate (though it seems attractive, if it can be made to work). Personally, I just want one of the following to correctly listen on const { hostname, port } = new URL('http://localhost:3000');
server.listen({ hostname, port }, () => {}); const { host } = new URL('http://localhost:3000');
server.listen({ host }, () => {}); |
Throw error ERR_PROPERTY_NOT_IN_OBJECT if the port property does not exist in the options object argument when calling server.listen(). Refs: nodejs#16712
Throw error ERR_INVALID_ARG_VALUE if the port and path properties do not exist in the options object argument when calling server.listen(). Refs: #16712 PR-URL: #22085 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@sholladay If I'm not mistaken, the first example now works as expected in Node.js 10 and 11. Is that sufficient to close this issue? |
@Trott I'm not able to get either to work as expected on Node 11 (haven't tried 10). The first example still seems to listen on any address instead of const { hostname, port } = new URL('http://localhost:3000');
const server = http.createServer();
server.listen({ hostname, port });
server.address();
// => { address: '::', family: 'IPv6', port: 3000 } And for the other example: const { host } = new URL('https://localhost:3000');
const server = http.createServer();
server.listen({ host });
// TypeError [ERR_INVALID_ARG_VALUE]: The argument 'options' must have the property "port" or "path". Received { host: 'localhost:3000' } The behavior of that first example is unsafe in the sense that it could easily lead to people exposing their servers publicly without intending to. That said, Node's documentation explicitly states that the option for |
@sholladay That's not what I see on 11.1.0, did you mistype your example?
I'm not sure what you expected in the above. You parse a URL, but don't pass the host or port from the URL to
This example parses a URL, extracts the host (and discards the port), then listens on the host and default port (not 3000, which was never extracted from the url). I'm not sure what the expectations are here, and I think some of the examples might have typos, which confuses things. |
@sam-github apologies, I missed an important line when copy/pasting from my terminal back to GitHub. Needless to say, the URL stuff is only relevant if you pass the data to Please see my updated comment above. I'm trying to demonstrate that the APIs are asymmetrical and this leads to surprising, confusing, and arguably unsafe behavior. |
Interestingly, I misread your example, because it didn't occur to me that So, do you expect Node itself mostly uses There have been periodic suggestions that in the various listen/connect APIs that if the Also, I just checked, and I think a Those are some of my thoughts. I can understand your critique, but its not clear to me what you think should be done. You want |
I wrote a section about my expectations in the original comment at the top of the thread, which I've edited a few times for clarity. Might help to re-read that. But in short: I like almost everything about the existing |
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment. For more information on how the project manages feature requests, please consult the feature request management document. |
There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment. For more information on how the project manages feature requests, please consult the feature request management document. |
Version: Node 8 and others
Problem
The semantics of the
host
option forserver.listen()
are incompatible with the conventions of other APIs, both in Node itself and in browsers, etc. This is a footgun that could be fixed by making the options for.listen()
be consistent with other APIs.Context
Most systems I interact with distinguish between a
hostname
and ahost
, where the latter includes a port number and the former does not.In general, Node behaves this way, too (see
os.hostname()
, WHATWG URL, and the legacyurl
module APIs). However, the option object passed toserver.listen(option)
is weird. It only understandshost
(nothostname
), but a port number is disallowed. This is confusing and leads to very surprising results in some cases.Prior art
In browsers
In Node.js
Current behavior
The
server.listen()
API understands ahost
option, but it is incompatible withhost
from other APIs, including WHATWG URL and others. This is because.listen()
refuses perfectly valid hosts such aslocalhost:3000
. A reasonable expectation would be to tryhostname
instead, which is the more common name for the current behavior of thehost
option. Unfortunately, that doesn't work either, as.listen()
does not understandhostname
, and it will be silently ignored.Expected behavior
APIs that accept a
host
should respect their port number to avoid confusion and inconsistency. Having separate options forhostname
andport
is also awesome, but in that case it should be namedhostname
instead ofhost
. I would argue that Node should only supporthostname
instead ofhost
(leaving parsinghost
s to userland) but that would be a breaking change, so I'm not sure how feasible that is. At the very least, I think a newhostname
option could be introduced with the correct behavior, and thenhost
could be extended to respect port numbers, and the fact thathost
continues to be in the API would purely be for backwards compatibility reasons.The text was updated successfully, but these errors were encountered: