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

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index f026a0229aff833990b5bd389f8db111ba8a6dac..60620e387c4a489a493cd98e4ce8dd055e6c4aa2 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -11,6 +11,7 @@
 #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"
@@ -276,15 +277,6 @@ void OpenWindowOnUI(
       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();
-}
-
 }  // namespace
 
 ServiceWorkerVersion::ServiceWorkerVersion(
@@ -1169,22 +1161,26 @@ void ServiceWorkerVersion::OnOpenWindow(int request_id, const GURL& url) {
   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));
-    return;
+  GURL sanitized_url = url;
+
+  // Blink consider all about: scheme URLs as about:blank. We need to sanitize
+  // them accordingly to prevent CanRequestURL() call below to fail on them.
+  if (sanitized_url.SchemeIs(url::kAboutScheme))
+    sanitized_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

[falken, 2015/03/06 16:03:08]

nit: "receive requests to open disallowed URLs"?

+  // renderer side checks are slightly different. For example, view-source
+  // scheme will not be filtered out by Blink.
+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+          embedded_worker_->process_id(), sanitized_url)) {
+    sanitized_url = GURL(url::kAboutBlankURL);

[falken, 2015/03/06 16:03:08]

Maybe I'm missing something... why open about:blank instead of rejecting
openWindow() with an error for such URLs?

[mlamouri (slow - plz ping), 2015/03/06 16:17:19]

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

[mlamouri (slow - plz ping), 2015/03/06 16:17:19]

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

[mlamouri (slow - plz ping), 2015/03/06 16:17:19]

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

[falken, 2015/03/06 16:43:32]

Ah that's interesting, I didn't know window.open('view-source://blahblah');
opens about:blank. I was originally thinking it'd be weird for the developer to
not get an indication that the URL wasn't really opened, but don't feel
strongly. Following window.open sounds OK. I agree a comment to that effect
would be nice.

   }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&OpenWindowOnUI,
-                 url,
+                 sanitized_url,
                  script_url_,
                  embedded_worker_->process_id(),
                  make_scoped_refptr(context_->wrapper()),