fix: suback Error Codes Handling #1887

tsteinholz · 2024-06-13T14:18:53Z

Add error handling for failed suback requests
- specified constant the suback return code value
- copied error object used in other packet handlers, adjusted for this packet
- added error callback method used in other packet handlers
- passed error object to the callback method

Now the error creates the expected Exception with detail and is available from all error callbacks, consistent with the rest of the library.

Test Cases

Subscribing to Unauthorized topic

error: Error: Subscribe error: Not authorized
    at handleAck (/Users/tsteinholz/Development/MQTT.js/build/lib/handlers/ack.js:102:27)
    at handle (/Users/tsteinholz/Development/MQTT.js/build/lib/handlers/index.js:36:31)
    at work (/Users/tsteinholz/Development/MQTT.js/build/lib/client.js:221:40)
    at writable._write (/Users/tsteinholz/Development/MQTT.js/build/lib/client.js:246:13)
    at writeOrBuffer (/Users/tsteinholz/Development/MQTT.js/node_modules/readable-stream/lib/internal/streams/writable.js:334:12)
    at _write (/Users/tsteinholz/Development/MQTT.js/node_modules/readable-stream/lib/internal/streams/writable.js:283:10)
    at Writable.write (/Users/tsteinholz/Development/MQTT.js/node_modules/readable-stream/lib/internal/streams/writable.js:286:10)
    at TLSSocket.ondata (node:internal/streams/readable:1007:22)
    at TLSSocket.emit (node:events:519:28)
    at addChunk (node:internal/streams/readable:559:12) {
  code: 135
}
granted: [
  {
    topic: 'topic/user/does/NOT/have/access/to,
    qos: 1,
    nl: false,
    rap: false,
    rh: 0,
    properties: undefined
  }
]

Subscribing to authorized topic

error: undefined
granted: [
  {
    topic: 'topic/user/has/access/to',
    qos: 1,
    nl: false,
    rap: false,
    rh: 0,
    properties: undefined
  }
]

codecov · 2024-06-13T14:27:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.19%. Comparing base (9fc79df) to head (e883a3d).
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1887      +/-   ##
==========================================
+ Coverage   80.96%   81.19%   +0.23%     
==========================================
  Files          24       24              
  Lines        1408     1452      +44     
  Branches      331      340       +9     
==========================================
+ Hits         1140     1179      +39     
- Misses        185      188       +3     
- Partials       83       85       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

robertsLando

Also please add a unit test for this

src/lib/handlers/ack.ts

tsteinholz · 2024-06-13T14:37:02Z

The linter seems to have a problem with passing the error object to the callback

Error: 132:58 error Function declared in a loop contains unsafe references to variable(s) 'err' @typescript-eslint/no-loop-func
https://github.com/mqttjs/MQTT.js/actions/runs/9501359758/job/26187059098?pr=1887

Which might be why this error was not passed in the first place.

I am not sure why this is an error, given error is defined at the beginning of the method. Do we need to explicitly define this as null instead of leaving it implicitly undefined?

robertsLando

Looks good now, please add a test for this and then I will merge

tsteinholz · 2024-06-13T16:18:08Z

@robertsLando It seems like there is test coverage for the immediate subscribe method and the validation of the topic strings. This may be redundant as there is a return code for the MQTT Broker to do this validation and reject the subscription request as well.

