Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes url parsing for .888 hosts #14588

Closed
wants to merge 3 commits into from
Closed

Fixes url parsing for .888 hosts #14588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1123b8c
Fixes url parsing for .888 hosts
supermassive Aug 11, 2022
a6ecf5c
fixes per review
supermassive Aug 12, 2022
692c0ff
review fixes
supermassive Aug 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions browser/decentralized_dns/test/utils_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "chrome/test/base/scoped_testing_local_state.h"
#include "chrome/test/base/testing_browser_process.h"
#include "components/prefs/testing_pref_service.h"
#include "net/base/ip_address.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace decentralized_dns {
Expand All @@ -29,6 +30,7 @@ class UtilsUnitTest : public testing::Test {

TEST_F(UtilsUnitTest, IsUnstoppableDomainsTLD) {
EXPECT_TRUE(IsUnstoppableDomainsTLD(GURL("http://test.crypto")));
EXPECT_TRUE(IsUnstoppableDomainsTLD(GURL("http://test.888")));
EXPECT_FALSE(IsUnstoppableDomainsTLD(GURL("http://test.com")));
EXPECT_FALSE(IsUnstoppableDomainsTLD(GURL("http://test.eth")));
EXPECT_FALSE(IsUnstoppableDomainsTLD(GURL("http://crypto")));
Expand Down Expand Up @@ -95,4 +97,40 @@ TEST_F(UtilsUnitTest, ResolveMethodMigration) {
EXPECT_TRUE(IsENSResolveMethodAsk(local_state()));
}

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add 1.2.3.888 to check that it's a valid URL but not a valid IPv4 URL.

Copy link
Collaborator Author

@supermassive supermassive Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in a6ecf5c
It actually didn't pass as I didn't cover residual bits case in parse(I wrongly assumed that was not .888 case)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now DoIPv4AddressToNumber never reports BROKEN when last component is .888

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now DoIPv4AddressToNumber never reports BROKEN when last component is .888

Is that the right behavior? I mean if you have 1.2.3.888, which is not a valid IPv4 and try to convert this IPv4 URL to a number, it seems like it should fail because the input was not an IPv4 address in the first place, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For correct IPv4 address parser should say that it is exactly IPv4 address, so we don't break it
For Url parser now says it is not a broken ipv4 address, but something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so we now have this?

  • 1.2.3.4 -> valid URL, valid IPv4
  • 1.2.3.889 -> invalid URL, invalid IPv4
  • 1.2.3.888 -> valid URL, but not an IPv4 at all

It might be worth adding 1.2.3.889 as a test case too just to make sure that we don't change that one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 692c0ff
also now checking for 888 ending being an exact string to avoid 888 in hex or oct form.

"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.1.test.888", // Some test
"888.888", // Some test
"1.888.888", // Some test
".888", // Same as .com
};

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));
}
}

} // namespace decentralized_dns
4 changes: 4 additions & 0 deletions components/decentralized_dns/utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ void MigrateObsoleteLocalStatePrefs(PrefService* local_state) {
}

bool IsUnstoppableDomainsTLD(const GURL& url) {
if (!url.is_valid())
return false;
for (auto* domain : kUnstoppableDomains) {
if (base::EndsWith(url.host_piece(), domain))
return true;
Expand All @@ -78,6 +80,8 @@ bool IsUnstoppableDomainsResolveMethodEthereum(PrefService* local_state) {
}

bool IsENSTLD(const GURL& url) {
if (!url.is_valid())
return false;
return base::EndsWith(url.host_piece(), kEthDomain);
}

Expand Down
45 changes: 45 additions & 0 deletions patches/url-url_canon_ip.cc.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
diff --git a/url/url_canon_ip.cc b/url/url_canon_ip.cc
index 1706377bd850a42989627f9aed4e7efbcb499b08..295cb6854e06bb8baf1c88598a1525d0b0517e62 100644
--- a/url/url_canon_ip.cc
+++ b/url/url_canon_ip.cc
@@ -142,6 +142,7 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,
// number.
uint32_t component_values[4];
int existing_components = 0;
+ bool host_ends_with_888 = false;

int current_component_end = host.end();
int current_position = current_component_end;
@@ -165,7 +166,11 @@ 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;

++existing_components;

@@ -175,7 +180,8 @@ CanonHostInfo::Family DoIPv4AddressToNumber(const CHAR* spec,

// If there are more than 4 components, fail.
if (existing_components == 4)
- return CanonHostInfo::BROKEN;
+ return host_ends_with_888 ? CanonHostInfo::NEUTRAL
+ : CanonHostInfo::BROKEN;

current_component_end = current_position - 1;
--current_position;
@@ -187,7 +193,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())
- return CanonHostInfo::BROKEN;
+ return host_ends_with_888 ? CanonHostInfo::NEUTRAL
+ : CanonHostInfo::BROKEN;
address[existing_components - i - 1] =
static_cast<unsigned char>(component_values[i]);
}