From 32466ab3d4688e66c8c2f9fd24d4a98fcc871ff6 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Fri, 27 Jul 2018 13:52:14 +0200 Subject: [PATCH] [5680] Changes after review --- doc/examples/kea4/with-ddns.json | 4 +- doc/examples/kea6/with-ddns.json | 4 +- src/bin/dhcp4/tests/fqdn_unittest.cc | 30 ++++++------- src/lib/dhcpsrv/tests/d2_client_unittest.cc | 48 ++++++++++----------- src/lib/dns/tests/labelsequence_unittest.cc | 6 +++ src/lib/util/strutil.h | 8 ++-- src/lib/util/tests/strutil_unittest.cc | 6 +-- 7 files changed, 58 insertions(+), 48 deletions(-) diff --git a/doc/examples/kea4/with-ddns.json b/doc/examples/kea4/with-ddns.json index 57544f6685..e192c6e446 100644 --- a/doc/examples/kea4/with-ddns.json +++ b/doc/examples/kea4/with-ddns.json @@ -54,7 +54,9 @@ "override-client-update" : true, "replace-client-name" : "when-present", "generated-prefix" : "test.prefix", - "qualifying-suffix" : "test.suffix." + "qualifying-suffix" : "test.suffix.", + "hostname-char-set": "[^A-Za-z0-9.-]", + "hostname-char-replacement": "x" } }, diff --git a/doc/examples/kea6/with-ddns.json b/doc/examples/kea6/with-ddns.json index 6792429b7d..2f5bbe308c 100644 --- a/doc/examples/kea6/with-ddns.json +++ b/doc/examples/kea6/with-ddns.json @@ -56,7 +56,9 @@ "override-client-update" : true, "replace-client-name" : "when-present", "generated-prefix" : "test.prefix", - "qualifying-suffix" : "test.suffix." + "qualifying-suffix" : "test.suffix.", + "hostname-char-set": "[^A-Za-z0-9.-]", + "hostname-char-replacement": "x" } }, diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 2c1c6857fb..f1f3f8e229 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -1742,22 +1742,22 @@ TEST_F(NameDhcpv4SrvTest, sanitizeHost) { Pkt4Ptr resp; OptionStringPtr hostname; - for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) { - SCOPED_TRACE((*scenario).description_); + for (auto scenario : scenarios) { + SCOPED_TRACE((scenario).description_); { - // Set the hostname option. - ASSERT_NO_THROW(client.includeHostname((*scenario).original_)); - - // Send the DHCPDISCOVER and make sure that the server responded. - ASSERT_NO_THROW(client.doDiscover()); - resp = client.getContext().response_; - ASSERT_TRUE(resp); - ASSERT_EQ(DHCPOFFER, static_cast(resp->getType())); - - // Make sure the response hostname is what we expect. - hostname = boost::dynamic_pointer_cast(resp->getOption(DHO_HOST_NAME)); - ASSERT_TRUE(hostname); - EXPECT_EQ((*scenario).sanitized_, hostname->getValue()); + // Set the hostname option. + ASSERT_NO_THROW(client.includeHostname((scenario).original_)); + + // Send the DHCPDISCOVER and make sure that the server responded. + ASSERT_NO_THROW(client.doDiscover()); + resp = client.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPOFFER, static_cast(resp->getType())); + + // Make sure the response hostname is what we expect. + hostname = boost::dynamic_pointer_cast(resp->getOption(DHO_HOST_NAME)); + ASSERT_TRUE(hostname); + EXPECT_EQ((scenario).sanitized_, hostname->getValue()); } } } diff --git a/src/lib/dhcpsrv/tests/d2_client_unittest.cc b/src/lib/dhcpsrv/tests/d2_client_unittest.cc index 13cf020ac0..bf3a3d826c 100644 --- a/src/lib/dhcpsrv/tests/d2_client_unittest.cc +++ b/src/lib/dhcpsrv/tests/d2_client_unittest.cc @@ -1239,40 +1239,40 @@ TEST(D2ClientMgr, sanitizeFqdnV6) { std::vector scenarios = { { - "full FQDN, name unchanged", - "One.123.example.com.", - Option6ClientFqdn::FULL, - "one.123.example.com." + "full FQDN, name unchanged", + "One.123.example.com.", + Option6ClientFqdn::FULL, + "one.123.example.com." }, { - "partial FQDN, name unchanged, but qualified", - "One.123", - Option6ClientFqdn::PARTIAL, - "one.123.suffix.com." + "partial FQDN, name unchanged, but qualified", + "One.123", + Option6ClientFqdn::PARTIAL, + "one.123.suffix.com." }, { - "full FQDN, scrubbed", - "O#n^e.123.ex&a*mple.com.", - Option6ClientFqdn::FULL, - "oxnxe.123.exxaxmple.com." + "full FQDN, scrubbed", + "O#n^e.123.ex&a*mple.com.", + Option6ClientFqdn::FULL, + "oxnxe.123.exxaxmple.com." }, { - "partial FQDN, scrubbed and qualified", - "One.1+2|3", - Option6ClientFqdn::PARTIAL, - "one.1x2x3.suffix.com." + "partial FQDN, scrubbed and qualified", + "One.1+2|3", + Option6ClientFqdn::PARTIAL, + "one.1x2x3.suffix.com." }, { - "full FQDN with characters that get escaped", - "O n e.123.exa(m)ple.com.", - Option6ClientFqdn::FULL, - "oxnxe.123.exaxmxple.com." + "full FQDN with characters that get escaped", + "O n e.123.exa(m)ple.com.", + Option6ClientFqdn::FULL, + "oxnxe.123.exaxmxple.com." }, { - "full FQDN with escape sequences", - "O\032n\032e.123.example.com.", - Option6ClientFqdn::FULL, - "oxnxe.123.example.com." + "full FQDN with escape sequences", + "O\032n\032e.123.example.com.", + Option6ClientFqdn::FULL, + "oxnxe.123.example.com." } }; diff --git a/src/lib/dns/tests/labelsequence_unittest.cc b/src/lib/dns/tests/labelsequence_unittest.cc index 3cd1330e09..6db3cefbf6 100644 --- a/src/lib/dns/tests/labelsequence_unittest.cc +++ b/src/lib/dns/tests/labelsequence_unittest.cc @@ -638,6 +638,12 @@ TEST_F(LabelSequenceTest, toRawText) { LabelSequence l(n); EXPECT_EQ("a bc.$exa(m)ple.@org", l.toRawText(true)); EXPECT_EQ("a bc.$exa(m)ple.@org.", l.toRawText(false)); + + // toRawText is not supposed to do any sanity checks. + // Let's try with a very weird name. + Name n2("xtra\tchars\n.in.name"); + LabelSequence l2(n2); + EXPECT_EQ("xtra\tchars\n.in.name.", l2.toRawText(false)); } // The following are test data used in the getHash test below. Normally diff --git a/src/lib/util/strutil.h b/src/lib/util/strutil.h index 1cdb833c18..cf2000e3f9 100644 --- a/src/lib/util/strutil.h +++ b/src/lib/util/strutil.h @@ -263,7 +263,7 @@ class StringSanitizerImpl; /// /// The implementation uses C++11 regex IF the environemnt supports it /// (tested in configure.ac). If not it falls back to C lib regcomp/regexec. -/// Older compilers, such as pre Gnu 4.9.0, provided only experimental +/// Older compilers, such as pre Gnu g++ 4.9.0, provided only experimental /// implementations of regex which are recognized as buggy. class StringSanitizer { public: @@ -271,7 +271,7 @@ public: /// Constructor /// /// Compiles the given character set into a regular expression, and - /// retains the given character replacement. Thereafter, the instance + /// retains the given character replacement. Thereafter, the instance /// may be used to scrub an arbitrary number of strings. /// /// @param char_set string containing a regular expression (POSIX @@ -291,8 +291,8 @@ public: /// Returns a scrubbed copy of a given string /// - /// Replaces all occurrances of characters described by the regular - /// expression with the character replacement . + /// Replaces all occurrences of characters described by the regular + /// expression with the character replacement. /// /// @param original the string to scrub /// @throw Unexpected if an error occurs during scrubbing diff --git a/src/lib/util/tests/strutil_unittest.cc b/src/lib/util/tests/strutil_unittest.cc index 893409621d..2672521fb4 100644 --- a/src/lib/util/tests/strutil_unittest.cc +++ b/src/lib/util/tests/strutil_unittest.cc @@ -463,7 +463,7 @@ TEST(StringUtilTest, decodeFormattedHexString) { isc::BadValue); } -/// @brief Fucntion used to test StringSantitizer +/// @brief Function used to test StringSantitizer /// @param original - string to sanitize /// @param char_set - regular expression string describing invalid /// characters @@ -507,7 +507,7 @@ TEST(StringUtilTest, stringSanitizer) { sanitizeStringTest("abc.123", "[b-c2]", "*", "a**.1*3"); - // Inverted list of valid chars should work: (b,c,2 are invalid) + // Inverted list of valid chars should work: (b,c,2 are valid) sanitizeStringTest("abc.123", "[^b-c2]", "*", "*bc**2*"); @@ -527,7 +527,7 @@ TEST(StringUtilTest, stringSanitizer) { sanitizeStringTest("%%A%%B%%C%%", "[^A-Za-z0-9_]", "x", "xxAxxBxxCxx"); - // Removing than one non-matching in a row should work. + // Removing more than one non-matching in a row should work. sanitizeStringTest("%%A%%B%%C%%", "[^A-Za-z0-9_]", "", "ABC"); -- 2.47.2