Commit 5fa11e54 authored by Frédéric Wang's avatar Frédéric Wang Committed by Chromium LUCI CQ

Treat data: URLs as potentially trustworthy

According to [1], URLs whose scheme is "data" should be treated as
potentially trustworthy. This CL performs the change to align with the
specification and remove unnecessary special handling in various
places of the code. This CL does not contain significant behavioral
changes, and has no web-facing API changes. However, an intent email
has been sent [2] and a chromestatus page created [3]. Owners of the
files from which network::IsUrlPotentiallyTrustworthy is called were
informed and indicated the change is fine:

- NetworkFetcherImpl::DownloadToFile: Not exposed to the web at all.
  Used by an automated software updater which checks with a hardcoded
  Google endpoint for updates to certain Chrome modules.
- IdpNetworkRequestManager::Create: This code has been implemented
  recently and is not released yet.
- content_security_policy.cc's UpgradeInsecureRequest: already exits
  early for non-HTTP schemes.
- security state's GetSecurityLevel: already exits early for data
  scheme.
- password_manager_ios_util.mm: data URLs are not supported
  on other platforms so it should be excluded on iOS too. This is
  already the case since only localhost hosts, file: and cryptographic
  schemes returns true.
- CookieChangeSubscription, CookieAccessDelegateImpl: should be fine
  because code excludes data schemes (on non-iOS platforms) as well as
  opaque origins (like data:). Moreover, data: doesn't go over http.
- InsecureInputTabHelper: used to include data schemes in the past [4],
  uploaded [5] to preserve current behavior.
- sec_header_helpers.cc: Only matters for HTTP requests.
- TouchToFillController: Removed in [6].
- RemoteCopyMessageHandler::HandleImage: This is guarded by
  IsImageSourceAllowed. kRemoteCopyAllowedOrigins, defaults to
  "https://googleusercontent.com" and having a real use case where
  someone adds a "data:" host seems unlikely.

[1] https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-url
[2] https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/Hb1-VLwq54Y
[3] https://chromestatus.com/feature/5634194258526208
[4] https://chromium-review.googlesource.com/c/chromium/src/+/1986072
[5] https://chromium-review.googlesource.com/c/chromium/src/+/2580067
[6] https://chromium-review.googlesource.com/c/chromium/src/+/2574743

Bug: 1119740, 1153336
Change-Id: I4db1b71ab0dc4d7a0635e8524a3757dc4388edf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2563683Reviewed-by: default avatarChristoph Schwering <schwering@google.com>
Reviewed-by: default avatarMike West <mkwst@chromium.org>
Reviewed-by: default avatarMatt Falkenhagen <falken@chromium.org>
Commit-Queue: Frédéric Wang <fwang@igalia.com>
Cr-Commit-Position: refs/heads/master@{#836025}
parent d2ffe0ff
......@@ -14,14 +14,9 @@ bool IsInsecureFormAction(const GURL& action_url) {
// to same-origin contexts, so they are not blocked. Some forms use
// javascript URLs to handle submissions in JS, those don't count as mixed
// content either.
// The data scheme is explicitly allowed in order to match blink's equivalent
// check, since IsUrlPotentiallyTrustworthy excludes it.
// TODO(https://crbug.com/1119740): Remove the check for "data" scheme when
// it is handled by network::IsUrlPotentiallyTrustworthy.
if (action_url.SchemeIs(url::kJavaScriptScheme) ||
action_url.SchemeIs(url::kBlobScheme) ||
action_url.SchemeIs(url::kFileSystemScheme) ||
action_url.SchemeIs(url::kDataScheme)) {
action_url.SchemeIs(url::kFileSystemScheme)) {
return false;
}
return !network::IsUrlPotentiallyTrustworthy(action_url);
......
......@@ -2335,14 +2335,7 @@ void NavigationRequest::OnResponseStarted(
if (base::FeatureList::IsEnabled(
network::features::kCrossOriginEmbedderPolicy)) {
const auto& url = common_params_->url;
// https://w3c.github.io/webappsec-secure-contexts/#is-url-trustworthy
// returns "Potentially Trustworthy" for data URLs, but
// network::IsUrlPotentiallyTrustworthy returns false, so we need this
// extra condition.
// TODO(https://crbug.com/1119740): Remove the check for "data" scheme when
// it is handled by network::IsUrlPotentiallyTrustworthy.
if (network::IsUrlPotentiallyTrustworthy(url) ||
url.SchemeIs(url::kDataScheme)) {
if (network::IsUrlPotentiallyTrustworthy(url)) {
// https://mikewest.github.io/corpp/#process-navigation-response
if (auto* const parent = GetParentFrame()) {
const auto& parent_coep = parent->cross_origin_embedder_policy();
......
......@@ -272,9 +272,8 @@ bool IsUrlPotentiallyTrustworthy(const GURL& url) {
return true;
// 2. If url’s scheme is "data", return "Potentially Trustworthy".
// TODO(https://crbug.com/1119740): The spec says we should return true here.
if (url.SchemeIs(url::kDataScheme))
return false;
return true;
// 3. Return the result of executing §3.2 Is origin potentially trustworthy?
// on url’s origin.
......
......@@ -64,8 +64,7 @@ TEST(IsPotentiallyTrustworthy, Url) {
EXPECT_FALSE(IsUrlPotentiallyTrustworthy("about:about"));
// TODO(https://crbug.com/1119740): Should return true for data: URLs.
EXPECT_FALSE(IsUrlPotentiallyTrustworthy("data:test/plain;blah"));
EXPECT_TRUE(IsUrlPotentiallyTrustworthy("data:test/plain;blah"));
EXPECT_FALSE(IsUrlPotentiallyTrustworthy("javascript:alert('blah')"));
EXPECT_TRUE(IsUrlPotentiallyTrustworthy("file:///test/fun.html"));
......
......@@ -38,16 +38,6 @@ bool IsURLHandledByNetworkService(const GURL& url) {
}
bool IsOriginSecure(const GURL& url) {
// TODO(https://crbug.com/1119740): data: URLs (and opaque origins associated
// with them) should be considered insecure according to
// https://www.w3.org/TR/powerful-features/#is-url-trustworthy.
// Unfortunately, changing this behavior of NetworkUtils::IsOriginSecure
// breaks quite a few tests for now (e.g. considering data: insecure makes us
// think that https + data = mixed content), so fixing this is postponed to a
// follow-up CL. WIP CL @ https://crrev.com/c/1505897.
if (url.SchemeIs(url::kDataScheme))
return true;
return network::IsUrlPotentiallyTrustworthy(url);
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment