fix: PUBACK packet not compatible with RabbitMQ

alaendle commented 1 year ago

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]
The discussion starts if it is valid to only have the reason byte without the properties length (due to the unclear statement in 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

robertsLando commented 1 year ago

@alaendle remember to add a test suite that covers the issue

alaendle commented 1 year ago

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.

alaendle commented 1 year ago

Please note I have added a description of the PR just now - https://github.com/mqttjs/mqtt-packet/pull/142#issue-1788020845 - to make sure we discuss the same things 😄

robertsLando commented 1 year ago

@mcollina Thoughts on this? It looks good to me, if you agree could you merge and release a patch?

mqttjs / mqtt-packet

fix: PUBACK packet not compatible with RabbitMQ #142