fix: suback Error Codes Handling (#1887)

* add error handling for failed suback requests * define err default value, remove redudant cb invocation * add test case for suback errors * add unittest serverClient connect handler and error validation * add auto-lint docs, plus run auto lint fix * Update test/abstract_client.ts Co-authored-by: Daniel Lando <[email protected]> * increase unit test timeout * Revert "increase unit test timeout" This reverts commit d648ee1. * fix: correct test * style: fix lint issues and move test to client_mqtt5.ts --------- Co-authored-by: Daniel Lando <[email protected]>
mqttjs · Jun 18, 2024 · 2a98e5e · 2a98e5e
1 parent f9565fd
commit 2a98e5e
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 4 deletions.
diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md
@@ -68,3 +68,12 @@ node -r esbuild-register --test --test-name-pattern="should resend in-flight QoS
 ```
 
 You can also run tests in watch mode using the `--watch` flag.
+
+## Lint
+
+You can run and automatically fix linting issues with
+
+```sh
+npm run lint-fix
+```
+
diff --git a/src/lib/handlers/ack.ts b/src/lib/handlers/ack.ts
@@ -54,7 +54,7 @@ const handleAck: PacketHandler = (client, packet) => {
 	const type = packet.cmd
 	let response = null
 	const cb = client.outgoing[messageId] ? client.outgoing[messageId].cb : null
-	let err
+	let err = null
 
 	// Checking `!cb` happens to work, but it's not technically "correct".
 	//
@@ -117,7 +117,11 @@ const handleAck: PacketHandler = (client, packet) => {
 			client.messageIdProvider.deallocate(messageId)
 			const granted = packet.granted as number[]
 			for (let grantedI = 0; grantedI < granted.length; grantedI++) {
-				if ((granted[grantedI] & 0x80) !== 0) {
+				const subackRC = granted[grantedI]
+				if ((subackRC & 0x80) !== 0) {
+					err = new Error(`Subscribe error: ${ReasonCodes[subackRC]}`)
+					err.code = subackRC
+
 					// suback with Failure status
 					const topics = client.messageIdToTopic[messageId]
 					if (topics) {
@@ -129,7 +133,7 @@ const handleAck: PacketHandler = (client, packet) => {
 			}
 			delete client.messageIdToTopic[messageId]
 			client['_invokeStoreProcessingQueue']()
-			cb(null, packet)
+			cb(err, packet)
 			break
 		}
 		case 'unsuback': {

diff --git a/test/abstract_client.ts b/test/abstract_client.ts
@@ -2,7 +2,7 @@
  * Testing dependencies
  */
 import { assert } from 'chai'
-import sinon, { SinonSpy } from 'sinon'
+import sinon from 'sinon'
 import fs from 'fs'
 import levelStore from 'mqtt-level-store'
 import Store from '../src/lib/store'

diff --git a/test/client_mqtt5.ts b/test/client_mqtt5.ts
@@ -1136,6 +1136,40 @@ describe('MQTT 5.0', () => {
 		},
 	)
 
+	it('suback handling error codes', function _test(t, done) {
+		serverThatSendsErrors.listen(ports.PORTAND117)
+
+		serverThatSendsErrors.once('client', (serverClient) => {
+			serverClient.on('subscribe', (packet) => {
+				serverClient.suback({
+					messageId: packet.messageId,
+					granted: packet.subscriptions.map((e) => 135),
+				})
+			})
+		})
+
+		const client = mqtt.connect({
+			protocolVersion: 5,
+			port: ports.PORTAND117,
+			host: 'localhost',
+		})
+
+		client.subscribe('$SYS/#', (subErr) => {
+			client.end(true, (endErr) => {
+				serverThatSendsErrors.close((err2) => {
+					if (subErr) {
+						assert.strictEqual(
+							subErr.message,
+							'Subscribe error: Not authorized',
+						)
+						return done(err2 || endErr)
+					}
+					done(new Error('Suback errors do NOT work'))
+				})
+			})
+		})
+	})
+
 	it(
 		'server side disconnect',
 		{