Skip to content
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

fix: prevent url.parse to set path option #1871

Merged
merged 3 commits into from
May 23, 2024
Merged

fix: prevent url.parse to set path option #1871

merged 3 commits into from
May 23, 2024

Conversation

robertsLando
Copy link
Member

Fixes #1870

@robertsLando robertsLando merged commit de0174f into main May 23, 2024
5 checks passed
@robertsLando robertsLando deleted the fix-url-parse branch May 23, 2024 07:34
@Bacto
Copy link

Bacto commented May 23, 2024

Hi,

This update unfortunately breaks websockets connections when used with paths.

In my case, I connect using the URL wss://<hostname>/ws/mqtt.
With 5.6.1 it works great.
But since 5.6.2 it doesn't connect anymore (it uses path / in place of /ws/mqtt).

Could you have a look at this usecase?

@robertsLando
Copy link
Member Author

I think we need some better tests for this so, since I jump in maintaining this repo I find out the coverage is not that good as each release easily breaks something even if tests are passing 😅 I'm sorry for that and I will fix that tomorrow, feel free to open a PR if you can as the fix is not that hard (just look at my changes here and add the path to options when protocol i ws/wss and a test of course ;))

@ytimenkov
Copy link

Yeah... Took me like half an hour to understand how this simple fix added support for unix sockets and how it broke them...

It feels that we mixed 2 different OSI layers: for WebSockets, path is part of application layer (7) while for UNIX it's network address => probably as low as layer 3.

I don't see any good way to disambiguate the grammar except for explicitly state address family (inet(6) vs unix).

Either as git does (git+https) or as a stand-alone option (like curl's --unix-socket). Strictly speaking, you may have WebSocket connection over Unix socket 🤷‍♂️. (However I'm not sure how in this case separate socket path from address..., except for curl's option).

I also think that it is strange that url parsing library which you use yields / path for ipv6 address and undefined for ipv4.

@robertsLando
Copy link
Member Author

@ytimenkov I think it could make sense to add an option on mqtt client to explicitly say when you want to connect to a unix socket ot not, like isUnixSocket so this would be much more easy to handle.

I have drafted a PR here: #1874

But that's without that option, I would like to know your opinion about the new option and if we agree I can add that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: IPV6 connectivity broke in 5.5.4
3 participants