Remove "open in new tab" items from context menu if the process doesn't

have permission to open them directly.  For example, right-click on a
"chrome://" link in an ordinary window should show two items: Copy link
location and inspect element, but a full menu from a WebUI window itself.

NOTE: Fixing this issue requires a fix to ChildProcessSecurityPolicy, which as been silently too permissive.

BUG=104466
Review URL: http://codereview.chromium.org/8588039

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113818 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 176 insertions, 35 deletions

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 6856cbf..7cba0c3 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -428,6 +428,10 @@ bool ChromeContentBrowserClient::IsURLSameAsAnySiteInstance(const GURL& url) {
          url == GURL(chrome::kChromeUIShorthangURL);
 }
 
+bool ChromeContentBrowserClient::IsHandledURL(const GURL& url) {
+  return ProfileIOData::IsHandledURL(url);
+}
+
 bool ChromeContentBrowserClient::IsSuitableHost(
     content::RenderProcessHost* process_host,
     const GURL& site_url) {
diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
index 3e495a0..243b804 100644
--- a/chrome/browser/chrome_content_browser_client.h
+++ b/chrome/browser/chrome_content_browser_client.h
@@ -31,6 +31,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   virtual GURL GetEffectiveURL(content::BrowserContext* browser_context,
                                const GURL& url) OVERRIDE;
   virtual bool IsURLSameAsAnySiteInstance(const GURL& url) OVERRIDE;
+  virtual bool IsHandledURL(const GURL& url) OVERRIDE;
   virtual bool IsSuitableHost(content::RenderProcessHost* process_host,
                               const GURL& url) OVERRIDE;
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) OVERRIDE;
diff --git a/chrome/browser/custom_handlers/protocol_handler_registry_browsertest.cc b/chrome/browser/custom_handlers/protocol_handler_registry_browsertest.cc
index 6a5a887..91c8543 100644
--- a/chrome/browser/custom_handlers/protocol_handler_registry_browsertest.cc
+++ b/chrome/browser/custom_handlers/protocol_handler_registry_browsertest.cc
@@ -42,6 +42,7 @@ class RegisterProtocolHandlerBrowserTest : public InProcessBrowserTest {
     ContextMenuParams params;
     params.media_type = WebKit::WebContextMenuData::MediaTypeNone;
     params.link_url = url;
+    params.unfiltered_link_url = url;
     TabContents* tab_contents = browser()->GetSelectedTabContents();
     params.page_url = tab_contents->controller().GetActiveEntry()->url();
 #if defined(OS_MACOSX)
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index fe2ec8f9..a368929 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -507,7 +507,7 @@ void RenderViewContextMenu::AppendAllExtensionItems() {
 }
 
 void RenderViewContextMenu::InitMenu() {
-  bool has_link = !params_.link_url.is_empty();
+  bool has_link = !params_.unfiltered_link_url.is_empty();
   bool has_selection = !params_.selection_text.empty();
 
   if (AppendCustomItems()) {
@@ -667,19 +667,22 @@ void RenderViewContextMenu::AppendDeveloperItems() {
 }
 
 void RenderViewContextMenu::AppendLinkItems() {
-  menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKNEWTAB,
-                                  IDS_CONTENT_CONTEXT_OPENLINKNEWTAB);
-  menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKNEWWINDOW,
-                                  IDS_CONTENT_CONTEXT_OPENLINKNEWWINDOW);
-  if (params_.link_url.is_valid()) {
-    AppendProtocolHandlerSubMenu();
-  }
-  if (!external_) {
-    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD,
-                                    IDS_CONTENT_CONTEXT_OPENLINKOFFTHERECORD);
+  if (!params_.link_url.is_empty()) {
+    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKNEWTAB,
+                                    IDS_CONTENT_CONTEXT_OPENLINKNEWTAB);
+    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKNEWWINDOW,
+                                    IDS_CONTENT_CONTEXT_OPENLINKNEWWINDOW);
+    if (params_.link_url.is_valid()) {
+      AppendProtocolHandlerSubMenu();
+    }
+
+    if (!external_) {
+      menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD,
+                                      IDS_CONTENT_CONTEXT_OPENLINKOFFTHERECORD);
+    }
+    menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_SAVELINKAS,
+                                    IDS_CONTENT_CONTEXT_SAVELINKAS);
   }
