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

chrome/browser/extensions/api/debugger/debugger_api.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.
 
 // Implements the Chrome Extensions Debugger API.
 
 #include "chrome/browser/extensions/api/debugger/debugger_api.h"
 
 #include <map>
 #include <set>
@@ skipping 29 matching lines @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/url_utils.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_host.h"
 #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"
 
 using content::DevToolsAgentHost;
 using content::DevToolsClientHost;
 using content::DevToolsHttpHandler;
 using content::DevToolsManager;
 using content::RenderProcessHost;
 using content::RenderViewHost;
 using content::RenderWidgetHost;
@@ skipping 231 matching lines @@
   for (ClientHosts::iterator it = client_hosts_.begin();
        it != client_hosts_.end(); ++it) {
     ExtensionDevToolsClientHost* client_host = *it;
     if (manager->GetDevToolsAgentHostFor(client_host) == agent_host &&
         client_host->extension_id() == extension_id)
       return client_host;
   }
   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
 
 
 // ExtensionDevToolsClientHost ------------------------------------------------
 
 ExtensionDevToolsClientHost::ExtensionDevToolsClientHost(
     Profile* profile,
     DevToolsAgentHost* agent_host,
     const std::string& extension_id,
     const std::string& extension_name,
@@ skipping 183 matching lines @@
       format, keys::kTabTargetType, base::IntToString(*debuggee_.tab_id));
   else if (debuggee_.extension_id)
     error_ = ErrorUtils::FormatErrorMessage(
       format, keys::kBackgroundPageTargetType, *debuggee_.extension_id);
   else
     error_ = ErrorUtils::FormatErrorMessage(
       format, keys::kOpaqueTargetType, *debuggee_.target_id);
 }
 
 bool DebuggerFunction::InitAgentHost() {
+  const Extension* extension = GetExtension();
   if (debuggee_.tab_id) {
     WebContents* web_contents = NULL;
     bool result = ExtensionTabUtil::GetTabById(*debuggee_.tab_id,
                                                GetProfile(),
                                                include_incognito(),
                                                NULL,
                                                NULL,
                                                &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()
             ->GetBackgroundHostForExtension(*debuggee_.extension_id);
     if (extension_host) {
       agent_host_ = DevToolsAgentHost::GetOrCreateFor(
           extension_host->render_view_host());
     }
   } else if (debuggee_.target_id) {
     agent_host_ = DevToolsAgentHost::GetForId(*debuggee_.target_id);
@@ skipping 46 matching lines @@
         keys::kProtocolVersionNotSupportedError,
         params->required_version);
     return false;
   }
 
   if (agent_host_->IsAttached()) {
     FormatErrorMessage(keys::kAlreadyAttachedError);
     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);
   return true;
 }
 
 
 // DebuggerDetachFunction -----------------------------------------------------
 
 DebuggerDetachFunction::DebuggerDetachFunction() {
@@ skipping 117 matching lines @@
     const std::vector<DevToolsTargetImpl*>& target_list) {
   scoped_ptr<base::ListValue> result(new base::ListValue());
   for (size_t i = 0; i < target_list.size(); ++i)
     result->Append(SerializeTarget(*target_list[i]));
   STLDeleteContainerPointers(target_list.begin(), target_list.end());
   SetResult(result.release());
   SendResponse(true);
 }
 
 }  // namespace extensions