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 behavior when connecting wss url without TLS support #437

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

maugier
Copy link
Contributor

@maugier maugier commented Aug 2, 2024

Hi,

When compiled without TLS support, the client currently falls back to HTTP (and by default, on port 443) even if the URI has the wss scheme. This creates a problem that is hard to debug, as the call fails with a generic WSS protocol error.

If a wss:// URI is requested and no TLS support is available, the client should fail fast with a UrlError::TlsFeatureNotEnabled.

Cheers,

Copy link
Member

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a useful thing! I think it would be even more awesome thing if we could cover it with a small test 🙂

@maugier maugier force-pushed the fix-wss-handling-when-no-tls branch from 1e5a9ae to 2944367 Compare August 12, 2024 14:35
@maugier
Copy link
Contributor Author

maugier commented Aug 12, 2024

Writing the test made me realize that the failure should happen earlier, before we even attempt to connect the TCP stream.

I reverted the earlier patch and moved the check earlier. What do you think of this ?

@daniel-abramov
Copy link
Member

I reverted the earlier patch and moved the check earlier. What do you think of this ?

Looks very sensible, thanks!

@daniel-abramov
Copy link
Member

So the only job that still fails does this only due to our low MSRV, as discussed in #432 and other related issues, so I'll merge it and raise MSRV in a separate commit then.

@daniel-abramov daniel-abramov merged commit fb83cd1 into snapview:master Aug 13, 2024
5 of 6 checks passed
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.

2 participants