Merge 280354 "Have the Debugger extension api check that it has ..."

> Have the Debugger extension api check that it has access to the tab
> 
> Check PermissionsData::CanAccessTab() prior to attaching the debugger.
> 
> BUG=367567
> 
> Review URL: https://codereview.chromium.org/352523003

TBR=rdevlin.cronin@chromium.org

Review URL: https://codereview.chromium.org/439843002

git-svn-id: svn://svn.chromium.org/chrome/branches/2062/src@287394 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 310 insertions, 39 deletions

diff --git a/chrome/browser/extensions/api/debugger/debugger_api.cc b/chrome/browser/extensions/api/debugger/debugger_api.cc
index ebbc102..f9e5ad7 100644
--- a/chrome/browser/extensions/api/debugger/debugger_api.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_api.cc
@@ -47,8 +47,12 @@
 #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/manifest_constants.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"
 
@@ -503,6 +507,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,12 +518,10 @@ bool DebuggerFunction::InitAgentHost() {
                                                &web_contents,
                                                NULL);
     if (result && web_contents) {
-      if (content::HasWebUIScheme(web_contents->GetURL())) {
-        error_ = ErrorUtils::FormatErrorMessage(
-            keys::kAttachToWebUIError,
-            web_contents->GetURL().scheme());
+      // TODO(rdevlin.cronin) This should definitely be GetLastCommittedURL().
+      GURL url = web_contents->GetVisibleURL();
+      if (PermissionsData::IsRestrictedUrl(url, url, extension, &error_))
         return false;
-      }
       agent_host_ = DevToolsAgentHost::GetOrCreateFor(web_contents);
     }
   } else if (debuggee_.extension_id) {
@@ -527,6 +530,12 @@ bool DebuggerFunction::InitAgentHost() {
             ->process_manager()
             ->GetBackgroundHostForExtension(*debuggee_.extension_id);
     if (extension_host) {
+      if (PermissionsData::IsRestrictedUrl(extension_host->GetURL(),
+                                           extension_host->GetURL(),
+                                           extension,
+                                           &error_)) {
+        return false;
+      }
       agent_host_ = DevToolsAgentHost::GetOrCreateFor(
           extension_host->render_view_host());
     }
@@ -588,25 +597,26 @@ bool DebuggerAttachFunction::RunAsync() {
     return false;
   }
 
+  const Extension* extension = GetExtension();
   infobars::InfoBar* infobar = NULL;
   if (!CommandLine::ForCurrentProcess()->
-       HasSwitch(switches::kSilentDebuggerExtensionAPI)) {
+       HasSwitch(::switches::kSilentDebuggerExtensionAPI)) {
     // 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);
diff --git a/chrome/browser/extensions/api/debugger/debugger_api_constants.cc b/chrome/browser/extensions/api/debugger/debugger_api_constants.cc
index 70da45b..7ddcd82 100644
--- a/chrome/browser/extensions/api/debugger/debugger_api_constants.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_api_constants.cc
@@ -8,8 +8,6 @@ namespace debugger_api_constants {
 
 const char kAlreadyAttachedError[] =
     "Another debugger is already attached to the * with id: *.";
-const char kAttachToWebUIError[] =
-    "Can not attach to the page with the \"*://\" scheme.";
 const char kNoTargetError[] = "No * with given id *.";
 const char kInvalidTargetError[] =
     "Either tab id or extension id must be specified.";
diff --git a/chrome/browser/extensions/api/debugger/debugger_api_constants.h b/chrome/browser/extensions/api/debugger/debugger_api_constants.h
index 0c7eaa7..f79b7b8 100644
--- a/chrome/browser/extensions/api/debugger/debugger_api_constants.h
+++ b/chrome/browser/extensions/api/debugger/debugger_api_constants.h
@@ -15,7 +15,6 @@ extern const char kOnDetach[];
 
 // Errors.
 extern const char kAlreadyAttachedError[];
-extern const char kAttachToWebUIError[];
 extern const char kNoTargetError[];
 extern const char kInvalidTargetError[];
 extern const char kNotAttachedError[];
diff --git a/chrome/browser/extensions/api/debugger/debugger_apitest.cc b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
index 878135a..f50ecbb 100644
--- a/chrome/browser/extensions/api/debugger/debugger_apitest.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
@@ -2,10 +2,144 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "base/command_line.h"
+#include "base/memory/ref_counted.h"
+#include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/extensions/api/debugger/debugger_api.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_function_test_utils.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "extensions/browser/extension_function.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest_constants.h"
+#include "extensions/common/switches.h"
+#include "extensions/common/value_builder.h"
+
+namespace extensions {
+
+class DebuggerApiTest : public ExtensionApiTest {
+ protected:
+  virtual ~DebuggerApiTest() {}
+
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
+  virtual void SetUpOnMainThread() OVERRIDE;
+
+  // Run the attach function. If |expected_error| is not empty, then the
+  // function should fail with the error. Otherwise, the function is expected
+  // to succeed.
+  testing::AssertionResult RunAttachFunction(const GURL& url,
+                                             const std::string& expected_error);
+
+  const Extension* extension() const { return extension_; }
+  base::CommandLine* command_line() const { return command_line_; }
+
+ private:
+  // The command-line for the test process, preserved in order to modify
+  // mid-test.
+  base::CommandLine* command_line_;
+
+  // A basic extension with the debugger permission.
+  scoped_refptr<const Extension> extension_;
+};
+
+void DebuggerApiTest::SetUpCommandLine(base::CommandLine* command_line) {
+  ExtensionApiTest::SetUpCommandLine(command_line);
+  // We need to hold onto |command_line| in order to modify it during the test.
+  command_line_ = command_line;
+}
+
+void DebuggerApiTest::SetUpOnMainThread() {
+  ExtensionApiTest::SetUpOnMainThread();
+  extension_ =
+      ExtensionBuilder().SetManifest(
+          DictionaryBuilder().Set("name", "debugger")
+                             .Set("version", "0.1")
+                             .Set("manifest_version", 2)
+                             .Set("permissions",
+                                  ListBuilder().Append("debugger"))).Build();
+}
+
+testing::AssertionResult DebuggerApiTest::RunAttachFunction(
+    const GURL& url, const std::string& expected_error) {
+  ui_test_utils::NavigateToURL(browser(), url);
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  int tab_id = SessionID::IdForTab(web_contents);
+  scoped_refptr<DebuggerAttachFunction> attach_function =
+      new DebuggerAttachFunction();
+  attach_function->set_extension(extension_);
+  std::string args = base::StringPrintf("[{\"tabId\": %d}, \"1.1\"]", tab_id);
+
+  if (!expected_error.empty()) {
+    std::string actual_error =
+        extension_function_test_utils::RunFunctionAndReturnError(
+            attach_function, args, browser());
+    if (actual_error != expected_error) {
+      return testing::AssertionFailure() << "Did not get correct error: "
+          << "expected: " << expected_error << ", found: " << actual_error;
+    }
+  } else {
+    if (!RunFunction(attach_function,
+                     args,
+                     browser(),
+                     extension_function_test_utils::NONE)) {
+      return testing::AssertionFailure() << "Could not run function: "
+          << attach_function->GetError();
+    }
+
+    // Clean up and detach.
+    scoped_refptr<DebuggerDetachFunction> detach_function =
+        new DebuggerDetachFunction();
+    detach_function->set_extension(extension_);
+    if (!RunFunction(detach_function,
+                     base::StringPrintf("[{\"tabId\": %d}]", tab_id),
+                     browser(),
+                     extension_function_test_utils::NONE)) {
+      return testing::AssertionFailure() << "Could not detach: "
+          << detach_function->GetError();
+    }
+  }
+  return testing::AssertionSuccess();
+}
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Debugger) {
   ASSERT_TRUE(RunExtensionTest("debugger")) << message_;
 }
+
+IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
+                       DebuggerNotAllowedOnOtherExtensionPages) {
+  // Load another arbitrary extension with an associated resource (popup.html).
+  base::FilePath path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &path));
+  path = path.AppendASCII("extensions").AppendASCII("good_unpacked");
+  const Extension* another_extension = LoadExtension(path);
+  ASSERT_TRUE(another_extension);
+
+  GURL other_ext_url =
+      GURL(base::StringPrintf("chrome-extension://%s/popup.html",
+                              another_extension->id().c_str()));
+
+  // This extension should not be able to access another extension.
+  EXPECT_TRUE(RunAttachFunction(
+      other_ext_url, manifest_errors::kCannotAccessExtensionUrl));
+
+  // This extension *should* be able to debug itself.
+  EXPECT_TRUE(RunAttachFunction(
+                  GURL(base::StringPrintf("chrome-extension://%s/foo.html",
+                                          extension()->id().c_str())),
+                  std::string()));
+
+  // Append extensions on chrome urls switch. The extension should now be able
+  // to debug any extension.
+  command_line()->AppendSwitch(switches::kExtensionsOnChromeURLs);
+  EXPECT_TRUE(RunAttachFunction(other_ext_url, std::string()));
+}
+
+}  // namespace extensions
diff --git a/chrome/browser/extensions/api/debugger/debugger_extension_apitest.cc b/chrome/browser/extensions/api/debugger/debugger_extension_apitest.cc
index 753d3ff..2829a31 100644
--- a/chrome/browser/extensions/api/debugger/debugger_extension_apitest.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_extension_apitest.cc
@@ -5,12 +5,14 @@
 #include "base/command_line.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/common/chrome_switches.h"
