SetUpCodePairer: fail fast on PASE failure during NFC commissioning#72254
Open
woody-apple wants to merge 1 commit into
Open
SetUpCodePairer: fail fast on PASE failure during NFC commissioning#72254woody-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 attempts to immediately propagate PASE pairing failures when commissioning is initiated over NFC, preventing unnecessary delays from BLE/mDNS discovery timeouts. However, a critical issue was raised: Transport::Type::kNfc does not exist in the Transport::Type enum, which will cause a compilation error. Furthermore, because NFC is only used out-of-band to transmit the onboarding payload and the actual PASE session runs over BLE or IP, checking the transport type of the peer address will not work. It is recommended to use an explicit flag on SetUpCodePairer to track NFC-initiated commissioning instead.
| // fails, OnStatusUpdate can propagate the failure immediately rather | ||
| // than waiting for BLE/mDNS discovery (which can't produce a useful | ||
| // alternative for an NFC-tapped target). | ||
| mLastPASETransportWasNfc = (params.GetPeerAddress().GetTransportType() == Transport::Type::kNfc); |
Contributor
There was a problem hiding this comment.
This code will fail to compile because Transport::Type::kNfc does not exist in the Transport::Type enum (which only defines kUndefined, kUdp, kBle, and kTcp).
Furthermore, even if it compiled, NFC is not a transport used for running the PASE protocol in Matter. NFC is only used out-of-band to transmit the onboarding payload. The actual PASE session is established over BLE (Transport::Type::kBle) or IP (Transport::Type::kUdp / Transport::Type::kTcp). Therefore, params.GetPeerAddress().GetTransportType() will never return kNfc during a PASE attempt.
If you need to distinguish NFC-initiated commissioning to fail fast, you should introduce an explicit flag on SetUpCodePairer (e.g., set by the caller when initiating pairing via NFC) rather than relying on the transport type of the connection.
Contributor
Author
There was a problem hiding this comment.
(FYI: This is Claude) False positive. Transport::Type::kNfc is defined in src/transport/raw/PeerAddress.h:58. The enum includes kUndefined, kBle, kTcp, kUdp, kWiFiPAF, kNfc — gemini missed both kNfc and kWiFiPAF. CI is green precisely because this compiles. No code change needed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72254 +/- ##
==========================================
+ Coverage 55.52% 55.59% +0.07%
==========================================
Files 1629 1630 +1
Lines 111092 111141 +49
Branches 13415 13396 -19
==========================================
+ Hits 61683 61791 +108
+ Misses 49409 49350 -59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72254: Size comparison from 8ad5f7b to 082d318 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72254: Size comparison from 8ad5f7b to 036f1c0 Full report (25 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
nfc-pase-fail-fast
branch
from
036f1c0 to
7e047cd
Compare
|
PR #72254: Size comparison from 4894db9 to 195b643 Full report (31 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
nfc-pase-fail-fast
branch
2 times, most recently
from
aa79262 to
30e9b1a
Compare
|
PR #72254: Size comparison from 4894db9 to 5f889d7 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
nfc-pase-fail-fast
branch
2 times, most recently
from
15dc89e to
89c61a2
Compare
|
PR #72254: Size comparison from 4894db9 to 521f7c9 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
nfc-pase-fail-fast
branch
from
521f7c9 to
1b23c2d
Compare
|
PR #72254: Size comparison from 8a162c6 to 1b23c2d Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #72254: Size comparison from 8a162c6 to d34d3d6 Full report (25 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
woody-apple
added a commit
to woody-apple/connectedhomeip
that referenced
this pull request
May 30, 2026
…NFC fast-fail Round-2 review consensus on PR project-chip#72254 surfaced five issues: 1. OnStatusUpdate's NFC fast-fail forwarded SecurePairingFailed to the delegate while OnPairingComplete also fired with the real error, double-delivering the failure (e.g. duplicate commissioningSessionEstablishmentDone: callbacks on Darwin). 2. The two NFC fast-fail gates used asymmetric predicates (NonNetworkDiscoveryInProgress vs DiscoveryInProgress), allowing OnStatusUpdate to fire first and OnPairingComplete to also fire afterward. 3. OnStatusUpdate did not StopAllDiscoveryAttempts, so a late DNS-SD candidate could push a new entry into mDiscoveredParameters and restart PASE after the failure had been (incorrectly) propagated. 4. mLastPASETransportWasNfc was only cleared in ResetDiscoveryState, leaving it sticky-true across reused-pairer attempts. 5. OnPairingComplete's NFC fast-fail did not clear mRemoteId, so a late async cancel callback could re-enter StopPairingIfTransportsExhausted and re-fire OnSessionEstablishmentError with a stale mLastPASEError. Fixes: - OnStatusUpdate NFC fast-fail no longer forwards to the delegate. It logs, calls StopAllDiscoveryAttempts(), cancels the discovery timer, clears mLastPASETransportWasNfc, and returns. OnPairingComplete is now the single authoritative delegate-notification path for an NFC PASE failure (it carries the real error code). - OnPairingComplete NFC fast-fail snapshots mLastPASETransportWasNfc before PASEEstablishmentComplete (which now clears it), then on the fast-fail branch additionally clears mRemoteId and cancels the discovery timer (mirroring the success path's cleanup). Log message now includes the error code via CHIP_ERROR_FORMAT. - PASEEstablishmentComplete clears mLastPASETransportWasNfc to bound the flag's lifetime to a single PASE attempt regardless of which termination path runs. Test changes: - Existing OnStatusUpdate-propagation tests flipped to expect suppression (mStatusUpdateCount == 0) and cleanup (mWaitingForDiscovery cleared, flag cleared). - New NfcPASEFailure_OnStatusUpdate_DoesNotDoubleDeliver test exercises the production-shape sequence: OnStatusUpdate suppresses + cleans up, then OnPairingComplete delivers the real error exactly once. - New FlagClearedByOnStatusUpdateFastFail and FlagClearedOnPASEEstablishmentComplete tests exercise the production reset paths instead of round-tripping the accessor. - FlagDerivedFromNfcPeerAddress removed (post-call value is now always cleared by the new PASEEstablishmentComplete clear, so the test could no longer distinguish assignment from non-assignment). - SuccessiveNfcPairings_FlagTrackedPerAttempt rewritten to drive OnPairingComplete directly (where the NFC fast-fail's discovery teardown is observable per attempt). - SetUpCodePairerTestAccess.h: added kNFCTransport (gated on CHIP_DEVICE_CONFIG_ENABLE_NFC_BASED_COMMISSIONING) for future tests.
|
PR #72254: Size comparison from 8a162c6 to 5b97c70 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
added a commit
to woody-apple/connectedhomeip
that referenced
this pull request
May 30, 2026
…NFC fast-fail Round-2 review consensus on PR project-chip#72254 surfaced five issues: 1. OnStatusUpdate's NFC fast-fail forwarded SecurePairingFailed to the delegate while OnPairingComplete also fired with the real error, double-delivering the failure (e.g. duplicate commissioningSessionEstablishmentDone: callbacks on Darwin). 2. The two NFC fast-fail gates used asymmetric predicates (NonNetworkDiscoveryInProgress vs DiscoveryInProgress), allowing OnStatusUpdate to fire first and OnPairingComplete to also fire afterward. 3. OnStatusUpdate did not StopAllDiscoveryAttempts, so a late DNS-SD candidate could push a new entry into mDiscoveredParameters and restart PASE after the failure had been (incorrectly) propagated. 4. mLastPASETransportWasNfc was only cleared in ResetDiscoveryState, leaving it sticky-true across reused-pairer attempts. 5. OnPairingComplete's NFC fast-fail did not clear mRemoteId, so a late async cancel callback could re-enter StopPairingIfTransportsExhausted and re-fire OnSessionEstablishmentError with a stale mLastPASEError. Fixes: - OnStatusUpdate NFC fast-fail no longer forwards to the delegate. It logs, calls StopAllDiscoveryAttempts(), cancels the discovery timer, clears mLastPASETransportWasNfc, and returns. OnPairingComplete is now the single authoritative delegate-notification path for an NFC PASE failure (it carries the real error code). - OnPairingComplete NFC fast-fail snapshots mLastPASETransportWasNfc before PASEEstablishmentComplete (which now clears it), then on the fast-fail branch additionally clears mRemoteId and cancels the discovery timer (mirroring the success path's cleanup). Log message now includes the error code via CHIP_ERROR_FORMAT. - PASEEstablishmentComplete clears mLastPASETransportWasNfc to bound the flag's lifetime to a single PASE attempt regardless of which termination path runs. Test changes: - Existing OnStatusUpdate-propagation tests flipped to expect suppression (mStatusUpdateCount == 0) and cleanup (mWaitingForDiscovery cleared, flag cleared). - New NfcPASEFailure_OnStatusUpdate_DoesNotDoubleDeliver test exercises the production-shape sequence: OnStatusUpdate suppresses + cleans up, then OnPairingComplete delivers the real error exactly once. - New FlagClearedByOnStatusUpdateFastFail and FlagClearedOnPASEEstablishmentComplete tests exercise the production reset paths instead of round-tripping the accessor. - FlagDerivedFromNfcPeerAddress removed (post-call value is now always cleared by the new PASEEstablishmentComplete clear, so the test could no longer distinguish assignment from non-assignment). - SuccessiveNfcPairings_FlagTrackedPerAttempt rewritten to drive OnPairingComplete directly (where the NFC fast-fail's discovery teardown is observable per attempt). - SetUpCodePairerTestAccess.h: added kNFCTransport (gated on CHIP_DEVICE_CONFIG_ENABLE_NFC_BASED_COMMISSIONING) for future tests.
woody-apple
force-pushed
the
nfc-pase-fail-fast
branch
from
5b97c70 to
c9d4c8a
Compare
|
PR #72254: Size comparison from 8a162c6 to c9d4c8a 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
nfc-pase-fail-fast
branch
from
c9d4c8a to
aff596a
Compare
|
PR #72254: Size comparison from 8a162c6 to aff596a 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
nfc-pase-fail-fast
branch
from
aff596a to
338a2de
Compare
|
PR #72254: Size comparison from 8a162c6 to 338a2de 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
nfc-pase-fail-fast
branch
from
338a2de to
d450776
Compare
|
PR #72254: Size comparison from 8a162c6 to d450776 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
nfc-pase-fail-fast
branch
2 times, most recently
from
338a2de to
b6b98d7
Compare
|
PR #72254: Size comparison from 8a162c6 to b6b98d7 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Signed-off-by: Justin Wood <woody@apple.com>
woody-apple
force-pushed
the
nfc-pase-fail-fast
branch
from
b6b98d7 to
78a272c
Compare
|
PR #72254: Size comparison from 8a162c6 to 78a272c Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
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.
When commissioning over NFC, the user has tapped a specific tag — that device is the target. If PASE fails against it (e.g. SPAKE2+ MAC verify fails because the setup code derived from the NFC payload does not match the verifier),
SetUpCodePairercurrently keeps waiting for BLE/mDNS discovery (up toCHIP_CONFIG_SETUP_CODE_PAIRER_DISCOVERY_TIMEOUT_SECS) instead of surfacing the definitive failure. This change tracks whether the current PASE attempt was over NFC and, if so, propagatesSecurePairingFailedto the delegate immediately. BLE/mDNS behavior is unchanged. See commit message for details.Testing
Built the controller locally and ran the SetUpCodePairer / commissioning unit tests. Manual repro: initiate NFC commissioning against an accessory whose setup code does not match its SPAKE2+ verifier and confirm the failure now surfaces immediately rather than after the BLE/mDNS discovery timeout; BLE/mDNS commissioning paths are unchanged.
Testing