Have the Debugger extension api check that it has access to the tab

chrome/browser/extensions/api/debugger/debugger_api.cc

Index: chrome/browser/extensions/api/debugger/debugger_api.cc
diff --git a/chrome/browser/extensions/api/debugger/debugger_api.cc b/chrome/browser/extensions/api/debugger/debugger_api.cc
index ebbc10207223aa9f07f5fed2ca7ee92480921f65..6576c0a31050d95d21fc9d71aecc111444d84f64 100644
--- a/chrome/browser/extensions/api/debugger/debugger_api.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_api.cc
@@ -47,8 +47,11 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/switches.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -300,6 +303,15 @@ ExtensionDevToolsClientHost* AttachedClientHosts::Lookup(
   return NULL;
 }
 
+// Returns true if the given |extension| is allowed to run the debugger on
+// other extensions' pages.
+bool CanRunOnOtherExtensionPages(const Extension* extension) {

[meacer, 2014/06/24 18:15:54]

nit: CanDebugExtensionPages sounds slightly more descriptive.

[not at google - send to devlin, 2014/06/24 18:35:08]

+1

though I'm still worried that a blacklist is just going to regress. I don't like
that this API uses a different permissions mechanism than the rest of chrome.

can we at least put a static method in PermissionsData like IsRestricted which
takes an extension + GURL, which checks this flag + things like whether it's a
component extension?

[Devlin, 2014/06/24 18:45:40]

Done.

[Devlin, 2014/06/24 18:49:08]

I'm not entirely sure I follow - won't it _still_ use a different permission
mechanism, and just have the mechanism be in a different class?  I'd be worried
that if we do that, it increases the chance that other callers will accidentally
use that one.  If it's only used in this file, is there a compelling reason to
move it?  (I'd like to avoid putting yet-more one-call-site-only functions into
PermissionsData...)

[not at google - send to devlin, 2014/06/24 19:07:51]

IsRestricted would be checked from CanAccessPage, so it'd be called in 2 places.
I imagine there are other places in the codebase where we should be using that
function as well.

[Devlin, 2014/06/24 19:54:55]

Done...

+   return extension->permissions_data()->CanExecuteScriptEverywhere(

[meacer, 2014/06/24 18:15:54]

Static method, you can use PermissionsData::CanExecuteScriptEverywhere instead.

[Devlin, 2014/06/24 18:45:40]

Whoops! Done.

+              extension) ||
+          base::CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kExtensionsOnChromeURLs);

[meacer, 2014/06/24 18:15:54]

Is kExtensionsOnChromeURLs the right switch? Looks like
kSilentDebuggerExtensionAPI was actually introduced for this purpose (debugging
without infobars).

[not at google - send to devlin, 2014/06/24 18:35:08]

kSilentDebuggerExtensionAPI is subtly different, and the whole reason this bug
exists is because only that flag was being checked.

[Devlin, 2014/06/24 18:45:40]

I'm not sure - the chrome urls switch was taken from Ben's comment in the bug. 
But I'm not sure the silent debugging is right, either - can't we attach an
infobar to an extension's page?  Otherwise, we actually don't need to do
anything with this bug (since we already check the infobar requirement).

+}
+
 }  // namespace
 
 
@@ -503,6 +515,7 @@ void DebuggerFunction::FormatErrorMessage(const std::string& format) {
 }
 
 bool DebuggerFunction::InitAgentHost() {
+  const Extension* extension = GetExtension();
   if (debuggee_.tab_id) {
     WebContents* web_contents = NULL;
     bool result = ExtensionTabUtil::GetTabById(*debuggee_.tab_id,
@@ -513,15 +526,26 @@ bool DebuggerFunction::InitAgentHost() {
                                                &web_contents,
                                                NULL);
     if (result && web_contents) {
-      if (content::HasWebUIScheme(web_contents->GetURL())) {
+      GURL url = web_contents->GetVisibleURL();

[meacer, 2014/06/24 18:54:27]

Better to use GetLastCommittedURL instead.

[Devlin, 2014/06/24 19:54:55]

Agreed.  And tried that first.  Unfortunately, this broke one of the tests
(haven't investigated enough as to why).  Since the deprecated GetURL() is just
a forward of GetVisibleURL(), I figured it was okay to leave it functionally as
it has been for now.

+      if (content::HasWebUIScheme(url)) {
         error_ = ErrorUtils::FormatErrorMessage(
             keys::kAttachToWebUIError,
             web_contents->GetURL().scheme());
         return false;
       }
+      if (url.SchemeIs(kExtensionScheme) && url.host() != extension->id() &&
+          !CanRunOnOtherExtensionPages(extension)) {
+        error_ = keys::kAttachToOtherExtensionError;
+        return false;
+      }
       agent_host_ = DevToolsAgentHost::GetOrCreateFor(web_contents);
     }
   } else if (debuggee_.extension_id) {
+    if (*debuggee_.extension_id != extension->id() &&
+        !CanRunOnOtherExtensionPages(extension)) {
+      error_ = keys::kAttachToOtherExtensionError;
+      return false;
+    }
     ExtensionHost* extension_host =
         ExtensionSystem::Get(GetProfile())
             ->process_manager()
@@ -588,25 +612,26 @@ bool DebuggerAttachFunction::RunAsync() {
     return false;
   }
 
+  const Extension* extension = GetExtension();
   infobars::InfoBar* infobar = NULL;
   if (!CommandLine::ForCurrentProcess()->
-       HasSwitch(switches::kSilentDebuggerExtensionAPI)) {
+       HasSwitch(::switches::kSilentDebuggerExtensionAPI)) {

[meacer, 2014/06/24 18:54:27]

Is this flag going to be redundant with this change?

!kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can only debug web
pages
kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can debug all pages
!kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug bg but not 
chrome-extensions
kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug all pages

Given that the only difference between a bg page and chrome-extension page is
showing the infobar, kSilentDebuggerExtensionAPI doesn't have any value any
more?

[Devlin, 2014/06/24 19:54:55]

Hmm... good question.

The thing is, we don't even create the InfoBar if we don't have the switch on,
so it could also be a nice ui feature for people who don't wanna see it.  WDYT?

     // Do not attach to the target if for any reason the infobar cannot be shown
     // for this WebContents instance.
     infobar = ExtensionDevToolsInfoBarDelegate::Create(
-        agent_host_->GetRenderViewHost(), GetExtension()->name());
+        agent_host_->GetRenderViewHost(), extension->name());
     if (!infobar) {
       error_ = ErrorUtils::FormatErrorMessage(
           keys::kSilentDebuggingRequired,
-          switches::kSilentDebuggerExtensionAPI);
+          ::switches::kSilentDebuggerExtensionAPI);
       return false;
     }
   }
 
   new ExtensionDevToolsClientHost(GetProfile(),
                                   agent_host_.get(),
-                                  GetExtension()->id(),
-                                  GetExtension()->name(),
+                                  extension->id(),
+                                  extension->name(),
                                   debuggee_,
                                   infobar);
   SendResponse(true);