Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ Also user can manually register topic-alias pair using PUBLISH topic:'some', ta:
- [`mqtt.Client#endAsync()`](#end-async)
- [`mqtt.Client#removeOutgoingMessage()`](#removeOutgoingMessage)
- [`mqtt.Client#reconnect()`](#reconnect)
- [`mqtt.Client#reauthenticate()`](#client-reauthenticate)
- [`mqtt.Client#handleMessage()`](#handleMessage)
- [`mqtt.Client#connected`](#connected)
- [`mqtt.Client#reconnecting`](#reconnecting)
Expand Down Expand Up @@ -599,6 +600,16 @@ and connections
- `packet` received packet, as defined in
[mqtt-packet](https://github.com/mcollina/mqtt-packet)

#### Event `'reauth'`

`function (packet) {}`

Emitted when an MQTT 5 re-authentication completes successfully.

- `packet` the AUTH packet received from the broker.

Triggered after calling [`client.reauthenticate()`](#client-reauthenticate).

---

<a name="client-connect"></a>
Expand Down Expand Up @@ -742,6 +753,30 @@ Connect again using the same options as connect()

---

<a name="client-reauthenticate"></a>

### mqtt.Client#reauthenticate(reauthOptions, [callback])

Start an MQTT 5 re-authentication exchange.

- `reauthOptions`:
- `authenticationData` (`Buffer`)
- `reasonString` (`string`, optional)
- `userProperties` (`object`, optional)

- `callback` - `function (err, packet)`
- called when the AUTH exchange completes or fails

Errors:
- client is not connected
- MQTT version is not 5
- `authenticationData` is missing
- `authenticationMethod` is missing from the initial CONNECT

Emits `'reauth'` on successful completion.

---

<a name="handleMessage"></a>

### mqtt.Client#handleMessage(packet, callback)
Expand Down
95 changes: 94 additions & 1 deletion src/lib/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import DefaultMessageIdProvider, {
} from './default-message-id-provider'
import TopicAliasRecv from './topic-alias-recv'
import {
ErrorWithReasonCode,
type DoneCallback,
type ErrorWithReasonCode,
ErrorWithSubackPacket,
type GenericCallback,
type IStream,
Expand Down Expand Up @@ -433,6 +433,7 @@ export interface MqttClientEventCallbacks {
reconnect: VoidCallback
offline: VoidCallback
outgoingEmpty: VoidCallback
reauth: OnPacketCallback
}

/**
Expand Down Expand Up @@ -521,6 +522,9 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac

private connackPacket: IConnackPacket

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


public static defaultId() {
return `mqttjs_${Math.random().toString(16).substr(2, 8)}`
}
Expand Down Expand Up @@ -1678,6 +1682,91 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
return this
}

/**
* reauthenticate - MQTT 5.0 Re-authentication
* @param {Object} reauthOptions - Re-authentication properties
* @param {Buffer} [reauthOptions.authenticationData] - Binary data for auth exchange
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

* @param {string} [reauthOptions.reasonString] - Human-readable reason for re-auth
* @param {Object} [reauthOptions.userProperties] - Custom user properties
* @param {PacketCallback} [callback] - Fired when the AUTH exchange completes or fails
* @returns {MqttClient} - Returns the client instance
*/
public reauthenticate(
reauthOptions: Pick<
NonNullable<IAuthPacket['properties']>,
'authenticationData' | 'reasonString' | 'userProperties'
>,
callback?: PacketCallback,
): MqttClient {
const fail = (msg: string) => {
const err = new Error(`reauthenticate: ${msg}`)
if (callback) callback(err)
else this.emit('error', err)
return this
}

if (this._reauthCallback) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

this._handleReauth(
new Error(
'reauthenticate: interrupted by new reauthentication request',
),
)
}

if (this.options.protocolVersion !== 5)
return fail('this feature works only with mqtt-v5')
else if (!this.connected) return fail('client is not connected')
else if (!reauthOptions.authenticationData)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return fail('authenticationData is required')

const method = this.options.properties?.authenticationMethod

if (!method)
return fail('authenticationMethod is required from initial CONNECT')

const authPacket: IAuthPacket = {
cmd: 'auth',
reasonCode: 0x19, // Re-authentication (MQTT 5.0 Spec)
properties: {
...reauthOptions,
authenticationMethod: method,
},
}

this._reauthCallback = callback
this._sendPacket(authPacket)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return this
}

/**
* _handleReauth
* Internal method to finalize the re-authentication process.
* Clears the pending reauth callback and signals completion via callback or event.
* @param {Error | null} err - The error if the re-authentication failed
* @param {IAuthPacket} [packet] - The AUTH packet received from the broker
* @api private
*/
private _handleReauth(err: Error | null, packet?: IAuthPacket) {
if (!err && packet && packet.reasonCode !== 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High — companion to the auth.ts issue] _handleReauth collapses every non-zero reason code into a fatal error.

if (!err && packet && packet.reasonCode !== 0) {
    err = new ErrorWithReasonCode(`Re-auth failed: ${packet.reasonCode}`, packet.reasonCode)
}

This is the wrong abstraction layer for that decision — 0x18 Continue Authentication is not a failure, it's a request for another round-trip. By the time you reach this method you've already lost the ability to forward the challenge to the user's handleAuth callback. Move the success/failure classification out of here (see the auth-handler thread above) and let _handleReauth only deal with the pre-classified terminal result.

err = new ErrorWithReasonCode(
`Re-auth failed: ${packet.reasonCode}`,
packet.reasonCode,
)
}

if (this._reauthCallback) {
const cb = this._reauthCallback
this._reauthCallback = null
cb(err, packet)
} else if (err) {
this.emit('error', err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium — availability/security] emit('error', …) from a remote-triggered AUTH crashes Node.

If no _reauthCallback is set (e.g. broker-initiated AUTH the user wasn't expecting, or a stray frame), this branch emits 'error' on the client. Node's default 'error' behaviour is to crash the process unless a listener is attached.

That means a remote MQTT broker — which is not necessarily trusted — can trigger a process exit purely by sending an unexpected AUTH packet after CONNACK. At minimum, swallow-and-log when there is no listener, and ideally don't classify broker-initiated AUTH as an error at all (see the auth-handler thread).

}

if (!err && packet) {
this.emit('reauth', packet)
}
}
Comment thread
robertsLando marked this conversation as resolved.

/**
* PRIVATE METHODS
* =====================
Expand Down Expand Up @@ -1864,6 +1953,10 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
})
}

if (this._reauthCallback) {
this._handleReauth(new Error('client disconnected'))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

}

if (!this.disconnecting && !this.reconnecting) {
this.log(
'_cleanUp :: client not disconnecting/reconnecting. Clearing and resetting reconnect.',
Expand Down
5 changes: 5 additions & 0 deletions src/lib/handlers/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ const handleAuth: PacketHandler = (
return
}

if (client.connected) {
client['_handleReauth'](null, packet)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

return
}

client.handleAuth(
packet,
(err: ErrorWithReasonCode, packet2: IAuthPacket) => {
Expand Down
Loading