-  menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_SAVELINKAS,
-                                  IDS_CONTENT_CONTEXT_SAVELINKAS);
 
   menu_model_.AddItemWithStringId(
       IDC_CONTENT_CONTEXT_COPYLINKLOCATION,
diff --git a/chrome/browser/tab_contents/render_view_context_menu_browsertest.cc b/chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
new file mode 100644
index 0000000..b198ab1
--- /dev/null
+++ b/chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
@@ -0,0 +1,82 @@
+// Copyright (c) 2011 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 <string>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/tab_contents/render_view_context_menu.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/browser/tab_contents/tab_contents.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebContextMenuData.h"
+
+namespace {
+
+class TestRenderViewContextMenu : public RenderViewContextMenu {
+ public:
+  TestRenderViewContextMenu(TabContents* tab_contents, ContextMenuParams params)
+      : RenderViewContextMenu(tab_contents, params) { }
+
+  virtual void PlatformInit() { }
+  virtual bool GetAcceleratorForCommandId(
+      int command_id,
+      ui::Accelerator* accelerator) {
+    return false;
+  }
+
+  bool IsItemPresent(int command_id) {
+    return menu_model_.GetIndexOfCommandId(command_id) != -1;
+  }
+};
+
+class ContextMenuBrowserTest : public InProcessBrowserTest {
+ public:
+  ContextMenuBrowserTest() { }
+
+  TestRenderViewContextMenu* CreateContextMenu(GURL unfiltered_url, GURL url) {
+    ContextMenuParams params;
+    params.media_type = WebKit::WebContextMenuData::MediaTypeNone;
+    params.unfiltered_link_url = unfiltered_url;
+    params.link_url = url;
+    TabContents* tab_contents = browser()->GetSelectedTabContents();
+    params.page_url = tab_contents->controller().GetActiveEntry()->url();
+#if defined(OS_MACOSX)
+    params.writing_direction_default = 0;
+    params.writing_direction_left_to_right = 0;
+    params.writing_direction_right_to_left = 0;
+#endif  // OS_MACOSX
+    TestRenderViewContextMenu* menu = new TestRenderViewContextMenu(
+        browser()->GetSelectedTabContents(), params);
+    menu->Init();
+    return menu;
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
+                       OpenEntryPresentForNormalURLs) {
+  scoped_ptr<TestRenderViewContextMenu> menu(
+      CreateContextMenu(GURL("http://www.google.com/"),
+                        GURL("http://www.google.com/")));
+
+  ASSERT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_OPENLINKNEWTAB));
+  ASSERT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_OPENLINKNEWWINDOW));
+  ASSERT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_COPYLINKLOCATION));
+}
+
+IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest,
+                       OpenEntryAbsentForFilteredURLs) {
+  scoped_ptr<TestRenderViewContextMenu> menu(
+      CreateContextMenu(GURL("chrome://history"),
+                        GURL()));
+
+  ASSERT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_OPENLINKNEWTAB));
+  ASSERT_FALSE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_OPENLINKNEWWINDOW));
+  ASSERT_TRUE(menu->IsItemPresent(IDC_CONTENT_CONTEXT_COPYLINKLOCATION));
+}
+
+}  // namespace
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 0dce1f2..4abc169 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -2622,6 +2622,7 @@
         'browser/speech/speech_input_extension_apitest.cc',
         'browser/spellchecker/spellcheck_host_browsertest.cc',
         'browser/ssl/ssl_browser_tests.cc',
+        'browser/tab_contents/render_view_context_menu_browsertest.cc',
         'browser/tab_first_render_watcher_browsertest.cc',
         'browser/task_manager/task_manager_browsertest.cc',
         'browser/task_manager/task_manager_browsertest_util.cc',
diff --git a/content/browser/child_process_security_policy.cc b/content/browser/child_process_security_policy.cc
index cd61fd4..eeeac3f 100644
--- a/content/browser/child_process_security_policy.cc
+++ b/content/browser/child_process_security_policy.cc
@@ -10,6 +10,7 @@
 #include "base/platform_file.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
