Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of URLs. This fixes an issue with PlzNavigate where we could not go back to about:blank/#foo due to some discrepancy between the url in the FrameNavigationEntry & the url stored in the PageState of the same entry. BUG=575210 Review-Url: https://codereview.chromium.org/2644133002 Cr-Commit-Position: refs/heads/master@{#445525}
Denger-Network · Jan 23, 2017 · eff9252 · eff9252
1 parent 8f64e92
commit eff9252
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 13 deletions.
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
@@ -30,6 +30,7 @@
 #include "storage/browser/fileapi/isolated_context.h"
 #include "storage/common/fileapi/file_system_util.h"
 #include "url/gurl.h"
+#include "url/url_util.h"
 
 namespace content {
 
@@ -626,8 +627,9 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL(
     return false;  // Can't request invalid URLs.
 
   if (IsPseudoScheme(url.scheme())) {
-    // Every child process can request <about:blank> and <about:srcdoc>.
-    if (url == url::kAboutBlankURL || url == kAboutSrcDocURL)
+    // Every child process can request <about:blank>, <about:blank?foo>,
+    // <about:blank/#foo> and <about:srcdoc>.
+    if (url::IsAboutBlank(url) || url == kAboutSrcDocURL)
       return true;
     // URLs like <about:version>, <about:crash>, <view-source:...> shouldn't be
     // requestable by any child process.  Also, this case covers

diff --git a/content/common/navigation_params.cc b/content/common/navigation_params.cc
@@ -12,6 +12,7 @@
 #include "content/public/common/url_constants.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
+#include "url/url_util.h"
 
 namespace content {
 
@@ -23,7 +24,7 @@ bool ShouldMakeNetworkRequestForURL(const GURL& url) {
   // to the network stack.
   // TODO(clamy): same document navigations should not send requests to the
   // network stack. Neither should pushState/popState.
-  return url != url::kAboutBlankURL && !url.SchemeIs(url::kJavaScriptScheme) &&
+  return !url::IsAboutBlank(url) && !url.SchemeIs(url::kJavaScriptScheme) &&
          !url.is_empty() && !url.SchemeIs(url::kContentIDScheme) &&
          url != content::kAboutSrcDocURL;
 }

diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
@@ -5476,15 +5476,6 @@ WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation(
       info.urlRequest.checkForBrowserSideNavigation() &&
       ShouldMakeNetworkRequestForURL(url)) {
     if (info.defaultPolicy == blink::WebNavigationPolicyCurrentTab) {
-      if (RenderThreadImpl::current() &&
-          RenderThreadImpl::current()->layout_test_mode()) {
-        // Layout tests sometimes attempt to load urls of the form
-        // about:blank?foo which the browser doesn't expect and will convert to
-        // about:blank. Don't send these to the browser.
-        if (url.SchemeIs(url::kAboutScheme) && url.path() == "blank")
-          return info.defaultPolicy;
-      }
-
       BeginNavigation(info);
       return blink::WebNavigationPolicyHandledByClient;
     } else {

diff --git a/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation b/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
@@ -9,7 +9,6 @@ crbug.com/608372 virtual/mojo-loading/http/tests/history/back-to-post.html [ Tim
 crbug.com/608372 fast/forms/submit-to-url-fragment.html [ Failure ]
 
 # https://crbug.com/575210: PlzNavigate: history navigation support
-crbug.com/575210 fast/history/history-length-append-subframe-with-hash.html [ Failure ]
 crbug.com/575210 fast/loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html [ Crash Timeout ]
 crbug.com/575210 http/tests/navigation/history-back-across-form-submission-to-fragment.html [ Timeout ]
 crbug.com/575210 external/wpt/html/browsers/browsing-the-web/history-traversal/popstate_event.html [ Crash Failure Timeout ]

diff --git a/url/url_constants.cc b/url/url_constants.cc
@@ -8,6 +8,9 @@ namespace url {
 
 const char kAboutBlankURL[] = "about:blank";
 
+const char kAboutBlankPath[] = "blank";
+const char kAboutBlankWithHashPath[] = "blank/";
+
 const char kAboutScheme[] = "about";
 const char kBlobScheme[] = "blob";
 const char kContentScheme[] = "content";

diff --git a/url/url_constants.h b/url/url_constants.h
@@ -13,6 +13,9 @@ namespace url {
 
 URL_EXPORT extern const char kAboutBlankURL[];
 
+URL_EXPORT extern const char kAboutBlankPath[];
+URL_EXPORT extern const char kAboutBlankWithHashPath[];
+
 URL_EXPORT extern const char kAboutScheme[];
 URL_EXPORT extern const char kBlobScheme[];
 // The content scheme is specific to Android for identifying a stored file.

diff --git a/url/url_util.cc b/url/url_util.cc
@@ -10,6 +10,7 @@
 #include "base/debug/leak_annotations.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
+#include "url/gurl.h"
 #include "url/url_canon_internal.h"
 #include "url/url_constants.h"
 #include "url/url_file.h"
@@ -608,6 +609,21 @@ bool IsReferrerScheme(const char* spec, const Component& scheme) {
   return DoIsInSchemes(spec, scheme, &unused_scheme_type, *referrer_schemes);
 }
 
+bool IsAboutBlank(const GURL& url) {
+  if (!url.SchemeIs(url::kAboutScheme))
+    return false;
+
+  if (url.has_host() || url.has_username() || url.has_password() ||
+      url.has_port()) {
+    return false;
+  }
+
+  if (url.path() != kAboutBlankPath && url.path() != kAboutBlankWithHashPath)
+    return false;
+
+  return true;
+}
+
 bool FindAndCompareScheme(const char* str,
                           int str_len,
                           const char* compare,

diff --git a/url/url_util.h b/url/url_util.h
@@ -15,6 +15,8 @@
 #include "url/url_constants.h"
 #include "url/url_export.h"
 
+class GURL;
+
 namespace url {
 
 // Init ------------------------------------------------------------------------
@@ -153,6 +155,10 @@ URL_EXPORT bool GetStandardSchemeType(const char* spec,
                                       const Component& scheme,
                                       SchemeType* type);
 
+// Check whether the url is of the form about:blank, about:blank?foo or
+// about:blank/#foo.
+URL_EXPORT bool IsAboutBlank(const GURL& url);
+
 // Hosts  ----------------------------------------------------------------------
 
 // Returns true if the |canonicalized_host| matches or is in the same domain as

diff --git a/url/url_util_unittest.cc b/url/url_util_unittest.cc
@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
 #include "url/url_canon.h"
 #include "url/url_canon_stdstring.h"
@@ -417,4 +418,19 @@ TEST(URLUtilTest, TestDomainIs) {
   }
 }
 
+TEST(URLUtilTest, IsAboutBlank) {
+  const std::string kAboutBlankUrls[] = {"about:blank", "about:blank?foo",
+                                         "about:blank/#foo",
+                                         "about:blank?foo#foo"};
+  for (const auto& url : kAboutBlankUrls)
+    EXPECT_TRUE(IsAboutBlank(GURL(url)));
+
+  const std::string kNotAboutBlankUrls[] = {
+      "http:blank",      "about:blan",          "about://blank",
+      "about:blank/foo", "about://:8000/blank", "about://foo:foo@/blank",
+      "foo@about:blank", "foo:bar@about:blank", "about:blank:8000"};
+  for (const auto& url : kNotAboutBlankUrls)
+    EXPECT_FALSE(IsAboutBlank(GURL(url)));
+}
+
 }  // namespace url