Skip to content

Commit

Permalink
Tighten file: URLs exception for browser-vs-renderer origin comparison.
Browse files Browse the repository at this point in the history 
The CL tweaks VerifyThatBrowserAndRendererCalculatedOriginsToCommitMatch
so that the only remaining browser-vs-renderer exception is only applied
on Windows (the only platform where it is needed).  (The opaqueness
check is removed because it was redundant with checking that schemes
match.)

This CL also adds unit tests that demonstrate the root problem here
(that some problematic GURLs do not round-trip via IPC serialization).

Bug: 1214098
Change-Id: I9cdb5508b53c7c5a3d6d780d409da45532c55c39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2910502
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889101}
  • Loading branch information
@anforowicz
anforowicz authored and Chromium LUCI CQ committed Jun 4, 2021
1 parent 37fb09b commit d087c43
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 77 deletions.
13 changes: 8 additions & 5 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -830,15 +830,18 @@ void VerifyThatBrowserAndRendererCalculatedOriginsToCommitMatch(
if (renderer_side_origin.opaque() && browser_side_origin.opaque())
return;

// Navigating to file://localhost/... on windows causes `browser_side_origin`
// and `renderer_side_origin` to be different (file://localhost/ vs file:///).
// In particular, without the following block the test
#if defined(OS_WIN)
// TODO(https://crbug.com/1214098): Navigating to a test-crafted
// (GURL::ReplaceComponents-crafted) file://localhost/C:/dir/file.txt URL will
// fail to round-trip the URL causing `browser_side_origin` and
// `renderer_side_origin` to be different (file://localhost/ vs file:///). In
// particular, without the following "if" statement the test
// ContentSecurityPolicyBrowserTest.FileURLs fails.
if ((browser_side_origin.opaque() == renderer_side_origin.opaque()) &&
browser_side_origin.scheme() == url::kFileScheme &&
if (browser_side_origin.scheme() == url::kFileScheme &&
renderer_side_origin.scheme() == url::kFileScheme) {
return;
}
#endif

DCHECK_EQ(browser_side_origin, renderer_side_origin)
<< "; navigation_request->GetURL() = " << navigation_request->GetURL();
Expand Down
153 changes: 109 additions & 44 deletions url/ipc/url_param_traits_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,63 +11,128 @@
#include "url/gurl.h"
#include "url/ipc/url_param_traits.h"

namespace {

GURL BounceUrl(const GURL& input) {
IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
IPC::ParamTraits<GURL>::Write(&msg, input);

GURL output;
base::PickleIterator iter(msg);
EXPECT_TRUE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));

return output;
}

void ExpectSerializationRoundtrips(const GURL& input) {
SCOPED_TRACE(testing::Message()
<< "Input GURL: " << input.possibly_invalid_spec());
GURL output = BounceUrl(input);

// We want to test each component individually to make sure its range was
// correctly serialized and deserialized, not just the spec.
EXPECT_EQ(input.possibly_invalid_spec(), output.possibly_invalid_spec());
EXPECT_EQ(input.is_valid(), output.is_valid());
EXPECT_EQ(input.scheme(), output.scheme());
EXPECT_EQ(input.username(), output.username());
EXPECT_EQ(input.password(), output.password());
EXPECT_EQ(input.host(), output.host());
EXPECT_EQ(input.port(), output.port());
EXPECT_EQ(input.path(), output.path());
EXPECT_EQ(input.query(), output.query());
EXPECT_EQ(input.ref(), output.ref());
}

} // namespace

// Tests that serialize/deserialize correctly understand each other.
TEST(IPCMessageTest, Serialize) {
TEST(IPCMessageTest, SerializeGurl_Basic) {
const char* serialize_cases[] = {
"http://www.google.com/",
"http://user:pass@host.com:888/foo;bar?baz#nop",
};

for (size_t i = 0; i < base::size(serialize_cases); i++) {
GURL input(serialize_cases[i]);
IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
IPC::ParamTraits<GURL>::Write(&msg, input);

GURL output;
base::PickleIterator iter(msg);
EXPECT_TRUE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));

// We want to test each component individually to make sure its range was
// correctly serialized and deserialized, not just the spec.
EXPECT_EQ(input.possibly_invalid_spec(), output.possibly_invalid_spec());
EXPECT_EQ(input.is_valid(), output.is_valid());
EXPECT_EQ(input.scheme(), output.scheme());
EXPECT_EQ(input.username(), output.username());
EXPECT_EQ(input.password(), output.password());
EXPECT_EQ(input.host(), output.host());
EXPECT_EQ(input.port(), output.port());
EXPECT_EQ(input.path(), output.path());
EXPECT_EQ(input.query(), output.query());
EXPECT_EQ(input.ref(), output.ref());
for (const char* test_input : serialize_cases) {
SCOPED_TRACE(testing::Message() << "Test input: " << test_input);
GURL input(test_input);
ExpectSerializationRoundtrips(input);
}
}

