From bb3af28801d146c69616de278b0b325388c7ab1e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andreas=20L=C3=A4ndle?= <alaendle@users.noreply.github.com>
Date: Fri, 7 Jul 2023 09:27:12 +0200
Subject: [PATCH] fix: PUBACK packet not compatible with RabbitMQ (#142)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Hack to solve #92.

* Adapted tests - please note this is a breaking change on the wire - also it shouldn't be spec-wise.

* More comments and improved puback handling.

* Improved comments.

---------

Co-authored-by: Andreas Ländle <969523+alaendle@users.noreply.github.com>
---
 test.js          | 36 +++++++++++++++++++++++++++++++++---
 writeToStream.js | 14 ++++++++++----
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/test.js b/test.js
index 19e9942..dfe741a 100644
--- a/test.js
+++ b/test.js
@@ -719,6 +719,22 @@ testParseOnly('Version 5 PUBACK test 1', {
 )
 
 testParseAndGenerate('Version 5 PUBACK test 2', {
+  cmd: 'puback',
+  messageId: 42,
+  retain: false,
+  qos: 0,
+  dup: false,
+  length: 2,
+  topic: null,
+  payload: null,
+  reasonCode: 0
+}, Buffer.from([
+  64, 2, // Fixed Header (PUBACK, Remaining Length)
+  0, 42 // Variable Header (2 Bytes: Packet Identifier 42, Implied reason code: 0 Success, Implied no properties)
+]), { protocolVersion: 5 }
+)
+
+testParseOnly('Version 5 PUBACK test 2.1', {
   cmd: 'puback',
   messageId: 42,
   retain: false,
@@ -1868,18 +1884,32 @@ testParseError('Invalid header flag bits, must be 0x0 for puback packet', Buffer
   0, 2 // Message ID
 ]))
 
+testParseGenerate('puback without reason and no MQTT 5 properties', {
+  cmd: 'puback',
+  retain: false,
+  qos: 0,
+  dup: false,
+  length: 2,
+  messageId: 2,
+  reasonCode: 0
+}, Buffer.from([
+  64, 2, // Header
+  0, 2 // Message ID
+]), { protocolVersion: 5 })
+
 testParseGenerate('puback with reason and no MQTT 5 properties', {
   cmd: 'puback',
   retain: false,
   qos: 0,
   dup: false,
-  length: 3,
+  length: 4,
   messageId: 2,
   reasonCode: 16
 }, Buffer.from([
-  64, 3, // Header
+  64, 4, // Header
   0, 2, // Message ID
-  16 // reason code
+  16, // reason code
+  0 // no user properties
 ]), { protocolVersion: 5 })
 
 testParseGenerate('puback MQTT 5 properties', {
diff --git a/writeToStream.js b/writeToStream.js
index 00c0b7e..74ca766 100644
--- a/writeToStream.js
+++ b/writeToStream.js
@@ -409,20 +409,26 @@ function confirmation (packet, stream, opts) {
   // Header
   stream.write(protocol.ACKS[type][qos][dup][0])
 
-  // Length
+  // Length === 3 is only true of version === 5 and no properties; therefore if reasonCode === 0 we are allowed to skip both bytes - but if we write the reason code we also have to write property length [MQTT-3.4.2-1].
+  if (length === 3) length += reasonCode !== 0 ? 1 : -1
   writeVarByteInt(stream, length)
 
   // Message ID
   writeNumber(stream, id)
 
-  // reason code in header
-  if (version === 5) {
+  // reason code in header - but only if it couldn't be omitted - indicated by length !== 2.
+  if (version === 5 && length !== 2) {
     stream.write(Buffer.from([reasonCode]))
   }
 
-  // properies mqtt 5
+  // properties mqtt 5
   if (propertiesData !== null) {
     propertiesData.write()
+  } else {
+    if (length === 4) {
+      // we have no properties but have written a reason code - so we need to indicate empty properties by filling in a zero.
+      stream.write(Buffer.from([0]))
+    }
   }
   return true
 }