+#include "extensions/common/switches.h"
 
 class ExtensionApiTestWithSwitch : public ExtensionApiTest {
  public:
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     ExtensionApiTest::SetUpCommandLine(command_line);
     command_line->AppendSwitch(switches::kSilentDebuggerExtensionAPI);
+    command_line->AppendSwitch(extensions::switches::kExtensionsOnChromeURLs);
   }
 };
 
diff --git a/chrome/test/data/extensions/api_test/debugger/background.js b/chrome/test/data/extensions/api_test/debugger/background.js
index a6e2e8d..b902f86 100644
--- a/chrome/test/data/extensions/api_test/debugger/background.js
+++ b/chrome/test/data/extensions/api_test/debugger/background.js
@@ -108,7 +108,7 @@ chrome.test.runTests([
     chrome.tabs.create({url:"chrome://version"}, function(tab) {
       var debuggee = {tabId: tab.id};
       chrome.debugger.attach(debuggee, protocolVersion,
-          fail("Can not attach to the page with the \"chrome://\" scheme."));
+          fail("Cannot access a chrome:// URL"));
       chrome.tabs.remove(tab.id);
     });
   },
diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
index 72717f7..1b160ca 100644
--- a/extensions/common/permissions/permissions_data.cc
+++ b/extensions/common/permissions/permissions_data.cc
@@ -17,6 +17,7 @@
 #include "extensions/common/url_pattern_set.h"
 #include "extensions/common/user_script.h"
 #include "url/gurl.h"
