Support "always allow" for runtime script execution

chrome/browser/extensions/permissions_updater.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/permissions_updater.h"
 
 #include "base/json/json_writer.h"
 #include "base/memory/ref_counted.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 18 matching lines @@
 
 using content::RenderProcessHost;
 using extensions::permissions_api_helpers::PackPermissionSet;
 
 namespace extensions {
 
 namespace permissions = api::permissions;
 
 namespace {
 
+URLPatternSet FilterSingleOriginPermissions(const URLPatternSet& permissions) {

[Devlin, 2014/08/13 16:55:04]

Comment on the function

[gpdavis, 2014/08/13 22:18:36]

Done.

+  URLPatternSet single_origin_permissions;
+  for (URLPatternSet::const_iterator iter = permissions.begin();
+       iter != permissions.end();
+       ++iter) {
+    if (iter->MatchesSingleOrigin())

[not at google - send to devlin, 2014/08/13 17:17:47]

I just realised (after chatting to mek@) that this isn't quite right.

Say an extension requests *.google.com in its manifest, and we always-run on
http://www.google.com. The active permissions is {*.google.com,
http://www.google.com}.

Then the extension drops *.google.com from its permissions. The
GetBoundedActivePermissions logic correctly removes *.google.com from the active
permission set, and www.google.com. That latter is somewhat by accident (and the
problem that FilterSingleOriginPermissions etc fixes) *however* it introduces a
different bug. www.google.com *should* have been removed.

To fix this, we should only add back these permissions *if* they're granted by
the bounded active permissions. For *that* we need in addition to
MatchesSingleOrigin a method which extracts that origin. Shouldn't be too hard.
So we'd have:

if (iter->MatchesSingleOrigin() &&
    bounded_active->Matches(iter->ToOrigin()) {
  // add back.
}

I'm not liking the name MatchesSingleOrigin anymore. IsOrigin might be better.

[gpdavis, 2014/08/13 22:18:37]

Alright, this makes sense to me.  Can you think of a good way to test this case
in the unittest?  (I figure maybe you can point me in the right direction, but
I'll see what I can come up with as well).

+      single_origin_permissions.AddPattern(*iter);
+  }
+  return single_origin_permissions;
+}
+
 // Returns a PermissionSet that has the active permissions of the extension,
 // bounded to its current manifest.
 scoped_refptr<const PermissionSet> GetBoundedActivePermissions(
-    const Extension* extension, ExtensionPrefs* extension_prefs) {
+    const Extension* extension,
+    const PermissionSet* active_permissions) {
   // If the extension has used the optional permissions API, it will have a
   // custom set of active permissions defined in the extension prefs. Here,
   // we update the extension's active permissions based on the prefs.
-  scoped_refptr<const PermissionSet> active_permissions =
-      extension_prefs->GetActivePermissions(extension->id());

[Devlin, 2014/08/13 16:55:04]

any particular reason for the change?

[gpdavis, 2014/08/13 22:18:36]

Kalman suggested we pass active permissions in instead of prefs because we need
the active permissions in InitializePermissions in the code that was added.

   if (!active_permissions)
     return extension->permissions_data()->active_permissions();
 
   scoped_refptr<const PermissionSet> required_permissions =
       PermissionsParser::GetRequiredPermissions(extension);
 
   // We restrict the active permissions to be within the bounds defined in the
   // extension's manifest.
   //  a) active permissions must be a subset of optional + default permissions
   //  b) active permissions must contains all default permissions
@@ skipping 79 matching lines @@
   if (!Manifest::IsUnpackedLocation(extension->location()) &&
       extension->location() != Manifest::INTERNAL)
     return;
 
   ExtensionPrefs::Get(browser_context_)->AddGrantedPermissions(
       extension->id(),
       extension->permissions_data()->active_permissions().get());
 }
 
 void PermissionsUpdater::InitializePermissions(const Extension* extension) {
+  scoped_refptr<const PermissionSet> active_permissions =
+      ExtensionPrefs::Get(browser_context_)
+          ->GetActivePermissions(extension->id());
   scoped_refptr<const PermissionSet> bounded_active =
-      GetBoundedActivePermissions(extension,
-                                  ExtensionPrefs::Get(browser_context_));
+      GetBoundedActivePermissions(extension, active_permissions.get());
 
   // We withhold permissions iff the switch to do so is enabled, the extension
   // shows up in chrome:extensions (so the user can grant withheld permissions),
   // the extension is not part of chrome or corporate policy, and also not on
   // the scripting whitelist. Additionally, we don't withhold if the extension
   // has the preference to allow scripting on all urls.
   bool should_withhold_permissions =
       FeatureSwitch::scripts_require_action()->IsEnabled() &&
       extension->ShouldDisplayInExtensionSettings() &&
       !Manifest::IsPolicyLocation(extension->location()) &&
       !Manifest::IsComponentLocation(extension->location()) &&
       !PermissionsData::CanExecuteScriptEverywhere(extension) &&
       !util::AllowedScriptingOnAllUrls(extension->id(), browser_context_);
 
   URLPatternSet granted_explicit_hosts;
   URLPatternSet withheld_explicit_hosts;
   SegregateUrlPermissions(bounded_active->explicit_hosts(),
                           should_withhold_permissions,
                           &granted_explicit_hosts,
                           &withheld_explicit_hosts);
 
   URLPatternSet granted_scriptable_hosts;
   URLPatternSet withheld_scriptable_hosts;
   SegregateUrlPermissions(bounded_active->scriptable_hosts(),
                           should_withhold_permissions,
                           &granted_scriptable_hosts,
                           &withheld_scriptable_hosts);
 
+  // After withholding permissions, add back any origins to the active set that
+  // may have been lost during the set operations that would have dropped them.
+  // For example, the union of <all_urls> and <anything> is <all_urls>, so we

[Devlin, 2014/08/13 16:55:04]

anything is bad.  Use example.com or something.

[gpdavis, 2014/08/13 22:18:36]

Done.

+  // may lose the <anything>. However, that <anything> is important once
+  // <all_urls> is stripped during withholding.
+  if (active_permissions) {
+    granted_explicit_hosts.AddPatterns(

[Devlin, 2014/08/13 16:55:04]

You need to check here that the extension still wants these.

[gpdavis, 2014/08/13 22:18:36]

Is this what kalman was suggesting in the above comment on line 44?  Checking
the manifest permissions to see if these active permissions are still relevant?

+        FilterSingleOriginPermissions(active_permissions->explicit_hosts()));
+    granted_scriptable_hosts.AddPatterns(
+        FilterSingleOriginPermissions(active_permissions->scriptable_hosts()));
+  }
+
   bounded_active = new PermissionSet(bounded_active->apis(),
                                      bounded_active->manifest_permissions(),
                                      granted_explicit_hosts,
                                      granted_scriptable_hosts);
 
   scoped_refptr<const PermissionSet> withheld =
       new PermissionSet(APIPermissionSet(),
                         ManifestPermissionSet(),
                         withheld_explicit_hosts,
                         withheld_scriptable_hosts);
@@ skipping 133 matching lines @@
             Profile::FromBrowserContext(host->GetBrowserContext()))) {
       host->Send(new ExtensionMsg_UpdatePermissions(params));
     }
   }
 
   // Trigger the onAdded and onRemoved events in the extension.
   DispatchEvent(extension->id(), event_name, changed);
 }
 
 }  // namespace extensions