search

peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.19k stars 145 forks source link

v5.0.0-beta.6 breaks IPv6 connectivity #206

Closed jeffreykog closed 2 years ago

jeffreykog commented 3 years ago

I just upgraded from 5.0.0-beta4 to 5.0.0-beta8 and noticed that configuring the LocalBootstrap with an IPv6 literal address throws an error. I was able to trace it back to the following line: https://github.com/peter-murray/node-hue-api/commit/7eb54d9850e81ec7c238eeb89450cefd4961fbfe#diff-c4a87cd5eb211b3a0ed9b3d49084ba200c40ef268f5117d0f4d48d1546d821b3R34

The issue can be reproduced with the following simple code:

const v3 = require('node-hue-api').v3;
v3.api.createLocal('fe80::1').connect('test');

It doesn't matter that the IPv6 address is not reachable when testing, as it is the validation that breaks here. The linked line of code puts :443 behind the IPv6 address, adding it as an additional octet which is invalid. url.format does handle this properly, by adding square brackets around the address (like this: [fe80::1]:443 instead of fe80::1:443).

This causes the following error to be thrown:

TypeError [ERR_INVALID_URL]: Invalid URL: https://fe80::1:443
    at onParseError (internal/url.js:259:9)
    at new URL (internal/url.js:335:5)
    at new LocalBootstrap (/data/jk-5/development/artnet-hue-entertainment/node_modules/node-hue-api/dist/cjs/api/http/LocalBootstrap.js:41:24)
    at Object.createLocal (/data/jk-5/development/artnet-hue-entertainment/node_modules/node-hue-api/dist/cjs/api/index.js:24:12)
    at Object.<anonymous> (/data/jk-5/development/artnet-hue-entertainment/reproduce.js:3:8)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12) {
  input: 'https://fe80::1:443',
  code: 'ERR_INVALID_URL'
}

I can see why the url.format() call has been removed, as it was deprecated. But it seems that the deprecation has been revoked as the newer URL utility provides no good alternative for url.format. See https://github.com/nodejs/node/issues/25099 and https://github.com/nodejs/node/commit/c8383dd99f4fb2021823c5bfe9673057d59964d2.

That would mean it's probably safe to restore to the old url.format approach, fixing IPv6 support in the process as it adds square brackets when needed.

Just posting this here to document the issue and start a discussion. If the maintainers agree to restore to url.format i'd be happy to contribute that.

peter-murray commented 2 years ago

Thank you for the detailed issue 🙇

I am somewhat cautious to add back in a Node.js specific library here, as this library is used in the browser as well as on Node.js code bases.

To satisfy the needs IPv6 support do you have knowledge in advance that you have an IPv6 address, or is this more open and you are providing a hostname, IPv4 or IPv6 address to the library in your use case?

jeffreykog commented 2 years ago

I did not realise this was also used in the browser, so then i can understand the choice to remove it. Specifying the address family could be an option, but that presents a breaking API change (but that might be fine because this is 5.0.0). That will move the address family detection into my responsibility. But that still requires a change in node-hue-api, as baseUrl will need to be constructed with square brackets, and hostname without.

We have 3 possible inputs. An IPv4 address, an IPv6 address and a DNS hostname. We need to add brackets for the IPv6 address. Detecting these would not be to complicated, as a colon is only valid for an IPv6 address (and not for v4 or DNS). So we could simply check if an IPv6 address is given by checking .indexOf(':') !== -1, and adding square brackets if that's the case.

An other alternative would be to add a minor dependency on https://github.com/sindresorhus/ip-regex. But i do realise node-hue-api has very little dependencies currently, and keeping it that way is nice. But an ipv6 regex could be borrowed from that repository, as their license allows it.

My personal preference would go against specifying the address family, and in favor of the other two options.

peter-murray commented 2 years ago

I have just made the changes to v5.0.0-beta.9 which is published to the npm registry to be picked up.

I have added handling for the hostname to support all forms of IPv6 including if you happened to wrap it in []s. Can you try this out @jeffreykog and report back if it is working for you :bow: ?

jeffreykog commented 2 years ago

@peter-murray Awesome! I have tested it with an ipv4 literal address, ipv6 literal address and a hostname, and it all seems to work. Closing this 🎉