From: Thomas Markwalder Date: Tue, 3 Mar 2020 19:55:09 +0000 (-0500) Subject: [#608] Addressed review comments X-Git-Tag: Kea-1.7.6~88 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b219c2dd9fb591a1179d9c09ba93c7576443fcae;p=thirdparty%2Fkea.git [#608] Addressed review comments src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc TEST_F(CSVLeaseFile4Test, embeddedEscapes) - new test src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc TEST_F(CSVLeaseFile6Test, embeddedEscapes) - new test src/lib/util/csv_file.cc CSVRow::escapeCharacters() - now automatically also escapes first char of escape_tag to preserve input that happens to match escape tag src/lib/util/tests/csv_file_unittest.cc TEST(CSVRowTest, escapeUnescape) - added embedded escape tests plus misc clean up --- diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc index 57fef44090..cbb423da9c 100644 --- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc +++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc @@ -599,6 +599,48 @@ TEST_F(CSVLeaseFile4Test, embeddedCommas) { EXPECT_EQ(context_str, lease->getContext()->str()); } +// Verifies that it is possible to write and read a lease with +// escape tags and sequences in hostname and user context. +TEST_F(CSVLeaseFile4Test, embeddedEscapes) { + CSVLeaseFile4 lf(filename_); + ASSERT_NO_THROW(lf.recreate()); + ASSERT_TRUE(io_.exists()); + + std::string hostname("hostxampleˌom"); + std::string context_str("{ \"ºr\": true, \"foo\": false, \"x\": \"fac,tor\" }"); + + // Create a lease with commas in the hostname. + Lease4Ptr lease(new Lease4(IOAddress("192.0.3.2"), + hwaddr0_, + NULL, 0, + 0xFFFFFFFF, time(0), + 8, true, true, + hostname)); + + // Add the user context with commas. + lease->setContext(Element::fromJSON(context_str)); + + // Write this lease out to the lease file. + ASSERT_NO_THROW(lf.append(*lease)); + + // Close the lease file. + lf.close(); + + Lease4Ptr lease_read; + + // Re-open the file for reading. + ASSERT_NO_THROW(lf.open()); + + // Read the lease and make sure it is successful. + EXPECT_TRUE(lf.next(lease_read)); + ASSERT_TRUE(lease_read); + + // Expect the hostname and user context to retain the commas + // they started with. + EXPECT_EQ(hostname, lease->hostname_); + EXPECT_EQ(context_str, lease->getContext()->str()); +} + /// @todo Currently we don't check invalid lease attributes, such as invalid /// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6 /// should be extended with the function that validates lease attributes. Once diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc index 66220ba9b9..5eb5be7de2 100644 --- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc +++ b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc @@ -658,6 +658,47 @@ TEST_F(CSVLeaseFile6Test, embeddedCommas) { EXPECT_EQ(context_str, lease->getContext()->str()); } +// Verifies that it is possible to write and read a lease with +// escape tags and sequences in hostname and user context. +TEST_F(CSVLeaseFile6Test, embeddedEscapes) { + CSVLeaseFile6 lf(filename_); + ASSERT_NO_THROW(lf.recreate()); + ASSERT_TRUE(io_.exists()); + + std::string hostname("hostxampleˌom"); + std::string context_str("{ \"ºr\": true, \"foo\": false, \"x\": \"fac,tor\" }"); + + // Create a lease with commas in the hostname. + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"), + makeDUID(DUID0, sizeof(DUID0)), + 7, 100, 0xFFFFFFFF, 8, true, true, + hostname)); + + // Add the user context with commas. + lease->setContext(Element::fromJSON(context_str)); + + // Write this lease out to the lease file. + ASSERT_NO_THROW(lf.append(*lease)); + + // Close the lease file. + lf.close(); + + Lease6Ptr lease_read; + + // Re-open the file for reading. + ASSERT_NO_THROW(lf.open()); + + // Read the lease and make sure it is successful. + EXPECT_TRUE(lf.next(lease_read)); + ASSERT_TRUE(lease_read); + + // Expect the hostname and user context to retain the commas + // they started with. + EXPECT_EQ(hostname, lease->hostname_); + EXPECT_EQ(context_str, lease->getContext()->str()); +} + + /// @todo Currently we don't check invalid lease attributes, such as invalid diff --git a/src/lib/util/csv_file.cc b/src/lib/util/csv_file.cc index b6fd591316..8f939537e0 100644 --- a/src/lib/util/csv_file.cc +++ b/src/lib/util/csv_file.cc @@ -430,27 +430,32 @@ CSVRow::escapeCharacters(const std::string& orig_str, const std::string& charact size_t char_pos = 0; size_t prev_pos = 0; - // Check for a first occurance. If none, just return a + // We add the first character of the escape tag to the list of + // characters to escape. This ensures input which happens to + // be valid esacpe sequences will be escaped. + std::string escape_chars(characters + escape_tag[0]); + + // Check for a first occurence. If none, just return a // copy of the original. - char_pos = orig_str.find_first_of(characters, prev_pos); + char_pos = orig_str.find_first_of(escape_chars, prev_pos); if (char_pos == std::string::npos) { return(orig_str); } std::stringstream ss; while (char_pos < orig_str.size()) { - // Copy everything upto the charcater to escape. + // Copy everything upto the character to escape. ss << orig_str.substr(prev_pos, char_pos - prev_pos); // Copy the escape tag followed by the hex digits of the character. ss << escape_tag << std::hex << std::setw(2) - << (uint16_t)(orig_str[char_pos]); + << static_cast(orig_str[char_pos]); ++char_pos; prev_pos = char_pos; // Find the next character to escape. - char_pos = orig_str.find_first_of(characters, prev_pos); + char_pos = orig_str.find_first_of(escape_chars, prev_pos); // If no more, copy the remainder of the string. if (char_pos == std::string::npos) { @@ -492,9 +497,9 @@ CSVRow::unescapeCharacters(const std::string& escaped_str) { uint8_t digit = escaped_str[dig_pos]; if (digit >= 'a' && digit <= 'f') { - digit = (digit - 'a' + 10); + digit = digit - 'a' + 10; } else if (digit >= 'A' && digit <= 'F') { - digit = (digit - 'A' + 10); + digit = digit - 'A' + 10; } else if (digit >= '0' && digit <= '9') { digit -= '0'; } else { @@ -503,7 +508,7 @@ CSVRow::unescapeCharacters(const std::string& escaped_str) { } if (i == 0) { - escaped_char = (digit << 4); + escaped_char = digit << 4; } else { escaped_char |= digit; } diff --git a/src/lib/util/csv_file.h b/src/lib/util/csv_file.h index 1ffdc369f9..9bf64f13d0 100644 --- a/src/lib/util/csv_file.h +++ b/src/lib/util/csv_file.h @@ -113,7 +113,7 @@ public: /// characters. /// /// Returns a copy of the internal container value at the given index - /// which has had all escaped characters replaced with their unesacped + /// which has had all escaped characters replaced with their unescaped /// values. Escaped characters embedded using the following format: /// /// This function fetches the value at the given index and passes it @@ -199,7 +199,7 @@ public: writeAt(at, value.c_str()); } - /// @brief Replaces the value at specified index with a value that has + /// @brief Replaces the value at the specified index with a value that has /// had special characters escaped /// /// This function first calls @c CSVRow::esacpeCharacters to replace @@ -274,7 +274,7 @@ public: /// escaping. /// @param characters list of characters which require escaping. /// - /// The escaped characters will use the followin format: + /// The escaped characters will use the following format: /// /// &#x{xx} /// diff --git a/src/lib/util/tests/csv_file_unittest.cc b/src/lib/util/tests/csv_file_unittest.cc index e5450a1230..957bf681e9 100644 --- a/src/lib/util/tests/csv_file_unittest.cc +++ b/src/lib/util/tests/csv_file_unittest.cc @@ -28,11 +28,19 @@ TEST(CSVRowTest, escapeUnescape) { std::string unescaped = CSVRow::unescapeCharacters(escaped); EXPECT_EQ (orig, unescaped); - // Make sure that an incident occurance of the escape tag - // characters is left intact. + // Make sure that an incident occurrence of just the escape tag + // is left intact. orig = ("noscape"); + escaped = CSVRow::escapeCharacters(orig, ","); unescaped = CSVRow::unescapeCharacters(orig); EXPECT_EQ (orig, unescaped); + + // Make sure that an incidental occurrence of a valid + // escape tag sequence left intact. + orig = ("noˎscape"); + escaped = CSVRow::escapeCharacters(orig, ","); + unescaped = CSVRow::unescapeCharacters(escaped); + EXPECT_EQ (orig, unescaped); } // This test checks that the single data row is parsed.