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

Validate "mqtt" exists in comma delimited subprotocol header #726

Merged
merged 2 commits into from
Oct 7, 2023

Conversation

swanandx
Copy link
Member

@swanandx swanandx commented Oct 7, 2023

This PR adds up on #724 and fixes behavior incase the header contains multiple subprotocols listed in a comma-delimited format.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

@swanandx swanandx merged commit 0f8cc40 into main Oct 7, 2023
3 checks passed
@swanandx swanandx deleted the ws-subprotocol-fix branch October 7, 2023 12:35
@de-sh
Copy link
Contributor

de-sh commented Oct 8, 2023

Here we are verifying the response from the websocket server, we should expect only one protocol in the response.

The server will respond using the Sec-WebSocket-Protocol header to confirm which subprotocol will be used. This server will include this header only once in the response.

This is where I was confused by your reply to the earlier PR. We should ideally return to checking if the value equals "mqtt", not entirely sure if whitespace would be expected.

@swanandx
Copy link
Member Author

swanandx commented Oct 9, 2023

Header will be included only once, but it's valid to specify multiple subprotocols in it right?

e.g. if we want to specify two protocols, one option is to add that header multiple times

Sec-WebSocket-Protocol: mqtt
Sec-WebSocket-Protocol: chat

Or we can only include it once

Sec-WebSocket-Protocol: mqtt, chat

I think the quoted text is referring to later case.

@de-sh
Copy link
Contributor

de-sh commented Oct 9, 2023

The main point of the quote is:

The server will respond using the Sec-WebSocket-Protocol header to confirm which subprotocol will be used

i.e. the server must only respond with one chosen protocol, the first one from the list provided as header in the connection request

@swanandx
Copy link
Member Author

swanandx commented Oct 9, 2023

Okie! thanks for pointing out, the standards should be more explicit ig, causing confusion in interpretations haha. Anyways, i tested different clients:
paho ( python ) - doesn't verify the header at all!
mqttx - accepts only when it is "mqtt" as you mentioned.
mosquitto - can't connect over ws

So it would be fine if we just match "mqtt". 😄

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