Fix the handling of the protocol requirements for MQTT V5 topic aliases maximum#1507
Open
Fix the handling of the protocol requirements for MQTT V5 topic aliases maximum#1507
Conversation
…5 topic aliases maximum Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Alvin <alvin@emqx.io>
Contributor
📝 WalkthroughWalkthroughThis PR adds MQTT5 support for topic alias maximum outbound parameter. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bec68d9b-eec5-4802-8ce1-fcdba5722ac0
📒 Files selected for processing (7)
include/nng/nng.hsrc/core/message.hsrc/nng.csrc/sp/protocol/mqtt/mqtt_parser.csrc/sp/transport/mqtt/broker_tcp.csrc/supplemental/mqtt/mqtt_codec.csrc/supplemental/mqtt/mqtt_msg.c
Comment on lines
+427
to
463
| conn_param *cparam = p->tcp_cparam; | ||
| property *props = cparam->properties; | ||
| if (props != NULL) { | ||
| // CONNECT's Topic Alias Maximum is client | ||
| // capability and must not be reused as | ||
| // broker's CONNACK capability. | ||
| property_remove(props, TOPIC_ALIAS_MAXIMUM); | ||
| } | ||
| if (cparam->max_packet_size == 0) { | ||
| // set default max packet size for client | ||
| p->tcp_cparam->max_packet_size = | ||
| p->conf == NULL | ||
| cparam->max_packet_size = p->conf == NULL | ||
| ? NANO_MAX_PACKET_SIZE | ||
| : p->conf->client_max_packet_size; | ||
| if (p->tcp_cparam->properties != NULL) { | ||
| property_remove(p->tcp_cparam->properties, | ||
| MAXIMUM_PACKET_SIZE); | ||
| property_append(p->tcp_cparam->properties, | ||
| property_set_value_u32(MAXIMUM_PACKET_SIZE, | ||
| p->tcp_cparam->max_packet_size)); | ||
| if (props != NULL) { | ||
| property_remove( | ||
| props, MAXIMUM_PACKET_SIZE); | ||
| property_append(props, | ||
| property_set_value_u32( | ||
| MAXIMUM_PACKET_SIZE, | ||
| cparam->max_packet_size)); | ||
| } | ||
| } | ||
| // Advertise broker's Topic Alias Maximum in CONNACK | ||
| // for MQTT v5. Default to 65535 to allow clients to | ||
| // use Topic Aliases. | ||
| if (p->pro_ver == MQTT_PROTOCOL_VERSION_v5) { | ||
| if (props == NULL) { | ||
| props = property_alloc(); | ||
| cparam->properties = props; | ||
| } | ||
| if (props != NULL) { | ||
| cparam->topic_alias_max_out = 65535; | ||
| property_append(props, | ||
| property_set_value_u16( | ||
| TOPIC_ALIAS_MAXIMUM, | ||
| cparam->topic_alias_max_out)); | ||
| } |
Contributor
There was a problem hiding this comment.
Build CONNACK properties from a dedicated list instead of mutating CONNECT properties
At Line 428, props is sourced from cparam->properties (CONNECT-side data) and then edited for CONNACK. Removing only TOPIC_ALIAS_MAXIMUM is partial; other CONNECT-only properties can still leak into CONNACK, and original CONNECT metadata is mutated in place.
Suggested direction
- conn_param *cparam = p->tcp_cparam;
- property *props = cparam->properties;
+ conn_param *cparam = p->tcp_cparam;
+ property *conn_props = cparam->properties; // keep CONNECT properties
+ property *props = property_alloc(); // build CONNACK properties only
+ if (props == NULL) {
+ rv = NNG_ENOMEM;
+ code = SERVER_UNAVAILABLE;
+ goto error;
+ }
+ // Use `props` only for CONNACK-legal properties (e.g. MAXIMUM_PACKET_SIZE,
+ // TOPIC_ALIAS_MAXIMUM), and keep CONNECT properties separate.
+ // (If your encoder currently reads cparam->properties for CONNACK,
+ // introduce/route a dedicated CONNACK property container.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix the following issue:
Description
Summary
NanoMQ accepts client-supplied MQTT v5 Topic Alias values and reuses them in later empty-topic PUBLISH packets even when the server's CONNACK did not negotiate Topic Alias Maximum.
In other words, the broker creates and consumes connection-local alias state without explicitly advertising support for that capability to the client.
Details
The core issue is that NanoMQ's inbound PUBLISH handling lacks a state check for whether Topic Alias support was actually negotiated for the connection.
In nanomq/pub_handler.c, the MQTT v5 PUBLISH path does the following:
if (proto == MQTT_PROTOCOL_VERSION_v5) {
property_data *pdata = property_get_value(
work->pub_packet->var_header.publish.properties,
TOPIC_ALIAS);
if (len > 0 && topic != NULL) {
if (pdata) {
dbhash_insert_atpair(
work->pid.id, pdata->p_value.u16, topic);
}
} else {
if (pdata) {
const char *tp = dbhash_find_atpair(
work->pid.id, pdata->p_value.u16);
if (tp) {
topic = work->pub_packet->var_header
.publish.topic_name.body =
nng_strdup(tp);
len = work->pub_packet->var_header
.publish.topic_name.len =
strlen(tp);
} else {
return TOPIC_FILTER_INVALID;
}
}
}
}
Relevant locations:
nanomq/pub_handler.c:1651
nanomq/pub_handler.c:1657
nanomq/pub_handler.c:1663
This means:
if a PUBLISH contains both a topic and TOPIC_ALIAS, NanoMQ immediately stores the alias mapping;
if a later PUBLISH contains the same alias and an empty topic, NanoMQ restores the topic from that mapping; and
there is no visible check here that Topic Alias support was previously negotiated on this connection.
On the handshake side, the broker does not proactively add TOPIC_ALIAS_MAXIMUM to CONNACK by default. In nng/src/sp/transport/mqtt/broker_tcp.c, the negotiation path only injects MAXIMUM_PACKET_SIZE when needed and does not automatically set TOPIC_ALIAS_MAXIMUM:
nng/src/sp/transport/mqtt/broker_tcp.c:426
nng/src/sp/transport/mqtt/broker_tcp.c:432
Although the CONNACK encoder is capable of encoding TOPIC_ALIAS_MAXIMUM:
nng/src/supplemental/mqtt/mqtt_codec.c:4280
nng/src/supplemental/mqtt/mqtt_codec.c:4300
in the actual broker path tested here, the observed CONNACK was:
20 09 00 00 06 2a 01 29 01 28 01
This packet does not contain Topic Alias Maximum, yet subsequent Topic Alias usage was still accepted and persisted as connection-local state.
PoC
The following Python PoC reproduces the issue over the normal network path.
Reproduction steps:
A normal subscriber subscribes to alias/test
An MQTT v5 publisher connects and receives a CONNACK that does not advertise Topic Alias Maximum
It sends PUBLISH(topic="alias/test", topic_alias=1, payload="ONE")
It then sends PUBLISH(topic="", topic_alias=1, payload="TWO")
If NanoMQ incorrectly accepts unnegotiated alias state, the subscriber will receive both messages
import socket
import struct
import time
HOST = "127.0.0.1"
PORT = 18838
def enc_varint(x):
out = []
while True:
b = x % 128
x //= 128
if x > 0:
b |= 0x80
out.append(b)
if x == 0:
break
return bytes(out)
def recv_some(sock, n=4096, timeout=0.5):
sock.settimeout(timeout)
try:
return sock.recv(n)
except Exception:
return b""
def conn_v5(client_id: bytes):
vh = b"\x00\x04MQTT" + b"\x05" + b"\x02" + b"\x00\x3c" + b"\x00"
payload = struct.pack("!H", len(client_id)) + client_id
pkt = b"\x10" + enc_varint(len(vh) + len(payload)) + vh + payload
s = socket.create_connection((HOST, PORT), timeout=1)
s.sendall(pkt)
return s, recv_some(s)
def conn_v4(client_id: bytes):
vh = b"\x00\x04MQTT" + b"\x04" + b"\x02" + b"\x00\x3c"
payload = struct.pack("!H", len(client_id)) + client_id
pkt = b"\x10" + enc_varint(len(vh) + len(payload)) + vh + payload
s = socket.create_connection((HOST, PORT), timeout=1)
s.sendall(pkt)
return s, recv_some(s)
def sub_v4(pid: int, topic: bytes):
body = struct.pack("!H", pid) + struct.pack("!H", len(topic)) + topic + b"\x00"
return b"\x82" + enc_varint(len(body)) + body
def pub_v5_alias(topic: bytes, alias: int, payload: bytes):
props = b"\x23" + struct.pack("!H", alias)
body = struct.pack("!H", len(topic)) + topic + enc_varint(len(props)) + props + payload
return b"\x30" + enc_varint(len(body)) + body
sub, sub_connack = conn_v4(b"subv4")
print("SUB_CONNACK:", sub_connack.hex())
topic = b"alias/test"
sub.sendall(sub_v4(1, topic))
print("SUBACK:", recv_some(sub).hex())
pub, connack = conn_v5(b"aliaspub")
print("PUB_CONNACK:", connack.hex())
pub.sendall(pub_v5_alias(topic, 1, b"ONE"))
time.sleep(0.2)
print("FORWARDED1:", recv_some(sub).hex())
pub.sendall(pub_v5_alias(b"", 1, b"TWO"))
time.sleep(0.2)
print("FORWARDED2:", recv_some(sub).hex())
pub.close()
sub.close()
Observed output during testing:
PUB_CONNACK: 20090000062a0129012801
FORWARDED1: 300f000a616c6961732f746573744f4e45
FORWARDED2: 300f000a616c6961732f7465737454574f
Interpretation:
PUB_CONNACK does not contain Topic Alias Maximum
FORWARDED1 shows that the first alias-bearing publish created the alias mapping
FORWARDED2 shows that the second empty-topic publish successfully resolved the alias and was actually routed
Impact
This is a connection-state semantic flaw with the following impact:
the broker accepts client Topic Alias usage without negotiating Topic Alias Maximum;
the broker creates and maintains alias mappings for that connection anyway;
later empty-topic PUBLISH packets can successfully depend on that unnegotiated state; and
this breaks the consistency between MQTT v5 capability negotiation and later stateful feature use.
Summary by CodeRabbit