service worker: Do allowed origin check on StartWorker.

We were only doing this check at registration time previously, which is
sketchy since the browser or content settings could change in the
meantime after registration.

Actually the motivating example was if you register
an extension service worker once and then run Chrome with
--disable-extensions and then try to start the worker via
chrome://serviceworker-internals. It turns out the checks for
JavaScript and cookies in
ChromeContentBrowserClient::AllowServiceWorker were disallowing
start worker in that case, but it seems fragile to rely on those.

Change-Id: Iea3706ccf08ec72ed9e2306b01a6ddfd0fa3b9eb
Reviewed-on: https://chromium-review.googlesource.com/1027171Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553483}

content/browser/service_worker/service_worker_context_unittest.cc

@@ -200,8 +200,8 @@ class RecordableEmbeddedWorkerInstanceClient
 
 // Make sure basic registration is working.
 TEST_F(ServiceWorkerContextTest, Register) {
-  GURL pattern("http://www.example.com/");
-  GURL script_url("http://www.example.com/service_worker.js");
+  GURL pattern("https://www.example.com/");
+  GURL script_url("https://www.example.com/service_worker.js");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
@@ -245,8 +245,8 @@ TEST_F(ServiceWorkerContextTest, Register) {
 // registration callback should indicate success, but there should be no waiting
 // or active worker in the registration.
 TEST_F(ServiceWorkerContextTest, Register_RejectInstall) {
-  GURL pattern("http://www.example.com/");
-  GURL script_url("http://www.example.com/service_worker.js");
+  GURL pattern("https://www.example.com/");
+  GURL script_url("https://www.example.com/service_worker.js");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
@@ -293,8 +293,8 @@ TEST_F(ServiceWorkerContextTest, Register_RejectInstall) {
 // Test registration when the service worker rejects the activate event. The
 // worker should be activated anyway.
 TEST_F(ServiceWorkerContextTest, Register_RejectActivate) {
-  GURL pattern("http://www.example.com/");
-  GURL script_url("http://www.example.com/service_worker.js");
+  GURL pattern("https://www.example.com/");
+  GURL script_url("https://www.example.com/service_worker.js");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
@@ -340,14 +340,14 @@ TEST_F(ServiceWorkerContextTest, Register_RejectActivate) {
 
 // Make sure registrations are cleaned up when they are unregistered.
 TEST_F(ServiceWorkerContextTest, Unregister) {
-  GURL pattern("http://www.example.com/");
+  GURL pattern("https://www.example.com/");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
   bool called = false;
   int64_t registration_id = blink::mojom::kInvalidServiceWorkerRegistrationId;
   context()->RegisterServiceWorker(
-      GURL("http://www.example.com/service_worker.js"), options,
+      GURL("https://www.example.com/service_worker.js"), options,
       MakeRegisteredCallback(&called, &registration_id));
 
   ASSERT_FALSE(called);
@@ -380,10 +380,10 @@ TEST_F(ServiceWorkerContextTest, Unregister) {
 
 // Make sure registrations are cleaned up when they are unregistered in bulk.
 TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
-  GURL origin1_p1("http://www.example.com/test");
-  GURL origin1_p2("http://www.example.com/hello");
-  GURL origin2_p1("http://www.example.com:8080/again");
-  GURL origin3_p1("http://www.other.com/");
+  GURL origin1_p1("https://www.example.com/test");
+  GURL origin1_p2("https://www.example.com/hello");
+  GURL origin2_p1("https://www.example.com:8080/again");
+  GURL origin3_p1("https://www.other.com/");
   int64_t registration_id1 = blink::mojom::kInvalidServiceWorkerRegistrationId;
   int64_t registration_id2 = blink::mojom::kInvalidServiceWorkerRegistrationId;
   int64_t registration_id3 = blink::mojom::kInvalidServiceWorkerRegistrationId;
@@ -394,7 +394,7 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
     blink::mojom::ServiceWorkerRegistrationOptions options;
     options.scope = origin1_p1;
     context()->RegisterServiceWorker(
-        GURL("http://www.example.com/service_worker.js"), options,
+        GURL("https://www.example.com/service_worker.js"), options,
         MakeRegisteredCallback(&called, &registration_id1));
     ASSERT_FALSE(called);
     base::RunLoop().RunUntilIdle();
@@ -406,7 +406,7 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
     blink::mojom::ServiceWorkerRegistrationOptions options;
     options.scope = origin1_p2;
     context()->RegisterServiceWorker(
-        GURL("http://www.example.com/service_worker2.js"), options,
+        GURL("https://www.example.com/service_worker2.js"), options,
         MakeRegisteredCallback(&called, &registration_id2));
     ASSERT_FALSE(called);
     base::RunLoop().RunUntilIdle();
@@ -418,7 +418,7 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
     blink::mojom::ServiceWorkerRegistrationOptions options;
     options.scope = origin2_p1;
     context()->RegisterServiceWorker(
-        GURL("http://www.example.com:8080/service_worker3.js"), options,
+        GURL("https://www.example.com:8080/service_worker3.js"), options,
         MakeRegisteredCallback(&called, &registration_id3));
     ASSERT_FALSE(called);
     base::RunLoop().RunUntilIdle();
@@ -430,7 +430,7 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
     blink::mojom::ServiceWorkerRegistrationOptions options;
     options.scope = origin3_p1;
     context()->RegisterServiceWorker(
-        GURL("http://www.other.com/service_worker4.js"), options,
+        GURL("https://www.other.com/service_worker4.js"), options,
         MakeRegisteredCallback(&called, &registration_id4));
     ASSERT_FALSE(called);
     base::RunLoop().RunUntilIdle();
@@ -497,7 +497,7 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
 
 // Make sure registering a new script shares an existing registration.
 TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
-  GURL pattern("http://www.example.com/");
+  GURL pattern("https://www.example.com/");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
@@ -505,7 +505,7 @@ TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
   int64_t old_registration_id =
       blink::mojom::kInvalidServiceWorkerRegistrationId;
   context()->RegisterServiceWorker(
-      GURL("http://www.example.com/service_worker.js"), options,
+      GURL("https://www.example.com/service_worker.js"), options,
       MakeRegisteredCallback(&called, &old_registration_id));
 
   ASSERT_FALSE(called);
@@ -518,7 +518,7 @@ TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
   int64_t new_registration_id =
       blink::mojom::kInvalidServiceWorkerRegistrationId;
   context()->RegisterServiceWorker(
-      GURL("http://www.example.com/service_worker_new.js"), options,
+      GURL("https://www.example.com/service_worker_new.js"), options,
       MakeRegisteredCallback(&called, &new_registration_id));
 
   ASSERT_FALSE(called);
@@ -541,8 +541,8 @@ TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
 // Make sure that when registering a duplicate pattern+script_url
 // combination, that the same registration is used.
 TEST_F(ServiceWorkerContextTest, RegisterDuplicateScript) {
-  GURL pattern("http://www.example.com/");
-  GURL script_url("http://www.example.com/service_worker.js");
+  GURL pattern("https://www.example.com/");
+  GURL script_url("https://www.example.com/service_worker.js");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 
@@ -583,8 +583,8 @@ TEST_F(ServiceWorkerContextTest, RegisterDuplicateScript) {
 TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
   const int kRenderProcessId1 = 1;
   const int kRenderProcessId2 = 2;
-  const GURL kOrigin1 = GURL("http://www.example.com/");
-  const GURL kOrigin2 = GURL("https://www.example.com/");
+  const GURL kOrigin1 = GURL("https://www.example.com/");
+  const GURL kOrigin2 = GURL("https://another-origin.example.net/");
   int provider_id = 1;
   std::vector<ServiceWorkerRemoteProviderEndpoint> remote_endpoints;
 
@@ -617,14 +617,15 @@ TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
   // CreateProviderHostForServiceWorkerContext, the provider_id is not a fixed
   // number.
   blink::mojom::ServiceWorkerRegistrationOptions registration_opt;
-  registration_opt.scope = GURL("http://www.example.com/test/");
+  registration_opt.scope = GURL("https://another-origin.example.net/test/");
   scoped_refptr<ServiceWorkerRegistration> registration =
       base::MakeRefCounted<ServiceWorkerRegistration>(
           registration_opt, 1L /* registration_id */,
           helper_->context()->AsWeakPtr());
   scoped_refptr<ServiceWorkerVersion> version =
       base::MakeRefCounted<ServiceWorkerVersion>(
-          registration.get(), GURL("http://www.example.com/test/script_url"),
+          registration.get(),
+          GURL("https://another-origin.example.net/test/script_url"),
           1L /* version_id */, helper_->context()->AsWeakPtr());
   // CreateProviderHostForServiceWorkerContext calls
   // ServiceWorkerProviderHost::CompleteStartWorkerPreparation, which requires a
@@ -685,8 +686,8 @@ class ServiceWorkerContextRecoveryTest
 };
 
 TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
-  GURL pattern("http://www.example.com/");
-  GURL script_url("http://www.example.com/service_worker.js");
+  GURL pattern("https://www.example.com/");
+  GURL script_url("https://www.example.com/service_worker.js");
   blink::mojom::ServiceWorkerRegistrationOptions options;
   options.scope = pattern;
 

content/browser/service_worker/service_worker_dispatcher_host_unittest.cc

@@ -251,8 +251,8 @@ TEST_F(ServiceWorkerDispatcherHostTest, ProviderCreatedAndDestroyed) {
 }
 
 TEST_F(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash) {
-  GURL pattern = GURL("http://www.example.com/");
-  GURL script_url = GURL("http://www.example.com/service_worker.js");
+  GURL pattern = GURL("https://www.example.com/");
+  GURL script_url = GURL("https://www.example.com/service_worker.js");
   int process_id = helper_->mock_render_process_id();
 
   SendProviderCreated(blink::mojom::ServiceWorkerProviderType::kForWindow,

content/browser/service_worker/service_worker_version.cc

@@ -504,15 +504,7 @@ void ServiceWorkerVersion::StartWorker(ServiceWorkerMetrics::EventType purpose,
         base::BindOnce(std::move(callback), SERVICE_WORKER_ERROR_REDUNDANT));
     return;
   }
-
-  // Check that the worker is allowed to start on the given scope. Since this
-  // worker might not be used for a specific tab, pass a null callback as
-  // WebContents getter.
-  // resource_context() can return null in unit tests.
-  if (context_->wrapper()->resource_context() &&
-      !GetContentClient()->browser()->AllowServiceWorker(
-          scope_, scope_, context_->wrapper()->resource_context(),
-          base::Callback<WebContents*(void)>())) {
+  if (!IsStartWorkerAllowed()) {
     RecordStartWorkerResult(purpose, status_, kInvalidTraceId,
                             is_browser_startup_complete,
                             SERVICE_WORKER_ERROR_DISALLOWED);
@@ -1991,4 +1983,28 @@ void ServiceWorkerVersion::OnNoWorkInBrowser() {
   idle_timer_fired_in_renderer_ = false;
 }
 
+bool ServiceWorkerVersion::IsStartWorkerAllowed() const {
+  // Check that the worker is allowed on this origin. It's possible a
+  // worker was previously allowed and installed, but later the embedder's
+  // policy or binary changed to disallow this origin.
+  if (!ServiceWorkerUtils::AllOriginsMatchAndCanAccessServiceWorkers(
+          {script_url_})) {
+    return false;
+  }
+
+  // Check that the worker is allowed on the given scope. It's possible a worker
+  // was previously allowed and installed, but later content settings changed to
+  // disallow this scope. Since this worker might not be used for a specific
+  // tab, pass a null callback as WebContents getter.
+  // resource_context() can return null in unit tests.
+  if ((context_->wrapper()->resource_context() &&
+       !GetContentClient()->browser()->AllowServiceWorker(
+           scope_, scope_, context_->wrapper()->resource_context(),
+           base::Callback<WebContents*(void)>()))) {
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_version.h

@@ -722,6 +722,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // has been fired or the worker has been stopped.
   void OnNoWorkInBrowser();
 
+  bool IsStartWorkerAllowed() const;
+
   const int64_t version_id_;
   const int64_t registration_id_;
   const GURL script_url_;

content/browser/service_worker/service_worker_version_unittest.cc

@@ -177,7 +177,8 @@ class ServiceWorkerVersionTest : public testing::Test {
     blink::mojom::ServiceWorkerRegistrationOptions options;
     options.scope = pattern_;
     registration_ = new ServiceWorkerRegistration(
-        options, 1L, helper_->context()->AsWeakPtr());
+        options, helper_->context()->storage()->NewRegistrationId(),
+        helper_->context()->AsWeakPtr());
     version_ = new ServiceWorkerVersion(
         registration_.get(),
         GURL("https://www.example.com/test/service_worker.js"),
@@ -1266,6 +1267,26 @@ TEST_F(ServiceWorkerVersionTest, RendererCrashDuringEvent) {
                                        base::Time::Now()));
 }
 
+// Test starting a service worker from a disallowed origin.
+TEST_F(ServiceWorkerVersionTest, BadOrigin) {
+  const GURL scope("bad-origin://www.example.com/test/");
+  blink::mojom::ServiceWorkerRegistrationOptions options;
+  options.scope = scope;
+  auto registration = base::MakeRefCounted<ServiceWorkerRegistration>(
+      options, helper_->context()->storage()->NewRegistrationId(),
+      helper_->context()->AsWeakPtr());
+  auto version = base::MakeRefCounted<ServiceWorkerVersion>(
+      registration_.get(),
+      GURL("bad-origin://www.example.com/test/service_worker.js"),
+      helper_->context()->storage()->NewVersionId(),
+      helper_->context()->AsWeakPtr());
+  ServiceWorkerStatusCode status = SERVICE_WORKER_OK;
+  version->StartWorker(ServiceWorkerMetrics::EventType::UNKNOWN,
+                       CreateReceiverOnCurrentThread(&status));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(SERVICE_WORKER_ERROR_DISALLOWED, status);
+}
+
 TEST_F(ServiceWorkerFailToStartTest, FailingWorkerUsesNewRendererProcess) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
   ServiceWorkerContextCore* context = helper_->context();