feat(mqtt5): implement re-authentication and handle auth success

[abdelrahmanSheref101]

Description

• This feature was requested at issue [feat] Add support for MQTT 5 refresh auth #1795
• This PR implements the MQTT 5.0 Re-authentication flow and enhances the handling of AUTH packets within the client. This allows for dynamic credential updates or extended authentication exchanges without disconnecting the session.

Key Changes:

• MqttClient API: Added a public .reauthenticate(options) method to trigger the re-auth flow.
• Packet Handling: Updated src/lib/handlers/auth.ts to correctly process Reason Code 0x00 (Success) for both initial handshakes and mid-session re-authentication.
• Event System: Introduced a new 'reauth' event that emits upon a successful authentication exchange, providing the server's response packet to the user.
• Validation: Added guards to ensure re-authentication is only attempted on MQTT 5.0 connections with a pre-established authenticationMethod.

---

Test Plan

I have added a new suite of tests in test/client_mqtt5.ts to ensure stability:

• [Success Path]: Verified end-to-end AUTH exchange using a mock broker, ensuring the 'reauth' event triggers correctly.
• [Protocol Guard]: Verified the client throws an error when attempting re-authentication on non-v5 (e.g., v3.1.1 or v4) connections.
• [Constraint Check]: Confirmed validation logic prevents calls if no authenticationMethod was defined during the initial connection.

Local Results:

• npm test: Passes (verified specifically for MQTT 5.0 client tests).

[robertsLando]

Code Review

Thanks for tackling #1795! The direction is good and the surface is small. Below are correctness, API-design, and test issues to address before merging.

Correctness

1. _sendPacket callback semantics don't match an "operation completed" callback
src/lib/client.ts line 1740 calls this._sendPacket(authPacket, callback). _sendPacket's callback fires once the packet has been written to the wire — not when the broker replies. Users almost certainly expect client.reauthenticate(opts, cb) to fire cb when the AUTH exchange completes (similar to how publish/subscribe callbacks correlate with their ack). As written, cb fires immediately and the user must also subscribe to 'reauth' to learn the result. Pick one model — I'd suggest the callback receives (err, responsePacket) correlated by the handler.

2. Double signaling on validation errors

if (callback) callback(error)
this.emit('error', error)
return this

Both the callback and 'error' are fired for the same failure. Other public methods (publish, subscribe) call back with the error and do not re-emit 'error', since unhandled 'error' crashes the process. Surprising for users who pass a callback and don't have an 'error' listener. Pick one path.

3. Handler rc === 0 only emits when client.connected

} else if (rc === 0) {
    if (client.connected) {
        client.emit('reauth', packet)
    }
}

If rc === 0 arrives while not connected (early frame, malformed broker), it is now silently swallowed. Either drop the guard (the spec only allows AUTH rc=0 while connected anyway) or emit an explicit protocol error.

Also: the handler still calls client.handleAuth(packet, ...) which is meant for the application to compute a continue-auth response. On a reauth Success, the user callback runs and any returned packet2 is silently discarded — either document or skip for rc=0.

4. Debug log compares Buffers by reference

if (reAuthOptions.authenticationData === this.options.properties.authenticationData) {
    this.log('reauthenticate: sending same authenticationData as initial connection')
}

authenticationData is a Buffer — === will essentially never match. Use .equals() or remove the log; it doesn't pull weight as-is.

5. properties spread can let callers override authenticationMethod

properties: {
    authenticationMethod: method,
    ...reAuthOptions,
}

Today the Pick<> type excludes authenticationMethod, so this is safe at the type level — but JS callers or as any can still override it, silently violating the spec (the method must match the initial CONNECT). Move authenticationMethod: method after the spread for runtime defense.

API / style

• JSDoc is minimal. Compare to surrounding methods (publish, subscribe, end). Please document reAuthOptions, the 'reauth' event, and the three error conditions.
• Naming: reAuthOptions is inconsistent with the codebase — prefer reauthOptions or options.
• Magic number: Use the named constant from mqtt-packet if one exists, or add 0x19 // Re-authenticate as an inline comment.
• Discoverability: the new reauth: OnPacketCallback event should be mentioned in README/typings.

