From 9b5d9b273b2aa1cf9b66c4ae6caab10c68ee697b Mon Sep 17 00:00:00 2001 From: Rakina Zata Amni Date: Mon, 12 Jul 2021 16:19:37 +0000 Subject: [PATCH] Reparse File GURLs when doing ReplaceComponents on Win32 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 Auto-Submit: Rakina Zata Amni Reviewed-by: Matt Falkenhagen Reviewed-by: Daniel Cheng Cr-Commit-Position: refs/heads/master@{#900501} --- .../renderer_host/render_frame_host_impl.cc | 13 --- url/gurl.cc | 37 ++++++-- url/gurl.h | 2 + url/ipc/url_param_traits_unittest.cc | 92 +++++++++++-------- url/mojom/url_gurl_mojom_traits_unittest.cc | 92 +++++++++++-------- 5 files changed, 139 insertions(+), 97 deletions(-) diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc index 86c2df1f0c405a..0b14bf1fce75da 100644 --- a/content/browser/renderer_host/render_frame_host_impl.cc +++ b/content/browser/renderer_host/render_frame_host_impl.cc @@ -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(); } diff --git a/url/gurl.cc b/url/gurl.cc index cf50eccecfa034..734aa0b8a82a5a 100644 --- a/url/gurl.cc +++ b/url/gurl.cc @@ -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(result.spec_.data(), result.parsed_.Length(), - *result.parsed_.inner_parsed(), true); - } + + ProcessFileOrFileSystemURLAfterReplaceComponents(result); return result; } @@ -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(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(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. diff --git a/url/gurl.h b/url/gurl.h index 74c6e7465c6ded..38ba8318687605 100644 --- a/url/gurl.h +++ b/url/gurl.h @@ -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_; diff --git a/url/ipc/url_param_traits_unittest.cc b/url/ipc/url_param_traits_unittest.cc index 7115394b70a23f..9103f8091cdf47 100644 --- a/url/ipc/url_param_traits_unittest.cc +++ b/url/ipc/url_param_traits_unittest.cc @@ -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()); + } } diff --git a/url/mojom/url_gurl_mojom_traits_unittest.cc b/url/mojom/url_gurl_mojom_traits_unittest.cc index 357a701703a2d7..886ca37564d96a 100644 --- a/url/mojom/url_gurl_mojom_traits_unittest.cc +++ b/url/mojom/url_gurl_mojom_traits_unittest.cc @@ -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.