When connecting `ws://[::1]:8883`, an error is thrown - Uncaught TypeError TypeError [ERR_INVALID_URL]: Invalid URL

Steve-Mcl commented 1 year ago

Hi, I am getting an exception when connecting to IPv6 localhost address ::1

I have found this was reported and fixed in https://github.com/mqttjs/MQTT.js/issues/1130 / #1147 (commit https://github.com/mqttjs/MQTT.js/commit/70a247c29e0b05ddd8755e7b9c8c41a4c25b431b) however it seems to have been reverted and the current V4.3.7 is once more using the depreciated url.parse as you can see here: https://github.com/mqttjs/MQTT.js/blob/main/lib/connect/index.js#L64

Here is a minimal repro...

        const userEnteredURL = 'ws://[::1]:8883'
        const client = mqtt.connect(userEnteredURL, this.brokerConfig)

Here is why I think the underlying issue exists: mqtt.js manually rebuilds the broker URL using hostname + port in https://github.com/mqttjs/MQTT.js/blob/main/lib/connect/ws.js#L20

let url = opts.protocol + '://' + opts.hostname + ':' + opts.port + opts.path

... but, url.parse().hostname drops the squarebrackets off [::1] which causes the above code to generate the url http://::1:8883. Further downstream, the ws package uses new URL(...) with this re-assembled URL string & the ERR_INVALID_URL error occurs.

For anyone else reading, here is a minimal (hacky) workaround...

        const userEnteredURL = 'ws://[::1]:8883'
        const tempNewURL= new URL(userEnteredURL)
        const brokerURL = url.parse(userEnteredURL)
        brokerURL.hostname = tempNewURL.hostname // force the hostname back to [::1]
        const client = mqtt.connect(brokerURL, this.brokerConfig)

Additional info...

Calling mqtt.connect(new URL('http://[::1]:8883')) fails because connect only accepts a string or url.parse object
Calling mqtt.connect(url.parse('http://[::1]:8883') fails because unlike new URL, url.parse drops the square brackets off hostname and mqtt.js useshostnameto reassemble theurl`
websocket code uses new URL so when mqtt.js passes the reassembled IP http://::1:8883 it failes with error ERR_INVALID_URL

`url.parse` vs `new URL`

`url.parse('http://[::1]:8883')`

Url {
  protocol: 'ws:',
  slashes: true,
  auth: null,
  host: '[::1]:8883',
  port: '8883',
  hostname: '::1', //<< square brackets are dropped
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  path: '/',
  href: 'ws://[::1]:8883/'
}

`new URL('http://[::1]:8883')`

URL {
  href: 'ws://[::1]:8883/',
  origin: 'ws://[::1]:8883',
  protocol: 'ws:',
  username: '',
  password: '',
  host: '[::1]:8883',
  hostname: '[::1]', //<< square brackets are kept
  port: '8883',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

mklinke commented 1 year ago

Might be relevant: https://github.com/nodejs/node/pull/44731

Steve-Mcl commented 1 year ago

@mklinke no, I dont think so.

I strongly believe this is a bug in how this lib (MQTT.js) breaks down and re-assembles the URL as detailed in my initial post:

I have found this was reported and fixed in #1130 / #1147 (commit 70a247c) however it seems to have been reverted and the current V4.3.7 is once more using the depreciated url.parse as you can see here: https://github.com/mqttjs/MQTT.js/blob/main/lib/connect/index.js#L64

Here is a minimal repro...
        const userEnteredURL = 'ws://[::1]:8883'
        const client = mqtt.connect(userEnteredURL, this.brokerConfig)
Here is why I think the underlying issue exists: mqtt.js manually rebuilds the broker URL using hostname + port in https://github.com/mqttjs/MQTT.js/blob/main/lib/connect/ws.js#L20

robertsLando commented 1 year ago

Could you submit a PR to fix this?

robertsLando commented 1 year ago

MQTT 5.0.0 BETA is now available! Try it out and give us feedback: npm i mqtt@beta. It may fix your issues

kmitchel commented 5 months ago

While testing using a severs ipv6 address, I've noticed this issue. I construct a websocket url using window.location.hostname, when the error is thrown the URL is missing brackets. IPv6 connections using a name work properly, just not bracket notation. Using 5.5.0

rockey2020 commented 5 months ago

same question.. when my url be mqtts://::1:14509 it cant be working , but i change to mqtts://127.0.0.1:14509 it work

robertsLando commented 5 months ago

Can anyone submit a PR to fix this issue? I think it's just a matter of url parsing here:

https://github.com/mqttjs/MQTT.js/blob/9fc79dfc855882558450d433b71092176921d634/src/lib/connect/index.ts#L71

Steve-Mcl commented 1 month ago

MQTT 5.0.0 BETA is now available! Try it out and give us feedback: npm i mqtt@beta. It may fix your issues

My sincere apologies. I will spare you the reasons but we are only just now looking to make the switch from MQTT 4.x to 5.x

I tested this issue today and unfortunately it remains in the 5.x stream.

Details

I have looked at the src for V5 and it still uses the depreciated url.parse to deconstruct the URL then reconstructs it manually resulting in the loss of square brackets which is what the workaround in my OP was fixing.

Can anyone submit a PR to fix this issue? I think it's just a matter of url parsing here:

https://github.com/mqttjs/MQTT.js/blob/9fc79dfc855882558450d433b71092176921d634/src/lib/connect/index.ts#L71

I believe the correct solution is to use the non depreciated way of URL parsing AKA "WHATWG URL API" REF: https://nodejs.org/docs/latest-v12.x/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost REF: https://github.com/orgs/nodejs/discussions/23694

To provide some weight to this, the ws package uses WHATWG URL API (new URL(...)) with the URL that MQTT LIB has re-assembled & thus an error occurs.

Let me look at what is involved in a PR (hopefully not too many places where url.parse is being used.

mqttjs / MQTT.js