Skip to content

Commit

Permalink
Reparse File GURLs when doing ReplaceComponents on Win32
Browse files Browse the repository at this point in the history 
File GURLs with Windows-drive-letter are stripped of its hostname when
parsed, but this doesn't happen when GURL was created through
ReplaceComponents, as it doesn't go through the special DoParseUNC path.
This leads to unexpected changes to the URL when it gets serialized
and reparsed (e.g. for IPC calls). To avoid this, this CL makes it so
that ReplaceComponents also goes through the parsing logic by forcing
reparsing of file URLS on Windows.

Bug: 1214098
Change-Id: Ib92f4bd503766fadc1dd80fd31d94714c174f14a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2992912
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Auto-Submit: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#900501}
  • Loading branch information
@rakina
rakina authored and Chromium LUCI CQ committed Jul 12, 2021
1 parent 6459e91 commit 9b5d9b2
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 97 deletions.
13 changes: 0 additions & 13 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -853,19 +853,6 @@ void VerifyThatBrowserAndRendererCalculatedOriginsToCommitMatch(
if (renderer_side_origin.opaque() && browser_side_origin.opaque())
return;

#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.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
37 changes: 27 additions & 10 deletions url/gurl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,8 @@ GURL GURL::ReplaceComponents(
NULL, &output, &result.parsed_);

output.Complete();
if (result.is_valid_ && result.SchemeIsFileSystem()) {
result.inner_url_ =
std::make_unique<GURL>(result.spec_.data(), result.parsed_.Length(),
*result.parsed_.inner_parsed(), true);
}

ProcessFileOrFileSystemURLAfterReplaceComponents(result);
return result;
}

Expand All @@ -260,14 +257,34 @@ GURL GURL::ReplaceComponents(
NULL, &output, &result.parsed_);

output.Complete();
if (result.is_valid_ && result.SchemeIsFileSystem()) {
result.inner_url_ =
std::make_unique<GURL>(result.spec_.data(), result.parsed_.Length(),
*result.parsed_.inner_parsed(), true);
}

ProcessFileOrFileSystemURLAfterReplaceComponents(result);

return result;
}

void GURL::ProcessFileOrFileSystemURLAfterReplaceComponents(GURL& url) const {
if (!url.is_valid_)
return;
if (url.SchemeIsFileSystem()) {
url.inner_url_ =
std::make_unique<GURL>(url.spec_.data(), url.parsed_.Length(),
*url.parsed_.inner_parsed(), true);
}
#ifdef WIN32
if (url.SchemeIsFile()) {
// On Win32, some file URLs created through ReplaceComponents used to lose
// its hostname after getting reparsed (e.g. when it's sent through IPC) due
// to special handling of file URLs with Windows-drive paths in the URL
// parser. To make the behavior for URLs modified through ReplaceComponents
// (instead of getting fully reparsed) the same, immediately reparse the
// URL here to trigger the special handling.
// See https://crbug.com/1214098.
url = GURL(url.spec());
}
#endif
}