I think I found similar tests in describe('topic validations when subscribing' to base the suback return code validations on, however, it does not appear there is any Access Control List (ACL) enforced on this demo broker.

it('should return an error (via callbacks) for topic it does not have access to', function _test(t, done) {
	const client = connect()
	client.subscribe('$SYS/#', (subErr) => {
		client.end(true, (endErr) => {
			if (subErr) {
				return done(endErr)
			}
			done(new Error('Suback errors do NOT work'))
		})
	})
})

The output of this test is the following:

Packet {
  cmd: 'suback',
  retain: false,
  qos: 0,
  dup: false,
  length: 3,
  topic: null,
  payload: null,
  granted: [ 0 ],
  messageId: 29845
}
2024-06-13T16:06:38.820Z mqttjs connecting to an MQTT broker...
2024-06-13T16:06:38.820Z mqttjs calling streambuilder for mqtt
    ✖ should return an error (via callbacks) for topic it does not have access to (1.584458ms)
      Error: Suback errors do NOT work
          at <anonymous> (/Users/tsteinholz/Development/MQTT.js/test/abstract_client.ts:549:11)
          at <anonymous> (/Users/tsteinholz/Development/MQTT.js/src/lib/client.ts:1454:7)
          at Store.close (/Users/tsteinholz/Development/MQTT.js/src/lib/store.ts:163:4)
          at <anonymous> (/Users/tsteinholz/Development/MQTT.js/src/lib/client.ts:1446:24)
          at Store.close (/Users/tsteinholz/Development/MQTT.js/src/lib/store.ts:163:4)
          at closeStores (/Users/tsteinholz/Development/MQTT.js/src/lib/client.ts:1445:23)
          at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Since the topic filters are validated on the client side and don't get sent to the broker, I cannot set an invalid topic string to initate an error subnack from the broker. Other possible suback errors are listed here: #1886

What is the recommended way to emulate the suback packet or set an Access Control List (ACL) for the test broker?

The suback packet I am getting from my test environment and would like to emulate is

Packet {
  cmd: 'suback',
  retain: false,
  qos: 0,
  dup: false,
  length: 4,
  topic: null,
  payload: null,
  granted: [ 135 ],
  messageId: 32508
}

tsteinholz · 2024-06-13T16:52:38Z

I was able to find what I was looking for from the describe('with alternate server client' test cases

robertsLando

I think you should also check that the error event to be emitted as it was not before

robertsLando

@tsteinholz It's good now just remember to fix lint issues npm run lint-fix

test/abstract_client.ts

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

tsteinholz · 2024-06-17T14:19:44Z

Seems like the new unit test is passing:

Not exactly sure why the unit test suite are timing out

Seems like there is some side-effects added in the test suite. Is there a timeout configured in the test and adding the new test is making it take too long to execute?

tsteinholz · 2024-06-17T14:25:00Z

Increased timeout from 60 seconds / 1 minute to 90 seconds / 1 minute and 30 seconds.

robertsLando · 2024-06-17T14:28:48Z

@tsteinholz it's not a timeout problem it's tests that are flacky, could you remove the change to timeout please? When I notice them are failing because of their "flackyness" I simply re-run them

This reverts commit d648ee1.

robertsLando · 2024-06-18T09:26:12Z

@tsteinholz test was wrong, because it was running on both v5 and v3.1 tests and that reason codes are only supported in v5

tsteinholz · 2024-06-18T11:44:13Z

@robertsLando While reason codes are only supported in the v5 specification. The v3 specification still returns an error, it's just always a "Unspecified Error".

The v3 test would just need to be validating that 0x80 returns an "Unspecified Error".

http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718068

robertsLando · 2024-06-18T15:14:34Z

@tsteinholz Yeah I know that but if I return 135 in granted the mqtt parser doens't like the packet and rejects it with protocol 3/4 (that's why test were failing) so I simply prefer to run this in mqtt v5 tests

add error handling for failed suback requests

944bbf0

tsteinholz mentioned this pull request Jun 13, 2024

[Bug]: Suback Error Codes are ignored and interpreted as QoS #1886

Closed

tsteinholz changed the title ~~Suback Error Codes are ignored and interpreted as QoS #1886~~ fix: Suback Error Codes Handling Jun 13, 2024

robertsLando changed the title ~~fix: Suback Error Codes Handling~~ fix: suback Error Codes Handling Jun 13, 2024

robertsLando requested changes Jun 13, 2024

View reviewed changes

src/lib/handlers/ack.ts Outdated Show resolved Hide resolved

define err default value, remove redudant cb invocation

fae963b

robertsLando requested changes Jun 13, 2024

View reviewed changes

add test case for suback errors

3ac35ce

robertsLando requested changes Jun 14, 2024

View reviewed changes

add unittest serverClient connect handler and error validation

09212dc

robertsLando approved these changes Jun 17, 2024

View reviewed changes

robertsLando requested changes Jun 17, 2024

View reviewed changes

add auto-lint docs, plus run auto lint fix

bc4c0de

robertsLando enabled auto-merge (squash) June 17, 2024 12:59

robertsLando requested changes Jun 17, 2024

View reviewed changes

test/abstract_client.ts Outdated Show resolved Hide resolved

Update test/abstract_client.ts

5b56a46

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

auto-merge was automatically disabled June 17, 2024 13:40
Head branch was pushed to by a user without write access

tsteinholz requested a review from robertsLando June 17, 2024 13:52

increase unit test timeout

d648ee1

Revert "increase unit test timeout"

58fbe45

This reverts commit d648ee1.

robertsLando enabled auto-merge (squash) June 18, 2024 06:44

robertsLando added 2 commits June 18, 2024 09:51

Merge branch 'main' into main

4afc6fc

Merge branch 'main' into main

f06c0d5

fix: correct test

f1bb034

style: fix lint issues and move test to client_mqtt5.ts

e883a3d

robertsLando disabled auto-merge June 18, 2024 11:44

robertsLando merged commit 2a98e5e into mqttjs:main Jun 18, 2024
6 checks passed

snyk-io bot mentioned this pull request Jul 18, 2024

[Snyk] Upgrade mqtt from 4.3.8 to 5.7.3 WontonSam/Bekki13-cloud-node#1143

Open

snyk-io bot mentioned this pull request Jul 26, 2024

[Snyk] Upgrade mqtt from 4.3.8 to 5.8.0 WontonSam/Bekki13-cloud-node#2230

Open

snyk-io bot mentioned this pull request Aug 19, 2024

[Snyk] Upgrade mqtt from 4.3.8 to 5.9.0 WontonSam/Bekki13-cloud-node#2561

Open

snyk-io bot mentioned this pull request Aug 29, 2024

[Snyk] Upgrade mqtt from 4.3.8 to 5.9.1 WontonSam/Bekki13-cloud-node#2581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: suback Error Codes Handling #1887

fix: suback Error Codes Handling #1887

tsteinholz commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

robertsLando left a comment

tsteinholz commented Jun 13, 2024

robertsLando left a comment

tsteinholz commented Jun 13, 2024 •

edited

Loading

tsteinholz commented Jun 13, 2024

robertsLando left a comment

robertsLando left a comment

tsteinholz commented Jun 17, 2024

tsteinholz commented Jun 17, 2024

robertsLando commented Jun 17, 2024 •

edited

Loading

robertsLando commented Jun 18, 2024

tsteinholz commented Jun 18, 2024

robertsLando commented Jun 18, 2024 •

edited

Loading

fix: suback Error Codes Handling #1887

fix: suback Error Codes Handling #1887

Conversation

tsteinholz commented Jun 13, 2024 • edited Loading

Test Cases

Subscribing to Unauthorized topic

Subscribing to authorized topic

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

robertsLando left a comment

Choose a reason for hiding this comment

tsteinholz commented Jun 13, 2024

robertsLando left a comment

Choose a reason for hiding this comment

tsteinholz commented Jun 13, 2024 • edited Loading

tsteinholz commented Jun 13, 2024

robertsLando left a comment

Choose a reason for hiding this comment

robertsLando left a comment

Choose a reason for hiding this comment

tsteinholz commented Jun 17, 2024

tsteinholz commented Jun 17, 2024

robertsLando commented Jun 17, 2024 • edited Loading

robertsLando commented Jun 18, 2024

tsteinholz commented Jun 18, 2024

robertsLando commented Jun 18, 2024 • edited Loading

tsteinholz commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

tsteinholz commented Jun 13, 2024 •

edited

Loading

robertsLando commented Jun 17, 2024 •

edited

Loading

robertsLando commented Jun 18, 2024 •

edited

Loading