// Test an excessively long GURL.
{
const std::string url = std::string("http://example.org/").append(
url::kMaxURLChars + 1, 'a');
GURL input(url.c_str());
IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
IPC::ParamTraits<GURL>::Write(&msg, input);

GURL output;
base::PickleIterator iter(msg);
EXPECT_TRUE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));
EXPECT_TRUE(output.is_empty());
}
// Test of an excessively long GURL.
TEST(IPCMessageTest, SerializeGurl_ExcessivelyLong) {
const std::string url =
std::string("http://example.org/").append(url::kMaxURLChars + 1, 'a');
GURL input(url.c_str());
GURL output = BounceUrl(input);
EXPECT_TRUE(output.is_empty());
}

// Test an invalid GURL.
{
IPC::Message msg;
msg.WriteString("#inva://idurl/");
GURL output;
base::PickleIterator iter(msg);
EXPECT_FALSE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));
}
// Test of an invalid GURL.
TEST(IPCMessageTest, SerializeGurl_InvalidUrl) {
IPC::Message msg;
msg.WriteString("#inva://idurl/");
GURL output;
base::PickleIterator iter(msg);
EXPECT_FALSE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));
}

// Also test the corrupt case.
// Test of a corrupt deserialization input.
TEST(IPCMessageTest, SerializeGurl_CorruptPayload) {
IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
msg.WriteInt(99);
GURL output;
base::PickleIterator iter(msg);
EXPECT_FALSE(IPC::ParamTraits<GURL>::Read(&msg, &iter, &output));
}