+#include "url/url_constants.h"
 
 namespace extensions {
 
@@ -70,6 +71,48 @@ bool PermissionsData::CanExecuteScriptEverywhere(const Extension* extension) {
          whitelist.end();
 }
 
+// static
+bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
+                                      const GURL& top_frame_url,
+                                      const Extension* extension,
+                                      std::string* error) {
+  if (CanExecuteScriptEverywhere(extension))
+    return false;
+
+  // Check if the scheme is valid for extensions. If not, return.
+  if (!URLPattern::IsValidSchemeForExtensions(document_url.scheme()) &&
+      document_url.spec() != url::kAboutBlankURL) {
+    if (error) {
+      *error = ErrorUtils::FormatErrorMessage(
+                   manifest_errors::kCannotAccessPage,
+                   document_url.spec());
+    }
+    return true;
+  }
+
+  if (!ExtensionsClient::Get()->IsScriptableURL(document_url, error))
+    return true;
+
+  bool allow_on_chrome_urls = base::CommandLine::ForCurrentProcess()->HasSwitch(
+                                  switches::kExtensionsOnChromeURLs);
+  if (document_url.SchemeIs(content::kChromeUIScheme) &&
+      !allow_on_chrome_urls) {
+    if (error)
+      *error = manifest_errors::kCannotAccessChromeUrl;
+    return true;
+  }
+
+  if (top_frame_url.SchemeIs(kExtensionScheme) &&
+      top_frame_url.host() != extension->id() &&
+      !allow_on_chrome_urls) {
+    if (error)
+      *error = manifest_errors::kCannotAccessExtensionUrl;
+    return true;
+  }
+
+  return false;
+}
+
 void PermissionsData::SetActivePermissions(
     const PermissionSet* permissions) const {
   base::AutoLock auto_lock(runtime_lock_);
@@ -283,30 +326,8 @@ bool PermissionsData::CanRunOnPage(const Extension* extension,
     return false;
   }
 
-  bool can_execute_everywhere = CanExecuteScriptEverywhere(extension);
-  if (!can_execute_everywhere &&
-      !ExtensionsClient::Get()->IsScriptableURL(document_url, error)) {
-    return false;
-  }
-
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kExtensionsOnChromeURLs)) {
-    if (document_url.SchemeIs(content::kChromeUIScheme) &&
-        !can_execute_everywhere) {
-      if (error)
-        *error = manifest_errors::kCannotAccessChromeUrl;
-      return false;
-    }
-  }
-
-  if (top_frame_url.SchemeIs(kExtensionScheme) &&
-      top_frame_url.GetOrigin() !=
-          Extension::GetBaseURLFromExtensionId(extension->id()).GetOrigin() &&
-      !can_execute_everywhere) {
-    if (error)
-      *error = manifest_errors::kCannotAccessExtensionUrl;
+  if (IsRestrictedUrl(document_url, top_frame_url, extension, error))
     return false;
-  }
 
   if (HasTabSpecificPermissionToExecuteScript(tab_id, top_frame_url))
     return true;
