Remove ExtensionURLInfo, make security decisions in render process

chrome/renderer/extensions/dispatcher.cc

Index: chrome/renderer/extensions/dispatcher.cc
diff --git a/chrome/renderer/extensions/dispatcher.cc b/chrome/renderer/extensions/dispatcher.cc
index f8d9094a54945472f1378c7f906756a007a445a2..a8ea250be439f5d7616065ca11c9854ca55cc1f6 100644
--- a/chrome/renderer/extensions/dispatcher.cc
+++ b/chrome/renderer/extensions/dispatcher.cc
@@ -1015,11 +1015,13 @@ void Dispatcher::DidCreateScriptContext(
     extension_id = "";
   }
 
-  ExtensionURLInfo url_info(frame->document().securityOrigin(),
-      UserScriptSlave::GetDataSourceURLForFrame(frame));
+  // Frames loaded on a unique security origin are not accessible to extensions.

[not at google - send to devlin, 2013/07/15 22:09:12]

I've convinced myself that this seems kind of arbitrary and we should be
allowing it.

+  GURL effective_frame_url;
+  if (!frame->document().securityOrigin().isUnique())
+    effective_frame_url = UserScriptSlave::GetDataSourceURLForFrame(frame);
 
-  Feature::Context context_type =
-      ClassifyJavaScriptContext(extension_id, extension_group, url_info);
+  Feature::Context context_type = ClassifyJavaScriptContext(
+      extension_id, extension_group, effective_frame_url);

[Matt Perry, 2013/07/15 21:24:31]

The call to IsSandboxedPage in ClassifyJavaScriptContext will definitely return
false in the case that the security origin is unique. I have no idea if that's
what we want.

Maybe kalman knows.

[not at google - send to devlin, 2013/07/15 22:09:12]

yes I think this is another place where isUnique isn't needed

 
   ChromeV8Context* context =
       new ChromeV8Context(v8_context, frame, extension, context_type);
@@ -1127,18 +1129,17 @@ std::string Dispatcher::GetExtensionID(const WebFrame* frame, int world_id) {
     return user_script_slave_->GetExtensionIdForIsolatedWorld(world_id);
   }
 
+  if (frame->document().securityOrigin().isUnique())
+    return std::string();

[not at google - send to devlin, 2013/07/15 22:09:12]

also here

+
   // Extension pages (chrome-extension:// URLs).
   GURL frame_url = UserScriptSlave::GetDataSourceURLForFrame(frame);
-  return extensions_.GetExtensionOrAppIDByURL(
-      ExtensionURLInfo(frame->document().securityOrigin(), frame_url));
+  return extensions_.GetExtensionOrAppIDByURL(frame_url);
 }
 
 bool Dispatcher::IsWithinPlatformApp(const WebFrame* frame) {
-  // We intentionally don't use the origin parameter for ExtensionURLInfo since
-  // it would be empty (i.e. unique) for sandboxed resources and thus not match.
-  ExtensionURLInfo url_info(
-      UserScriptSlave::GetDataSourceURLForFrame(frame->top()));
-  const Extension* extension = extensions_.GetExtensionOrAppByURL(url_info);
+  GURL url(UserScriptSlave::GetDataSourceURLForFrame(frame->top()));
+  const Extension* extension = extensions_.GetExtensionOrAppByURL(url);
 
   return extension && extension->is_platform_app();
 }
@@ -1378,7 +1379,7 @@ void Dispatcher::OnCancelSuspend(const std::string& extension_id) {
 Feature::Context Dispatcher::ClassifyJavaScriptContext(
     const std::string& extension_id,
     int extension_group,
-    const ExtensionURLInfo& url_info) {
+    const GURL& url) {
   DCHECK_GE(extension_group, 0);
   if (extension_group == EXTENSION_GROUP_CONTENT_SCRIPTS) {
     return extensions_.Contains(extension_id) ?
@@ -1391,20 +1392,20 @@ Feature::Context Dispatcher::ClassifyJavaScriptContext(
   //    the extension is considered active.
   // 2. ScriptContext creation (which triggers bindings injection) happens
   //    before the SecurityContext is updated with the sandbox flags (after
-  //    reading the CSP header), so url_info.url().securityOrigin() is not
-  //    unique yet.
-  if (extensions_.IsSandboxedPage(url_info))
+  //    reading the CSP header), so the caller can't check if the context's
+  //    security origin is unique yet.
+  if (extensions_.IsSandboxedPage(url))
     return Feature::WEB_PAGE_CONTEXT;
 
   if (IsExtensionActive(extension_id))
     return Feature::BLESSED_EXTENSION_CONTEXT;
 
-  if (extensions_.ExtensionBindingsAllowed(url_info)) {
+  if (extensions_.ExtensionBindingsAllowed(url)) {
     return extensions_.Contains(extension_id) ?
         Feature::UNBLESSED_EXTENSION_CONTEXT : Feature::UNSPECIFIED_CONTEXT;
   }
 
-  if (url_info.url().is_valid())
+  if (url.is_valid())
     return Feature::WEB_PAGE_CONTEXT;
 
   return Feature::UNSPECIFIED_CONTEXT;
@@ -1433,9 +1434,9 @@ bool Dispatcher::CheckContextAccessToExtensionAPI(
   // Theoretically we could end up with bindings being injected into sandboxed
   // frames, for example content scripts. Don't let them execute API functions.
   WebKit::WebFrame* frame = context->web_frame();
-  ExtensionURLInfo url_info(frame->document().securityOrigin(),
-                            UserScriptSlave::GetDataSourceURLForFrame(frame));
-  if (extensions_.IsSandboxedPage(url_info)) {
+  if (frame->document().securityOrigin().isUnique() ||
+      extensions_.IsSandboxedPage(
+          UserScriptSlave::GetDataSourceURLForFrame(frame))) {

[not at google - send to devlin, 2013/07/15 22:09:12]

I don't actually think we should be preventing scripts from running in
web-sandboxed pages, just extension-sandboxed ones (then let host permissions do
the rest).

I think this would prevent extensions from running content scripts on file://
URLs which isn't desirable.

[not at google - send to devlin, 2013/07/15 22:27:10]

Ok I'm being dense and mixed up host and API permissions.

However it would prevent content scripts from calling any APIs on file:// URLs,
like chrome.runtime.sendMessage, which is nevertheless undesirable.

[jamesr, 2013/07/15 22:55:27]

It appears that chrome.runtime.sendMessage() fails in content scripts injected
into file:// URLs with or without this patch.  I agree that's bad but am not
sure exactly how it happens.  Removing this call doesn't make
chrome.runtime.sendMessage() work.

[not at google - send to devlin, 2013/07/15 23:00:53]

Yeah looks like there's an existing issue at play here too. However, as
discussed, removing the isUnque check here did make the chrome.storage functions
work?

     static const char kMessage[] =
         "%s cannot be used within a sandboxed frame.";
     std::string error_msg = base::StringPrintf(kMessage, function_name.c_str());