Low-severity hardening and compliance fixes

[aidangarske]

F-7630, F-7661, F-7666, F-7668, F-7669, F-7670, F-7671, F-7672, F-7673, F-7674, F-7675, F-7676, F-7677, F-7681,
F-7678, F-7664, F-7665, F-7663, F-7656, F-7679, F-7667, F-7657, F-7662, F-7680

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR applies a set of hardening/compliance fixes across MQTT and MQTT-SN client/broker codepaths, primarily tightening MQTT v5 property validation and improving state handling to prevent crashes, protocol violations, and message-loss edge cases.

Changes:

• Make MQTT decode-side UTF-8 validation mandatory; add additional MQTT v5 property validation (Auth method/data coupling, max packet size, will-property allow-list, subscribe property duplicate rules, publish response reason-code allow-lists).
• Improve MQTT v5 client flow-control/state handling (Receive Maximum, Topic Alias Maximum) and enforce protocol-fatal disconnect behavior.
• Broker reliability fixes: orphan session expiry sweeping, QoS2 de-dup survival across reconnect, retained/QoS fan-out via out-queue, and v5 retain handling + Will-delay semantics; add extensive regression tests.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
wolfmqtt/mqtt_sn_packet.h	Adds backing storage for GWINFO address decoding.
wolfmqtt/mqtt_packet.h	Updates documentation for WOLFMQTT_NO_UTF8_VALIDATION semantics.
wolfmqtt/mqtt_client.h	Adds v5 server property fields to client state.
wolfmqtt/mqtt_broker.h	Extends broker queue/orphan structs for v5 props, QoS2 dedup, and expiry sweep timing.
src/mqtt_sn_packet.c	Rejects GWINFO addr decode when destination pointer is NULL.
src/mqtt_sn_client.c	Defaults GWINFO decode destination to internal buffer.
src/mqtt_packet.c	Adds/strengthens v5 property validation + publish-resp reason code validation; makes UTF-8 decode validation unconditional.
src/mqtt_client.c	Latches server v5 limits, enforces topic-alias max, adds receive-max quota logic, and disconnects on fatal decode/protocol errors.
src/mqtt_broker.c	Adds v5 props cloning into out-queue, orphan expiry sweep, QoS2 dedup persistence, retain-handling enforcement, and will-delay/session-expiry interactions.
tests/test_mqtt_sn_client.c	Adds regression test for unsolicited GWINFO capture with nonblock ping.
tests/test_mqtt_sn.c	Adds regression test for GWINFO decode rejecting NULL gwAddr when addr is present.
tests/test_mqtt_packet.c	Adds tests for mandatory UTF-8 decode, v5 property rules, and publish-resp reason-code allow-lists.
tests/test_mqtt_client.c	Adds tests for v5 Receive Maximum / Topic Alias Maximum behavior and fatal disconnect on malformed fixed header.
tests/test_broker_connect.c	Adds broker regression tests for orphan/session expiry, QoS2 dedup, retain handling, will-delay/session-expiry, malformed PUBACK close, and v5 connect restrictions.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #567

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 15
15 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #567

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[embhorn]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 81 total — 38 posted, 44 skipped

Posted findings