Tests (test/node/client_mqtt5.ts)

• Port offsets +90/+91/+92 reuse the same PORTAND103 base as many surrounding tests. Real risk of collision — the convention here is to monotonically extend the offset, not jump backwards.
• Success-path test never asserts the broker received the expected AUTH — add assert.strictEqual(packet.reasonCode, 0x19) and verify packet.properties.authenticationData server-side.
• No callback test. Given the ambiguity in (1), decide the semantics and lock them in with a test.
• No "not connected" test. The !this.connected guard is uncovered.
• The non-v5 test uses port: port — use object shorthand to match surrounding style.
• The non-v5 test has a try/catch around the assertion but the other two don't — make them consistent (preferably without the try/catch; the test runner reports throws fine).

Risk summary

Area	Risk
Correctness	Medium — rc=0 handling is incomplete; double error signaling
API	Medium — callback semantics are misleading
Tests	Medium — happy-path test doesn't actually validate the wire format
Backwards compatibility	Low — purely additive
Security	Low — but the Buffer-equality bug should be fixed before anyone relies on it

Suggested next steps

1. Decide and document callback semantics; remove the double-emit on validation errors.
2. Drop the client.connected guard in the handler (or convert it to an explicit protocol error).
3. Strengthen the success-path test to assert the AUTH packet contents server-side.
4. Add a "callback receives error" test and a "not connected" test.
5. Pick a non-colliding port offset.
6. Flesh out JSDoc and the 'reauth' event documentation.
7. Replace === Buffer comparison with .equals() or remove the log.

[abdelrahmanSheref101]

Hi @robertsLando ,

Addressed all review feedback:

• Logic: Fixed callback semantics (waits for broker response) and removed double error emission.
• Correctness: Fixed Buffer comparison bug (.equals()) and added runtime protection for authenticationMethod.
• Docs: Updated README and JSDocs for the reauth event and method.
• Tests: Added coverage for "not connected," "non-v5," and "no-callback" cases; fixed port collisions.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.39%. Comparing base (a4e9a92) to head (0ae96d0).

Files with missing lines	Patch %	Lines
src/lib/client.ts	63.88%	12 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2048      +/-   ##
==========================================
- Coverage   83.42%   83.39%   -0.04%     
==========================================
  Files          25       25              
  Lines        1587     1626      +39     
  Branches      367      380      +13     
==========================================
+ Hits         1324     1356      +32     
- Misses        230      235       +5     
- Partials       33       35       +2     

Flag	Coverage Δ
unittests	83.39% <66.66%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robertsLando]

See inline comments. Main concerns: the handler dispatch should branch on client.connected rather than on the reason code, _handleReauthCompleted leaks as a public API symbol, and there is a silent-error path when the client is torn down mid-reauth without a callback. A few smaller cleanups and missing test coverage noted inline.

[abdelrahmanSheref101]

Hi @robertsLando,

First, I want to apologize for the initial messiness and the 'hustle' in the previous push. I really appreciate you taking the time to provide such detailed mentorship through your review. It means a lot to have a dev like you look over this.

I have addressed all your requested changes:

• Architectural Split: Refactored src/lib/handlers/auth.ts to branch on client.connected. Success/failure paths for in-flight re-auth are now explicitly handled via the internal completion method.
• API Privacy: renamed _handleReauthCompleted to _handleReauth , and made it strictly private, using bracket access for internal calls.
• Validation Logic: Flattened the validation in reauthenticate() into a single short-circuiting chain with a fail helper for better readability.
• Error Handling: Fixed the silent-error bug; if a disconnect occurs during re-auth without a callback, the error is now properly emitted.
• Test Coverage: Added new test cases in test/node/client_mqtt5.ts to cover both the non-zero reason code scenario and the interruption (double call) scenario.
• Cleanup: Fixed the JSDoc syntax, removed all console.log statements, and fixed the spacing in the README.

