[Background Fetch] Move mixed content checks to the controller.

Change-Id: Iff2fac58c58042d4ddf55ef962df61954414a1fd
Reviewed-on: https://chromium-review.googlesource.com/1230025
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595085}

chrome/browser/background_fetch/background_fetch_download_client.cc

@@ -31,7 +31,7 @@ BackgroundFetchFailureReason ToBackgroundFetchFailureReason(
     case download::Client::FailureReason::TIMEDOUT:
       return BackgroundFetchFailureReason::TIMEDOUT;
     case download::Client::FailureReason::UNKNOWN:
-      return BackgroundFetchFailureReason::UNKNOWN;
+      return BackgroundFetchFailureReason::FETCH_ERROR;
     case download::Client::FailureReason::ABORTED:
     case download::Client::FailureReason::CANCELLED:
       return BackgroundFetchFailureReason::CANCELLED;

content/browser/background_fetch/background_fetch_data_manager_unittest.cc

@@ -111,7 +111,7 @@ void AnnotateRequestInfoWithFakeDownloadManagerData(
     // Fill |request_info| with a failed result.
     request_info->SetResult(std::make_unique<BackgroundFetchResult>(
         std::move(response), base::Time::Now(),
-        BackgroundFetchResult::FailureReason::UNKNOWN));
+        BackgroundFetchResult::FailureReason::FETCH_ERROR));
     return;
   }
 

content/browser/background_fetch/background_fetch_job_controller.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "content/browser/background_fetch/background_fetch_job_controller.h"
+#include "content/public/common/origin_util.h"
 #include "third_party/blink/public/platform/modules/background_fetch/background_fetch.mojom.h"
 
 #include <utility>
@@ -74,6 +75,15 @@ bool BackgroundFetchJobController::HasMoreRequests() {
   return completed_downloads_ < total_downloads_;
 }
 
+bool BackgroundFetchJobController::IsMixedContent(
+    const BackgroundFetchRequestInfo& request) {
+  // Empty request is valid, it shouldn't fail the mixed content check.
+  if (request.fetch_request().url.is_empty())
+    return false;
+
+  return !IsOriginSecure(request.fetch_request().url);
+}
+
 void BackgroundFetchJobController::StartRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request,
     RequestFinishedCallback request_finished_callback) {
@@ -85,6 +95,15 @@ void BackgroundFetchJobController::StartRequest(
   active_request_downloaded_bytes_ = 0;
   active_request_finished_callback_ = std::move(request_finished_callback);
 
+  if (IsMixedContent(*request.get())) {
+    request->SetEmptyResultWithFailureReason(
+        BackgroundFetchResult::FailureReason::FETCH_ERROR);
+
+    ++completed_downloads_;
+    std::move(active_request_finished_callback_).Run(request);
+    return;
+  }
+
   delegate_proxy_->StartRequest(registration_id().unique_id(),
                                 registration_id().origin(), request);
 }

content/browser/background_fetch/background_fetch_job_controller.h

@@ -116,6 +116,13 @@ class CONTENT_EXPORT BackgroundFetchJobController final
       blink::mojom::BackgroundFetchFailureReason reason_to_abort) override;
 
  private:
+  // Performs mixed content checks on the |request| for Background Fetch.
+  // Background Fetch depends on Service Workers, which are restricted for use
+  // on secure origins. We can therefore assume that the registration's origin
+  // is secure. This test ensures that the origin for the url of every
+  // request is also secure.
+  bool IsMixedContent(const BackgroundFetchRequestInfo& request);
+
   // Options for the represented background fetch registration.
   BackgroundFetchOptions options_;
 

content/browser/background_fetch/background_fetch_job_controller_unittest.cc

@@ -78,6 +78,20 @@ class BackgroundFetchJobControllerTest : public BackgroundFetchTestBase {
     pending_requests_counts_[registration_id]--;
   }
 
