[nwprov] Resolve comments in #17038 add more checks (#17197)

* [nwprov] Resolve comments in #17038 add more checks

* Remove enums in include/platform/NetworkCommissioning.h use ssid.empty() instead of ssid.size()

* Fix template

* Regen

src/app/clusters/network-commissioning/network-commissioning.cpp

@@ -45,61 +45,14 @@ namespace {
 // For WiFi and Thread scan results, each item will cose ~60 bytes in TLV, thus 15 is a safe upper bound of scan results.
 constexpr size_t kMaxNetworksInScanResponse = 15;
 
-NetworkCommissioningStatus ToClusterObjectEnum(Status status)
+enum ValidWiFiCredentialLength
 {
- // clang-format off
- static_assert(to_underlying(NetworkCommissioningStatus::kSuccess ) == to_underlying(Status::kSuccess ), "kSuccess value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kOutOfRange ) == to_underlying(Status::kOutOfRange ), "kOutOfRange value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kBoundsExceeded ) == to_underlying(Status::kBoundsExceeded ), "kBoundsExceeded value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kNetworkIDNotFound ) == to_underlying(Status::kNetworkIDNotFound ), "kNetworkIDNotFound value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kDuplicateNetworkID ) == to_underlying(Status::kDuplicateNetworkID ), "kDuplicateNetworkID value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kNetworkNotFound ) == to_underlying(Status::kNetworkNotFound ), "kNetworkNotFound value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kRegulatoryError ) == to_underlying(Status::kRegulatoryError ), "kRegulatoryError value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kAuthFailure ) == to_underlying(Status::kAuthFailure ), "kAuthFailure value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kUnsupportedSecurity ) == to_underlying(Status::kUnsupportedSecurity ), "kUnsupportedSecurity value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kOtherConnectionFailure) == to_underlying(Status::kOtherConnectionFailure), "kOtherConnectionFailure value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kIPV6Failed ) == to_underlying(Status::kIPV6Failed ), "kIPV6Failed value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kIPBindFailed ) == to_underlying(Status::kIPBindFailed ), "kIPBindFailed value mismatch.");
- static_assert(to_underlying(NetworkCommissioningStatus::kUnknownError ) == to_underlying(Status::kUnknownError ), "kUnknownError value mismatch.");
- // clang-format on
- return static_cast<NetworkCommissioningStatus>(to_underlying(status));
-}
-
-NetworkCommissioning::WiFiBand ToClusterObjectEnum(DeviceLayer::NetworkCommissioning::WiFiBand band)
-{
- using ClusterObject = NetworkCommissioning::WiFiBand;
- using PlatfromInterface = DeviceLayer::NetworkCommissioning::WiFiBand;
-
- static_assert(to_underlying(ClusterObject::k2g4) == to_underlying(PlatfromInterface::k2g4), "k2g4 value mismatch.");
- static_assert(to_underlying(ClusterObject::k3g65) == to_underlying(PlatfromInterface::k3g65), "k3g65 value mismatch.");
- static_assert(to_underlying(ClusterObject::k5g) == to_underlying(PlatfromInterface::k5g), "k5g value mismatch.");
- static_assert(to_underlying(ClusterObject::k6g) == to_underlying(PlatfromInterface::k6g), "k6g value mismatch.");
- static_assert(to_underlying(ClusterObject::k60g) == to_underlying(PlatfromInterface::k60g), "k60g value mismatch.");
-
- return static_cast<ClusterObject>(to_underlying(band));
-}
-
-chip::BitFlags<NetworkCommissioning::WiFiSecurity>
-ToClusterObjectBitFlags(const chip::BitFlags<DeviceLayer::NetworkCommissioning::WiFiSecurity> & security)
-{
- using ClusterObject = NetworkCommissioning::WiFiSecurity;
- using PlatformInterface = NetworkCommissioning::WiFiSecurity;
-
- static_assert(to_underlying(ClusterObject::kUnencrypted) == to_underlying(PlatformInterface::kUnencrypted),
- "kUnencrypted value mismatch.");
- static_assert(to_underlying(ClusterObject::kWepPersonal) == to_underlying(PlatformInterface::kWepPersonal),
- "kWepPersonal value mismatch.");
- static_assert(to_underlying(ClusterObject::kWpaPersonal) == to_underlying(PlatformInterface::kWpaPersonal),
- "kWpaPersonal value mismatch.");
- static_assert(to_underlying(ClusterObject::kWpa2Personal) == to_underlying(PlatformInterface::kWpa2Personal),
- "kWpa2Personal value mismatch.");
- static_assert(to_underlying(ClusterObject::kWpa3Personal) == to_underlying(PlatformInterface::kWpa3Personal),
- "kWpa3Personal value mismatch.");
-
- chip::BitFlags<ClusterObject> ret;
- ret.SetRaw(security.Raw());
- return ret;
-}
+ kOpen = 0,
+ kWEP64 = 5,
+ kMinWPAPSK = 8,
+ kMaxWPAPSK = 63,
+ kWPAPSKHex = 64,
+};
 
 } // namespace
 
