diff --git a/components/url_formatter/elide_url_unittest.cc b/components/url_formatter/elide_url_unittest.cc index eaa56982896448..8ea66aaa09a1c1 100644 --- a/components/url_formatter/elide_url_unittest.cc +++ b/components/url_formatter/elide_url_unittest.cc @@ -636,7 +636,7 @@ TEST(TextEliderTest, FormatOriginForSecurityDisplay) { L"file://", L"file://", L"file://"}, {"Invalid scheme 1", "twelve://www.cyber.org/wow.php", L"", L"", L""}, {"Invalid scheme 2", "://www.cyber.org/wow.php", L"", L"", L""}, - {"Invalid port 1", "https://173.194.65.103:000", L"", L"", L""}, + {"Invalid port 1", "https://173.194.65.103:99999", L"", L"", L""}, {"Invalid port 2", "https://173.194.65.103:gruffle", L"", L"", L""}, {"Blob URL", "blob:http://www.html5rocks.com/4d4ff040-6d61-4446-86d3-13ca07ec9ab9", diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index 8ba4e1442d3510..4f4b5887750b84 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc @@ -10474,4 +10474,95 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTestNoServer, EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType()); } +// Test for a navigation that is +// 1) initiated by a cross-site frame +// 2) same-document +// 3) to a http URL with port 0. +// This is the scenario behind https://crbug.com/1065532. +IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, + SameDocumentNavigationToHttpPortZero) { + GURL page_url(embedded_test_server()->GetURL( + "foo.com", "/navigation_controller/simple_page_1.html")); + EXPECT_TRUE(NavigateToURL(shell(), page_url)); + + // Inject a HTTP subframe. + const char kSubframeScriptTemplate[] = R"( + var iframe = document.createElement('iframe'); + iframe.src = $1; + document.body.appendChild(iframe); + )"; + GURL subframe_initial_url = + embedded_test_server()->GetURL("another-site.com", "/title2.html"); + { + TestNavigationObserver subframe_injection_observer(shell()->web_contents(), + 1); + ASSERT_TRUE(ExecJs( + shell(), JsReplace(kSubframeScriptTemplate, subframe_initial_url))); + subframe_injection_observer.WaitForNavigationFinished(); + ASSERT_TRUE(subframe_injection_observer.last_navigation_succeeded()); + } + + // From the main page initiate a navigation of the cross-site subframe to a + // http URL that has port=0. Note that this is valid port according to the + // URL spec (https://url.spec.whatwg.org/#port-state). + // + // Before the fix for SchemeHostPort's handling of port=0 the 2nd iteration + // below would produce a browser process CHECK/crash: + // - Iteration #1: The navigation will error out (because nothing is listening + // on port zero in the test case). Since + // RenderFrameHostImpl::FailedNavigation doesn't call + // GetOriginForURLLoaderFactory, this iteration wouldn't trigger a crash. + // - Iteration #2: The navigation will be treated as a same-document + // navigation, because |old_url| and |new_url| will be considered the same + // when inspected by GetNavigationType in navigation_controller_impl.cc. + // This will trigger a call to GetOriginForURLLoaderFactory which will + // crash if port=0 confuses url::Origin::Resolve. It is unclear if treating + // the 2nd iteration as same-document should be considered a bug (see also + // https://crbug.com/1065532#c4). + // + // After the fix for SchemeHostPort's handling of port=0, both iterations + // would trigger a CANNOT_COMMIT_ORIGIN kill (see below). + GURL::Replacements replace_port_and_ref; + replace_port_and_ref.SetPortStr("0"); + replace_port_and_ref.SetRefStr("someRef"); + GURL subframe_ref_url = + subframe_initial_url.ReplaceComponents(replace_port_and_ref); + // Doing 2 iterations, to test the same-document navigation case as outlined + // in the big comment above. This test coverage will be important if we ever + // fix the renderer kill that started happening after fixing SchemeHostPort's + // handling of port=0.. + for (int i = 0; i < 2; i++) { + SCOPED_TRACE(::testing::Message() << "Navigation #" << i); + + // TODO(lukasza): https://crbug.com/1065532: blink::KURL and + // blink::SecurityOrigin treat port=0 as an invalid port, which leads to + // committing an origin with port=80 rather than port=0 which leads to a + // CANNOT_COMMIT_ORIGIN renderer kill. This is bad, but not as bad as a + // browser crash from the bug, so let's keep things this way going forward. + ASSERT_EQ(2u, shell()->web_contents()->GetAllFrames().size()); + RenderProcessHostBadIpcMessageWaiter bad_ipc_waiter( + shell()->web_contents()->GetAllFrames()[1]->GetProcess()); + + TestNavigationObserver subframe_ref_observer(shell()->web_contents(), 1); + ASSERT_TRUE( + ExecJs(shell(), JsReplace("document.querySelector('iframe').src = $1;", + subframe_ref_url))); + +#if 1 + // TODO(lukasza): https://crbug.com/1065532: blink::KURL and + // blink::SecurityOrigin treat port=0 as an invalid port [...] + // (see the same comment above). + EXPECT_EQ(bad_message::RFH_INVALID_ORIGIN_ON_COMMIT, bad_ipc_waiter.Wait()); + if (!AreAllSitesIsolatedForTesting()) { + // Without site-per-process the main frame and the subframe are hosted in + // the same (killed) renderer process, which makes it difficult to + // continue the test after the first kill. + return; + } +#else + subframe_ref_url.WaitForNavigationFinished(); +#endif + } +} + } // namespace content diff --git a/url/gurl_unittest.cc b/url/gurl_unittest.cc index 7a4e8727be969e..d0e2e7375a2ac1 100644 --- a/url/gurl_unittest.cc +++ b/url/gurl_unittest.cc @@ -5,9 +5,11 @@ #include #include "base/stl_util.h" +#include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" +#include "url/origin.h" #include "url/url_canon.h" #include "url/url_test_utils.h" @@ -223,16 +225,17 @@ TEST(GURLTest, CopyFileSystem) { TEST(GURLTest, IsValid) { const char* valid_cases[] = { - "http://google.com", - "unknown://google.com", - "http://user:pass@google.com", - "http://google.com:12345", - "http://google.com/path", - "http://google.com//path", - "http://google.com?k=v#fragment", - "http://user:pass@google.com:12345/path?k=v#fragment", - "http:/path", - "http:path", + "http://google.com", + "unknown://google.com", + "http://user:pass@google.com", + "http://google.com:12345", + "http://google.com:0", // 0 is a valid port + "http://google.com/path", + "http://google.com//path", + "http://google.com?k=v#fragment", + "http://user:pass@google.com:12345/path?k=v#fragment", + "http:/path", + "http:path", }; for (size_t i = 0; i < base::size(valid_cases); i++) { EXPECT_TRUE(GURL(valid_cases[i]).is_valid()) @@ -240,12 +243,14 @@ TEST(GURLTest, IsValid) { } const char* invalid_cases[] = { - "http://?k=v", - "http:://google.com", - "http//google.com", - "http://google.com:12three45", - "://google.com", - "path", + "http://?k=v", + "http:://google.com", + "http//google.com", + "http://google.com:12three45", + "file://server:123", // file: URLs cannot have a port + "file://server:0", + "://google.com", + "path", }; for (size_t i = 0; i < base::size(invalid_cases); i++) { EXPECT_FALSE(GURL(invalid_cases[i]).is_valid()) @@ -979,4 +984,43 @@ TEST(GURLTest, DebugAlias) { EXPECT_STREQ("https://foo.com/bar", url_debug_alias); } +TEST(GURLTest, PortZero) { + GURL port_zero_url("http://127.0.0.1:0/blah"); + + // https://url.spec.whatwg.org/#port-state says that the port 1) consists of + // ASCII digits (this excludes negative numbers) and 2) cannot be greater than + // 2^16-1. This means that port=0 should be valid. + EXPECT_TRUE(port_zero_url.is_valid()); + EXPECT_EQ("0", port_zero_url.port()); + EXPECT_EQ("127.0.0.1", port_zero_url.host()); + EXPECT_EQ("http", port_zero_url.scheme()); + + // https://crbug.com/1065532: SchemeHostPort would previously incorrectly + // consider port=0 to be invalid. + SchemeHostPort scheme_host_port(port_zero_url); + EXPECT_TRUE(scheme_host_port.IsValid()); + EXPECT_EQ(port_zero_url.scheme(), scheme_host_port.scheme()); + EXPECT_EQ(port_zero_url.host(), scheme_host_port.host()); + EXPECT_EQ(port_zero_url.port(), + base::NumberToString(scheme_host_port.port())); + + // https://crbug.com/1065532: The SchemeHostPort problem above would lead to + // bizarre results below - resolved origin would incorrectly be returned as an + // opaque origin derived from |another_origin|. + url::Origin another_origin = url::Origin::Create(GURL("http://other.com")); + url::Origin resolved_origin = + url::Origin::Resolve(port_zero_url, another_origin); + EXPECT_FALSE(resolved_origin.opaque()); + EXPECT_EQ(port_zero_url.scheme(), resolved_origin.scheme()); + EXPECT_EQ(port_zero_url.host(), resolved_origin.host()); + EXPECT_EQ(port_zero_url.port(), base::NumberToString(resolved_origin.port())); + + // port=0 and default HTTP port are different. + GURL default_port("http://127.0.0.1/foo"); + EXPECT_EQ(0, SchemeHostPort(port_zero_url).port()); + EXPECT_EQ(80, SchemeHostPort(default_port).port()); + url::Origin default_port_origin = url::Origin::Create(default_port); + EXPECT_FALSE(default_port_origin.IsSameOriginWith(resolved_origin)); +} + } // namespace url diff --git a/url/origin_unittest.cc b/url/origin_unittest.cc index a41281d1e8d779..60808c8a20ae5a 100644 --- a/url/origin_unittest.cc +++ b/url/origin_unittest.cc @@ -483,7 +483,8 @@ TEST_F(OriginTest, UnsafelyCreate) { {"http", "example.com", 123}, {"https", "example.com", 443}, {"https", "example.com", 123}, - {"file", "", 0}, + {"http", "example.com", 0}, // 0 is a valid port for http. + {"file", "", 0}, // 0 indicates "no port" for file: scheme. {"file", "example.com", 0}, }; @@ -539,12 +540,10 @@ TEST_F(OriginTest, UnsafelyCreateUniqueOnInvalidInput) { {"http", "example.com\rnot-example.com"}, {"http", "example.com\n"}, {"http", "example.com\r"}, - {"http", "example.com", 0}, {"unknown-scheme", "example.com"}, {"host-only", "\r", 0}, {"host-only", "example.com", 22}, - {"host-port-only", "example.com", 0}, - {"file", ""}}; + {"file", "", 123}}; // file: shouldn't have a port. for (const auto& test : cases) { SCOPED_TRACE(testing::Message() diff --git a/url/scheme_host_port.cc b/url/scheme_host_port.cc index 96d491267b4cc9..52d099ccbc004d 100644 --- a/url/scheme_host_port.cc +++ b/url/scheme_host_port.cc @@ -79,10 +79,12 @@ bool IsValidInput(const base::StringPiece& scheme, switch (scheme_type) { case SCHEME_WITH_HOST_AND_PORT: case SCHEME_WITH_HOST_PORT_AND_USER_INFORMATION: - // A URL with |scheme| is required to have the host and port (may be - // omitted in a serialization if it's the same as the default value). - // Return an invalid instance if either of them is not given. - if (host.empty() || port == 0) + // A URL with |scheme| is required to have the host and port, so return an + // invalid instance if host is not given. Note that a valid port is + // always provided by SchemeHostPort(const GURL&) constructor (a missing + // port is replaced with a default port if needed by + // GURL::EffectiveIntPort()). + if (host.empty()) return false; // Don't do an expensive canonicalization if the host is already diff --git a/url/scheme_host_port.h b/url/scheme_host_port.h index 6a438fe14e0245..fcc1d1619609f3 100644 --- a/url/scheme_host_port.h +++ b/url/scheme_host_port.h @@ -80,8 +80,8 @@ class COMPONENT_EXPORT(URL) SchemeHostPort { // Creates a (scheme, host, port) tuple. |host| must be a canonicalized // A-label (that is, '☃.net' must be provided as 'xn--n3h.net'). |scheme| - // must be a standard scheme. |port| must not be 0, unless |scheme| does not - // support ports (e.g. 'file'). In that case, |port| must be 0. + // must be a standard scheme. |port| must be 0 if |scheme| does not support + // ports (e.g. 'file'). // // Copies the data in |scheme| and |host|. SchemeHostPort(base::StringPiece scheme, diff --git a/url/scheme_host_port_unittest.cc b/url/scheme_host_port_unittest.cc index 2ebf7e78c9b6bb..9f41f5be8a7256 100644 --- a/url/scheme_host_port_unittest.cc +++ b/url/scheme_host_port_unittest.cc @@ -91,9 +91,10 @@ TEST_F(SchemeHostPortTest, ExplicitConstruction) { } cases[] = { {"http", "example.com", 80}, {"http", "example.com", 123}, + {"http", "example.com", 0}, // 0 is a valid port for http. {"https", "example.com", 443}, {"https", "example.com", 123}, - {"file", "", 0}, + {"file", "", 0}, // 0 indicates "no port" for file: scheme. {"file", "example.com", 0}, }; @@ -130,8 +131,7 @@ TEST_F(SchemeHostPortTest, InvalidConstruction) { {"http", "example.com\rnot-example.com", 80}, {"http", "example.com\n", 80}, {"http", "example.com\r", 80}, - {"http", "example.com", 0}, - {"file", "", 80}}; + {"file", "", 80}}; // Can''t have a port for file: scheme. for (const auto& test : cases) { SCOPED_TRACE(testing::Message() << test.scheme << "://" << test.host << ":"