+  // To be called when a request for |registration_id| has finished.
+  // Moves |request_info| to |out_request_info|.
+  void GetRequestInfoOnRequestFinished(
+      const BackgroundFetchRegistrationId& registration_id,
+      scoped_refptr<content::BackgroundFetchRequestInfo>* out_request_info,
+      scoped_refptr<content::BackgroundFetchRequestInfo> request_info) {
+    DCHECK(pending_requests_counts_.count(registration_id));
+    DCHECK(out_request_info);
+
+    EXPECT_GE(pending_requests_counts_[registration_id], 1);
+    pending_requests_counts_[registration_id]--;
+    *out_request_info = request_info;
+  }
+
   // Creates a new Background Fetch registration, whose id will be stored in the
   // |*registration_id|, and registers it with the DataManager for the included
   // |request_data|. If |auto_complete_requests| is true, the request will
@@ -238,6 +252,32 @@ TEST_F(BackgroundFetchJobControllerTest, SingleRequestJob) {
             GetCompletionStatus(registration_id));
 }
 
+TEST_F(BackgroundFetchJobControllerTest, SingleRequestJobWithInsecureOrigin) {
+  BackgroundFetchRegistrationId registration_id;
+
+  auto requests = CreateRegistrationForRequests(
+      &registration_id, {{GURL("http://example.com/funny_cat.png"), "GET"}},
+      true /* auto_complete_requests */);
+
+  EXPECT_EQ(JobCompletionStatus::kRunning,
+            GetCompletionStatus(registration_id));
+
+  std::unique_ptr<BackgroundFetchJobController> controller =
+      CreateJobController(registration_id, requests.size());
+
+  controller->StartRequest(
+      requests[0],
+      base::BindOnce(
+          &BackgroundFetchJobControllerTest::GetRequestInfoOnRequestFinished,
+          base::Unretained(this), registration_id, &requests[0]));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(JobCompletionStatus::kCompleted,
+            GetCompletionStatus(registration_id));
+  EXPECT_FALSE(requests[0]->IsResultSuccess());
+}
+
 TEST_F(BackgroundFetchJobControllerTest, MultipleRequestJob) {
   BackgroundFetchRegistrationId registration_id;
 
@@ -285,6 +325,46 @@ TEST_F(BackgroundFetchJobControllerTest, MultipleRequestJob) {
             GetCompletionStatus(registration_id));
 }
 
+TEST_F(BackgroundFetchJobControllerTest, MultipleRequestsJobWithMixedContent) {
+  BackgroundFetchRegistrationId registration_id;
+
+  auto requests = CreateRegistrationForRequests(
+      &registration_id,
+      {{GURL("http://example.com/funny_cat.png"), "GET"},
+       {GURL("https://example.com/scary_cat.png"), "GET"}},
+      true /* auto_complete_requests */);
+
+  EXPECT_EQ(JobCompletionStatus::kRunning,
+            GetCompletionStatus(registration_id));
+
+  std::unique_ptr<BackgroundFetchJobController> controller =
+      CreateJobController(registration_id, requests.size());
+
+  controller->StartRequest(
+      requests[0],
+      base::BindOnce(
+          &BackgroundFetchJobControllerTest::GetRequestInfoOnRequestFinished,
+          base::Unretained(this), registration_id, &requests[0]));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(JobCompletionStatus::kRunning,
+            GetCompletionStatus(registration_id));
+  EXPECT_FALSE(requests[0]->IsResultSuccess());
+
+  controller->StartRequest(
+      requests[1],
+      base::BindOnce(
+          &BackgroundFetchJobControllerTest::GetRequestInfoOnRequestFinished,
+          base::Unretained(this), registration_id, &requests[1]));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(JobCompletionStatus::kCompleted,
+            GetCompletionStatus(registration_id));
+  EXPECT_TRUE(requests[1]->IsResultSuccess());
+}
+
 TEST_F(BackgroundFetchJobControllerTest, Abort) {
   BackgroundFetchRegistrationId registration_id;
 

content/browser/background_fetch/background_fetch_request_info.cc

@@ -51,6 +51,14 @@ void BackgroundFetchRequestInfo::SetResult(
   PopulateWithResponse(std::move(result_->response));
 }
 
+void BackgroundFetchRequestInfo::SetEmptyResultWithFailureReason(
+    BackgroundFetchResult::FailureReason failure_reason) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  result_ = std::make_unique<BackgroundFetchResult>(
+      nullptr /* response */, base::Time::Now(), failure_reason);
+}
+
 void BackgroundFetchRequestInfo::PopulateWithResponse(
     std::unique_ptr<BackgroundFetchResponse> response) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);