@@ -261,15 +214,15 @@ CHIP_ERROR Instance::Write(const ConcreteDataAttributePath & aPath, AttributeVal
  }
 }
 
-void Instance::OnNetworkingStatusChange(DeviceLayer::NetworkCommissioning::Status aCommissioningError,
- Optional<ByteSpan> aNetworkId, Optional<int32_t> aConnectStatus)
+void Instance::OnNetworkingStatusChange(NetworkCommissioning::Status aCommissioningError, Optional<ByteSpan> aNetworkId,
+ Optional<int32_t> aConnectStatus)
 {
  if (aNetworkId.HasValue() && aNetworkId.Value().size() > kMaxNetworkIDLen)
  {
  ChipLogError(DeviceLayer, "Invalid network id received when calling OnNetworkingStatusChange");
  return;
  }
- mLastNetworkingStatusValue.SetNonNull(ToClusterObjectEnum(aCommissioningError));
+ mLastNetworkingStatusValue.SetNonNull(aCommissioningError);
  if (aNetworkId.HasValue())
  {
  memcpy(mLastNetworkID, aNetworkId.Value().data(), aNetworkId.Value().size());
@@ -304,6 +257,17 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw
  if (!nullableSSID.IsNull())
  {
  ssid = nullableSSID.Value();
+ if (ssid.empty())
+ {
+ // Normalize the zero value to null ByteSpan.
+ // Spec 7.17.1. Empty string is an equivalent of null.
+ ssid = ByteSpan();
+ }
+ }
+ if (!(ssid.size() <= DeviceLayer::Internal::kMaxWiFiSSIDLength))
+ {
+ ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+ return;
  }
  mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler);
  mpDriver.Get<WiFiDriver *>()->ScanNetworks(ssid, this);
@@ -323,7 +287,7 @@ namespace {
 
 void FillDebugTextAndNetworkIndex(Commands::NetworkConfigResponse::Type & response, MutableCharSpan debugText, uint8_t networkIndex)
 {
- if (debugText.size() > 0)
+ if (!debugText.empty())
  {
  response.debugText.SetValue(CharSpan(debugText.data(), debugText.size()));
  }
@@ -354,15 +318,50 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands
 
  VerifyOrReturn(CheckFailSafeArmed(ctx));
 
+ // Spec 11.8.8.4
+ // Valid Credentials length are:
+ // - 0 bytes: Unsecured (open) connection
+ // - 5 bytes: WEP-64 passphrase
+ // - 10 hexadecimal ASCII characters: WEP-64 40-bit hex raw PSK
+ // - 13 bytes: WEP-128 passphrase
+ // - 26 hexadecimal ASCII characters: WEP-128 104-bit hex raw PSK
+ // - 8..63 bytes: WPA/WPA2/WPA3 passphrase
+ // - 64 bytes: WPA/WPA2/WPA3 raw hex PSK
+ // Note 10 hex WEP64 and 13 bytes / 26 hex WEP128 passphrase are covered by 8~63 bytes WPA passphrase, so we don't check WEP64
+ // hex and WEP128 passphrase.
+ if (req.credentials.size() == ValidWiFiCredentialLength::kOpen || req.credentials.size() == ValidWiFiCredentialLength::kWEP64 ||
+ (req.credentials.size() >= ValidWiFiCredentialLength::kMinWPAPSK &&
+ req.credentials.size() <= ValidWiFiCredentialLength::kMaxWPAPSK))
+ {
+ // Valid length, the credentials can have any characters.
+ }
+ else if (req.credentials.size() == ValidWiFiCredentialLength::kWPAPSKHex)
+ {
+ for (size_t d = 0; d < req.credentials.size(); d++)
+ {
+ if (!isxdigit(req.credentials.data()[d]))
+ {
+ ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+ return;
+ }
+ }
+ }
+ else
+ {
+ // Invalid length
+ ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+ return;
+ }
+
  Commands::NetworkConfigResponse::Type response;
  MutableCharSpan debugText;
 #if CHIP_CONFIG_NETWORK_COMMISSIONING_DEBUG_TEXT_BUFFER_SIZE
  char debugTextBuffer[CHIP_CONFIG_NETWORK_COMMISSIONING_DEBUG_TEXT_BUFFER_SIZE];
  debugText = MutableCharSpan(debugTextBuffer);
 #endif
- uint8_t outNetworkIndex  = 0;
- response.networkingStatus = ToClusterObjectEnum(
- mpDriver.Get<WiFiDriver *>()->AddOrUpdateNetwork(req.ssid, req.credentials, debugText, outNetworkIndex));
+ uint8_t outNetworkIndex = 0;
+ response.networkingStatus =
+ mpDriver.Get<WiFiDriver *>()->AddOrUpdateNetwork(req.ssid, req.credentials, debugText, outNetworkIndex);
  FillDebugTextAndNetworkIndex(response, debugText, outNetworkIndex);
  ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
 }
@@ -381,7 +380,7 @@ void Instance::HandleAddOrUpdateThreadNetwork(HandlerContext & ctx, const Comman
 #endif
  uint8_t outNetworkIndex = 0;
  response.networkingStatus =
- ToClusterObjectEnum(mpDriver.Get<ThreadDriver *>()->AddOrUpdateNetwork(req.operationalDataset, debugText, outNetworkIndex));
+ mpDriver.Get<ThreadDriver *>()->AddOrUpdateNetwork(req.operationalDataset, debugText, outNetworkIndex);
  FillDebugTextAndNetworkIndex(response, debugText, outNetworkIndex);
  ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
 }
@@ -399,7 +398,7 @@ void Instance::HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveN
  debugText = MutableCharSpan(debugTextBuffer);
 #endif
  uint8_t outNetworkIndex = 0;
- response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->RemoveNetwork(req.networkID, debugText, outNetworkIndex));
+ response.networkingStatus = mpWirelessDriver->RemoveNetwork(req.networkID, debugText, outNetworkIndex);
  FillDebugTextAndNetworkIndex(response, debugText, outNetworkIndex);
  ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
 }