diff --git a/extensions/common/permissions/permissions_data.h b/extensions/common/permissions/permissions_data.h
index a23fb2b..9470d58 100644
--- a/extensions/common/permissions/permissions_data.h
+++ b/extensions/common/permissions/permissions_data.h
@@ -68,6 +68,14 @@ class PermissionsData {
   // whitelist of extensions that can script all pages.
   static bool CanExecuteScriptEverywhere(const Extension* extension);
 
+  // Returns true if the given |url| is restricted for the given |extension|,
+  // as is commonly the case for chrome:// urls.
+  // NOTE: You probably want to use CanAccessPage().
+  static bool IsRestrictedUrl(const GURL& document_url,
+                              const GURL& top_frame_url,
+                              const Extension* extension,
+                              std::string* error);
+
   // Sets the runtime permissions of the given |extension| to |permissions|.
   void SetActivePermissions(const PermissionSet* active) const;
 
diff --git a/extensions/common/permissions/permissions_data_unittest.cc b/extensions/common/permissions/permissions_data_unittest.cc
index dc3aa86..3df1d44 100644
--- a/extensions/common/permissions/permissions_data_unittest.cc
+++ b/extensions/common/permissions/permissions_data_unittest.cc
@@ -87,6 +87,71 @@ bool RequiresActionForScriptExecution(const std::string& extension_id,
       GURL::EmptyGURL());
 }
 
+// Checks that urls are properly restricted for the given extension.
+void CheckRestrictedUrls(const Extension* extension,
+                         bool block_chrome_urls) {
+  // We log the name so we know _which_ extension failed here.
+  const std::string& name = extension->name();
+  const GURL chrome_settings_url("chrome://settings/");
+  const GURL chrome_extension_url("chrome-extension://foo/bar.html");
+  const GURL google_url("https://www.google.com/");
+  const GURL self_url("chrome-extension://" + extension->id() + "/foo.html");
+  const GURL invalid_url("chrome-debugger://foo/bar.html");
+
+  std::string error;
+  EXPECT_EQ(block_chrome_urls,
+            PermissionsData::IsRestrictedUrl(
+                chrome_settings_url,
+                chrome_settings_url,
+                extension,
+                &error)) << name;
+  if (block_chrome_urls)
+    EXPECT_EQ(manifest_errors::kCannotAccessChromeUrl, error) << name;
+  else
+    EXPECT_TRUE(error.empty()) << name;
+
+  error.clear();
+  EXPECT_EQ(block_chrome_urls,
+            PermissionsData::IsRestrictedUrl(
+                chrome_extension_url,
+                chrome_extension_url,
+                extension,
+                &error)) << name;
+  if (block_chrome_urls)
+    EXPECT_EQ(manifest_errors::kCannotAccessExtensionUrl, error) << name;
+  else
+    EXPECT_TRUE(error.empty()) << name;
+
+  // Google should never be a restricted url.
+  error.clear();
+  EXPECT_FALSE(PermissionsData::IsRestrictedUrl(
+      google_url, google_url, extension, &error)) << name;
+  EXPECT_TRUE(error.empty()) << name;
+
+  // We should always be able to access our own extension pages.
+  error.clear();
+  EXPECT_FALSE(PermissionsData::IsRestrictedUrl(
+      self_url, self_url, extension, &error)) << name;
+  EXPECT_TRUE(error.empty()) << name;
+
+  // We should only allow other schemes for extensions when it's a whitelisted
+  // extension.
+  error.clear();
+  bool allow_on_other_schemes =
+      PermissionsData::CanExecuteScriptEverywhere(extension);
+  EXPECT_EQ(!allow_on_other_schemes,
+            PermissionsData::IsRestrictedUrl(
+                invalid_url, invalid_url, extension, &error)) << name;
+  if (!allow_on_other_schemes) {
+    EXPECT_EQ(ErrorUtils::FormatErrorMessage(
+                  manifest_errors::kCannotAccessPage,
+                  invalid_url.spec()),
+              error) << name;
+  } else {
+    EXPECT_TRUE(error.empty());
+  }
+}
+
 }  // namespace
 
 TEST(ExtensionPermissionsTest, EffectiveHostPermissions) {
@@ -242,6 +307,28 @@ TEST(ExtensionPermissionsTest, RequiresActionForScriptExecution) {
       extension, 0, GURL("https://www.google.com/")));
 }
 