GURL GURL::GetOrigin() const {
// This doesn't make sense for invalid or nonstandard URLs, so return
// the empty URL.
Expand Down
2 changes: 2 additions & 0 deletions url/gurl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ class COMPONENT_EXPORT(URL) GURL {
return base::StringPiece(&spec_[comp.begin], comp.len);
}

void ProcessFileOrFileSystemURLAfterReplaceComponents(GURL& url) const;

// The actual text of the URL, in canonical ASCII form.
std::string spec_;

Expand Down
92 changes: 55 additions & 37 deletions url/ipc/url_param_traits_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,49 +88,67 @@ TEST(IPCMessageTest, SerializeGurl_CorruptPayload) {
// 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());
{
// #1: Try creating a file URL with a non-empty hostname.
GURL url_without_windows_drive_letter("file://hostname/");
EXPECT_EQ("/", url_without_windows_drive_letter.path());
EXPECT_EQ("hostname", url_without_windows_drive_letter.host());
ExpectSerializationRoundtrips(url_without_windows_drive_letter);
}

{
// #2: Use GURL::Replacement to create a GURL with 1) a path that starts
// with a Windows drive letter and 2) has a non-empty hostname (inherited
// from `url_without_windows_drive_letter` above). This used to not go
// through the DoParseUNC path that normally strips the hostname (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 url_made_with_replace_components =
GURL("file://hostname/").ReplaceComponents(repl);

EXPECT_EQ(kNewPath, url_made_with_replace_components.path());
#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());
// Due to the reparsing logic in ReplaceComponents, the hostname is stripped
// for this URL too.
EXPECT_EQ("", url_made_with_replace_components.host());
EXPECT_EQ("file:///C:/dir/file.txt",
url_made_with_replace_components.spec());
#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);
EXPECT_EQ("hostname", url_made_with_replace_components.host());
EXPECT_EQ("file://hostname/C:/dir/file.txt",
url_made_with_replace_components.spec());
#endif
// This is the MAIN VERIFICATION in this test. This used to fail on Windows,
// see https://crbug.com/1214098.
ExpectSerializationRoundtrips(url_made_with_replace_components);
}

// 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());
{
// #3: Try to create a URL with a Windows drive letter and a non-empty
// hostname directly.
GURL url_created_directly("file://hostname/C:/dir/file.txt");
EXPECT_EQ("/C:/dir/file.txt", url_created_directly.path());
#ifdef WIN32
EXPECT_NE(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("", url2.host());
// On Win32, the hostname will be reset by DoParseUNC.
EXPECT_EQ("", url_created_directly.host());
EXPECT_EQ("file:///C:/dir/file.txt", url_created_directly.spec());
#else
EXPECT_EQ(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("hostname", url2.host());
// On other platforms, the hostname is kept.
EXPECT_EQ("hostname", url_created_directly.host());
EXPECT_EQ("file://hostname/C:/dir/file.txt", url_created_directly.spec());
#endif
EXPECT_EQ(url2.path(), url1_with_replaced_path.path());
ExpectSerializationRoundtrips(url2);
ExpectSerializationRoundtrips(url_created_directly);

// The URL created directly and the URL created through ReplaceComponents
// should be the same.
GURL::Replacements repl;
const std::string kNewPath = "/C:/dir/file.txt";
repl.SetPath(kNewPath.c_str(), url::Component(0, kNewPath.length()));
GURL url_made_with_replace_components =
GURL("file://hostname/").ReplaceComponents(repl);
EXPECT_EQ(url_created_directly.spec(),
url_made_with_replace_components.spec());
}
}
92 changes: 55 additions & 37 deletions url/mojom/url_gurl_mojom_traits_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,51 +99,69 @@ TEST_F(MojoGURLStructTraitsTest, ExcessivelyLongUrl) {
// 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());
{
// #1: Try creating a file URL with a non-empty hostname.
GURL url_without_windows_drive_letter("file://hostname/");
EXPECT_EQ("/", url_without_windows_drive_letter.path());
EXPECT_EQ("hostname", url_without_windows_drive_letter.host());
ExpectSerializationRoundtrips(url_without_windows_drive_letter);
}

{
// #2: Use GURL::Replacement to create a GURL with 1) a path that starts
// with a Windows drive letter and 2) has a non-empty hostname (inherited
// from `url_without_windows_drive_letter` above). This used to not go
// through the DoParseUNC path that normally strips the hostname (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 url_made_with_replace_components =
GURL("file://hostname/").ReplaceComponents(repl);

EXPECT_EQ(kNewPath, url_made_with_replace_components.path());
#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());
// Due to the reparsing logic in ReplaceComponents, the hostname is stripped
// for this URL too.
EXPECT_EQ("", url_made_with_replace_components.host());
EXPECT_EQ("file:///C:/dir/file.txt",
url_made_with_replace_components.spec());
#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);
EXPECT_EQ("hostname", url_made_with_replace_components.host());
EXPECT_EQ("file://hostname/C:/dir/file.txt",
url_made_with_replace_components.spec());
#endif
// This is the MAIN VERIFICATION in this test. This used to fail on Windows,
// see https://crbug.com/1214098.
ExpectSerializationRoundtrips(url_made_with_replace_components);
}

// 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());
{
// #3: Try to create a URL with a Windows drive letter and a non-empty
// hostname directly.
GURL url_created_directly("file://hostname/C:/dir/file.txt");
EXPECT_EQ("/C:/dir/file.txt", url_created_directly.path());
#ifdef WIN32
EXPECT_NE(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("", url2.host());
// On Win32, the hostname will be reset by DoParseUNC.
EXPECT_EQ("", url_created_directly.host());
EXPECT_EQ("file:///C:/dir/file.txt", url_created_directly.spec());
#else
EXPECT_EQ(url2.spec(), url1_with_replaced_path.spec());
EXPECT_EQ("hostname", url2.host());
// On other platforms, the hostname is kept.
EXPECT_EQ("hostname", url_created_directly.host());
EXPECT_EQ("file://hostname/C:/dir/file.txt", url_created_directly.spec());
#endif
EXPECT_EQ(url2.path(), url1_with_replaced_path.path());
ExpectSerializationRoundtrips(url2);
ExpectSerializationRoundtrips(url_created_directly);

// The URL created directly and the URL created through ReplaceComponents
// should be the same.
GURL::Replacements repl;
const std::string kNewPath = "/C:/dir/file.txt";
repl.SetPath(kNewPath.c_str(), url::Component(0, kNewPath.length()));
GURL url_made_with_replace_components =
GURL("file://hostname/").ReplaceComponents(repl);
EXPECT_EQ(url_created_directly.spec(),
url_made_with_replace_components.spec());
}
}

// Test of basic Origin serialization.
Expand Down

0 comments on commit 9b5d9b2

Please sign in to comment.