@@ -431,7 +430,7 @@ void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::Reorde
  char debugTextBuffer[CHIP_CONFIG_NETWORK_COMMISSIONING_DEBUG_TEXT_BUFFER_SIZE];
  debugText = MutableCharSpan(debugTextBuffer);
 #endif
- response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->ReorderNetwork(req.networkID, req.networkIndex, debugText));
+ response.networkingStatus = mpWirelessDriver->ReorderNetwork(req.networkID, req.networkIndex, debugText);
  FillDebugTextAndNetworkIndex(response, debugText, req.networkIndex);
  ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
 }
@@ -448,8 +447,8 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
  }
 
  Commands::ConnectNetworkResponse::Type response;
- response.networkingStatus = ToClusterObjectEnum(commissioningError);
- if (debugText.size() != 0)
+ response.networkingStatus = commissioningError;
+ if (!debugText.empty())
  {
  response.debugText.SetValue(debugText);
  }
@@ -467,7 +466,7 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
 
  mLastNetworkIDLen = mConnectingNetworkIDLen;
  memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen);
- mLastNetworkingStatusValue.SetNonNull(ToClusterObjectEnum(commissioningError));
+ mLastNetworkingStatusValue.SetNonNull(commissioningError);
 
  commandHandle->AddResponse(mPath, response);
 }
