Relax same-origin policy for ServiceWorker openWindow() in Chromium.

content/browser/service_worker/service_worker_version.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_version.h"
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/message_port_message_filter.h"
 #include "content/browser/message_port_service.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_utils.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/common/service_worker/service_worker_messages.h"
@@ skipping 240 matching lines @@
       url, Referrer::SanitizeForRequest(
                url, Referrer(script_url, blink::WebReferrerPolicyDefault)),
       NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
       true /* is_renderer_initiated */);
 
   GetContentClient()->browser()->OpenURL(
       browser_context, params,
       base::Bind(&DidOpenURL, callback));
 }
 
-void KillEmbeddedWorkerProcess(int process_id, ResultCode code) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  RenderProcessHost* render_process_host =
-      RenderProcessHost::FromID(process_id);
-  if (render_process_host->GetHandle() != base::kNullProcessHandle)
-    render_process_host->ReceivedBadMessage();
-}
-
 void ClearTick(base::TimeTicks* time) {
   *time = base::TimeTicks();
 }
 
 void RestartTick(base::TimeTicks* time) {
   *time = base::TimeTicks().Now();
 }
 
 base::TimeDelta GetTickDuration(const base::TimeTicks& time) {
   if (time.is_null())
@@ skipping 866 matching lines @@
   if (!callback) {
     NOTREACHED() << "Got unexpected message: " << request_id;
     return;
   }
 
   scoped_refptr<ServiceWorkerVersion> protect(this);
   callback->Run(SERVICE_WORKER_OK, accept_connection);
   RemoveCallbackAndStopIfDoomed(&cross_origin_connect_callbacks_, request_id);
 }
 
-void ServiceWorkerVersion::OnOpenWindow(int request_id, const GURL& url) {
+void ServiceWorkerVersion::OnOpenWindow(int request_id, GURL url) {
   // Just abort if we are shutting down.
   if (!context_)
     return;
 
-  if (url.GetOrigin() != script_url_.GetOrigin()) {
-    // There should be a same origin check by Blink, if the request is still not
-    // same origin, the process might be compromised and should be eliminated.
-    DVLOG(1) << "Received a cross origin openWindow() request from a service "
-                "worker. Killing associated process.";
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(&KillEmbeddedWorkerProcess,
-                                       embedded_worker_->process_id(),
-                                       RESULT_CODE_KILLED_BAD_MESSAGE));
+  // Blink consider all about: scheme URLs as about:blank. We need to sanitize
+  // them accordingly to prevent CanRequestURL() call below to fail on them.

[falken, 2015/03/08 12:23:49]

This is kinda inconsistent now... why not just let about: URLs that aren't
about:blank get rejected when they fail CanRequestURL, in the same way
view-source gets rejected?

[mlamouri (slow - plz ping), 2015/03/08 14:34:12]

I've updated the comment to have it match RPHImpl::FilterURL(). I tried to stay
consistent with the checks there which consider about: URLs different. My
understanding is that as much as view-source:// not being filtered out might be
an oversight, about:* URL might be on purpose.

[mlamouri (slow - plz ping), 2015/03/08 14:34:12]

I've updated the comment to have it match RPHImpl::FilterURL(). I tried to stay
consistent with the checks there which consider about: URLs different. My
understanding is that as much as view-source:// not being filtered out might be
an oversight, about:* URL might be on purpose.

[falken, 2015/03/08 15:01:50]

Ah I see, makes sense.

+  if (url.SchemeIs(url::kAboutScheme))
+    url = GURL(url::kAboutBlankURL);
+
+  // This call will check whether the process should be able to access the given
+  // URL. It is possible to receive requests to open such URLs because the
+  // renderer side checks are slightly different. For example, view-source
+  // scheme will not be filtered out by Blink. This is the reason why failing
+  // that call will reject the promise instead of killing the renderer.

[falken, 2015/03/08 12:23:49]

nits: "such URLs" reads like "URLs that the process can access", which is the
opposite of what you mean. Overall this comment can be tightened up, something
like: "Reject requests for URLs that the process is not allowed to access. It's
possible to receive such requests since the renderer-side checks are slightly
different. For example, the view-source scheme will not be filtered out by
Blink."

[mlamouri (slow - plz ping), 2015/03/08 14:34:12]

Done.

+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+          embedded_worker_->process_id(), url)) {
+    embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowError(
+        request_id, url.spec() + " cannot be opened."));
     return;
   }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&OpenWindowOnUI,
                  url,
                  script_url_,
                  embedded_worker_->process_id(),
                  make_scoped_refptr(context_->wrapper()),
                  base::Bind(&ServiceWorkerVersion::DidOpenWindow,
                             weak_factory_.GetWeakPtr(),
                             request_id)));
 }
 
 void ServiceWorkerVersion::DidOpenWindow(int request_id,
                                          int render_process_id,
                                          int render_frame_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (running_status() != RUNNING)
     return;
 
   if (render_process_id == ChildProcessHost::kInvalidUniqueID &&
       render_frame_id == MSG_ROUTING_NONE) {
-    embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowError(request_id));
+    embedded_worker_->SendMessage(ServiceWorkerMsg_OpenWindowError(
+        request_id, "Something went wrong while trying to open the window"));

[falken, 2015/03/08 12:23:49]

nit: Add a period to be consistent with the error message above (and other SW
exception messages try to do this too)

[mlamouri (slow - plz ping), 2015/03/08 14:34:12]

Done.

     return;
   }
 
   for (const auto& it : controllee_map_) {
     const ServiceWorkerProviderHost* provider_host = it.first;
     if (provider_host->process_id() != render_process_id ||
         provider_host->frame_id() != render_frame_id) {
       continue;
     }
 
@@ skipping 368 matching lines @@
     int request_id) {
   callbacks->Remove(request_id);
   if (is_doomed_) {
     // The stop should be already scheduled, but try to stop immediately, in
     // order to release worker resources soon.
     StopWorkerIfIdle();
   }
 }
 
 }  // namespace content