+#include "content/public/browser/content_browser_client.h"
 #include "content/browser/site_instance.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/url_constants.h"
@@ -382,8 +383,10 @@ bool ChildProcessSecurityPolicy::CanRequestURL(
     return false;
   }
 
-  if (!net::URLRequest::IsHandledURL(url))
+  if (!content::GetContentClient()->browser()->IsHandledURL(url) &&
+      !net::URLRequest::IsHandledURL(url)) {
     return true;  // This URL request is destined for ShellExecute.
+  }
 
   {
     base::AutoLock lock(lock_);
diff --git a/content/browser/child_process_security_policy_unittest.cc b/content/browser/child_process_security_policy_unittest.cc
index 592b582..032978f 100644
--- a/content/browser/child_process_security_policy_unittest.cc
+++ b/content/browser/child_process_security_policy_unittest.cc
@@ -2,40 +2,75 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/platform_file.h"
 #include "content/browser/child_process_security_policy.h"
+#include "content/browser/mock_content_browser_client.h"
 #include "content/common/test_url_constants.h"
 #include "content/public/common/url_constants.h"
-#include "net/url_request/url_request.h"
-#include "net/url_request/url_request_test_job.h"
+#include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+const int kRendererID = 42;
+const int kWorkerRendererID = kRendererID + 1;
+
+class ChildProcessSecurityPolicyTestBrowserClient
+    : public content::MockContentBrowserClient {
+ public:
+  ChildProcessSecurityPolicyTestBrowserClient() {}
+
+  virtual bool IsHandledURL(const GURL& url) {
+    return schemes_.find(url.scheme()) != schemes_.end();
+  }
+
+  void ClearSchemes() {
+    schemes_.clear();
+  }
+
+  void AddScheme(const std::string& scheme) {
+    schemes_.insert(scheme);
+  }
+
+ private:
+  std::set<std::string> schemes_;
+};
+
+}  // namespace
+
 class ChildProcessSecurityPolicyTest : public testing::Test {
- protected:
-  // testing::Test
+ public:
+  ChildProcessSecurityPolicyTest() : old_browser_client_(NULL) {
+  }
+
   virtual void SetUp() {
-    // In the real world, "chrome:" is a handled scheme.
-    RegisterProtocolFactory(chrome::kChromeUIScheme,
-                            &net::URLRequestTestJob::Factory);
+    old_browser_client_ = content::GetContentClient()->browser();
+    content::GetContentClient()->set_browser(&test_browser_client_);
+
+    // Claim to always handle chrome:// URLs because the CPSP's notion of
+    // allowing WebUI bindings is hard-wired to this particular scheme.
+    test_browser_client_.AddScheme("chrome");
   }
+
   virtual void TearDown() {
-    RegisterProtocolFactory(chrome::kChromeUIScheme, NULL);
+    test_browser_client_.ClearSchemes();
+    content::GetContentClient()->set_browser(old_browser_client_);
   }
 
-  static net::URLRequest::ProtocolFactory* RegisterProtocolFactory(
-      const std::string& scheme,
-      net::URLRequest::ProtocolFactory* factory) {
-    return net::URLRequest::Deprecated::RegisterProtocolFactory(
-        scheme, factory);
+ protected:
+  void RegisterTestScheme(const std::string& scheme) {
+    test_browser_client_.AddScheme(scheme);
   }
-};
 
-static int kRendererID = 42;
-static int kWorkerRendererID = kRendererID + 1;
+ private:
+  ChildProcessSecurityPolicyTestBrowserClient test_browser_client_;
+  content::ContentBrowserClient* old_browser_client_;
+};
 
 TEST_F(ChildProcessSecurityPolicyTest, IsWebSafeSchemeTest) {
   ChildProcessSecurityPolicy* p = ChildProcessSecurityPolicy::GetInstance();
@@ -174,8 +209,8 @@ TEST_F(ChildProcessSecurityPolicyTest, RegisterWebSafeSchemeTest) {
   // Currently, "asdf" is destined for ShellExecute, so it is allowed.
   EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL("asdf:rockers")));
 