@@ -484,22 +483,22 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI
  return;
  }
 
- mLastNetworkingStatusValue.SetNonNull(ToClusterObjectEnum(status));
+ mLastNetworkingStatusValue.SetNonNull(status);
  mLastConnectErrorValue.SetNull();
  mLastNetworkIDLen = 0;
 
  TLV::TLVWriter * writer;
  TLV::TLVType listContainerType;
  ThreadScanResponse scanResponse;
  size_t networksEncoded = 0;
- uint8_t extendedAddressBuffer[8];
+ uint8_t extendedAddressBuffer[Thread::kSizeExtendedPanId];
 
  SuccessOrExit(err = commandHandle->PrepareCommand(
  ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id)));
  VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
- SuccessOrExit(err = writer->Put(TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kNetworkingStatus)),
-  ToClusterObjectEnum(status)));
+ SuccessOrExit(
+ err = writer->Put(TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kNetworkingStatus)), status));
  if (debugText.size() != 0)
  {
  SuccessOrExit(err = DataModel::Encode(
@@ -547,7 +546,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte
  return;
  }
 
- mLastNetworkingStatusValue.SetNonNull(ToClusterObjectEnum(status));
+ mLastNetworkingStatusValue.SetNonNull(status);
  mLastConnectErrorValue.SetNull();
  mLastNetworkIDLen = 0;
 
@@ -560,8 +559,8 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte
  ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id)));
  VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
