Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens inbound ZCL command argument parsing so truncated/unparseable command payloads are rejected with ZCLError('MALFORMED_COMMAND') instead of being silently parsed (often as all-zero “SUCCESS”/“all-clear” values). This leverages existing Endpoint.handleFrame behavior to automatically emit a proper ZCL Default Response without additional wiring.
Changes:
- Added a shared
parseCommandArgs()helper that performs a minimum-length precheck (when applicable) and converts parse-time exceptions intoMALFORMED_COMMAND. - Updated
Cluster.handleFrameandBoundCluster.handleFrameto use the new helper instead of callingcommand.args.fromBuffer(...)directly. - Added tests to ensure truncated IAS Zone frames don’t invoke handlers and do produce
MALFORMED_COMMANDDefault Responses (with regressions for well-formed frames).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/testMalformedFrames.js | Adds regression + hardening tests verifying truncated payloads yield MALFORMED_COMMAND and do not call handlers. |
| lib/util/parseCommandArgs.js | Introduces defensive argument parsing helper with length precheck + parse error normalization. |
| lib/Cluster.js | Routes inbound cluster command arg parsing through parseCommandArgs(). |
| lib/BoundCluster.js | Routes inbound bound-cluster command arg parsing through parseCommandArgs(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merged
5 tasks done
nozols
approved these changes
May 21, 2026
RobinBol
added a commit
that referenced
this pull request
May 21, 2026
Follow-up to the command-arg hardening. Addresses two bypasses that remained after the first pass: 1. Attribute reports (cmdId 0x0A reportAttributes) parse a raw payload into ZCLAttributeDataRecord entries, where each entry's value field was read via `DataType.fromBuffer` without bounds checking. For fixed-width types like map16 (IAS Zone `zoneStatus`, attribute id 0x0002), a missing or short value silently produced an all-bits-false bitmap that propagates to drivers as "all alarms clear". Same fail- open path as the command-arg case from PR #193, just routed through the attribute-report channel. Add a length precheck in `ZCLAttributeDataRecord.fromBuf` before calling the value's `fromBuffer`. On short input, throw `ZCLError('MALFORMED_COMMAND')` which `Endpoint.handleFrame` already converts to a proper default-response back to the sender. 2. The previous `encodeMissingFieldsBehavior: 'skip'` exemption in `parseCommandArgs` was too broad: it skipped *all* length checking for those commands, so an empty payload to e.g. OTA `queryNextImageRequest` parsed as all-zero fields. Tighten to require at least the first field's byte width, which preserves the legitimate "omit trailing optional fields" case while rejecting truncated leading-discriminator payloads. 5 new tests, including regression guards for well-formed reports and for OTA's omit-trailing-fields path. 86/86 tests pass (was 81 + 5).
RobinBol
added a commit
that referenced
this pull request
Oct 23, 2026
Follow-up to the command-arg hardening. Addresses two bypasses that remained after the first pass: 1. Attribute reports (cmdId 0x0A reportAttributes) parse a raw payload into ZCLAttributeDataRecord entries, where each entry's value field was read via `DataType.fromBuffer` without bounds checking. For fixed-width types like map16 (IAS Zone `zoneStatus`, attribute id 0x0002), a missing or short value silently produced an all-bits-false bitmap that propagates to drivers as "all alarms clear". Same fail- open path as the command-arg case from PR #193, just routed through the attribute-report channel. Add a length precheck in `ZCLAttributeDataRecord.fromBuf` before calling the value's `fromBuffer`. On short input, throw `ZCLError('MALFORMED_COMMAND')` which `Endpoint.handleFrame` already converts to a proper default-response back to the sender. 2. The previous `encodeMissingFieldsBehavior: 'skip'` exemption in `parseCommandArgs` was too broad: it skipped *all* length checking for those commands, so an empty payload to e.g. OTA `queryNextImageRequest` parsed as all-zero fields. Tighten to require at least the first field's byte width, which preserves the legitimate "omit trailing optional fields" case while rejecting truncated leading-discriminator payloads. 5 new tests, including regression guards for well-formed reports and for OTA's omit-trailing-fields path.
RobinBol
force-pushed
the
fix/malformed-frame-hardening
branch
from
October 23, 2026 08:27
e50f2ea to
cfaaec0
Compare
| // Fixed-width field's length is a positive integer; varsize is negative or 0. | ||
| // For varsize first fields (uncommon for 'skip' commands), require at least | ||
| // 1 byte so the length prefix is present. | ||
| return firstField.length > 0 ? firstField.length : 1; |
| // > 0 : exact byte count for an all-fixed-size struct | ||
| // < 0 : negative of the fixed-portion size for a varsize struct | ||
| // = 0 : no payload required | ||
| return Math.abs(argsType.length); |
The parser primitives in @athombv/data-types silently substitute zero
values when the source buffer is shorter than the declared field length.
For a ZCL command struct, this means a truncated payload parses into an
object where every numeric field is 0 and every bitmap field has all
bits clear - which for alarm-routing clusters (IAS Zone) happens to look
like "all clear" and for status-bearing response frames happens to look
like "SUCCESS".
This change adds a length precheck and try/catch around command arg
parsing in Cluster.handleFrame and BoundCluster.handleFrame. Truncated
payloads now throw ZCLError('MALFORMED_COMMAND'), which Endpoint.handleFrame
already converts into a proper default-response back to the sender.
Commands declared with encodeMissingFieldsBehavior: 'skip' (OTA) are
exempt from the strict length precheck because they intentionally allow
trailing fields to be omitted; for those, only the try/catch fallback
applies.
Follow-up to the command-arg hardening. Addresses two bypasses that remained after the first pass: 1. Attribute reports (cmdId 0x0A reportAttributes) parse a raw payload into ZCLAttributeDataRecord entries, where each entry's value field was read via `DataType.fromBuffer` without bounds checking. For fixed-width types like map16 (IAS Zone `zoneStatus`, attribute id 0x0002), a missing or short value silently produced an all-bits-false bitmap that propagates to drivers as "all alarms clear". Same fail- open path as the command-arg case from PR #193, just routed through the attribute-report channel. Add a length precheck in `ZCLAttributeDataRecord.fromBuf` before calling the value's `fromBuffer`. On short input, throw `ZCLError('MALFORMED_COMMAND')` which `Endpoint.handleFrame` already converts to a proper default-response back to the sender. 2. The previous `encodeMissingFieldsBehavior: 'skip'` exemption in `parseCommandArgs` was too broad: it skipped *all* length checking for those commands, so an empty payload to e.g. OTA `queryNextImageRequest` parsed as all-zero fields. Tighten to require at least the first field's byte width, which preserves the legitimate "omit trailing optional fields" case while rejecting truncated leading-discriminator payloads. The tests run on the virtual device; iasZone.js and ota.js cover the well-formed frames.
RobinBol
force-pushed
the
fix/malformed-frame-hardening
branch
from
October 27, 2026 14:50
cfaaec0 to
6d00128
Compare
RobinBol
changed the title
This branch has not been deployed
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
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.
What
A command or attribute report whose payload is too short for its fields is now rejected with a
MALFORMED_COMMANDDefault Response, and its handler orattr.<name>event does not fire.Why
@athombv/data-typeszero-fills missing bytes instead of failing. A truncated IAS ZonezoneStatusChangeNotificationorzoneStatusreport therefore reached drivers as "all alarms clear", and a truncated status field asSUCCESS. A driver cannot tell that apart from a real report.How
lib/util/parseCommandArgs.js, used byCluster.handleFrameandBoundCluster.handleFrame, checks the payload against the fixed size of the command's args before parsing, and turns a parser throw intoMALFORMED_COMMAND.encodeMissingFieldsBehavior: 'skip'(OTA) may omit trailing fields, so for those only the first field is required. Every such command starts with a discriminator byte.ZCLAttributeDataRecordchecks that a fixed-width attribute value fits in the remaining bytes.Anything Else
Variable-length fields (
octstrand similar) are not covered:bufferFromBufreturns a partial buffer without throwing. The real fix belongs in@athombv/data-types, where short reads should fail instead of returning zeros; two related hardening changes landed in athombv/node-data-types#44.