nodejs / node

Node.js JavaScript runtime βœ¨πŸ’πŸš€βœ¨
https://nodejs.org
Other
106.53k stars 29.03k forks source link

server.listen's bind argument does not accept [::] for ipv6 #54441

Open ThisIsMissEm opened 3 weeks ago

ThisIsMissEm commented 3 weeks ago

Version

22, 20

Platform

Darwin the-beast-kitten.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6031 arm64

Subsystem

net or dns

What steps will reproduce the bug?

require('http').createServer((req, res) => {
  res.statusCode = 200;
  res.end("OK");
}).listen(4001, '[::]');

Outputs:

node:events:497
      throw er; // Unhandled 'error' event
      ^

Error: getaddrinfo ENOTFOUND [::]
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:26)
Emitted 'error' event on Server instance at:
    at GetAddrInfoReqWrap.doListen [as callback] (node:net:2132:12)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:17) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: '[::]'
}

Node.js v20.16.0

(also reproducible on 22.2.0, same error)

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Should listen on all interfaces for ipv6.

What do you see instead?

Server fails to start with an error.

Additional information

Using bind of :: works, so the following succeeds:

require('http').createServer((req, res) => {
  res.statusCode = 200;
  res.end("OK");
}).listen(4001, '::');

This causes compatibility issues if you've configuration that has a BIND parameter that needs to be passed to Node.js and another process, such as the Ruby on Rails built-in server, which doesn't accept bind of :: but does accept [::]

ThisIsMissEm commented 3 weeks ago

This was discovered via https://github.com/mastodon/mastodon/issues/31395

ThisIsMissEm commented 3 weeks ago

Interestingly, net.isIPv6('[::]') also returns false, when I'd expect it to return true, I think?

ThisIsMissEm commented 3 weeks ago

@RedYetiDev I believe this affects all net#listen calls, not just http#listen

RedYetiDev commented 3 weeks ago

@nodejs/net + @nodejs/dns

RedYetiDev commented 3 weeks ago
require('http').createServer((req, res) => {
  res.statusCode = 200;
  res.end("OK");
}).listen(4001, '[::]');
$ node repro.js                  
node:events:498
      throw er; // Unhandled 'error' event
      ^

Error: getaddrinfo ENOTFOUND [::]
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:26)
Emitted 'error' event on Server instance at:
    at GetAddrInfoReqWrap.doListen [as callback] (node:net:2130:12)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:17) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: '[::]'
}

Node.js v22.6.0
bnoordhuis commented 3 weeks ago

This has been reported and discussed before but [::] is a phrase GitHub's search function doesn't do well on...

[::] is URL syntax, whereas node's dns, net and http modules expect plain addresses and host names.

I think it should be possible to accept and ignore the brackets without introducing ambiguities. It is however definitely a change in behavior and therefore may have backwards compatibility and/or security implications for downstream users.

ThisIsMissEm commented 3 weeks ago

Yeah, it could also be ruby/rails which is wrong in using url syntax in their BIND environment variable.

i think automatically stripping brackets with a warning might be okay?

mcollina commented 3 weeks ago

I would not add any specific stripping to this; these kinds of things often end up as attack surfaces for vulnerability hunters. The loopback interface is ::, not [::], use the right one.

ThisIsMissEm commented 3 weeks ago

Would it be possible to throw a better error here? e.g., an invalid argument error that explains :: vs [::] if the input argument for bind starts with [ ?

mcollina commented 3 weeks ago

I think that would be a very good idea.

ThisIsMissEm commented 3 weeks ago

I'm not sure where / how I'd do a patch for something like that β€” it's been far too long since I've contributed to the Node.js codebase.

jazelly commented 2 weeks ago

My attempt to add a general regex check at a higher level is not feasible. The server.listen method doesn't perform validation on the host parameter; instead, it simply passes it down to the uv layer for lookup, which is why we're encountering this error. The method relies on the lookup process to reject invalid host values. Currently, it accepts almost any character, and there isn't a straightforward way to implement a high-level guard to cover what is valid domain name and what is not.

For this specific issue, I think it's kind of like a matter of deciding whether/how we want to enforce host validation within server.listen. I'm hesitant to introduce a check that only targets specific cases, such as [], but also don't have better idea.

Edit: for reference, ada-url parses [::], which comes back as is, and leave it to uv to lookup https://github.com/nodejs/node/blob/885692a34f582c99d74430ba351879431738e43a/src/cares_wrap.cc#L1596