Ready for another look when you have a moment. Thanks again for your patience!

[robertsLando]

Thanks for the changes! Much better now :) Considering we have a promised version of all methods I think it could make sense to also have an async version of reauthenticate

[abdelrahmanSheref101]

Hi @robertsLando ,
I’ve just pushed an update adding reauthenticateAsync (promise based reauthenticate). I implemented it following the manual Promise-wrapping pattern used elsewhere in the client (like publishAsync) to keep the API consistent
Ready for another look

[robertsLando]

In-depth review of the MQTT 5 re-authentication flow. Findings posted as separate inline threads below — grouped roughly by severity (protocol bugs first, then API/edge cases, then style/tests).

[robertsLando]

[High — protocol bug] Multi-step / broker-initiated AUTH is broken.

This branch routes every AUTH packet received while client.connected straight into _handleReauth(null, packet) as if it were a terminal completion. Per MQTT 5.0 §3.15, AUTH while connected can carry:

• 0x18 Continue Authentication — broker is sending a challenge; the client must reply with another AUTH containing the response (SCRAM, GSSAPI, Kerberos…).
• 0x19 Re-Authenticate — broker-initiated re-auth; the client must respond with new credentials.
• 0x00 Success — exchange complete.

Downstream, _handleReauth converts any non-zero reason code into Error('Re-auth failed: …'), so a perfectly normal 0x18 challenge gets surfaced as a hard failure and the in-flight _reauthCallback is rejected. Worse, when the broker initiates re-auth and there is no _reauthCallback, _handleReauth falls through to this.emit('error', …) — a remote peer can crash a Node process that doesn't have an error listener attached.

The handler needs to dispatch by reason code:

switch (packet.reasonCode) {
  case 0x00: return client._handleReauth(null, packet)            // complete
  case 0x18: return client.handleAuth(packet, …)                  // continue — let user respond
  case 0x19: return client.handleAuth(packet, …)                  // broker-initiated
  default:   return client._handleReauth(new ErrorWithReasonCode(…), packet)
}

This is the most important issue in the PR — it silently regresses the multi-step auth flow that the existing client.handleAuth hook was designed to support.

[abdelrahmanSheref101]

I’ve refactored the auth.ts handler to use a switch on the reason codes to properly handle the multi-step handshake (0x18).

Regarding the 0x19 (Re-authenticate) flow: I noticed Table 3.15.2.1 Authenticate Reason Code lists 0x19 as 'Sent by: Client' only, and §4.12.1 describes it as a client-initiated action. Is there a specific scenario or broker behavior where the server initiates with 0x19, or should we handle a server-sent 0x19 as a protocol error?

[robertsLando]

[Style] Bracket-access to bypass private.

client['_handleReauth'](…) defeats TypeScript's visibility checks. Either:

• mark the method as package-internal (underscore prefix is fine, but drop private), like handleAuth already is on the same client, or
• expose a small internal namespace.

As written, every refactor of _handleReauth will silently skip type-checking at this call site.

[robertsLando]

[High] The interrupt fires before the new request is validated.

if (this._reauthCallback) {
    this._handleReauth(new Error('reauthenticate: interrupted by new reauthentication request'))
}

if (this.options.protocolVersion !== 5) return fail(...)
else if (!this.connected) return fail(...)
else if (!reauthOptions.authenticationData) return fail(...)

A second call with invalid arguments (wrong protocol version, missing data, etc.) will still cancel a perfectly valid in-flight re-auth before its own validation runs. The first caller is rejected with interrupted even though the second call is rejected immediately and never sends a new AUTH packet — net result: a healthy re-auth gets killed by a bogus call.

Move the validation above the interrupt branch so that interruption only happens when the new request will actually go on the wire.

[robertsLando]

[Medium] Null-safety on reauthOptions.

