[Extensions] Propagate activeTab hosts to extension background pages

extensions/renderer/dispatcher.cc

Index: extensions/renderer/dispatcher.cc
diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index fcfb5a3a2b4cd526e5bb2407e38ef6fe86b203be..3c9afeb8b2eb5a36ac2201de66409bc7a82f34f3 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -77,6 +77,7 @@
 #include "extensions/renderer/script_injection_manager.h"
 #include "extensions/renderer/send_request_natives.h"
 #include "extensions/renderer/set_icon_natives.h"
+#include "extensions/renderer/tab_finder.h"
 #include "extensions/renderer/test_features_native_handler.h"
 #include "extensions/renderer/user_gestures_native_handler.h"
 #include "extensions/renderer/utils_native_handler.h"
@@ -769,6 +770,10 @@ bool Dispatcher::OnControlMessageReceived(const IPC::Message& message) {
   IPC_MESSAGE_HANDLER(ExtensionMsg_TransferBlobs, OnTransferBlobs)
   IPC_MESSAGE_HANDLER(ExtensionMsg_Unloaded, OnUnloaded)
   IPC_MESSAGE_HANDLER(ExtensionMsg_UpdatePermissions, OnUpdatePermissions)
+  IPC_MESSAGE_HANDLER(ExtensionMsg_UpdateTabSpecificPermissions,
+                      OnUpdateTabSpecificPermissions)
+  IPC_MESSAGE_HANDLER(ExtensionMsg_ClearTabSpecificPermissions,
+                      OnClearTabSpecificPermissions)
   IPC_MESSAGE_HANDLER(ExtensionMsg_UsingWebRequestAPI, OnUsingWebRequestAPI)
   IPC_MESSAGE_FORWARD(ExtensionMsg_WatchPages,
                       content_watcher_.get(),
@@ -1028,17 +1033,65 @@ void Dispatcher::OnUpdatePermissions(
   scoped_refptr<const PermissionSet> withheld =
       params.withheld_permissions.ToPermissionSet();
 
-  if (is_webkit_initialized_) {
-    UpdateOriginPermissions(
-        extension,
-        extension->permissions_data()->GetEffectiveHostPermissions(),
-        active->effective_hosts());
-  }
-
+  URLPatternSet old_effective =
+      extension->permissions_data()->GetEffectiveHostPermissions();
   extension->permissions_data()->SetPermissions(active, withheld);
+
+  if (is_webkit_initialized_)
+    UpdateOriginPermissions(extension, old_effective);
+
   UpdateBindings(extension->id());
 }
 
+void Dispatcher::OnUpdateTabSpecificPermissions(
+    const GURL& visible_url,
+    const std::string& extension_id,
+    const URLPatternSet& new_hosts,
+    int tab_id) {
+  // Check against the URL to avoid races. If we can't find the tab, it's
+  // because this is an extension's background page (run in a different
+  // process) - in this case, we can't perform the security check. However,
+  // since activeTab is only granted via user action, this isn't a huge concern.
+  content::RenderView* render_view = TabFinder::Find(tab_id);
+  if (render_view &&
+      render_view->GetWebView()->mainFrame()->document().url() != visible_url) {
+    return;
+  }
+
+  const Extension* extension = extensions_.GetByID(extension_id);
+  if (!extension)
+    return;
+
+  URLPatternSet old_effective =
+      extension->permissions_data()->GetEffectiveHostPermissions();
+  extension->permissions_data()->UpdateTabSpecificPermissions(
+      tab_id,
+      new extensions::PermissionSet(extensions::APIPermissionSet(),
+                                    extensions::ManifestPermissionSet(),
+                                    new_hosts,
+                                    extensions::URLPatternSet()));
+
+  if (is_webkit_initialized_ &&
+      ExtensionHelper::GetBackgroundPage(extension_id)) {
+    UpdateOriginPermissions(extension, old_effective);

[not at google - send to devlin, 2015/02/03 19:29:12]

I was wondering why this is only happening for the background page. I think it's
because there has always been a bug: it appears as though we're not giving
extensions the permission to XHR to the tab it's running on?

It looks like you can fix that bug, and make this easier for me to understand
generally, all at once.

(1) Do what I suggest below, and make UpdateOriginPermissions dumber.
(2) Don't make that GetEffectiveHostPermissions change to PermissionsData,
instead add a separate method which returns the union of all tab specific
permissions (say, GetAllTabSpecificPermissions for the sake of this comment).
(3) Instead of only running UpdateOriginPermissions on
ExtensionHelper::GetBackgroundPage, just gate the call to
GetAllTabSpecificPermissions to it.

[Devlin, 2015/02/04 22:26:02]

As discussed offline, we don't want to update the origin access whitelist for
content scripts, since they share an origin with their current page (and the
whitelist works by saying "request from origin A to origin B granted").  Also
discussed offline, GetEffectiveHostPermissions was probably the right change to
make (though it needed a better comment).

+  }
+}
+
+void Dispatcher::OnClearTabSpecificPermissions(
+    const std::vector<std::string>& extension_ids,
+    int tab_id) {
+  for (const std::string& id : extension_ids) {
+    const Extension* extension = extensions_.GetByID(id);
+    if (extension) {
+      URLPatternSet old_effective =
+          extension->permissions_data()->GetEffectiveHostPermissions();
+      extension->permissions_data()->ClearTabSpecificPermissions(tab_id);
+      if (ExtensionHelper::GetBackgroundPage(id))
+        UpdateOriginPermissions(extension, old_effective);
+    }
+  }
+}
+
 void Dispatcher::OnUsingWebRequestAPI(bool webrequest_used) {
   webrequest_used_ = webrequest_used;
 }
@@ -1058,16 +1111,11 @@ void Dispatcher::UpdateActiveExtensions() {
 void Dispatcher::InitOriginPermissions(const Extension* extension) {
   delegate_->InitOriginPermissions(extension,
                                    IsExtensionActive(extension->id()));
-  UpdateOriginPermissions(
-      extension,
-      URLPatternSet(),  // No old permissions.
-      extension->permissions_data()->GetEffectiveHostPermissions());
+  UpdateOriginPermissions(extension, URLPatternSet());  // No old permissions.
 }
 
-void Dispatcher::UpdateOriginPermissions(
-    const Extension* extension,
-    const URLPatternSet& old_patterns,
-    const URLPatternSet& new_patterns) {
+void Dispatcher::UpdateOriginPermissions(const Extension* extension,

[not at google - send to devlin, 2015/02/03 19:29:12]

Even though this method can determine new_patterns from the state of
|extension|, I find this shortcut confusing[*], and prefer the old code. In
fact, how about changing it so that you pass the extension URL in instead of the
extension, to avoid the temptation of this change?

[*] I find it confusing for two reasons:

Firstly, because of the duality of state. We could be in a tab, or we could be
in a background page, and these must be treated differently. The call to
GetEffectiveHostPermissions forces me to try to reason about the difference, and
track down callers. The fact that sometimes this method is gated on there being
a background page, and other times it isn't, is confusing.

Secondly, a method called "Update" taking the *old* state is odd.

[Devlin, 2015/02/04 22:26:02]

Dumbified.

+                                         const URLPatternSet& old_patterns) {
   static const char* kSchemes[] = {
       url::kHttpScheme,
       url::kHttpsScheme,
@@ -1075,28 +1123,36 @@ void Dispatcher::UpdateOriginPermissions(
       content::kChromeUIScheme,
       url::kFtpScheme,
   };
+  URLPatternSet new_patterns =
+      extension->permissions_data()->GetEffectiveHostPermissions();
+
+  // Remove those patterns that aren't present in the current permissions...
+  URLPatternSet to_remove;
+  URLPatternSet::CreateDifference(old_patterns, new_patterns, &to_remove);
+
+  // ...And add the new ones.
+  URLPatternSet to_add;
+  URLPatternSet::CreateDifference(new_patterns, old_patterns, &to_add);
+
   for (size_t i = 0; i < arraysize(kSchemes); ++i) {
     const char* scheme = kSchemes[i];
-    // Remove all old patterns...
-    for (URLPatternSet::const_iterator pattern = old_patterns.begin();
-         pattern != old_patterns.end(); ++pattern) {
-      if (pattern->MatchesScheme(scheme)) {
+    for (const URLPattern& pattern : to_remove) {
+      if (pattern.MatchesScheme(scheme)) {
         WebSecurityPolicy::removeOriginAccessWhitelistEntry(
             extension->url(),
             WebString::fromUTF8(scheme),
-            WebString::fromUTF8(pattern->host()),
-            pattern->match_subdomains());
+            WebString::fromUTF8(pattern.host()),
+            pattern.match_subdomains());
       }
     }
-    // ...And add the new ones.
-    for (URLPatternSet::const_iterator pattern = new_patterns.begin();
-         pattern != new_patterns.end(); ++pattern) {
-      if (pattern->MatchesScheme(scheme)) {
+
+    for (const URLPattern& pattern : to_add) {
+      if (pattern.MatchesScheme(scheme)) {
         WebSecurityPolicy::addOriginAccessWhitelistEntry(
             extension->url(),
             WebString::fromUTF8(scheme),
-            WebString::fromUTF8(pattern->host()),
-            pattern->match_subdomains());
+            WebString::fromUTF8(pattern.host()),
+            pattern.match_subdomains());
       }
     }
   }