• [High] Will Delay ignores Session Expiry == 0, contradicting the PR's own new v5 default — src/mqtt_broker.c:3875-3893
• [High] Will Delay clamp removed: unbounded pending-will retention (DoS); BROKER_MAX_WILL_DELAY_SEC now dead — src/mqtt_broker.c:5183-5190
• [High] no-WILL build refuses a CONNECT carrying a Will only after session takeover/reclaim, destroying the session it refuses — src/mqtt_broker.c:5111-5127
• [High] Reserved v5 Receive Maximum quota unit leaked by MqttClient_CancelMessage() and write-only/abandoned publishes — src/mqtt_client.c:2524-2530
• [High] disconnect_v5_session_expiry_0_to_nonzero_protocol_error fails at runtime in a WOLFMQTT_BROKER_NO_WILL build — tests/test_broker_connect.c:3899-3966
• [High] Will-unsupported CONNECT refusal has zero runtime coverage, and its v5 sub-branch is never compiled in CI — src/mqtt_broker.c:5110-5127
• [High] Retain Handling = 1 is completely untested; the new BrokerSubs_Add return value exists only for it — src/mqtt_broker.c:5445-5462
• [Medium] MQTT_CODE_PUB_CONTINUE treated as terminal in blocking chunked-publish builds, over-crediting the quota — src/mqtt_client.c:2592-2606
• [Medium] Receive Maximum replenish ceiling (server_recv_max_negotiated) is never exercised or asserted — src/mqtt_client.c:417-430
• [Medium] ChangeLog.md has no entry despite multiple user-visible behavior changes, against explicit repo convention — ChangeLog.md:1-30
• [Medium] Receive Maximum quota can be double-replenished when a publish times out and the ack is consumed elsewhere — src/mqtt_client.c:2647-2659
• [Medium] Fatal-protocol-error teardown destroys healthy connections on locally-generated encode errors — src/mqtt_client.c:1744-1767
• [Medium] Unit-test build breaks with -Werror when WOLFMQTT_NO_TIME is defined — tests/test_mqtt_client.c:82-87
• [Medium] Every clean_session=0 v3.1.1 client now leaves a never-expiring orphan record, even with nothing to preserve — src/mqtt_broker.c:2923-2954
• [Medium] v5 Clean Start=0 with no Session Expiry builds an orphan the sweep tears down ~1s later — src/mqtt_broker.c:2925-2954
• [Medium] New v5 CONNECT validations (Receive Maximum 0, Auth Method) are compiled out of WOLFMQTT_STATIC_MEMORY builds — src/mqtt_broker.c:4794-4842
• [Medium] MqttWillProps_ValidateType allow-list is dead for 4 of its 7 entries; gPropMatrix already rejects spec-legal Will Properties — src/mqtt_packet.c:1542-1554
• [Medium] WOLFMQTT_NO_UTF8_VALIDATION silently loses its documented effect; encoder may emit strings the always-on decoder rejects — src/mqtt_packet.c:589-694
• [Medium] orphan_expire_sweep_removes_zero_expiry_session passes even if the orphan was never created — tests/test_broker_connect.c:3825-3898
• [Medium] subscribe_v5_retain_handling_2_never_delivers has no positive control — tests/test_broker_connect.c:3968-4026
• [Medium] Two new tests are over-guarded with WOLFMQTT_V5 despite using pure v3.1.1 wire against non-V5 code paths — tests/test_broker_connect.c:3589-3686, 4096-4164
• [Medium] Two of four MqttClient_RestoreRecvQuota call sites are unreachable by any existing test — src/mqtt_client.c:2578-2606
• [Medium] MULTITHREAD branch of the WaitType fatal teardown asserts nothing about the disconnect callback — src/mqtt_client.c:1751-1765
• [Medium] Five of six MqttClient_IsFatalProtoError codes are never driven through the teardown path — src/mqtt_client.c:1224-1233
• [Medium] PUBREL/PUBCOMP reject direction of the new Reason Code allow-list is untested — src/mqtt_packet.c:273-305
• [Medium] Duplicate Subscription Identifier is now allowed only in PUBLISH, but the allowed direction has no test — src/mqtt_packet.c:1008-1030
• [Medium] Maximum Packet Size = 0 rejection also fires for CONNACK, but only the CONNECT direction is tested — src/mqtt_packet.c:1103-1107
• [Medium] Auth Data without Auth Method check applies to AUTH and CONNACK, but only CONNECT is tested — src/mqtt_packet.c:1274-1277
• [Medium] Will Properties allow-list: six of the seven permitted types have no accept-direction test — src/mqtt_packet.c:1542-1554
• [Medium] BrokerProps_Clone string and binary deep-copy branches are never exercised — src/mqtt_broker.c:1666-1700
• [Medium] Orphan offline-queue v5 property clone is enqueued but never verified end-to-end — src/mqtt_broker.c:2756-2833
• [Medium] BrokerOrphan_ExpireSweep backward-clock guard has no test, despite an existing harness pattern for exactly this — src/mqtt_broker.c:2866-2881
• [Medium] No settable mock clock, so several new time-dependent orphan/expiry paths cannot be tested — src/mqtt_broker.c:2854-2884
• [Medium] PUBCOMP fatal-decode close is untested while its PUBACK twin got a test — src/mqtt_broker.c:6396-6424
• [Medium] Accepted DISCONNECT Session Expiry update is untested — src/mqtt_broker.c:6484-6498
• [Medium] BrokerClient_PublishWillImmediate QoS>=1 out_q route is never exercised — src/mqtt_broker.c:4386-4409
• [Medium] MqttClient_Publish doxygen does not document the new pre-send MQTT_CODE_ERROR_SERVER_PROP rejections — wolfmqtt/mqtt_client.h:380-431
• [Medium] MqttClient_WaitMessage doxygen does not document the new fatal-protocol-error disconnect semantics — wolfmqtt/mqtt_client.h:578-586

Skipped findings

