Setup payload parse failures (Manual + QR) silently flattened to InvalidArgument#72270
Open
woody-apple wants to merge 1 commit into
Open
Setup payload parse failures (Manual + QR) silently flattened to InvalidArgument#72270woody-apple wants to merge 1 commit into
woody-apple wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new initializer initWithManualPairingCode:error: to MTRSetupPayload to allow propagating detailed errors (such as integrity check failures for incorrect Verhoeff check digits) during manual pairing code parsing instead of flattening them to a generic invalid argument error. The concrete device controller, manual setup payload parser, and onboarding payload parsing helper are updated to leverage this new initializer, and comprehensive unit tests are added. A review comment suggests adding a defensive nil check for manualCode in the new initializer to prevent potential crashes from passing nil to the underlying parser.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Darwin (MTR) error-propagation issue where a Manual Pairing Code with a bad Verhoeff check digit would fail with CHIP_ERROR_INTEGRITY_CHECK_FAILED in the CHIP parser, but the Objective-C layer would flatten the failure to MTRErrorCodeInvalidArgument, preventing UI from surfacing a user-actionable “integrity check failed / mistyped code” error.
Changes:
- Add an
NSError**-bearing-[MTRSetupPayload initWithManualPairingCode:error:]and route manual-code parsing through it to preserve the underlyingCHIP_ERRORmapping. - Update downstream manual-code parsing entry points (
MTRManualSetupPayloadParser,pairDevice:onboardingPayload:error:, and+setupPayloadWithOnboardingPayload:error:manual path) to use the new initializer. - Add XCTests that pin the integrity-check error shape and a valid manual-code happy path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/darwin/Framework/CHIPTests/MTRSetupPayloadTests.m | Adds tests validating MTRErrorCodeIntegrityCheckFailed is surfaced for bad manual codes and validates a known-good manual code. |
| src/darwin/Framework/CHIP/MTRSetupPayload.mm | Implements the new error-bearing manual-code initializer and routes manual onboarding parsing through it. |
| src/darwin/Framework/CHIP/MTRSetupPayload.h | Declares the new initWithManualPairingCode:error: API with documentation about integrity-check failures. |
| src/darwin/Framework/CHIP/MTRManualSetupPayloadParser.mm | Routes parser entry point through the new initializer to preserve error fidelity. |
| src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm | Uses +setupPayloadWithOnboardingPayload:error: so pairing can surface integrity-check failures for manual codes. |
Comments suppressed due to low confidence (1)
src/darwin/Framework/CHIP/MTRSetupPayload.mm:807
pairDevice:onboardingPayload:error:used to return an NSError created viaerrorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENTwhen onboarding payload parsing failed, which includes a localized description and associates the underlying CHIP_ERROR. After routing through+setupPayloadWithOnboardingPayload:error:, QR/unknown payload failures now useerrorWithCode:MTRErrorCodeInvalidArgument(no description/userInfo), which is a behavioral regression for error reporting.
MTRSetupPayload * payload = [[MTRSetupPayload alloc] initWithPayload:onboardingPayload];
if (!payload && error) {
*error = [MTRError errorWithCode:MTRErrorCodeInvalidArgument];
}
return payload;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #72270: Size comparison from adba29b to be20ffc Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72270 +/- ##
==========================================
+ Coverage 55.52% 55.57% +0.04%
==========================================
Files 1630 1630
Lines 111127 111223 +96
Branches 13418 13409 -9
==========================================
+ Hits 61706 61812 +106
+ Misses 49421 49411 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
be20ffc to
b4c1c57
Compare
|
PR #72270: Size comparison from 4894db9 to b4c1c57 Full report (32 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
b4c1c57 to
b3b41c8
Compare
woody-apple
force-pushed
the
dev/woody/177309700
branch
2 times, most recently
from
8fb929d to
bb01cf7
Compare
woody-apple
changed the title
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
bb01cf7 to
889f141
Compare
github-actions
Bot
added
documentation
|
PR #72270: Size comparison from 8a162c6 to 603c1d2 Full report (31 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72270: Size comparison from 8a162c6 to a98f213 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72270: Size comparison from 8a162c6 to 8de52c2 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
8de52c2 to
ae961a1
Compare
|
PR #72270: Size comparison from 8a162c6 to fd5a4fe Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
fd5a4fe to
3ecfae1
Compare
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
3ecfae1 to
370089c
Compare
|
PR #72270: Size comparison from 8a162c6 to 370089c Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
370089c to
05e8305
Compare
|
PR #72270: Size comparison from 8a162c6 to 05e8305 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
05e8305 to
540e012
Compare
woody-apple
force-pushed
the
dev/woody/177309700
branch
from
540e012 to
5be6f46
Compare
|
PR #72270: Size comparison from f1767a8 to 14f7640 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72270: Size comparison from 9cf1485 to f523708 Full report (22 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
… flattened to InvalidArgument) Add NSError**-bearing inits on MTRSetupPayload (-initWithManualPairingCode:error: public, -initWithQRCode:error: internal) that preserve the underlying CHIP_ERROR via MTRError, so callers can distinguish typo/integrity-check failures from generic parse rejection. Route MTRManualSetupPayloadParser, MTRQRCodeSetupPayloadParser, +setupPayloadWithOnboardingPayload:error:, and -pairDevice:onboardingPayload:error: through the new variants. No-error inits preserved as thin wrappers. The error-bearing -initWithManualPairingCode:error: is declared once, on the public MTRSetupPayload.h; the redundant MTR_DIRECT_MEMBERS re-declaration in MTRSetupPayload_Internal.h is removed to avoid the duplicate/direct-conflict declaration that broke the Darwin/tvOS framework builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
PR #72270: Size comparison from 9cf1485 to 590f035 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
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.
Summary
A Manual Pairing Code with a wrong Verhoeff check digit causes
ManualSetupPayloadParser::populatePayloadto returnCHIP_ERROR_INTEGRITY_CHECK_FAILED, but the Darwin (MTR) layer was discarding that specific code:-[MTRSetupPayload initWithManualPairingCode:]had noNSError**out-parameter, and downstream call sites (+setupPayloadWithOnboardingPayload:error:,-[MTRManualSetupPayloadParser populatePayload:],-[MTRDeviceController pairDevice:onboardingPayload:error:]) flattened every parse failure toMTRErrorCodeInvalidArgument. Callers had no way to distinguish "the user typed a bad setup code" from any other parse error, so a stale/typo'd manual code would silently stall instead of surfacing as an integrity-check failure to the UI.Adversarial audit — broader scope
A second pass found the QR-code parse path had the same shape:
-[MTRSetupPayload initWithQRCode:]has no error out-parameter, discarding theCHIP_ERRORreturned byQRCodeSetupPayloadParser::populatePayloads.-[MTRQRCodeSetupPayloadParser populatePayload:]rewrote whatever failure happened toMTRErrorCodeInvalidArgument.+setupPayloadWithOnboardingPayload:error:did the same — calling the no-error init and then forcingMTRErrorCodeInvalidArgumenton nil.So a scanner glitch or typed QR string with (e.g.) a bad Base38 character (
CHIP_ERROR_INVALID_INTEGER_VALUE) or an undecodable chunk length (CHIP_ERROR_INVALID_STRING_LENGTH) was indistinguishable from any other failure — same "silent stall" symptom as the manual-code path.NFC commissioning shares the QR parse path (an NFC tag carries a Matter URL whose payload is the same Base38 string), so this fix covers NFC payload failures too.
Fix
NSError**-bearing variants onMTRSetupPayload:-initWithManualPairingCode:error:(already in the previous revision of this PR) and now-initWithQRCode:error:(internal). Both map the underlyingCHIP_ERRORvia[MTRError errorForCHIPErrorCode:].-[MTRManualSetupPayloadParser populatePayload:]-[MTRQRCodeSetupPayloadParser populatePayload:]+[MTRSetupPayload setupPayloadWithOnboardingPayload:error:](both QR and manual branches collapse to a single dispatch)-[MTRDeviceController pairDevice:onboardingPayload:error:]CHIP_ERROR -> MTRErrormappings already exist inMTRError.mm:CHIP_ERROR_INTEGRITY_CHECK_FAILED->MTRErrorCodeIntegrityCheckFailedCHIP_ERROR_INVALID_INTEGER_VALUE->MTRErrorCodeInvalidIntegerValueCHIP_ERROR_INVALID_STRING_LENGTH->MTRErrorCodeInvalidStringLengthTesting
MTRSetupPayloadTests.m:MTRErrorCodeIntegrityCheckFailed(pins both-initWithManualPairingCode:error:and+setupPayloadWithOnboardingPayload:error:, and theMTRManualSetupPayloadParsersurface), plus the happy path.?) ->MTRErrorCodeInvalidIntegerValue(pins both the+setupPayloadWithOnboardingPayload:error:andMTRQRCodeSetupPayloadParsersurfaces).MTRErrorCodeInvalidStringLength.XCTAssertNotEqualthe pre-fixMTRErrorCodeInvalidArgumentto lock the regression.MTRPairingBackwardsCompatTests.m:test003_PairDeviceWithBadManualPairingCode_SurfacesIntegrityCheckFailedis an end-to-end integration pin on-[MTRDeviceController pairDevice:onboardingPayload:error:]— fails at parse time, no server app needed.TestManualCode.TestPayloadParser_InvalidEntryleft intact.Test plan