Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.3] NetworkCommissioning Cluster reads SSID field for Thread server instances #32875

Closed
s-mcclain opened this issue Apr 5, 2024 · 2 comments · Fixed by #32884
Closed

[1.3] NetworkCommissioning Cluster reads SSID field for Thread server instances #32875

s-mcclain opened this issue Apr 5, 2024 · 2 comments · Fixed by #32884
Labels
bug Something isn't working needs triage

Comments

@s-mcclain
Copy link
Contributor

s-mcclain commented Apr 5, 2024

Reproduction steps

There are some Matter Admins which may populate an empty SSID for the ScanNetworks command in NetworkCommissioning cluster in the field. The recent changes to the SDK now always checks if the SSID field exists regardless if the field is empty or not.

This will result in a Matter Accessory with these changes to always fail the ScanNetworks request.

The Matter specification states the following about the SSID FIeld:

11.9.7.1


SSID Field
This field, if present, SHALL contain the SSID for a directed scan of that particular Wi-Fi SSID. Otherwise, if the field is absent, or if it is null, this SHALL indicate scanning of all BSSID in range. This field SHALL be ignored for ScanNetworks invocations on non-WiFi server instances.

The last sentence indicates a requirement for the SSID field to be ignored for non-WiFi servers.

This was introduced in #32019

Recommendation:
Ignore the field entirely. Or at the very least, add check if ssid.HasValue() && !ssid.Value().empty()

Bug prevalence

Always

GitHub hash of the SDK that was being used

660c928

Platform

core

Platform Version(s)

1.3

Type

Spec Compliance Issue

Anything else?

No response

@s-mcclain s-mcclain added bug Something isn't working needs triage labels Apr 5, 2024
@bzbarsky-apple
Copy link
Contributor

The last sentence indicates a requirement for the SSID field to be ignored for non-WiFi servers.

The problem is that this sentence contradicts the normative requirement in the Conformance column, which says that this field is only allowed to be present when the WI feature is supported.

But yes, the fact that the spec says self-contradictory things is not good, and we need to change one part or the other here... @tcarmelveilleux thoughts?

@tcarmelveilleux
Copy link
Contributor

The issue is that the test plan for TC-CNET-4.22 took the SHALL:

This field SHALL be ignored for ScanNetworks invocations on non-WiFi server instances

and misinterpreted that it had to be allowed for TH. I think we have to clean-up the conformance here. Since 1.0, TC-CNET-4.22 has been enforcing (in theory) that a TH server tolerates the NULL. It's just that this is not checked in CI since Thread based commissioning is not in CI, and the SDK auto-commissioner anyway doesn't make sure of ScanNetworks.

@mergify mergify bot closed this as completed in #32884 Apr 9, 2024
mergify bot pushed a commit that referenced this issue Apr 9, 2024
…Thread feature) (#32884)

* Temp fix for 32875

* Clarify comments
jmartinez-silabs pushed a commit to jmartinez-silabs/connectedhomeip that referenced this issue Apr 17, 2024
…for SSID on Thread feature) (project-chip#32884)

* Temp fix for 32875

* Clarify comments
jmartinez-silabs added a commit that referenced this issue Apr 17, 2024
…Thread feature) (#32884) (#33032)

* Temp fix for 32875

* Clarify comments

Co-authored-by: chrisdecenzo <[email protected]>
jmartinez-silabs added a commit to SiliconLabs/matter that referenced this issue May 8, 2024
…ix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884)

Merge in WMN_TOOLS/matter from cherry-pick/remove_no_ssid_check to RC_2.3.0-1.3

Squashed commit of the following:

commit 29c1eb165d6e322bb106ef200950c57485118b52
Author: chrisdecenzo <[email protected]>
Date:   Tue Apr 9 08:27:01 2024 -0700

    Temp fix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884)

    * Temp fix for 32875

    * Clarify comments
rcasallas-silabs pushed a commit to rcasallas-silabs/connectedhomeip that referenced this issue Jun 20, 2024
…ix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884)

Merge in WMN_TOOLS/matter from cherry-pick/remove_no_ssid_check to RC_2.3.0-1.3

Squashed commit of the following:

commit 29c1eb165d6e322bb106ef200950c57485118b52
Author: chrisdecenzo <[email protected]>
Date:   Tue Apr 9 08:27:01 2024 -0700

    Temp fix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884)

    * Temp fix for 32875

    * Clarify comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
3 participants