-
Notifications
You must be signed in to change notification settings - Fork 255
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
Conversation
2c87dcf
to
a20ed3a
Compare
@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 |
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?
will do! |
There was a problem hiding this 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 💯
@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! 🎊 |
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
Checklist:
cargo fmt
CHANGELOG.md
if it's relevant to the users of the library. If it's not relevant mention why.