- SuccessOrExit(err = writer->Put(TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kNetworkingStatus)),
-  ToClusterObjectEnum(status)));
+ SuccessOrExit(
+ err = writer->Put(TLV::ContextTag(to_underlying(Commands::ScanNetworksResponse::Fields::kNetworkingStatus)), status));
  if (debugText.size() != 0)
  {
  SuccessOrExit(err = DataModel::Encode(
@@ -574,11 +573,11 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte
  for (; networks != nullptr && networks->Next(scanResponse) && networksEncoded < kMaxNetworksInScanResponse; networksEncoded++)
  {
  Structs::WiFiInterfaceScanResult::Type result;
- result.security = ToClusterObjectBitFlags(scanResponse.security);
+ result.security = scanResponse.security;
  result.ssid = ByteSpan(scanResponse.ssid, scanResponse.ssidLen);
  result.bssid = ByteSpan(scanResponse.bssid, sizeof(scanResponse.bssid));
  result.channel = scanResponse.channel;
- result.wiFiBand = ToClusterObjectEnum(scanResponse.wiFiBand);
+ result.wiFiBand = scanResponse.wiFiBand;
  result.rssi = scanResponse.rssi;
  SuccessOrExit(err = DataModel::Encode(*writer, TLV::AnonymousTag(), result));
  }

src/app/zap-templates/templates/app/cluster-enums.zapt

@@ -1,8 +1,10 @@
 {{> header}}
 
+#pragma once
+
 #include <stdint.h>
 
-#pragma once
+#include <app-common/zap-generated/enums.h>
 
 namespace chip {
 namespace app {

src/include/platform/NetworkCommissioning.h

@@ -29,6 +29,8 @@
 #include <lib/support/ThreadOperationalDataset.h>
 #include <platform/internal/DeviceNetworkInfo.h>
 
+#include <app-common/zap-generated/cluster-enums.h>
+
 #include <limits>
 
 namespace chip {
@@ -73,36 +75,6 @@ class Iterator
  Iterator() = default;
 };
 
-/**
- * The content should match the one in zap_generated/cluster-objects.h.
- * Matching is validated by cluster code.
- */
-enum class Status : uint8_t
-{
- kSuccess = 0x00,
- kOutOfRange = 0x01,
- kBoundsExceeded = 0x02,
- kNetworkIDNotFound = 0x03,
- kDuplicateNetworkID = 0x04,
- kNetworkNotFound = 0x05,
- kRegulatoryError = 0x06,
- kAuthFailure = 0x07,
- kUnsupportedSecurity = 0x08,
- kOtherConnectionFailure = 0x09,
- kIPV6Failed = 0x0A,
- kIPBindFailed = 0x0B,
- kUnknownError = 0x0C,
-};
-
-enum class WiFiBand : uint8_t
-{
- k2g4 = 0x00,
- k3g65 = 0x01,
- k5g = 0x02,
- k6g = 0x03,
- k60g = 0x04,
-};
-
 // The following structs follows the generated cluster object structs.
 struct Network
 {
@@ -114,24 +86,15 @@ struct Network
 static_assert(sizeof(Network::networkID) <= std::numeric_limits<decltype(Network::networkIDLen)>::max(),
  "Max length of networkID ssid exceeds the limit of networkIDLen field");
 
-enum class WiFiSecurity : uint8_t
-{
- kUnencrypted = 0x1,
- kWepPersonal = 0x2,
- kWpaPersonal = 0x4,
- kWpa2Personal = 0x8,
- kWpa3Personal = 0x10,
-};
-
 struct WiFiScanResponse
 {
 public:
- chip::BitFlags<WiFiSecurity> security;
+ chip::BitFlags<app::Clusters::NetworkCommissioning::WiFiSecurity> security;
  uint8_t ssid[DeviceLayer::Internal::kMaxWiFiSSIDLength];
  uint8_t ssidLen;
  uint8_t bssid[6];
  uint16_t channel;
- WiFiBand wiFiBand;
+ app::Clusters::NetworkCommissioning::WiFiBand wiFiBand;
  int8_t rssi;
 };
 
@@ -157,6 +120,9 @@ static_assert(sizeof(ThreadScanResponse::networkName) <= std::numeric_limits<dec
 using NetworkIterator = Iterator<Network>;
 using WiFiScanResponseIterator = Iterator<WiFiScanResponse>;
 using ThreadScanResponseIterator = Iterator<ThreadScanResponse>;
+using Status = app::Clusters::NetworkCommissioning::NetworkCommissioningStatus;
+using WiFiBand = app::Clusters::NetworkCommissioning::WiFiBand;
+using WiFiSecurity = app::Clusters::NetworkCommissioning::WiFiSecurity;
 
 // BaseDriver and WirelessDriver are the common interfaces for a network driver, platform drivers should not implement this
 // directly, instead, users are expected to implement WiFiDriver, ThreadDriver and EthernetDriver.
@@ -304,7 +270,8 @@ class WiFiDriver : public Internal::WirelessDriver
  * @brief Initializes a WiFi network scan. callback->OnFinished must be called, on both success and error. Callback can
  * be called inside ScanNetworks.
  *
- * @param ssid The interested SSID, the scanning MAY be restricted to to the given SSID.
+ * @param ssid The interested SSID, the scanning SHALL be restricted to the given SSID if the ssid is not empty (i.e.
+ * ssid.empty() is false).
  * @param callback Callback that will be invoked upon finishing the scan
  */
  virtual void ScanNetworks(ByteSpan ssid, ScanCallback * callback) = 0;

src/platform/Ameba/NetworkCommissioningWiFiDriver.cpp

@@ -185,7 +185,7 @@ void AmebaWiFiDriver::ConnectNetwork(ByteSpan networkId, ConnectCallback * callb
 
 CHIP_ERROR AmebaWiFiDriver::StartScanWiFiNetworks(ByteSpan ssid)
 {
- if (ssid.data()) // ssid is given, only scan this network
+ if (!ssid.empty()) // ssid is given, only scan this network
  {
  matter_scan_networks_with_ssid(ssid.data(), ssid.size());
  }

src/platform/EFR32/NetworkCommissioningWiFiDriver.cpp

@@ -211,7 +211,7 @@ bool SlWiFiDriver::StartScanWiFiNetworks(ByteSpan ssid)
 {
  bool scanStarted = false;
  ChipLogProgress(DeviceLayer, "Start Scan WiFi Networks");
- if (ssid.size()) // ssid is given, only scan this network
+ if (!ssid.empty()) // ssid is given, only scan this network
  {
  char cSsid[DeviceLayer::Internal::kMaxWiFiSSIDLength] = {};
  memcpy(cSsid, ssid.data(), ssid.size());