Skip to content

fix: validate subprotocol mqtt#724

Merged
de-sh merged 5 commits intomainfrom
vilayat/fix/validate-response-header-mqtt
Oct 7, 2023
Merged

fix: validate subprotocol mqtt#724
de-sh merged 5 commits intomainfrom
vilayat/fix/validate-response-header-mqtt

Conversation

@Vilayat-Ali
Copy link
Contributor

@Vilayat-Ali Vilayat-Ali commented Oct 5, 2023 •
edited by de-sh
Loading

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.

de-sh
de-sh suggested changes Oct 5, 2023
View reviewed changes
@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
Contributor

de-sh commented Oct 6, 2023 •
edited
Loading

@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
Contributor

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!

swanandx
swanandx reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@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 💯

@de-sh
Copy link
Contributor

de-sh commented Oct 7, 2023 •
edited
Loading

@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
@de-sh de-sh deleted the vilayat/fix/validate-response-header-mqtt branch October 7, 2023 07:55
@swanandx swanandx mentioned this pull request Oct 7, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@swanandx swanandx swanandx left review comments

@de-sh de-sh de-sh requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rumqttc: Validate response header

3 participants

@Vilayat-Ali @de-sh @swanandx