[ServiceWorker] Remove |source_origin| parameter of...

[ServiceWorker] Remove |source_origin| parameter of ServiceWorkerObjectHost.PostMessageToServiceWorker()

This CL lets browser-side get the source origin of the posted message
correctly by itself rather than relying on the |source_origin| passed
from renderer-side.
This is for security consideration, because browser-side can not trust
renderer-side to tell the truth.

BUG=772713

Change-Id: Ifd8d92a6bb150de245feb6a4ef7f0efb73c28119
Reviewed-on: https://chromium-review.googlesource.com/954983
Commit-Queue: Han Leon <leon.han@intel.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542761}

content/browser/service_worker/service_worker_handle.cc

@@ -175,6 +175,7 @@ ServiceWorkerHandle::ServiceWorkerHandle(
     : dispatcher_host_(dispatcher_host),
       context_(context),
       provider_host_(provider_host),
+      provider_origin_(url::Origin::Create(provider_host->document_url())),
       provider_id_(provider_host->provider_id()),
       handle_id_(context->GetNewServiceWorkerHandleId()),
       version_(version),
@@ -223,16 +224,14 @@ void ServiceWorkerHandle::RegisterIntoDispatcherHost(
 }
 
 void ServiceWorkerHandle::PostMessageToServiceWorker(
-    ::blink::TransferableMessage message,
-    const url::Origin& source_origin) {
+    ::blink::TransferableMessage message) {
   // When this method is called the encoded_message inside message could just
   // point to the IPC message's buffer. But that buffer can become invalid
   // before the message is passed on to the service worker, so make sure
   // message owns its data.
   message.EnsureDataIsOwned();
 
-  DispatchExtendableMessageEvent(std::move(message), source_origin,
-                                 base::DoNothing());
+  DispatchExtendableMessageEvent(std::move(message), base::DoNothing());
 }
 
 void ServiceWorkerHandle::TerminateForTesting(
@@ -242,18 +241,19 @@ void ServiceWorkerHandle::TerminateForTesting(
 
 void ServiceWorkerHandle::DispatchExtendableMessageEvent(
     ::blink::TransferableMessage message,
-    const url::Origin& source_origin,
     StatusCallback callback) {
   if (!context_ || !provider_host_) {
     std::move(callback).Run(SERVICE_WORKER_ERROR_FAILED);
     return;
   }
+  DCHECK_EQ(provider_origin_,
+            url::Origin::Create(provider_host_->document_url()));
   switch (provider_host_->provider_type()) {
     case blink::mojom::ServiceWorkerProviderType::kForWindow:
       service_worker_client_utils::GetClient(
           provider_host_.get(),
           base::BindOnce(&DispatchExtendableMessageEventFromClient, version_,
-                         std::move(message), source_origin,
+                         std::move(message), provider_origin_,
                          std::move(callback)));
       return;
     case blink::mojom::ServiceWorkerProviderType::kForServiceWorker: {
@@ -265,7 +265,7 @@ void ServiceWorkerHandle::DispatchExtendableMessageEvent(
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
           base::BindOnce(&DispatchExtendableMessageEventFromServiceWorker,
-                         version_, std::move(message), source_origin,
+                         version_, std::move(message), provider_origin_,
                          base::make_optional(timeout), std::move(callback),
                          provider_host_));
       return;

content/browser/service_worker/service_worker_handle.h

@@ -15,10 +15,7 @@
 #include "content/common/service_worker/service_worker_types.h"
 #include "mojo/public/cpp/bindings/associated_binding_set.h"
 #include "third_party/WebKit/public/mojom/service_worker/service_worker_object.mojom.h"
-
-namespace url {
-class Origin;
-}  // namespace url
+#include "url/origin.h"
 
 namespace content {
 
@@ -83,13 +80,15 @@ class CONTENT_EXPORT ServiceWorkerHandle
                       ServiceWorkerVersion* version);
 
   // Implements blink::mojom::ServiceWorkerObjectHost.
-  void PostMessageToServiceWorker(::blink::TransferableMessage message,
-                                  const url::Origin& source_origin) override;
+  void PostMessageToServiceWorker(
+      ::blink::TransferableMessage message) override;
   void TerminateForTesting(TerminateForTestingCallback callback) override;
 
+  // TODO(leonhsl): Remove |callback| parameter because it's just for unit tests
+  // and production code does not use it. We need to figure out another way to
+  // observe the dispatch result in unit tests.
   void DispatchExtendableMessageEvent(
       ::blink::TransferableMessage message,
-      const url::Origin& source_origin,
       base::OnceCallback<void(ServiceWorkerStatusCode)> callback);
 
   base::WeakPtr<ServiceWorkerHandle> AsWeakPtr();
@@ -103,6 +102,11 @@ class CONTENT_EXPORT ServiceWorkerHandle
   ServiceWorkerDispatcherHost* dispatcher_host_;
   base::WeakPtr<ServiceWorkerContextCore> context_;
   base::WeakPtr<ServiceWorkerProviderHost> provider_host_;
+  // The origin of the |provider_host_|. Note that this is const because once a
+  // JavaScript ServiceWorker object is created for an execution context, we
+  // don't expect that context to change origins and still hold on to the
+  // object.
+  const url::Origin provider_origin_;
   const int provider_id_;
   const int handle_id_;
   scoped_refptr<ServiceWorkerVersion> version_;

content/browser/service_worker/service_worker_handle_unittest.cc

@@ -183,9 +183,8 @@ class ServiceWorkerHandleTest : public testing::Test {
   void CallDispatchExtendableMessageEvent(
       ServiceWorkerHandle* handle,
       ::blink::TransferableMessage message,
-      const url::Origin& source_origin,
       base::OnceCallback<void(ServiceWorkerStatusCode)> callback) {
-    handle->DispatchExtendableMessageEvent(std::move(message), source_origin,
+    handle->DispatchExtendableMessageEvent(std::move(message),
                                            std::move(callback));
   }
 
@@ -302,7 +301,6 @@ TEST_F(ServiceWorkerHandleTest,
   status = SERVICE_WORKER_ERROR_MAX_VALUE;
   CallDispatchExtendableMessageEvent(
       sender_worker_handle.get(), std::move(message),
-      url::Origin::Create(version_->scope().GetOrigin()),
       base::BindOnce(&SaveStatusCallback, &called, &status));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);
@@ -349,6 +347,7 @@ TEST_F(ServiceWorkerHandleTest, DispatchExtendableMessageEvent_FromClient) {
       ServiceWorkerProviderHost::Create(
           frame_host->GetProcess()->GetID(), std::move(provider_host_info),
           helper_->context()->AsWeakPtr(), dispatcher_host_->AsWeakPtr());
+  provider_host->SetDocumentUrl(pattern);
   // Prepare a ServiceWorkerHandle for the above |provider_host|.
   blink::mojom::ServiceWorkerObjectInfoPtr info;
   // ServiceWorkerHandle lifetime is controlled by |info| and is also owned by
@@ -364,7 +363,6 @@ TEST_F(ServiceWorkerHandleTest, DispatchExtendableMessageEvent_FromClient) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
   CallDispatchExtendableMessageEvent(
       handle.get(), std::move(message),
-      url::Origin::Create(version_->scope().GetOrigin()),
       base::BindOnce(&SaveStatusCallback, &called, &status));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);
@@ -406,6 +404,7 @@ TEST_F(ServiceWorkerHandleTest, DispatchExtendableMessageEvent_Fail) {
       ServiceWorkerProviderHost::Create(
           frame_host->GetProcess()->GetID(), std::move(provider_host_info),
           helper_->context()->AsWeakPtr(), dispatcher_host_->AsWeakPtr());
+  provider_host->SetDocumentUrl(pattern);
   // Prepare a ServiceWorkerHandle for the above |provider_host|.
   blink::mojom::ServiceWorkerObjectInfoPtr info;
   // ServiceWorkerHandle lifetime is controlled by |info| and is also owned by
@@ -422,7 +421,6 @@ TEST_F(ServiceWorkerHandleTest, DispatchExtendableMessageEvent_Fail) {
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
   CallDispatchExtendableMessageEvent(
       handle.get(), std::move(message),
-      url::Origin::Create(version_->scope().GetOrigin()),
       base::BindOnce(&SaveStatusCallback, &called, &status));
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);

content/browser/service_worker/service_worker_provider_host.cc

@@ -643,6 +643,7 @@ ServiceWorkerProviderHost::CompleteStartWorkerPreparation(
   DCHECK(dispatcher_host);
   render_process_id_ = process_id;
   dispatcher_host_ = dispatcher_host->AsWeakPtr();
+  SetDocumentUrl(running_hosted_version()->script_url());
 
   // Retrieve the registration associated with |version|. The registration
   // must be alive because the version keeps it during starting worker.
@@ -677,10 +678,6 @@ ServiceWorkerProviderHost::CompleteStartWorkerPreparation(
       mojom::kNavigation_ServiceWorkerSpec, process_id,
       mojo::MakeRequest(&provider_info->interface_provider)));
 
-  // Set the document URL to the script url in order to allow
-  // register/unregister/getRegistration on ServiceWorkerGlobalScope.
-  SetDocumentUrl(running_hosted_version()->script_url());
-
   return provider_info;
 }
 

content/browser/service_worker/service_worker_provider_host.h

@@ -232,6 +232,12 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // Sets the |document_url_|.  When this object is for a client,
   // |matching_registrations_| gets also updated to ensure that |document_url_|
   // is in scope of all |matching_registrations_|.
+  // |document_url_| is the service worker script URL if this is hosting a
+  // running service worker, it will be used when creating ServiceWorkerHandle
+  // or handling ServiceWorkerRegistration#{*} calls etc.
+  // TODO(leonhsl): We should rename |document_url_| to something more
+  // appropriate and/or split this class into one for clients vs one for service
+  // workers.
   void SetDocumentUrl(const GURL& url);
   const GURL& document_url() const { return document_url_; }
 

content/renderer/service_worker/service_worker_dispatcher_unittest.cc

@@ -38,8 +38,8 @@ class MockServiceWorkerObjectHost
 
  private:
   // Implements blink::mojom::ServiceWorkerObjectHost.
-  void PostMessageToServiceWorker(::blink::TransferableMessage message,
-                                  const url::Origin& source_origin) override {
+  void PostMessageToServiceWorker(
+      ::blink::TransferableMessage message) override {
     NOTREACHED();
   }
   void TerminateForTesting(TerminateForTestingCallback callback) override {

content/renderer/service_worker/service_worker_provider_context_unittest.cc

@@ -61,8 +61,8 @@ class MockServiceWorkerObjectHost
 
  private:
   // Implements blink::mojom::ServiceWorkerObjectHost.
-  void PostMessageToServiceWorker(::blink::TransferableMessage message,
-                                  const url::Origin& source_origin) override {
+  void PostMessageToServiceWorker(
+      ::blink::TransferableMessage message) override {
     NOTREACHED();
   }
   void TerminateForTesting(TerminateForTestingCallback callback) override {

content/renderer/service_worker/web_service_worker_impl.cc

@@ -13,11 +13,9 @@
 #include "content/renderer/service_worker/service_worker_dispatcher.h"
 #include "content/renderer/service_worker/web_service_worker_provider_impl.h"
 #include "third_party/WebKit/public/platform/WebRuntimeFeatures.h"
-#include "third_party/WebKit/public/platform/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerProxy.h"
 
-using blink::WebSecurityOrigin;
 using blink::WebString;
 
 namespace content {
@@ -98,10 +96,8 @@ blink::mojom::ServiceWorkerState WebServiceWorkerImpl::GetState() const {
 }
 
 void WebServiceWorkerImpl::PostMessageToServiceWorker(
-    blink::TransferableMessage message,
-    const WebSecurityOrigin& source_origin) {
-  GetObjectHost()->PostMessageToServiceWorker(std::move(message),
-                                              url::Origin(source_origin));
+    blink::TransferableMessage message) {
+  GetObjectHost()->PostMessageToServiceWorker(std::move(message));
 }
 
 void WebServiceWorkerImpl::TerminateForTesting(

content/renderer/service_worker/web_service_worker_impl.h

@@ -59,9 +59,7 @@ class CONTENT_EXPORT WebServiceWorkerImpl
   blink::WebServiceWorkerProxy* Proxy() override;
   blink::WebURL Url() const override;
   blink::mojom::ServiceWorkerState GetState() const override;
-  void PostMessageToServiceWorker(
-      blink::TransferableMessage message,
-      const blink::WebSecurityOrigin& source_origin) override;
+  void PostMessageToServiceWorker(blink::TransferableMessage message) override;
   void TerminateForTesting(
       std::unique_ptr<TerminateForTestingCallback> callback) override;
 

third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp

@@ -41,7 +41,6 @@
 #include "modules/EventTargetModules.h"
 #include "modules/serviceworkers/ServiceWorkerContainerClient.h"
 #include "platform/bindings/ScriptState.h"
-#include "public/platform/WebSecurityOrigin.h"
 #include "public/platform/WebString.h"
 #include "third_party/WebKit/public/mojom/service_worker/service_worker_state.mojom-blink.h"
 
@@ -79,8 +78,7 @@ void ServiceWorker::postMessage(ScriptState* script_state,
   }
 
   handle_->ServiceWorker()->PostMessageToServiceWorker(
-      ToTransferableMessage(std::move(msg)),
-      WebSecurityOrigin(GetExecutionContext()->GetSecurityOrigin()));
+      ToTransferableMessage(std::move(msg)));
 }
 
 ScriptPromise ServiceWorker::InternalsTerminate(ScriptState* script_state) {

third_party/WebKit/public/mojom/BUILD.gn

@@ -73,7 +73,6 @@ mojom("mojom_core") {
     ":mojom_platform",
     "//skia/public/interfaces",
     "//url/mojom:url_mojom_gurl",
-    "//url/mojom:url_mojom_origin",
   ]
 
   overridden_deps_blink = [ ":mojom_platform" ]

third_party/WebKit/public/mojom/service_worker/service_worker_object.mojom

@@ -6,7 +6,6 @@ module blink.mojom;
 
 import "third_party/WebKit/public/mojom/message_port/message_port.mojom";
 import "third_party/WebKit/public/mojom/service_worker/service_worker_state.mojom";
-import "url/mojom/origin.mojom";
 import "url/mojom/url.mojom";
 
 const int32 kInvalidServiceWorkerHandleId = -1;
@@ -38,13 +37,7 @@ struct ServiceWorkerObjectInfo {
 // implement ServiceWorker methods.
 interface ServiceWorkerObjectHost {
   // Corresponds to ServiceWorker#postMessage().
-  // TODO(leonhsl): Remove |source_origin| by having the browser process keep
-  // track of the origin of this endpoint at binding time. The original legacy
-  // IPC code this is converted from sent the origin in the IPC message, so it
-  // was kept as a convenience during migration to Mojo, but we should remove it
-  // now.
-  PostMessageToServiceWorker(TransferableMessage message,
-                             url.mojom.Origin source_origin);
+  PostMessageToServiceWorker(TransferableMessage message);
 
   // Tells the browser process to terminate the service worker. Used in layout
   // tests to verify behavior when a service worker isn't running. The callback

third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorker.h

@@ -41,7 +41,6 @@
 
 namespace blink {
 
-class WebSecurityOrigin;
 class WebServiceWorkerProxy;
 
 class WebServiceWorker {
@@ -70,8 +69,7 @@ class WebServiceWorker {
     return mojom::ServiceWorkerState::kUnknown;
   }
 
-  virtual void PostMessageToServiceWorker(TransferableMessage,
-                                          const WebSecurityOrigin&) = 0;
+  virtual void PostMessageToServiceWorker(TransferableMessage) = 0;
 
   using TerminateForTestingCallback = WebCallbacks<void, void>;
   virtual void TerminateForTesting(

third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h

@@ -42,6 +42,7 @@ namespace blink {
 struct WebBackgroundFetchSettledFetch;
 struct WebCanMakePaymentEventData;
 class WebDataConsumerHandle;
+class WebSecurityOrigin;
 class WebServiceWorkerRequest;
 class WebString;
 struct WebNotificationData;