review fixes

brave · Aug 15, 2022 · 692c0ff · 692c0ff
1 parent a6ecf5c
commit 692c0ff
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 39 deletions.
diff --git a/browser/decentralized_dns/test/utils_unittest.cc b/browser/decentralized_dns/test/utils_unittest.cc
@@ -98,40 +98,50 @@ TEST_F(UtilsUnitTest, ResolveMethodMigration) {
 }
 
 TEST_F(UtilsUnitTest, Dot888Test) {
-  net::IPAddress ip_address;
-  // These tests were passing without .888 fix in `url_canon_ip.cc`.
-  EXPECT_TRUE(GURL("http://1.1.888").is_valid());
-  EXPECT_TRUE(ip_address.AssignFromIPLiteral("1.1.888"));
-  EXPECT_TRUE(GURL("http://123.888").is_valid());
-  EXPECT_TRUE(ip_address.AssignFromIPLiteral("123.888"));
-  EXPECT_TRUE(GURL("http://1.123.888").is_valid());
-  EXPECT_TRUE(ip_address.AssignFromIPLiteral("1.123.888"));
-  EXPECT_TRUE(GURL("http://.com").is_valid());
-  EXPECT_FALSE(ip_address.AssignFromIPLiteral(".com"));
-
-  // These tests start passing with .888 fix in `url_canon_ip.cc`.
-  const char* cases[] = {
-      "test.888",         // Non-ipv4 component case.
-      "test1.test2.888",  // Non-ipv4 component case.
-      "1.2.3.4.888",      // Too many components case.
-      "1.2.3.4.5.888",    // Too many components case.
-      "555.888",          // Non-last component overflow case.
-      "555.1.888",        // Non-last component overflow case.
-      "555.1.1.888",      // Non-last component overflow case.
-      "1.2.3.888",        // Last component residual bits.
-      "1.1.test.888",     // Some test
-      "888.888",          // Some test
-      "1.888.888",        // Some test
-      ".888",             // Same as .com
+  auto validate_string = [](const std::string& test_case, bool url_result,
+                            bool ip_result) {
+    SCOPED_TRACE(testing::Message() << test_case);
+    EXPECT_EQ(url_result, GURL("http://" + test_case).is_valid());
+    net::IPAddress ip_address;
+    EXPECT_EQ(ip_result, ip_address.AssignFromIPLiteral(test_case));
   };
 
-  for (auto* test_case : cases) {
-    SCOPED_TRACE(testing::Message() << test_case);
-    // Ok for being a host for url.
-    EXPECT_TRUE(GURL(std::string("http://") + test_case).is_valid());
-    // Still not ok to be an ipv4 address.
-    EXPECT_FALSE(ip_address.AssignFromIPLiteral(test_case));
-  }
+  // These tests were passing without .888 fix in `url_canon_ip.cc`.
+  validate_string("1.1.888", true, true);
+  validate_string("123.888", true, true);
+  validate_string("1.123.888", true, true);
+  validate_string(".com", true, false);
+
+  // Not breaking something near 888.
+  validate_string("1.2.889", true, true);
+  validate_string("1.2.3.889", false, false);
+  validate_string("test.889", false, false);
+
+  // Different form of dec 888.
+  validate_string("1.2.0x378", true, true);  // 888 as hex.
+  validate_string("test.0x378", false, false);
+  validate_string("1.2.01570", true, true);  // 888 as oct.
+  validate_string("test.01570", false, false);
+
+  // These tests start passing with .888 fix in `url_canon_ip.cc`. Ok to be an
+  // url, but still not valid ipv4 address.
+  // Non-ipv4 component case.
+  validate_string("test.888", true, false);
+  validate_string("test1.test2.888", true, false);
+  // Too many components case.
+  validate_string("1.2.3.4.888", true, false);
+  validate_string("1.2.3.4.5.888", true, false);
+  // Non-last component overflow case.
+  validate_string("555.888", true, false);
+  validate_string("555.1.888", true, false);
+  validate_string("555.1.1.888", true, false);
+  // Last component residual bits.
+  validate_string("1.2.3.888", true, false);
+  // Some tests.
+  validate_string("1.1.test.888", true, false);
+  validate_string("888.888", true, false);
+  validate_string("1.888.888", true, false);
+  validate_string(".888", true, false);  // Same as .com
 }
 
 }  // namespace decentralized_dns
diff --git a/patches/url-url_canon_ip.cc.patch b/patches/url-url_canon_ip.cc.patch
@@ -1,5 +1,5 @@
 diff --git a/url/url_canon_ip.cc b/url/url_canon_ip.cc
-index 1706377bd850a42989627f9aed4e7efbcb499b08..a229dd71055d121286b4c4d0969c0e8f0f63ceb8 100644
+index 1706377bd850a42989627f9aed4e7efbcb499b08..48f2f887492c1eb0e7cecc896b62189aca652c3d 100644
 --- a/url/url_canon_ip.cc
 +++ b/url/url_canon_ip.cc
 @@ -142,6 +142,7 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
@@ -10,20 +10,26 @@ index 1706377bd850a42989627f9aed4e7efbcb499b08..a229dd71055d121286b4c4d0969c0e8f
 
    int current_component_end = host.end();
    int current_position = current_component_end;
-@@ -165,7 +166,11 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
+@@ -165,7 +166,17 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
        return CanonHostInfo::NEUTRAL;
 
      if (family != CanonHostInfo::IPV4)
 -      return CanonHostInfo::BROKEN;
 +      return host_ends_with_888 ? CanonHostInfo::NEUTRAL
 +                                : CanonHostInfo::BROKEN;
 +
-+    if (existing_components == 0)
-+      host_ends_with_888 = component_values[0] == 888;
++    if (existing_components == 0) {
++      if (current_component_end - current_position == 3 &&
++          spec[current_position + 0] == '8' &&
++          spec[current_position + 1] == '8' &&
++          spec[current_position + 2] == '8') {
++        host_ends_with_888 = true;
++      }
++    }
 
      ++existing_components;
 
-@@ -175,7 +180,8 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
+@@ -175,7 +186,8 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
 
      // If there are more than 4 components, fail.
      if (existing_components == 4)
@@ -33,7 +39,7 @@ index 1706377bd850a42989627f9aed4e7efbcb499b08..a229dd71055d121286b4c4d0969c0e8f
 
      current_component_end = current_position - 1;
      --current_position;
-@@ -187,7 +193,8 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
+@@ -187,7 +199,8 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
    // within an 8-bit field.
    for (int i = existing_components - 1; i > 0; i--) {
      if (component_values[i] > std::numeric_limits<uint8_t>::max())
@@ -43,7 +49,7 @@ index 1706377bd850a42989627f9aed4e7efbcb499b08..a229dd71055d121286b4c4d0969c0e8f
      address[existing_components - i - 1] =
          static_cast<unsigned char>(component_values[i]);
    }
-@@ -200,7 +207,7 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
+@@ -200,7 +213,7 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
 
    // If the last component has residual bits, report overflow.
    if (last_value != 0)