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: validate subprotocol mqtt #724

Merged
merged 5 commits into from
Oct 7, 2023

Conversation

Vilayat-Ali
Copy link
Contributor

@Vilayat-Ali Vilayat-Ali commented Oct 5, 2023

Fixes #640

Added validation to check for response headers for websocket subprotocol for "mqtt" and returns a new error WebsocketSubprotocol variant if absent.

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.

mosquitto.conf Outdated Show resolved Hide resolved
rumqttc/examples/websocket.rs Outdated Show resolved Hide resolved
rumqttc/src/eventloop.rs Outdated Show resolved Hide resolved
rumqttc/src/eventloop.rs Outdated Show resolved Hide resolved
@Vilayat-Ali Vilayat-Ali force-pushed the vilayat/fix/validate-response-header-mqtt branch from 2c87dcf to a20ed3a Compare October 6, 2023 10:43
@de-sh
Copy link
Member

de-sh commented Oct 6, 2023

@swanandx any plans on fixing clippy suggestions?

Please vet this PR and merge if it looks good to go. Test against a broker that supports websocket

@swanandx
Copy link
Member

swanandx commented Oct 6, 2023

any plans on fixing clippy suggestions?

me must haha, should we open separate PR for it ( would need to sync up this branch after merging it )? or just fix it in this one?

Test against a broker that supports websocket

will do!

Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

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

Tested with rumqttd when it sends "mqtt" and when doesn't ( in which case it closed the connection as expected ). Though this PR isn't covering all the cases ( explained in review comments ), please address them!

Otherwise, great work with PR 💯

rumqttc/src/websockets.rs Outdated Show resolved Hide resolved
rumqttc/src/websockets.rs Outdated Show resolved Hide resolved
rumqttc/src/websockets.rs Outdated Show resolved Hide resolved
@de-sh
Copy link
Member

de-sh commented Oct 7, 2023

@Vilayat-Ali please go through the changes suggested by @swanandx and the last two commits made to fix the PR along the repo code style. Please note the reorg of code and the naming conventions used.

Congrats on your first PR being merged! 🎊

@de-sh de-sh merged commit 49a4946 into main Oct 7, 2023
0 of 3 checks passed
@de-sh de-sh deleted the vilayat/fix/validate-response-header-mqtt branch October 7, 2023 07:55
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.

Rumqttc: Validate response header
3 participants