No-access schemes should always translate into an opaque origin.

Before this CL, there would be an inconsistency between origins
calculated by Blink vs //url for standard, no-access scheme URLs
which meant that (browser-side, //url-based) NavigationRequest::
GetOriginForURLLoaderFactory would return a non-opaque origin,
while (renderer-side, SecurityOrigin-based) Blink would commit
an opaque origin.

The CL ensures that IsValidInput in //url's scheme_host_port.cc
considers no-access first, before any consideration of whether the
scheme is standard - this makes it more consistent with how
ShouldTreatAsOpaqueOrigin in Blink's security_origin.cc works.

After this CL (i.e. after returning an opaque origin for
no-access schemes), we still want to treat no-access-scheme URLs
as potentially trustworthy - this requires small additional
tweaks in IsUrlPotentiallyTrustworthy.

Bug: 888079, 1021779, 1020201
Change-Id: I5e957700011186c312366c818bf143a027f025ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2602402Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842306}

chrome/common/chrome_content_client_unittest.cc

@@ -14,6 +14,7 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/buildflags/buildflags.h"
 #include "extensions/common/constants.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "services/network/public/cpp/is_potentially_trustworthy.h"
@@ -103,8 +104,28 @@ TEST(ChromeContentClientTest, AdditionalSchemes) {
   EXPECT_EQ("chrome-extension://abcdefghijklmnopqrstuvwxyzabcdef",
             origin.Serialize());
 
-  EXPECT_TRUE(
-      network::IsUrlPotentiallyTrustworthy(GURL("chrome-native://newtab/")));
+  // IsUrlPotentiallyTrustworthy assertions test for https://crbug.com/734581.
+  constexpr const char* kChromeLayerUrlsRegisteredAsSecure[] = {
+    // The schemes below are registered both as secure and no-access.  Product
+    // code needs to treat such URLs as trustworthy, even though no-access
+    // schemes translate into an opaque origin (which is untrustworthy).
+    "chrome-native://newtab/",
+    "chrome-error://foo/",
+    // The schemes below are registered as secure (but not as no-access).
+    "chrome://foo/",
+    "chrome-untrusted://foo/",
+    "chrome-search://foo/",
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+    "chrome-extension://foo/",
+#endif
+    "devtools://foo/",
+  };
+  for (const std::string& str : kChromeLayerUrlsRegisteredAsSecure) {
+    SCOPED_TRACE(str);
+    GURL url(str);
+    EXPECT_TRUE(base::Contains(url::GetSecureSchemes(), url.scheme()));
+    EXPECT_TRUE(network::IsUrlPotentiallyTrustworthy(url));
+  }
 
   GURL chrome_url(content::GetWebUIURL("dummyurl"));
   EXPECT_TRUE(network::IsUrlPotentiallyTrustworthy(chrome_url));

content/browser/child_process_security_policy_impl.cc

@@ -1533,19 +1533,6 @@ CanCommitStatus ChildProcessSecurityPolicyImpl::CanCommitOriginAndUrl(
       return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
     }
 
-    // Allow "no access" schemes to commit even though |url_origin| and
-    // |origin| tuples don't match. We have to allow this because Blink's
-    // SecurityOrigin::CreateWithReferenceOrigin() and url::Origin::Resolve()
-    // handle "no access" URLs differently. CreateWithReferenceOrigin() treats
-    // "no access" like data: URLs and returns an opaque origin with |origin|
-    // as a precursor. Resolve() returns a non-opaque origin consisting of the
-    // scheme and host portions of the original URL.
-    //
-    // TODO(1020201): Make CreateWithReferenceOrigin() & Resolve() consistent
-    // with each other and then remove this exception.
-    if (base::Contains(url::GetNoAccessSchemes(), url_info.url.scheme()))
-      return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
-
     return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
   }
 

content/browser/navigation_browsertest.cc

@@ -3314,19 +3314,10 @@ class NavigationUrlRewriteBrowserTest : public NavigationBaseBrowserTest {
   url::ScopedSchemeRegistryForTests scoped_registry_;
 };
 
-// TODO(1021779): Figure out why this fails on the kitkat-dbg builder
-// and re-enable for all platforms.
-#if defined(OS_ANDROID)
-#define DISABLE_ON_ANDROID(x) DISABLED_##x
-#else
-#define DISABLE_ON_ANDROID(x) x
-#endif
-
 // Tests navigating to a URL that gets rewritten to a "no access" URL. This
 // mimics the behavior of navigating to special URLs like chrome://newtab and
 // chrome://history which get rewritten to "no access" chrome-native:// URLs.