-  // Once we register a ProtocolFactory for "asdf", we default to deny.
-  RegisterProtocolFactory("asdf", &net::URLRequestTestJob::Factory);
+  // Once we register "asdf", we default to deny.
+  RegisterTestScheme("asdf");
   EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("asdf:rockers")));
 
   // We can allow new schemes by adding them to the whitelist.
@@ -183,9 +218,6 @@ TEST_F(ChildProcessSecurityPolicyTest, RegisterWebSafeSchemeTest) {
   EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL("asdf:rockers")));
 
   // Cleanup.
-  RegisterProtocolFactory("asdf", NULL);
-  EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL("asdf:rockers")));
-
   p->Remove(kRendererID);
 }
 
diff --git a/content/browser/mock_content_browser_client.cc b/content/browser/mock_content_browser_client.cc
index 507eac5..13de94f 100644
--- a/content/browser/mock_content_browser_client.cc
+++ b/content/browser/mock_content_browser_client.cc
@@ -89,6 +89,10 @@ bool MockContentBrowserClient::IsURLSameAsAnySiteInstance(const GURL& url) {
   return false;
 }
 
+bool MockContentBrowserClient::IsHandledURL(const GURL& url) {
+  return false;
+}
+
 bool MockContentBrowserClient::IsSuitableHost(
     RenderProcessHost* process_host,
     const GURL& site_url) {
diff --git a/content/browser/mock_content_browser_client.h b/content/browser/mock_content_browser_client.h
index fc6f5e77..37e866e 100644
--- a/content/browser/mock_content_browser_client.h
+++ b/content/browser/mock_content_browser_client.h
@@ -38,6 +38,7 @@ class MockContentBrowserClient : public ContentBrowserClient {
   virtual bool ShouldUseProcessPerSite(BrowserContext* browser_context,
                                        const GURL& effective_url) OVERRIDE;
   virtual bool IsURLSameAsAnySiteInstance(const GURL& url) OVERRIDE;
+  virtual bool IsHandledURL(const GURL& url) OVERRIDE;
   virtual bool IsSuitableHost(RenderProcessHost* process_host,
                               const GURL& site_url) OVERRIDE;
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) OVERRIDE;
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 080a403..d987fc0 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -133,6 +133,10 @@ class ContentBrowserClient {
   // SiteInstance.
   virtual bool IsURLSameAsAnySiteInstance(const GURL& url) = 0;
 
+  // Returns whether a specified URL is handled by the embedder's internal
+  // protocol handlers.
+  virtual bool IsHandledURL(const GURL& url) = 0;
+
   // Returns whether a new view for a given |site_url| can be launched in a
   // given |process_host|.
   virtual bool IsSuitableHost(content::RenderProcessHost* process_host,
diff --git a/content/shell/shell_content_browser_client.cc b/content/shell/shell_content_browser_client.cc
index 0958917..8218605 100644
--- a/content/shell/shell_content_browser_client.cc
+++ b/content/shell/shell_content_browser_client.cc
@@ -82,6 +82,10 @@ bool ShellContentBrowserClient::IsURLSameAsAnySiteInstance(const GURL& url) {
   return false;
 }
 
+bool ShellContentBrowserClient::IsHandledURL(const GURL& url) {
+  return false;
+}
+
 bool ShellContentBrowserClient::IsSuitableHost(
     RenderProcessHost* process_host,
     const GURL& site_url) {
diff --git a/content/shell/shell_content_browser_client.h b/content/shell/shell_content_browser_client.h
index 65219e0..383ebfd 100644
--- a/content/shell/shell_content_browser_client.h
+++ b/content/shell/shell_content_browser_client.h
@@ -50,6 +50,7 @@ class ShellContentBrowserClient : public ContentBrowserClient
   virtual bool ShouldUseProcessPerSite(BrowserContext* browser_context,
                                        const GURL& effective_url) OVERRIDE;
   virtual bool IsURLSameAsAnySiteInstance(const GURL& url) OVERRIDE;
+  virtual bool IsHandledURL(const GURL& url) OVERRIDE;
   virtual bool IsSuitableHost(RenderProcessHost* process_host,
                               const GURL& site_url) OVERRIDE;
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) OVERRIDE;