reauthOptions.authenticationData is dereferenced without first checking that reauthOptions itself is non-null. client.reauthenticate(null) / client.reauthenticate(undefined) (an easy mistake, especially from JS callers) crashes with a TypeError instead of a clean validation error. Add an if (!reauthOptions) return fail('reauthOptions is required') at the top of the validation block.

[robertsLando]

[Medium] _sendPacket is fire-and-forget — callback can hang forever.

Two related issues here:

1. No error path on send. _sendPacket(packet, cb) exists for a reason — if the underlying stream write fails, the user callback is never invoked and _reauthCallback stays set until _cleanUp runs (i.e. until the client is destroyed). Use:

   this._sendPacket(authPacket, (err) => { if (err) this._handleReauth(err) })

2. No timeout. A silent broker leaves _reauthCallback pending indefinitely. Other request/response paths in this client (subscribe, unsubscribe, publish-QoS>0) have inflight tracking and reconnection-aware retries; re-auth has neither. Add a timeout (configurable, default ~10–30s) and clear it in _handleReauth.

[robertsLando]

[Style] Type and JSDoc nits on the field declaration.

/* @type {PacketCallback} - callback receives IAuthPacket */
private _reauthCallback: PacketCallback = null

• /* … */ (single asterisk) is not parsed as JSDoc — this comment is invisible to tooling. Use /** … */ if you want the annotation, or just delete it (the TS type already documents it).
• The type should be PacketCallback | null. It compiles today only because strict: false in tsconfig.json, but the project has been steadily tightening — this will trip the moment strictNullChecks is enabled.

[robertsLando]

[Style] as IAuthPacket cast hides a real type gap.

PacketCallback is typed against the generic Packet union, so packet here is Packet | undefined. Casting to IAuthPacket papers over the fact that _reauthCallback's signature should really be (err, packet?: IAuthPacket) => void. Tightening the field type (see the _reauthCallback thread) lets you drop the cast and keeps the Promise branch honest about resolving with undefined on error.

[robertsLando]

[Doc] JSDoc says authenticationData is optional, the code says it's required.

 * @param {Buffer} [reauthOptions.authenticationData] - Binary data for auth exchange

The square brackets in JSDoc denote an optional parameter, but reauthenticate immediately rejects calls without it (return fail('authenticationData is required')) and the README correctly lists it as required. Drop the brackets in both reauthenticate and reauthenticateAsync JSDoc blocks.

[robertsLando]

[Minor] Re-auth cleanup placement in _cleanUp.

The call is fine, but consider whether 'client disconnected' is the right message in the disconnecting / planned-end path — the user explicitly closed the client, so a 'client disconnected' callback error reads like an unexpected failure. A small refinement: pass a more specific error (or none, with a 'cancelled'-shaped result) when this.disconnecting is true.

[robertsLando]

[Test gaps] This suite covers the happy path well but misses the cases that actually exercise the protocol bugs flagged in auth.ts / client.ts:

1. No test for 0x18 Continue Authentication multi-step flow — the broker should be able to reply with a challenge that the client forwards to handleAuth, then send a follow-up 0x00. As written, the current implementation would fail this test.
2. No test for broker-initiated re-auth (broker sends AUTH 0x19 first). Today this would emit 'error' on the client.
3. No test for the _sendPacket failure path (e.g. write after stream end).
4. The interrupt test never asserts the second call completes. It only checks that the first callback receives interrupted. Add an assertion that the second AUTH actually reaches the server and resolves.
5. PORTAND327 + 82 in the non-zero-reason-code test is an outlier — the rest of the suite uses +20..+26. Either consolidate or document why +82.
6. The 'should error if reauthenticate is called while disconnected' test races on connect timing. It's reliable today only because mqtt.connect returns synchronously with connected=false; a future eager-connect refactor would silently flip the assertion. Consider hooking on a deterministic event instead.
7. client.stream?.destroy?.() plus try {} catch {} workarounds in the auth-method test are symptomatic of the missing _sendPacket error wiring — once that's fixed they should disappear.