fix: PUBACK packet not compatible with RabbitMQ #142
Conversation
|
@alaendle remember to add a test suite that covers the issue |
…lso it shouldn't be spec-wise.
|
Please note that this is a breaking change on the wire - however it shouldn't be with regards to the specification. It could be further optimized by striving for length === 2 (but this would also need an additional complexity by checking the reason code). So I basically adapted the existing test code - because I fear there is no backwards compatible way to implement this. Tested against the "old" version of RabbitMQ that treated length === 3 as a protocol error. |
There was a problem hiding this comment.
I would also add mqtt specs links to specific section in the comments.
Also could you explain me how this could be a breaking change? Let's say I have a broker using mqtt-packet with this patch version and an mqtt client (using mqttjs that uses this library too) with an older mqtt-packet implementation. The broker sends the puback packet, it is encoded following this new standard, will mqttjs client be able to decode it with the older version?
If nope does this only happens when using mqtt5 protocol?
There was a problem hiding this comment.
I would also add mqtt specs links to specific section in the comments.
Also could you explain me how this could be a breaking change? Let's say I have a broker using mqtt-packet with this patch version and an mqtt client (using mqttjs that uses this library too) with an older mqtt-packet implementation (consider also the opposite case). The broker sends the puback packet, it is encoded following this new standard, will mqttjs client be able to decode it with the older version?
If nope does this only happens when using mqtt5 protocol?
|
Please note I have added a description of the PR just now - #142 (comment) - to make sure we discuss the same things 😄 |
robertsLando
changed the title
|
@mcollina Thoughts on this? It looks good to me, if you agree could you merge and release a patch? |
MQTT5 spec is not so clear about the possible remaining length of PUBACK pakets.
We have:
If there are no properties, this MUST be indicated by including a Property Length of zero [MQTT-2.2.2-1].This clearly allows us to have packets like [64, 4, 0 42, 0, 0] (puback, length:4, pakedId: 0 42, reason: 0, properties: 0]
The Client or Server sending the PUBACK packet MUST use one of the PUBACK Reason Codes [MQTT-3.4.2-1]. The Reason Code and Property Length can be omitted if the Reason Code is 0x00 (Success) and there are no Properties. In this case the PUBACK has a Remaining Length of 2.This allows packets like [64, 2, 0, 42]
3.4.2.2.1 Property Length The length of the Properties in the PUBACK packet Variable Header encoded as a Variable Byte Integer. If the Remaining Length is less than 4 there is no Property Length and the value of 0 is used.)So basically - ist it valid to have [64, 3, 0, 42, 0]? I would argue no, the remaining length should be 2 or >= 4. So this patch strives to only generate packets with this remaining length. However we are still able to parse packets with a remaining length of 3.
To state my belief - if it were intended to be able to omit both bytes individually, it wouldn't makle sense limit this to reason code 0x00 - why should [64, 3, 0, 42, 0] be allowed, while [64, 3, 0, 42, 135] is not?
Fixes #92