Fix handling of short discriminator in QRCodeSetupPayloadGenerator (p…

…roject-chip#33250) * Fix handling of short discriminator in QRCodeSetupPayloadGenerator If a SetupPayload contains a short discriminator then - isValidQRCodePayload() should return false - trying to generate a QR Code should return INVALID_ARGUMENT - generating with SetAllowInvalidPayload(true) should work (not die) * SetupPayload tweaks Make IsCommonTag, IsVendorTag, and getOptionalVendorData(tag, &info) public. The first two are just encoding spec rules that are useful for clients, and the latter allows clients to read vendor data by tag instead of having to read the whole list. Also use default values instead of explicit constructors for OptionalQRCodeInfo and fix up some doc comments to correctly reference the vendor tag range. * Address review comments Elaborate on the use case for AllowInvalidPayload in the comments, and encode a missing long discriminator as 0, to avoid a client encoding invalid payloads from relying on round-tripping a short discriminator through a QR code. * Handle rendezvousInformation and discriminator the same way * Address review comment: use ValueOr
cecille · May 3, 2024 · fcfc9bc · fcfc9bc
1 parent ddc06d4
commit fcfc9bc
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 45 deletions.
diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.cpp b/src/setup_payload/QRCodeSetupPayloadGenerator.cpp
@@ -162,6 +162,11 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi
     size_t totalPayloadSizeInBits = kTotalPayloadDataSizeInBits + (tlvDataLengthInBytes * 8);
     VerifyOrReturnError(bits.size() * 8 >= totalPayloadSizeInBits, CHIP_ERROR_BUFFER_TOO_SMALL);
 
+    // isValidQRCodePayload() has already performed all relevant checks (including that we have a
+    // long discriminator and rendevouz information). But if AllowInvalidPayload is set these
+    // requirements might be violated; in that case simply encode 0 for the relevant fields.
+    // Encoding an invalid (or partially valid) payload is useful for clients that need to be able
+    // to serialize and deserialize partially populated or invalid payloads.
     ReturnErrorOnFailure(
         populateBits(bits.data(), offset, payload.version, kVersionFieldLengthInBits, kTotalPayloadDataSizeInBits));
     ReturnErrorOnFailure(
@@ -170,10 +175,11 @@ static CHIP_ERROR generateBitSet(PayloadContents & payload, MutableByteSpan & bi
         populateBits(bits.data(), offset, payload.productID, kProductIDFieldLengthInBits, kTotalPayloadDataSizeInBits));
     ReturnErrorOnFailure(populateBits(bits.data(), offset, static_cast<uint64_t>(payload.commissioningFlow),
                                       kCommissioningFlowFieldLengthInBits, kTotalPayloadDataSizeInBits));
-    VerifyOrReturnError(payload.rendezvousInformation.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
-    ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.rendezvousInformation.Value().Raw(),
+    ReturnErrorOnFailure(populateBits(bits.data(), offset,
+                                      payload.rendezvousInformation.ValueOr(RendezvousInformationFlag::kNone).Raw(),
                                       kRendezvousInfoFieldLengthInBits, kTotalPayloadDataSizeInBits));
-    ReturnErrorOnFailure(populateBits(bits.data(), offset, payload.discriminator.GetLongValue(),
+    auto const & pd = payload.discriminator;
+    ReturnErrorOnFailure(populateBits(bits.data(), offset, (!pd.IsShortDiscriminator() ? pd.GetLongValue() : 0),
                                       kPayloadDiscriminatorFieldLengthInBits, kTotalPayloadDataSizeInBits));
     ReturnErrorOnFailure(
         populateBits(bits.data(), offset, payload.setUpPINCode, kSetupPINCodeFieldLengthInBits, kTotalPayloadDataSizeInBits));

diff --git a/src/setup_payload/SetupPayload.cpp b/src/setup_payload/SetupPayload.cpp
@@ -33,18 +33,6 @@
 
 namespace chip {
 
-// Spec 5.1.4.2 CHIPCommon tag numbers are in the range [0x00, 0x7F]
-bool SetupPayload::IsCommonTag(uint8_t tag)
-{
-    return tag < 0x80;
-}
-
-// Spec 5.1.4.1 Manufacture-specific tag numbers are in the range [0x80, 0xFF]
-bool SetupPayload::IsVendorTag(uint8_t tag)
-{
-    return !IsCommonTag(tag);
-}
-
 // Check the Setup Payload for validity
 //
 // `vendor_id` and `product_id` are allowed all of uint16_t
@@ -79,7 +67,11 @@ bool PayloadContents::isValidQRCodePayload() const
         return false;
     }
 
-    // Discriminator validity is enforced by the SetupDiscriminator class.
+    // General discriminator validity is enforced by the SetupDiscriminator class, but it can't be short for QR a code.
+    if (discriminator.IsShortDiscriminator())
+    {
+        return false;
+    }
 
     if (setUpPINCode >= 1 << kSetupPINCodeFieldLengthInBits)
     {

diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h
@@ -153,29 +153,19 @@ enum optionalQRCodeInfoType
  */
 struct OptionalQRCodeInfo
 {
-    OptionalQRCodeInfo() { int32 = 0; }
-
     /*@{*/
     uint8_t tag;                      /**< the tag number of the optional info */
     enum optionalQRCodeInfoType type; /**< the type (String or Int) of the optional info */
     std::string data;                 /**< the string value if type is optionalQRCodeInfoTypeString, otherwise should not be set */
-    int32_t int32;                    /**< the integer value if type is optionalQRCodeInfoTypeInt, otherwise should not be set */
+    int32_t int32 = 0;                /**< the integer value if type is optionalQRCodeInfoTypeInt32, otherwise should not be set */
     /*@}*/
 };
 
 struct OptionalQRCodeInfoExtension : OptionalQRCodeInfo
 {
-    OptionalQRCodeInfoExtension()
-    {
-        int32  = 0;
-        int64  = 0;
-        uint32 = 0;
-        uint64 = 0;
-    }
-
-    int64_t int64;
-    uint64_t uint32;
-    uint64_t uint64;
+    int64_t int64   = 0; /**< the integer value if type is optionalQRCodeInfoTypeInt64, otherwise should not be set */
+    uint64_t uint32 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt32, otherwise should not be set */
+    uint64_t uint64 = 0; /**< the integer value if type is optionalQRCodeInfoTypeUInt64, otherwise should not be set */
 };
 
 class SetupPayload : public PayloadContents
@@ -193,17 +183,25 @@ class SetupPayload : public PayloadContents
     CHIP_ERROR addOptionalVendorData(uint8_t tag, std::string data);
 
     /** @brief A function to add an optional vendor data
-     * @param tag 7 bit [0-127] tag number
+     * @param tag tag number in the [0x80-0xFF] range
      * @param data Integer representation of data to add
      * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
      **/
     CHIP_ERROR addOptionalVendorData(uint8_t tag, int32_t data);
 
     /** @brief A function to remove an optional vendor data
-     * @param tag 7 bit [0-127] tag number
+     * @param tag tag number in the [0x80-0xFF] range
      * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise
      **/
     CHIP_ERROR removeOptionalVendorData(uint8_t tag);
+
+    /** @brief A function to retrieve an optional QR Code info vendor object
+     * @param tag tag number in the [0x80-0xFF] range
+     * @param info retrieved OptionalQRCodeInfo object
+     * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise
+     **/
+    CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const;
+
     /**
      * @brief A function to retrieve the vector of OptionalQRCodeInfo infos
      * @return Returns a vector of optionalQRCodeInfos
@@ -235,21 +233,21 @@ class SetupPayload : public PayloadContents
 
     bool operator==(const SetupPayload & input) const;
 
-private:
-    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);
+    static bool IsCommonTag(uint8_t tag) { return tag < 0x80; }
 
     /** @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);
+    static bool IsVendorTag(uint8_t tag) { return !IsCommonTag(tag); }
+
+private:
+    std::map<uint8_t, OptionalQRCodeInfo> optionalVendorData;
+    std::map<uint8_t, OptionalQRCodeInfoExtension> optionalExtensionData;
 
     /** @brief A function to add an optional QR Code info vendor object
      * @param info Optional QR code info object to add
@@ -269,13 +267,6 @@ class SetupPayload : public PayloadContents
      **/
     std::vector<OptionalQRCodeInfoExtension> getAllOptionalExtensionData() const;
 
-    /** @brief A function to retrieve an optional QR Code info vendor object
-     * @param tag 7 bit [0-127] tag number
-     * @param info retrieved OptionalQRCodeInfo object
-     * @return Returns a CHIP_ERROR_KEY_NOT_FOUND on error, CHIP_NO_ERROR otherwise
-     **/
-    CHIP_ERROR getOptionalVendorData(uint8_t tag, OptionalQRCodeInfo & info) const;
-
     /** @brief A function to retrieve an optional QR Code info extended object
      * @param tag 8 bit [128-255] tag number
      * @param info retrieved OptionalQRCodeInfoExtension object

diff --git a/src/setup_payload/tests/TestQRCode.cpp b/src/setup_payload/tests/TestQRCode.cpp
@@ -381,6 +381,44 @@ TEST(TestQRCode, TestQRCodeToPayloadGeneration)
     EXPECT_EQ(result, true);
 }
 
+TEST(TestQRCode, TestGenerateWithShortDiscriminatorInvalid)
+{
+    SetupPayload payload = GetDefaultPayload();
+    EXPECT_TRUE(payload.isValidQRCodePayload());
+
+    // A short discriminator isn't valid for a QR Code
+    payload.discriminator.SetShortValue(1);
+    EXPECT_FALSE(payload.isValidQRCodePayload());
+
+    // QRCodeSetupPayloadGenerator should therefore return an error
+    string base38Rep;
+    QRCodeSetupPayloadGenerator generator(payload);
+    EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT);
+
+    // If we allow invalid payloads we should be able to encode
+    generator.SetAllowInvalidPayload(true);
+    EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR);
+}
+
+TEST(TestQRCode, TestGenerateWithoutRendezvousInformation)
+{
+    SetupPayload payload = GetDefaultPayload();
+    EXPECT_TRUE(payload.isValidQRCodePayload());
+
+    // Rendezvouz Information is required for a QR code
+    payload.rendezvousInformation.ClearValue();
+    EXPECT_FALSE(payload.isValidQRCodePayload());
+
+    // QRCodeSetupPayloadGenerator should therefore return an error
+    string base38Rep;
+    QRCodeSetupPayloadGenerator generator(payload);
+    EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_ERROR_INVALID_ARGUMENT);
+
+    // If we allow invalid payloads we should be able to encode
+    generator.SetAllowInvalidPayload(true);
+    EXPECT_EQ(generator.payloadBase38Representation(base38Rep), CHIP_NO_ERROR);
+}
+
 TEST(TestQRCode, TestExtractPayload)
 {
     EXPECT_EQ(QRCodeSetupPayloadParser::ExtractPayload(string("MT:ABC")), string("ABC"));