// Test for the GURL testcase based on https://crbug.com/1214098 (which in turn
// was based on ContentSecurityPolicyBrowserTest.FileURLs).
TEST(IPCMessageTest, SerializeGurl_WindowsDriveInPathReplacement) {
GURL url1("file://hostname/");
ExpectSerializationRoundtrips(url1);
EXPECT_EQ("/", url1.path());
EXPECT_EQ("hostname", url1.host());

// Use GURL::Replacement to create a GURL with 1) a path that starts with a C:
// drive letter and 2) has a non-empty hostname (inherited from `url1` above).
// Without GURL::Replacement we would just get `url2` below, with an empty
// hostname, because of how DoParseUNC resets the hostname on Win32 (for more
// details see https://crbug.com/1214098#c4).
GURL::Replacements repl;
const std::string kNewPath = "/C:/dir/file.txt";
repl.SetPath(kNewPath.c_str(), url::Component(0, kNewPath.length()));
GURL url1_with_replaced_path = url1.ReplaceComponents(repl);
EXPECT_EQ(kNewPath, url1_with_replaced_path.path());
EXPECT_EQ("hostname", url1_with_replaced_path.host());

#ifdef WIN32
// TODO(https://crbug.com/1214098): All GURLs should round-trip when bounced
// through IPC, but this doesn't work for `url1_with_replaced_path` on
// Windows.
GURL roundtrip = BounceUrl(url1_with_replaced_path);
EXPECT_NE(roundtrip.host(), url1_with_replaced_path.host());
#else
// This is the MAIN VERIFICATION in this test. The fact that this
// verification fails on Windows is the bug tracked in
// https://crbug.com/1214098.
ExpectSerializationRoundtrips(url1_with_replaced_path);
#endif

// On Windows, `url1_with_replaced_path` will round-trip as `url2`. (There is
// nothing wrong with `url2` - its serialization round-trips just fine; the
// test assertions below just help explain the lack of round-tripping of
// `url1_with_replaced_path` above.)
GURL url2("file://hostname/C:/dir/file.txt");
ExpectSerializationRoundtrips(url2);
#ifdef WIN32
EXPECT_EQ(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ(url2.path(), url1_with_replaced_path.path());
EXPECT_EQ(url2.host(), url1_with_replaced_path.host());
EXPECT_EQ("/C:/dir/file.txt", url2.path());
EXPECT_EQ("", url2.host());
#else
EXPECT_EQ("/C:/dir/file.txt", url2.path());
EXPECT_EQ("hostname", url2.host());
#endif
}
130 changes: 102 additions & 28 deletions url/mojom/url_gurl_mojom_traits_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,21 @@ class UrlTestImpl : public mojom::UrlTest {
mojo::Receiver<UrlTest> receiver_;
};

// Mojo version of chrome IPC test in url/ipc/url_param_traits_unittest.cc.
TEST(MojoGURLStructTraitsTest, Basic) {
base::test::SingleThreadTaskEnvironment task_environment;

mojo::Remote<mojom::UrlTest> remote;
UrlTestImpl impl(remote.BindNewPipeAndPassReceiver());

const char* serialize_cases[] = {
"http://www.google.com/", "http://user:pass@host.com:888/foo;bar?baz#nop",
};
class MojoGURLStructTraitsTest : public ::testing::Test {
public:
MojoGURLStructTraitsTest()
: url_test_impl_(url_test_remote_.BindNewPipeAndPassReceiver()) {}

for (size_t i = 0; i < base::size(serialize_cases); i++) {
GURL input(serialize_cases[i]);
GURL BounceUrl(const GURL& input) {
GURL output;
EXPECT_TRUE(remote->BounceUrl(input, &output));
EXPECT_TRUE(url_test_remote_->BounceUrl(input, &output));
return output;
}

void ExpectSerializationRoundtrips(const GURL& input) {
SCOPED_TRACE(testing::Message()
<< "Input GURL: " << input.possibly_invalid_spec());
GURL output = BounceUrl(input);

// We want to test each component individually to make sure its range was
// correctly serialized and deserialized, not just the spec.
Expand All @@ -62,22 +62,97 @@ TEST(MojoGURLStructTraitsTest, Basic) {
EXPECT_EQ(input.ref(), output.ref());
}

// Test an excessively long GURL.
{
const std::string url =
std::string("http://example.org/").append(kMaxURLChars + 1, 'a');
GURL input(url.c_str());
GURL output;
EXPECT_TRUE(remote->BounceUrl(input, &output));
EXPECT_TRUE(output.is_empty());
Origin BounceOrigin(const Origin& input) {
Origin output;
EXPECT_TRUE(url_test_remote_->BounceOrigin(input, &output));
return output;
}

// Test basic Origin serialization.
private:
base::test::SingleThreadTaskEnvironment task_environment;
mojo::Remote<mojom::UrlTest> url_test_remote_;
UrlTestImpl url_test_impl_;
};

// Mojo version of chrome IPC test in url/ipc/url_param_traits_unittest.cc.
TEST_F(MojoGURLStructTraitsTest, Basic) {
const char* serialize_cases[] = {
"http://www.google.com/",
"http://user:pass@host.com:888/foo;bar?baz#nop",
};

for (const char* test_input : serialize_cases) {
SCOPED_TRACE(testing::Message() << "Test input: " << test_input);
GURL input(test_input);
ExpectSerializationRoundtrips(input);
}
}

// Test of an excessively long GURL.
TEST_F(MojoGURLStructTraitsTest, ExcessivelyLongUrl) {
const std::string url =
std::string("http://example.org/").append(kMaxURLChars + 1, 'a');
GURL input(url.c_str());
GURL output = BounceUrl(input);
EXPECT_TRUE(output.is_empty());
}

// Test for the GURL testcase based on https://crbug.com/1214098 (which in turn
// was based on ContentSecurityPolicyBrowserTest.FileURLs).
TEST_F(MojoGURLStructTraitsTest, WindowsDriveInPathReplacement) {
GURL url1("file://hostname/");
ExpectSerializationRoundtrips(url1);
EXPECT_EQ("/", url1.path());
EXPECT_EQ("hostname", url1.host());

// Use GURL::Replacement to create a GURL with 1) a path that starts with a C:
// drive letter and 2) has a non-empty hostname (inherited from `url1` above).
// Without GURL::Replacement we would just get `url2` below, with an empty
// hostname, because of how DoParseUNC resets the hostname on Win32 (for more
// details see https://crbug.com/1214098#c4).
GURL::Replacements repl;
const std::string kNewPath = "/C:/dir/file.txt";
repl.SetPath(kNewPath.c_str(), url::Component(0, kNewPath.length()));
GURL url1_with_replaced_path = url1.ReplaceComponents(repl);
EXPECT_EQ(kNewPath, url1_with_replaced_path.path());
EXPECT_EQ("hostname", url1_with_replaced_path.host());

#ifdef WIN32
// TODO(https://crbug.com/1214098): All GURLs should round-trip when bounced
// through IPC, but this doesn't work for `url1_with_replaced_path` on
// Windows.
GURL roundtrip = BounceUrl(url1_with_replaced_path);
EXPECT_NE(roundtrip.host(), url1_with_replaced_path.host());
#else
// This is the MAIN VERIFICATION in this test. The fact that this
// verification fails on Windows is the bug tracked in
// https://crbug.com/1214098.
ExpectSerializationRoundtrips(url1_with_replaced_path);
#endif

// On Windows, IPC will serialize/deserialze `url1_with_replaced_path` as
// `url2` (i.e. it won't round-trip the URL spec). The test assertions below
// help illustrate why we can't assert ExpectSerializationRoundtrips above (on
// Windows).
EXPECT_EQ("file://hostname/C:/dir/file.txt", url1_with_replaced_path.spec());
GURL url2(url1_with_replaced_path.spec());
#ifdef WIN32
EXPECT_NE(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("", url2.host());
#else
EXPECT_EQ(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("hostname", url2.host());
#endif
EXPECT_EQ(url2.path(), url1_with_replaced_path.path());
ExpectSerializationRoundtrips(url2);
}

// Test of basic Origin serialization.
TEST_F(MojoGURLStructTraitsTest, OriginSerialization) {
Origin non_unique = Origin::UnsafelyCreateTupleOriginWithoutNormalization(
"http", "www.google.com", 80)
.value();
Origin output;
EXPECT_TRUE(remote->BounceOrigin(non_unique, &output));
Origin output = BounceOrigin(non_unique);
EXPECT_EQ(non_unique, output);
EXPECT_FALSE(output.opaque());

Expand All @@ -86,19 +161,18 @@ TEST(MojoGURLStructTraitsTest, Basic) {
EXPECT_NE(unique1, unique2);
EXPECT_NE(unique2, unique1);
EXPECT_NE(unique2, non_unique);
EXPECT_TRUE(remote->BounceOrigin(unique1, &output));
output = BounceOrigin(unique1);
EXPECT_TRUE(output.opaque());
EXPECT_EQ(unique1, output);
Origin output2;
EXPECT_TRUE(remote->BounceOrigin(unique2, &output2));
Origin output2 = BounceOrigin(unique2);
EXPECT_EQ(unique2, output2);
EXPECT_NE(unique2, output);
EXPECT_NE(unique1, output2);

Origin normalized =
Origin::CreateFromNormalizedTuple("http", "www.google.com", 80);
EXPECT_EQ(normalized, non_unique);
EXPECT_TRUE(remote->BounceOrigin(normalized, &output));
output = BounceOrigin(normalized);
EXPECT_EQ(normalized, output);
EXPECT_EQ(non_unique, output);
EXPECT_FALSE(output.opaque());
Expand Down
9 changes: 9 additions & 0 deletions url/origin_abstract_tests.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,15 @@ TYPED_TEST_P(AbstractOriginTest, TupleOrigins) {
{"file://example.com/etc/passwd", {"file", "example.com", 0}},
{"file:///", {"file", "", 0}},

#ifdef WIN32
// TODO(https://crbug.com/1214098): Consider unifying URL parsing behavior
// on all platforms (or at least make sure that serialization always
// round-trips - see https://crbug.com/1214098).
{"file://hostname/C:/dir/file.txt", {"file", "", 0}},
#else
{"file://hostname/C:/dir/file.txt", {"file", "hostname", 0}},
#endif

// HTTP URLs
{"http://example.com/", {"http", "example.com", 80}},
{"http://example.com:80/", {"http", "example.com", 80}},
Expand Down

0 comments on commit d087c43

Please sign in to comment.