-IN_PROC_BROWSER_TEST_F(NavigationUrlRewriteBrowserTest,
-                       DISABLE_ON_ANDROID(RewriteToNoAccess)) {
+IN_PROC_BROWSER_TEST_F(NavigationUrlRewriteBrowserTest, RewriteToNoAccess) {
   // Perform an initial navigation.
   {
     TestNavigationObserver observer(web_contents());

services/network/public/cpp/is_potentially_trustworthy.cc

@@ -199,6 +199,27 @@ bool IsAllowlisted(const std::vector<std::string>& allowlist,
   return false;
 }
 
+bool IsSchemeConsideredAuthenticated(base::StringPiece scheme) {
+  // The code below is based on the specification at
+  // https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin
+
+  // 7. If origin’s scheme component is one which the user agent considers to be
+  //    authenticated, return "Potentially Trustworthy".
+  //    Note: See §7.1 Packaged Applications for detail here.
+  //
+  // Note that this ignores some schemes that are considered trustworthy by
+  // higher layers (e.g. see GetSchemesBypassingSecureContextCheck in //chrome).
+  //
+  // See also
+  // - content::ContentClient::AddAdditionalSchemes and
+  //   content::ContentClient::Schemes::local_schemes and
+  //   content::ContentClient::Schemes::secure_schemes
+  // - url::AddLocalScheme
+  // - url::AddSecureScheme
+  return base::Contains(url::GetSecureSchemes(), scheme) ||
+         base::Contains(url::GetLocalSchemes(), scheme);
+}
+
 }  // namespace
 
 bool IsOriginPotentiallyTrustworthy(const url::Origin& origin) {
@@ -228,27 +249,16 @@ bool IsOriginPotentiallyTrustworthy(const url::Origin& origin) {
 
   // 6. If origin’s scheme component is file, return "Potentially Trustworthy".
   //
-  // This is somewhat redundant with the GetLocalSchemes-based check below.
+  // This is somewhat redundant with the GetLocalSchemes-based
+  // IsSchemeConsideredAuthenticated check below.
   if (origin.scheme() == url::kFileScheme)
     return true;
 
   // 7. If origin’s scheme component is one which the user agent considers to be
   //    authenticated, return "Potentially Trustworthy".
   //    Note: See §7.1 Packaged Applications for detail here.
-  //
-  // Note that this ignores some schemes that are considered trustworthy by
-  // higher layers (e.g. see GetSchemesBypassingSecureContextCheck in //chrome).
-  //
-  // See also
-  // - content::ContentClient::AddAdditionalSchemes and
-  //   content::ContentClient::Schemes::local_schemes and
-  //   content::ContentClient::Schemes::secure_schemes
-  // - url::AddLocalScheme
-  // - url::AddSecureScheme
-  if (base::Contains(url::GetSecureSchemes(), origin.scheme()) ||
-      base::Contains(url::GetLocalSchemes(), origin.scheme())) {
+  if (IsSchemeConsideredAuthenticated(origin.scheme()))
     return true;
-  }
 
   // 8. If origin has been configured as a trustworthy origin, return
   //    "Potentially Trustworthy".
@@ -278,7 +288,15 @@ bool IsUrlPotentiallyTrustworthy(const GURL& url) {
   //    Note: The origin of blob: and filesystem: URLs is the origin of the
   //    context in which they were created. Therefore, blobs created in a
   //    trustworthy origin will themselves be potentially trustworthy.
-  return IsOriginPotentiallyTrustworthy(url::Origin::Create(url));
+  url::Origin origin = url::Origin::Create(url);
+  if (origin.opaque() && IsSchemeConsideredAuthenticated(url.scheme_piece())) {
+    // Authenticated schemes should be treated as trustworthy, even if they
+    // translate into an opaque origin (e.g. because some of them might also be
+    // registered as a no-access, like the //content-layer chrome-error:// or
+    // the //chrome-layer chrome-native://).
+    return true;
+  }
+  return IsOriginPotentiallyTrustworthy(origin);
 }
 
 // static

services/network/public/cpp/is_potentially_trustworthy_unittest.cc

@@ -66,7 +66,7 @@ TEST(IsPotentiallyTrustworthy, Url) {
   EXPECT_TRUE(IsUrlPotentiallyTrustworthy("about:srcdoc#ref"));
   EXPECT_TRUE(IsUrlPotentiallyTrustworthy("about:srcdoc?x=2#ref"));
 
-  EXPECT_FALSE(IsUrlPotentiallyTrustworthy("about:about"));
+  EXPECT_TRUE(IsUrlPotentiallyTrustworthy("about:mumble"));
 
   EXPECT_TRUE(IsUrlPotentiallyTrustworthy("data:test/plain;blah"));
   EXPECT_FALSE(IsUrlPotentiallyTrustworthy("javascript:alert('blah')"));
@@ -151,37 +151,39 @@ TEST(IsPotentiallyTrustworthy, Url) {
       IsUrlPotentiallyTrustworthy("quic-transport://example.com/counter"));
 }
 
-// Tests the trustworthiness of an URL and origin whose scheme was added to the
-// custom sets of standard and secure schemes. A scheme must be added to both
-// to be considered trustworthy.
-TEST(IsPotentiallyTrustworthy, CustomScheme) {
-  const char* custom_scheme = "custom-scheme";
-  const char* custom_scheme_example = "custom-scheme://example.com";
-
-  EXPECT_FALSE(IsOriginPotentiallyTrustworthy(custom_scheme_example));
-  EXPECT_FALSE(IsUrlPotentiallyTrustworthy(custom_scheme_example));
-
-  {
-    url::ScopedSchemeRegistryForTests scoped_registry;
-    url::AddSecureScheme(custom_scheme);
-    EXPECT_FALSE(IsOriginPotentiallyTrustworthy(custom_scheme_example));
-    EXPECT_FALSE(IsUrlPotentiallyTrustworthy(custom_scheme_example));
-  }
-
-  {
-    url::ScopedSchemeRegistryForTests scoped_registry;
-    url::AddStandardScheme(custom_scheme, url::SchemeType::SCHEME_WITH_HOST);
-    EXPECT_FALSE(IsOriginPotentiallyTrustworthy(custom_scheme_example));
-    EXPECT_FALSE(IsUrlPotentiallyTrustworthy(custom_scheme_example));
-  }
+TEST(IsPotentiallyTrustworthy, CustomSchemes) {
+  url::ScopedSchemeRegistryForTests scoped_registry;
+  url::AddSecureScheme("sec-nonstd-scheme");
+  url::AddSecureScheme("sec-std-scheme");
+  url::AddStandardScheme("sec-std-scheme", url::SCHEME_WITH_HOST);
+  url::AddSecureScheme("sec-noaccess-scheme");
+  url::AddNoAccessScheme("sec-noaccess-scheme");
+  url::AddNoAccessScheme("nonsec-noaccess-scheme");
+
+  // Unrecognized / unknown schemes are not trustworthy.
+  EXPECT_FALSE(IsOriginPotentiallyTrustworthy("unknown-scheme://example.com"));
+  EXPECT_FALSE(IsUrlPotentiallyTrustworthy("unknown-scheme://example.com"));
+
+  // Secure URLs are trustworthy, even if their scheme is also marked as
+  // no-access, or are not marked as standard.  See also //chrome-layer
+  // ChromeContentClientTest.AdditionalSchemes test and
+  // https://crbug.com/734581.
+  EXPECT_TRUE(IsUrlPotentiallyTrustworthy("sec-nonstd-scheme://blah/x.js"));
+  EXPECT_TRUE(IsUrlPotentiallyTrustworthy("sec-std-scheme://blah/x.js"));
+  EXPECT_TRUE(IsUrlPotentiallyTrustworthy("sec-noaccess-scheme://blah/x.js"));
+  EXPECT_TRUE(IsOriginPotentiallyTrustworthy("sec-std-scheme://blah/x.js"));
+  // No-access and non-standard/non-local schemes translate into an
+  // untrustworthy, opaque origin.
+  // TODO(lukasza): Maybe if the spec had a notion of an origin *precursor*,
+  // then it could inspect the scheme of the precursor.  After this, it may be
+  // possible to EXPECT_TRUE below...
+  EXPECT_FALSE(IsOriginPotentiallyTrustworthy("sec-nonstd-scheme://blah/x.js"));
+  EXPECT_FALSE(
+      IsOriginPotentiallyTrustworthy("sec-noaccess-scheme://blah/x.js"));
 
-  {
-    url::ScopedSchemeRegistryForTests scoped_registry;
-    url::AddStandardScheme(custom_scheme, url::SchemeType::SCHEME_WITH_HOST);
-    url::AddSecureScheme(custom_scheme);
-    EXPECT_TRUE(IsOriginPotentiallyTrustworthy(custom_scheme_example));
-    EXPECT_TRUE(IsUrlPotentiallyTrustworthy(custom_scheme_example));
-  }
+  // No-access, non-secure schemes are untrustworthy.
+  EXPECT_FALSE(IsUrlPotentiallyTrustworthy("nonsec-noaccess-scheme:blah"));
+  EXPECT_FALSE(IsOriginPotentiallyTrustworthy("nonsec-noaccess-scheme:blah"));
 }
 
 // Tests that were for the removed blink::network_utils::IsOriginSecure.

third_party/blink/renderer/platform/weborigin/security_origin.cc

@@ -88,6 +88,10 @@ KURL SecurityOrigin::ExtractInnerURL(const KURL& url) {
   return KURL(url.GetPath());
 }
 
+// Note: When changing ShouldTreatAsOpaqueOrigin, consider also updating
+// IsValidInput in //url/scheme_host_port.cc (there might be existing
+// differences in behavior between these 2 layers, but we should avoid
+// introducing new differences).
 static bool ShouldTreatAsOpaqueOrigin(const KURL& url) {
   if (!url.IsValid())
     return true;

third_party/blink/renderer/platform/weborigin/security_origin_test.cc

@@ -745,6 +745,7 @@ TEST_F(SecurityOriginTest, CanonicalizeHost) {
 
 TEST_F(SecurityOriginTest, UrlOriginConversions) {
   url::ScopedSchemeRegistryForTests scoped_registry;
+  url::AddNoAccessScheme("no-access");
   url::AddLocalScheme("nonstandard-but-local");
   struct TestCases {
     const char* const url;
@@ -780,6 +781,9 @@ TEST_F(SecurityOriginTest, UrlOriginConversions) {
       {"mailto:localhost/", "", "", 0, true},
       {"about:blank", "", "", 0, true},
 
+      // Custom no-access scheme.
+      {"no-access:blah", "", "", 0, true},
+
       // Registered URLs
       {"ftp://example.com/", "ftp", "example.com", 21},
       {"ws://example.com/", "ws", "example.com", 80},
@@ -958,6 +962,23 @@ TEST_F(SecurityOriginTest, ToTokenForFastCheck) {
   }
 }
 
+// See also OriginTest.ConstructFromGURL_OpaqueOrigin and
+// NavigationUrlRewriteBrowserTest.RewriteToNoAccess.
+TEST_F(SecurityOriginTest, StandardNoAccessScheme) {
+  url::ScopedSchemeRegistryForTests scoped_registry;
+  url::AddStandardScheme("std-no-access", url::SCHEME_WITH_HOST);
+  url::AddNoAccessScheme("std-no-access");
+  url::AddStandardScheme("std", url::SCHEME_WITH_HOST);
+
+  scoped_refptr<const SecurityOrigin> custom_standard_noaccess_origin =
+      SecurityOrigin::CreateFromString("std-no-access://host");
+  EXPECT_TRUE(custom_standard_noaccess_origin->IsOpaque());
+
+  scoped_refptr<const SecurityOrigin> custom_standard_origin =
+      SecurityOrigin::CreateFromString("std://host");
+  EXPECT_FALSE(custom_standard_origin->IsOpaque());
+}
+
 TEST_F(SecurityOriginTest, NonStandardScheme) {
   scoped_refptr<const SecurityOrigin> origin =
       SecurityOrigin::CreateFromString("cow://");

url/origin_unittest.cc

@@ -229,8 +229,14 @@ TEST_F(OriginTest, OpaqueOriginComparison) {
       "local-but-nonstandar:foo",  // Prefix of registered scheme.
       "but-nonstandard:foo",       // Suffix of registered scheme.
       "local-and-standard:",       // Standard scheme needs a hostname.
-      "standard-but-noaccess:",    // Standard scheme needs a hostname.
       "blob:blob:http://www.example.com/guid-goes-here",  // Double blob.
+
+      // Scheme (registered in SetUp()) that's standard but marked as noaccess.
+      // See also SecurityOriginTest.StandardNoAccessScheme and
+      // NavigationUrlRewriteBrowserTest.RewriteToNoAccess.
+      "standard-but-noaccess:",     // Standard scheme needs a hostname.
+      "standard-but-noaccess:foo",  // Standard scheme needs a hostname.
+      "standard-but-noaccess://bar",
   };
 
   for (auto* test_url : urls) {
@@ -349,12 +355,6 @@ TEST_F(OriginTest, ConstructFromGURL) {
       {"local-but-nonstandard://bar", "local-but-nonstandard", "", 0},
       {"also-local-but-nonstandard://bar", "also-local-but-nonstandard", "", 0},
 
-      // Scheme (registered in SetUp()) that's standard but marked as noaccess.
-      // url::Origin doesn't currently take the noaccess property into account,
-      // so these aren't expected to result in opaque origins.
-      {"standard-but-noaccess:foo", "standard-but-noaccess", "foo", 0},
-      {"standard-but-noaccess://bar", "standard-but-noaccess", "bar", 0},
-
       // file: URLs
       {"file:///etc/passwd", "file", "", 0},
       {"file://example.com/etc/passwd", "file", "example.com", 0},
@@ -814,10 +814,10 @@ TEST_F(OriginTest, CanBeDerivedFrom) {
       {"standard-but-noaccess://a.com/foo", &regular_origin, false},
       {"standard-but-noaccess://a.com/foo", &opaque_precursor_origin, false},
       {"standard-but-noaccess://a.com/foo", &opaque_unique_origin, true},
-      {"standard-but-noaccess://a.com/foo", &no_access_origin, false},
+      {"standard-but-noaccess://a.com/foo", &no_access_origin, true},
       {"standard-but-noaccess://a.com/foo", &no_access_opaque_precursor_origin,
-       false},
-      {"standard-but-noaccess://b.com/foo", &no_access_origin, false},
+       true},
+      {"standard-but-noaccess://b.com/foo", &no_access_origin, true},
       {"standard-but-noaccess://b.com/foo", &no_access_opaque_precursor_origin,
        true},
 

url/scheme_host_port.cc

@@ -49,6 +49,10 @@ bool IsCanonicalHost(const base::StringPiece& host) {
   return host == canon_host;
 }
 
+// Note: When changing IsValidInput, consider also updating
+// ShouldTreatAsOpaqueOrigin in Blink (there might be existing differences in
+// behavior between these 2 layers, but we should avoid introducing new
+// differences).
 bool IsValidInput(const base::StringPiece& scheme,
                   const base::StringPiece& host,
                   uint16_t port,
@@ -57,25 +61,27 @@ bool IsValidInput(const base::StringPiece& scheme,
   if (scheme.empty())
     return false;
 
+  // about:blank and other no-access schemes translate into an opaque origin.
+  // This helps consistency with ShouldTreatAsOpaqueOrigin in Blink.
+  if (base::Contains(GetNoAccessSchemes(), scheme))
+    return false;
+
   SchemeType scheme_type = SCHEME_WITH_HOST_PORT_AND_USER_INFORMATION;
   bool is_standard = GetStandardSchemeType(
       scheme.data(),
       Component(0, base::checked_cast<int>(scheme.length())),
       &scheme_type);
   if (!is_standard) {
-    // To be consistent with blink, local non-standard schemes are currently
-    // allowed to be tuple origins. Nonstandard schemes don't have hostnames,
-    // so their tuple is just ("protocol", "", 0).
+    // To be consistent with ShouldTreatAsOpaqueOrigin in Blink, local
+    // non-standard schemes are currently allowed to be tuple origins.
+    // Nonstandard schemes don't have hostnames, so their tuple is just
+    // ("protocol", "", 0).
     //
     // TODO: Migrate "content:" and "externalfile:" to be standard schemes, and
     // remove this local scheme exception.
     if (base::Contains(GetLocalSchemes(), scheme) && host.empty() && port == 0)
       return true;
 
-    // about:blank and other no-access schemes translate into an opaque origin.
-    if (base::Contains(GetNoAccessSchemes(), scheme))
-      return false;
-
     // Otherwise, allow non-standard schemes only if the Android WebView
     // workaround is enabled.
     return AllowNonStandardSchemesForAndroidWebView();