Fix TLV tags in the Onboarding payload and update unit tests (#17122)

project-chip · Oct 17, 2023 · 6710868 · 6710868
1 parent 2ce117d
commit 6710868
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 30 deletions.
diff --git a/src/darwin/Framework/CHIPTests/CHIPSetupPayloadParserTests.m b/src/darwin/Framework/CHIPTests/CHIPSetupPayloadParserTests.m
@@ -128,10 +128,10 @@ - (void)testOnboardingPayloadParser_QRCode_WrongType
 - (void)testOnboardingPayloadParser_NFC_NoError
 {
     NSError * error;
-    CHIPSetupPayload * payload =
-        [CHIPOnboardingPayloadParser setupPayloadForOnboardingPayload:@"MT:R5L90MP500K64J0A33P0GQ670.QT52B.E23O6DE0Y3U10O0"
-                                                               ofType:CHIPOnboardingPayloadTypeNFC
-                                                                error:&error];
+    CHIPSetupPayload * payload = [CHIPOnboardingPayloadParser
+        setupPayloadForOnboardingPayload:@"MT:R5L90MP500K64J0A33P0SET70.QT52B.E23-WZE0WISA0DK5N1K8SQ1RYCU1O0"
+                                  ofType:CHIPOnboardingPayloadTypeNFC
+                                   error:&error];
 
     XCTAssertNotNil(payload);
     XCTAssertNil(error);
@@ -148,10 +148,10 @@ - (void)testOnboardingPayloadParser_NFC_NoError
 - (void)testOnboardingPayloadParser_NFC_WrongType
 {
     NSError * error;
-    CHIPSetupPayload * payload =
-        [CHIPOnboardingPayloadParser setupPayloadForOnboardingPayload:@"MT:R5L90MP500K64J0A33P0GQ670.QT52B.E23O6DE0Y3U10O0"
-                                                               ofType:CHIPOnboardingPayloadTypeManualCode
-                                                                error:&error];
+    CHIPSetupPayload * payload = [CHIPOnboardingPayloadParser
+        setupPayloadForOnboardingPayload:@"MT:R5L90MP500K64J0A33P0SET70.QT52B.E23-WZE0WISA0DK5N1K8SQ1RYCU1O0"
+                                  ofType:CHIPOnboardingPayloadTypeManualCode
+                                   error:&error];
 
     XCTAssertNil(payload);
     XCTAssertEqual(error.code, CHIPErrorCodeIntegrityCheckFailed);
@@ -219,8 +219,8 @@ - (void)testQRCodeParser
 - (void)testQRCodeParserWithOptionalData
 {
     NSError * error;
-    CHIPQRCodeSetupPayloadParser * parser =
-        [[CHIPQRCodeSetupPayloadParser alloc] initWithBase38Representation:@"MT:R5L90MP500K64J0A33P0GQ670.QT52B.E23O6DE0Y3U10O0"];
+    CHIPQRCodeSetupPayloadParser * parser = [[CHIPQRCodeSetupPayloadParser alloc]
+        initWithBase38Representation:@"MT:R5L90MP500K64J0A33P0SET70.QT52B.E23-WZE0WISA0DK5N1K8SQ1RYCU1O0"];
     CHIPSetupPayload * payload = [parser populatePayload:&error];
 
     XCTAssertNotNil(payload);
@@ -233,16 +233,16 @@ - (void)testQRCodeParserWithOptionalData
     XCTAssertEqual(payload.productID.unsignedIntegerValue, 1);
     XCTAssertEqual(payload.commissioningFlow, kCommissioningFlowStandard);
     XCTAssertEqual(payload.rendezvousInformation, kRendezvousInformationSoftAP);
-    XCTAssertTrue([payload.serialNumber isEqualToString:@"1"]);
+    XCTAssertTrue([payload.serialNumber isEqualToString:@"123456789"]);
 
     NSArray<CHIPOptionalQRCodeInfo *> * vendorOptionalInfo = [payload getAllOptionalVendorData:&error];
     XCTAssertNil(error);
     XCTAssertEqual([vendorOptionalInfo count], 2);
     for (CHIPOptionalQRCodeInfo * info in vendorOptionalInfo) {
-        if (info.tag.intValue == 2) {
+        if (info.tag.intValue == 130) {
             XCTAssertEqual(info.infoType.intValue, kOptionalQRCodeInfoTypeString);
             XCTAssertTrue([info.stringValue isEqualToString:@"myData"]);
-        } else if (info.tag.intValue == 3) {
+        } else if (info.tag.intValue == 131) {
             XCTAssertEqual(info.infoType.intValue, kOptionalQRCodeInfoTypeInt32);
             XCTAssertEqual(info.integerValue.intValue, 12);
         }

diff --git a/src/setup_payload/QRCodeSetupPayloadParser.cpp b/src/setup_payload/QRCodeSetupPayloadParser.cpp
@@ -208,7 +208,7 @@ CHIP_ERROR QRCodeSetupPayloadParser::retrieveOptionalInfos(SetupPayload & outPay
             elemType = outPayload.getNumericTypeFor(tagNumber);
         }
 
-        if (IsCHIPTag(tagNumber))
+        if (SetupPayload::IsCommonTag(tagNumber))
         {
             OptionalQRCodeInfoExtension info;
             info.tag = tagNumber;

diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp
@@ -32,14 +32,16 @@
 
 namespace chip {
 
-bool IsCHIPTag(uint8_t tag)
+// Spec 5.1.4.2 CHIPCommon tag numbers are in the range [0x00, 0x7F]
+bool SetupPayload::IsCommonTag(uint8_t tag)
 {
-    return tag >= (1 << kRawVendorTagLengthInBits);
+    return tag < 0x80;
 }
 
-bool IsVendorTag(uint8_t tag)
+// Spec 5.1.4.1 Manufacture-specific tag numbers are in the range [0x80, 0xFF]
+bool SetupPayload::IsVendorTag(uint8_t tag)
 {
-    return tag < (1 << kRawVendorTagLengthInBits);
+    return !IsCommonTag(tag);
 }
 
 // Check the Setup Payload for validity
@@ -211,7 +213,7 @@ CHIP_ERROR SetupPayload::addOptionalVendorData(const OptionalQRCodeInfo & info)
 
 CHIP_ERROR SetupPayload::addOptionalExtensionData(const OptionalQRCodeInfoExtension & info)
 {
-    VerifyOrReturnError(IsCHIPTag(info.tag), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsCommonTag(info.tag), CHIP_ERROR_INVALID_ARGUMENT);
     optionalExtensionData[info.tag] = info;
 
     return CHIP_NO_ERROR;

diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h
@@ -65,7 +65,8 @@ const int kManualSetupCodeChunk3CharLength = 4;
 const int kManualSetupVendorIdCharLength   = 5;
 const int kManualSetupProductIdCharLength  = 5;
 
-const uint8_t kSerialNumberTag = 128;
+// Spec 5.1.4.2 CHIP-Common Reserved Tag (kTag_SerialNumber)
+const uint8_t kSerialNumberTag = 0;
 
 // clang-format off
 const int kTotalPayloadDataSizeInBits =
@@ -160,9 +161,6 @@ struct OptionalQRCodeInfoExtension : OptionalQRCodeInfo
     uint64_t uint64;
 };
 
-bool IsCHIPTag(uint8_t tag);
-bool IsVendorTag(uint8_t tag);
-
 class SetupPayload : public PayloadContents
 {
 
@@ -224,6 +222,18 @@ class SetupPayload : public PayloadContents
     std::map<uint8_t, OptionalQRCodeInfo> optionalVendorData;
     std::map<uint8_t, OptionalQRCodeInfoExtension> optionalExtensionData;
 
+    /** @brief Checks if the tag is CHIP Common type
+     * @param tag Tag to be checked
+     * @return Returns True if the tag is of Common type
+     **/
+    static bool IsCommonTag(uint8_t tag);
+
+    /** @brief Checks if the tag is vendor-specific
+     * @param tag Tag to be checked
+     * @return Returns True if the tag is Vendor-specific
+     **/
+    static bool IsVendorTag(uint8_t tag);
+
     /** @brief A function to add an optional QR Code info vendor object
      * @param info Optional QR code info object to add
      * @return Returns a CHIP_ERROR_INVALID_ARGUMENT on error, CHIP_NO_ERROR otherwise

diff --git a/src/setup_payload/tests/TestHelpers.h b/src/setup_payload/tests/TestHelpers.h
@@ -30,10 +30,10 @@ namespace chip {
 const uint16_t kSmallBufferSizeInBytes   = 1;
 const uint16_t kDefaultBufferSizeInBytes = 512;
 
-const uint8_t kOptionalDefaultStringTag      = 2;
+const uint8_t kOptionalDefaultStringTag      = 0x82; // Vendor "test" tag
 constexpr char kOptionalDefaultStringValue[] = "myData";
 
-const uint8_t kOptionalDefaultIntTag    = 3;
+const uint8_t kOptionalDefaultIntTag    = 0x83; // Vendor "test" tag
 const uint32_t kOptionalDefaultIntValue = 12;
 
 constexpr char kSerialNumberDefaultStringValue[] = "123456789";

diff --git a/src/setup_payload/tests/TestQRCodeTLV.cpp b/src/setup_payload/tests/TestQRCodeTLV.cpp
@@ -105,19 +105,19 @@ void TestOptionalTagValues(nlTestSuite * inSuite, void * inContext)
     SetupPayload payload = GetDefaultPayload();
     CHIP_ERROR err;
 
-    err = payload.addOptionalVendorData(kOptionalDefaultStringTag, kOptionalDefaultStringValue);
+    err = payload.addOptionalVendorData(kOptionalDefaultStringTag, kOptionalDefaultStringValue); // Vendor specific tag
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
-    err = payload.addOptionalVendorData(0, kOptionalDefaultStringValue);
+    err = payload.addOptionalVendorData(0x80, kOptionalDefaultStringValue); // Vendor specific tag
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
-    err = payload.addOptionalVendorData(127, kOptionalDefaultStringValue);
+    err = payload.addOptionalVendorData(0x82, kOptionalDefaultStringValue); // Vendor specific tag
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
-    err = payload.addOptionalVendorData(128, kOptionalDefaultStringValue);
+    err = payload.addOptionalVendorData(127, kOptionalDefaultStringValue); // Common tag
     NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT);
 
-    err = payload.addOptionalVendorData(255, kOptionalDefaultStringValue);
+    err = payload.addOptionalVendorData(0, kOptionalDefaultStringValue); // Common tag
     NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_ARGUMENT);
 }