content/browser/background_fetch/background_fetch_request_info.h

@@ -20,6 +20,7 @@
 #include "content/browser/background_fetch/background_fetch_constants.h"
 #include "content/common/content_export.h"
 #include "content/common/service_worker/service_worker_types.h"
+#include "content/public/browser/background_fetch_response.h"
 #include "url/gurl.h"
 
 namespace storage {
@@ -50,6 +51,11 @@ class CONTENT_EXPORT BackgroundFetchRequestInfo
 
   void SetResult(std::unique_ptr<BackgroundFetchResult> result);
 
+  // Creates an empty result, with no response, and assigns |failure_reason|
+  // as its failure_reason.
+  void SetEmptyResultWithFailureReason(
+      BackgroundFetchResult::FailureReason failure_reason);
+
   // Returns the index of this request within a Background Fetch registration.
   int request_index() const { return request_index_; }
 

content/browser/background_fetch/mock_background_fetch_delegate.cc

@@ -147,7 +147,7 @@ void MockBackgroundFetchDelegate::DownloadUrl(
         std::vector<GURL>({url}), test_response->headers);
     auto result = std::make_unique<BackgroundFetchResult>(
         std::move(response), base::Time::Now(),
-        BackgroundFetchResult::FailureReason::UNKNOWN);
+        BackgroundFetchResult::FailureReason::FETCH_ERROR);
     PostAbortCheckingTask(
         job_unique_id,
         base::BindOnce(&BackgroundFetchDelegate::Client::OnDownloadComplete,

content/public/browser/background_fetch_response.h

@@ -52,8 +52,9 @@ struct CONTENT_EXPORT BackgroundFetchResult {
     // Used when the download was cancelled by the user.
     CANCELLED,
 
-    // Used when the failure reason is unknown.
-    UNKNOWN,
+    // Catch-all error. Used when the failure reason is unknown or not exposed
+    // to the developer.
+    FETCH_ERROR,
   };
 
   // Constructor for failed downloads.

content/shell/browser/layout_test/layout_test_background_fetch_delegate.cc

@@ -103,7 +103,8 @@ class LayoutTestBackgroundFetchDelegate::LayoutTestBackgroundFetchDownloadClient
             content::BackgroundFetchResult::FailureReason::TIMEDOUT;
         break;
       case download::Client::FailureReason::UNKNOWN:
-        failure_reason = content::BackgroundFetchResult::FailureReason::UNKNOWN;
+        failure_reason =
+            content::BackgroundFetchResult::FailureReason::FETCH_ERROR;
         break;
       case download::Client::FailureReason::ABORTED:
       case download::Client::FailureReason::CANCELLED:

third_party/WebKit/LayoutTests/external/wpt/background-fetch/fetch.https.window-expected.txt

@@ -2,11 +2,13 @@ This is a testharness.js-based test.
 PASS Background Fetch requires an activated Service Worker
 PASS Argument verification is done for BackgroundFetchManager.fetch()
 PASS IDs must be unique among active Background Fetch registrations
+PASS Empty URL is OK.
 PASS Using Background Fetch to successfully fetch a single resource
 PASS Background Fetch that exceeds the quota throws a QuotaExceededError
 FAIL Fetches can have requests with duplicate URLs promise_test: Unhandled rejection with value: object "TypeError: Fetches with duplicate requests are not yet supported. Consider adding query params to make the requests unique. For updates check http://crbug.com/871174"
 FAIL Fetches can have requests with a body promise_test: Unhandled rejection with value: object "TypeError: Requests with a body are not yet supported. For updates check http://crbug.com/774054"
 PASS recordsAvailable is false after onbackgroundfetchsuccess finishes execution.
 PASS Using Background Fetch to fetch a non-existent resource should fail.
+PASS Fetches with mixed content should fail.
 Harness: the test ran to completion.
 

third_party/WebKit/LayoutTests/external/wpt/background-fetch/fetch.https.window.js

@@ -67,6 +67,21 @@ backgroundFetchTest(async (test, backgroundFetch) => {
 
 }, 'IDs must be unique among active Background Fetch registrations');
 
+backgroundFetchTest(async (test, backgroundFetch) => {
+  const registrationId = uniqueId();
+  const registration =
+    await backgroundFetch.fetch(registrationId, '');
+
+  assert_equals(registration.id, registrationId);
+
+  const {type, eventRegistration, results} = await getMessageFromServiceWorker();
+
+  assert_equals('backgroundfetchsuccess', type);
+  assert_equals(eventRegistration.result, 'success');
+  assert_equals(eventRegistration.failureReason, '');
+
+}, 'Empty URL is OK.');
+
 backgroundFetchTest(async (test, backgroundFetch) => {
   const registrationId = uniqueId();
   const registration =
@@ -190,4 +205,20 @@ backgroundFetchTest(async (test, backgroundFetch) => {
   assert_equals(eventRegistration.result, 'failure');
   assert_equals(eventRegistration.failureReason, 'bad-status');
 
-}, 'Using Background Fetch to fetch a non-existent resource should fail.');
\ No newline at end of file
+}, 'Using Background Fetch to fetch a non-existent resource should fail.');
+
+backgroundFetchTest(async (test, backgroundFetch) => {
+  const registration = await backgroundFetch.fetch(
+                         'my-id',
+                         ['https://example.com', 'http://example.com']);
+
+  const {type, eventRegistration, results} = await getMessageFromServiceWorker();
+
+  assert_equals('backgroundfetchfail', type);
+  assert_equals(eventRegistration.failureReason, 'fetch-error');
+
+  assert_equals(results.length, 2);
+  assert_true(results[0].url.includes('https://example.com'));
+  assert_equals(results[1].url, '');
+
+}, 'Fetches with mixed content should fail.');

third_party/WebKit/LayoutTests/external/wpt/background-fetch/mixed-content-and-allowed-schemes.https.window.js

@@ -30,34 +30,6 @@ backgroundFetchTest((t, bgFetch) => {
   return bgFetch.fetch(uniqueId(), 'http://localhost');
 }, 'localhost http: fetch should register ok');
 
-backgroundFetchTest((t, bgFetch) => {
-  return promise_rejects(t, new TypeError(),
-                         bgFetch.fetch(uniqueId(), 'http://example.com'));
-}, 'non-loopback http: fetch should reject');
-
-backgroundFetchTest((t, bgFetch) => {
-  return promise_rejects(t, new TypeError(),
-                         bgFetch.fetch(uniqueId(), 'http://192.0.2.0'));
-}, 'non-loopback IPv4 http: fetch should reject');
-
-backgroundFetchTest((t, bgFetch) => {
-  return promise_rejects(t, new TypeError(),
-                         bgFetch.fetch(uniqueId(), 'http://[2001:db8::1]'));
-}, 'non-loopback IPv6 http: fetch should reject');
-
-backgroundFetchTest((t, bgFetch) => {
-  return promise_rejects(t, new TypeError(),
-                         bgFetch.fetch(uniqueId(), ['https://example.com',
-                                                    'http://example.com']));
-}, 'https: and non-loopback http: fetch should reject');
-
-backgroundFetchTest((t, bgFetch) => {
-  return promise_rejects(t, new TypeError(),
-                         bgFetch.fetch(uniqueId(), ['http://example.com',
-                                                    'https://example.com']));
-}, 'non-loopback http: and https: fetch should reject');
-
-
 backgroundFetchTest((t, bgFetch) => {
   return promise_rejects(t, new TypeError(),
                          bgFetch.fetch(uniqueId(), 'wss:127.0.0.1'));

third_party/WebKit/LayoutTests/external/wpt/background-fetch/service_workers/sw.js

@@ -27,5 +27,3 @@ function handleBackgroundFetchUpdateEvent(event) {
 
 self.addEventListener('backgroundfetchsuccess', handleBackgroundFetchUpdateEvent);
 self.addEventListener('backgroundfetchfail', handleBackgroundFetchUpdateEvent);
-
-

third_party/blink/renderer/modules/background_fetch/background_fetch_manager.cc

@@ -14,7 +14,6 @@
 #include "third_party/blink/renderer/core/frame/csp/content_security_policy.h"
 #include "third_party/blink/renderer/core/frame/deprecation.h"
 #include "third_party/blink/renderer/core/frame/use_counter.h"
-#include "third_party/blink/renderer/core/loader/mixed_content_checker.h"
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_bridge.h"
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h"
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_options.h"
@@ -84,14 +83,6 @@ bool ShouldBlockScheme(const KURL& request_url) {
          !request_url.ProtocolIs(WTF::g_https_atom);
 }
 
-bool ShouldBlockMixedContent(ExecutionContext* execution_context,
-                             const KURL& request_url) {
-  // TODO(crbug.com/757441): Using MixedContentChecker::ShouldBlockFetch would
-  // log better metrics.
-  return MixedContentChecker::IsMixedContent(
-      execution_context->GetSecurityOrigin(), request_url);
-}
-
 bool ShouldBlockDanglingMarkup(const KURL& request_url) {
   // "If request's url's potentially-dangling-markup flag is set, and request's
   // url's scheme is an HTTP(S) scheme, then set response to a network error."
@@ -201,14 +192,6 @@ ScriptPromise BackgroundFetchManager::fetch(
   // the Download Service in the browser process can use it without having to
   // spin up a renderer process.
   for (const WebServiceWorkerRequest& web_request : web_requests) {
-    // TODO(crbug.com/757441): Decide whether to support upgrading requests to
-    // potentially secure URLs (https://w3c.github.io/webappsec-upgrade-
-    // insecure-requests/) and/or HSTS rewriting. Since this is a new API only
-    // exposed on Secure Contexts, and the Mixed Content check below will block
-    // any requests to insecure contexts, it'd be cleanest not to support it.
-    // Depends how closely compatible with Fetch we want to be. If support is
-    // added, make sure to report CSP violations before upgrading the URL.
-
     KURL request_url(web_request.Url());
 
     if (!request_url.IsValid()) {
@@ -225,7 +208,7 @@ ScriptPromise BackgroundFetchManager::fetch(
     }
 
     // Check this before mixed content, so that if mixed content is blocked by
-    // CSP they get a CSP warning rather than a mixed content warning.
+    // CSP they get a CSP warning rather than a mixed content failure.
     if (ShouldBlockDueToCSP(execution_context, request_url)) {
       return RejectWithTypeError(script_state, request_url,
                                  "it violates the Content Security Policy");
@@ -247,13 +230,6 @@ ScriptPromise BackgroundFetchManager::fetch(
                                  "for loopback IPs");
     }
 
-    // Blocking fetches due to mixed content is done after Content Security
-    // Policy to prioritize warnings caused by the latter.
-    if (ShouldBlockMixedContent(execution_context, request_url)) {
-      return RejectWithTypeError(script_state, request_url,
-                                 "it is insecure; use https instead");
-    }
-
     if (ShouldBlockDanglingMarkup(request_url)) {
       return RejectWithTypeError(script_state, request_url,
                                  "it contains dangling markup");