+TEST(ExtensionPermissionsTest, IsRestrictedUrl) {
+  scoped_refptr<const Extension> extension =
+      GetExtensionWithHostPermission("normal_extension",
+                                     kAllHostsPermission,
+                                     Manifest::INTERNAL);
+  // Chrome urls should be blocked for normal extensions.
+  CheckRestrictedUrls(extension, true);
+
+  scoped_refptr<const Extension> component =
+      GetExtensionWithHostPermission("component",
+                                     kAllHostsPermission,
+                                     Manifest::COMPONENT);
+  // Chrome urls should be accessible by component extensions.
+  CheckRestrictedUrls(component, false);
+
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kExtensionsOnChromeURLs);
+  // Enabling the switch should allow all extensions to access chrome urls.
+  CheckRestrictedUrls(extension, false);
+
+}
+
 TEST(ExtensionPermissionsTest, GetPermissionMessages_ManyAPIPermissions) {
   scoped_refptr<Extension> extension;
   extension = LoadManifest("permissions", "many-apis.json");
@@ -553,8 +640,8 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, PermissionsWithChromeURLsEnabled) {
   EXPECT_TRUE(AllowedScript(extension.get(), https_url, http_url_with_path));
   EXPECT_TRUE(AllowedScript(extension.get(), http_url, within_extension_url));
   EXPECT_TRUE(AllowedScript(extension.get(), https_url, within_extension_url));
-  EXPECT_TRUE(BlockedScript(extension.get(), http_url, extension_url));
-  EXPECT_TRUE(BlockedScript(extension.get(), https_url, extension_url));
+  EXPECT_TRUE(AllowedScript(extension.get(), http_url, extension_url));
+  EXPECT_TRUE(AllowedScript(extension.get(), https_url, extension_url));
 
   const PermissionsData* permissions_data = extension->permissions_data();
   EXPECT_FALSE(permissions_data->HasHostPermission(settings_url));
diff --git a/extensions/common/url_pattern.cc b/extensions/common/url_pattern.cc
index c7a3711..ee7d164 100644
--- a/extensions/common/url_pattern.cc
+++ b/extensions/common/url_pattern.cc
@@ -109,6 +109,15 @@ std::string StripTrailingWildcard(const std::string& path) {
 
 }  // namespace
 
+// static
+bool URLPattern::IsValidSchemeForExtensions(const std::string& scheme) {
+  for (size_t i = 0; i < arraysize(kValidSchemes); ++i) {
+    if (scheme == kValidSchemes[i])
+      return true;
+  }
+  return false;
+}
+
 URLPattern::URLPattern()
     : valid_schemes_(SCHEME_NONE),
       match_all_urls_(false),
diff --git a/extensions/common/url_pattern.h b/extensions/common/url_pattern.h
index 70109db..b4c566b 100644
--- a/extensions/common/url_pattern.h
+++ b/extensions/common/url_pattern.h
@@ -78,6 +78,9 @@ class URLPattern {
   // The <all_urls> string pattern.
   static const char kAllUrlsPattern[];
 
+  // Returns true if the given |scheme| is considered valid for extensions.
+  static bool IsValidSchemeForExtensions(const std::string& scheme);
+
   explicit URLPattern(int valid_schemes);
 
   // Convenience to construct a URLPattern from a string. If the string is not