• [Low] Fatal decode failure on a v5 DISCONNECT is still ignored, inconsistent with the PUBACK/PUBCOMP tightening in the same PR
• [Low] Handle_ConnectAck_Props aborts the property scan on Receive Maximum 0, leaving partially latched CONNACK state
• [Low] MULTITHREAD fatal-error teardown clears IS_CONNECTED without closing the socket, changing reconnect behaviour
• [Low] Topic Alias validation re-runs on every non-blocking re-entry and stops at the first Topic Alias property
• [Low] Fatal-error teardown reads client->flags without lockClient and discards MqttSocket_Disconnect's return code
• [Low] Retained and Will QoS>=1 enqueue paths silently drop the message on queue-full or allocation failure
• [Low] v5 PUBLISH properties are cloned even for v3.1.1 subscribers, and a clone OOM drops the message entirely
• [Low] Forwarded v5 Message Expiry Interval is not decremented by the time spent waiting
• [Low] DISCONNECT-time Session Expiry update is a no-op for v5 Clean Start=1 clients
• [Low] MqttEncode_PublishResp now hard-fails instead of degrading when an application supplies an out-of-table Reason Code
• [Low] Publish-response Reason Code allow-list enforced only on encode, not on decode
• [Low] RecvQuotaReserve decrements word16 with no floor; exhaustion check reads the counter outside lockClient
• [Low] Any received PUBACK/PUBCOMP replenishes the quota, including for unknown packet ids
• [Low] MqttClient_RestoreRecvQuota fires on non-terminal CONTINUE in multithread builds without NONBLOCK
• [Low] publish_v5_props_survive_queued_delivery pins only the total remaining length, not which property survived
• [Low] connect_v5_max_packet_size_zero_protocol_error assertions cannot distinguish the tested behaviour from any other malformed CONNECT
• [Low] The other two MqttClient_RestoreRecvQuota call sites execute but no test asserts the quota was restored
• [Low] New MqttClient fields are not asserted at Init and their per-connect reset is untested
• [Low] Topic Alias validation has only negative tests; the accept path and the boundary value are untested
• [Low] Caller-supplied gwAddr branch of the new GWINFO default is untested
• [Low] New public SN_GwInfo.gwAddrBuf field and the gwAddr ownership rule are not in any user-facing doc
• [Low] BrokerSubs_OrphanClient count==0 with Session Expiry 0 early return is executed but never asserted
• [Low] Retain Handling = 0 deliver path is never taken with a v5 subscriber
• [Low] BROKER_MAX_WILL_DELAY_SEC is now a dead macro but is still documented as an enforced cap
• [Low] BROKER.md omits both new CONNECT rejections (Enhanced Auth, Will-disabled)
• [Low] BROKER.md not updated for the new v5 Session Expiry enforcement behavior
• [Low] WOLFMQTT_BROKER_TIME_T is documented as an override point but the new build-breaking unsigned requirement is not stated
• [Low] New tests leak the broker on assertion failure (ASSERT_* returns before MqttBroker_Free)
• [Low] Three new bare scope blocks violate the project ban on standalone { ... } scopes
• [Low] New gwAddrBuf makes gwAddr self-referential; copying SN_GwInfo/SN_SearchGw by value leaves it pointing at the source
• [Low] Public struct layouts grew without a libtool version-info bump
• [Low] SN_Decode_GWInfo rejects a well-formed GWINFO frame with BAD_ARG based on caller state
• [Low] Will publish_time saturation branch is untested and unreachable in the CI build
• [Low] No allocation-failure injection, so the new clone OOM cleanup branches are untestable
• [Low] Lock-failure early returns in the new quota helpers are unreachable from the unit-test binary
• [Low] WOLFMQTT_NO_UTF8_VALIDATION is exercised by no build config, and encode-side tests are not guarded by it
• [Low] Retained QoS>=1 full-queue drop and alloc-failure branches are untested
• [Info] RUN_TEST registration uses 10 duplicated nested #ifdef pairs instead of the file's combined-guard style
• [Info] Two added lines exceed the 80-column limit
• [Info] Spec citations for CONNACK Receive Maximum / Topic Alias Maximum point at the CONNECT sections
• [Info] Stale comment in MqttDecode_Subscribe now contradicted by the new Retain Handling enforcement
• [Info] Two new v5 Protocol Error checks added in the same change return different error codes
• [Info] New BrokerOutPub.props member inserted mid-struct rather than appended
• [Medium] Two new tests are over-guarded with WOLFMQTT_V5 despite using pure v3.1.1 wire against non-V5 code paths

Review generated by Skoll via Claude/Codex

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #567

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.