NTP: don't allow navigateContentWindow to navigate where it pleases.

BUG=509313

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

Cr-Commit-Position: refs/heads/master@{#373598}
diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc
index 9b62978..f21b684e9 100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -42,10 +42,12 @@
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/url_data_source.h"
+#include "content/public/common/url_constants.h"
 #include "grit/theme_resources.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/image/image_skia.h"
+#include "url/url_constants.h"
 
 #if !defined(OS_ANDROID)
 #include "chrome/browser/search/local_ntp_source.h"
@@ -300,6 +302,30 @@
       search::GetSearchURLs(profile_), search::GetNewTabPageURL(profile_)));
 }
 
+bool InstantService::IsValidURLForNavigation(const GURL& url) const {
+  // Certain URLs are privileged and should never be considered valid
+  // navigation targets.
+  // TODO(treib): Ideally this should deny by default and only allow if the
+  // scheme passes the content::ChildProcessSecurityPolicy::IsWebSafeScheme()
+  // check.
+  if (url.SchemeIs(content::kChromeUIScheme))
+    return false;
+
+  // javascript: URLs never make sense as a most visited item either.
+  if (url.SchemeIs(url::kJavaScriptScheme))
+    return false;
+
+  for (const auto& item : most_visited_items_) {
+    if (item.url == url)
+      return true;
+  }
+  for (const auto& item : suggestions_items_) {
+    if (item.url == url)
+      return true;
+  }
+  return false;
+}
+
 void InstantService::OnRendererProcessTerminated(int process_id) {
   process_ids_.erase(process_id);
 
diff --git a/chrome/browser/search/instant_service.h b/chrome/browser/search/instant_service.h
index e711898..49f37d3 100644
--- a/chrome/browser/search/instant_service.h
+++ b/chrome/browser/search/instant_service.h
@@ -87,6 +87,10 @@
   // Sends the current set of search URLs to a renderer process.
   void SendSearchURLsToRenderer(content::RenderProcessHost* rph);
 
+  // Used to validate that the URL the NTP is trying to navigate to is actually
+  // a URL on the most visited items / suggested items list.
+  bool IsValidURLForNavigation(const GURL& url) const;
+
   InstantSearchPrerenderer* instant_search_prerenderer() {
     return instant_prerenderer_.get();
   }
diff --git a/chrome/browser/search/instant_service_unittest.cc b/chrome/browser/search/instant_service_unittest.cc
index dcfe926b..e2836f3 100644
--- a/chrome/browser/search/instant_service_unittest.cc
+++ b/chrome/browser/search/instant_service_unittest.cc
@@ -50,6 +50,14 @@
     return instant_service_->instant_search_prerenderer();
   }
 
+  std::vector<InstantMostVisitedItem>& most_visited_items() {
+    return instant_service_->most_visited_items_;
+  }
+
+  std::vector<InstantMostVisitedItem>& suggestions_items() {
+    return instant_service_->suggestions_items_;
+  }
+
   scoped_ptr<MockInstantServiceObserver> instant_service_observer_;
 };
 
@@ -175,3 +183,45 @@
   ASSERT_EQ(1, (int)items.size());
   ASSERT_FALSE(items[0].is_server_side_suggestion);
 }
+
+TEST_F(InstantServiceTest, IsValidURLForNavigation) {
+  // chrome:// URLs should never appear in the most visited items list, but even
+  // if it does, deny navigation anyway.
+  InstantMostVisitedItem settings_item;
+  settings_item.url = GURL("chrome://settings");
+  most_visited_items().push_back(settings_item);
+  EXPECT_FALSE(instant_service_->IsValidURLForNavigation(settings_item.url));
+
+  // If a chrome-extension:// URL appears in the most visited items list, allow
+  // navigation to it.
+  InstantMostVisitedItem extension_item;
+  extension_item.url = GURL("chrome-extension://awesome");
+  most_visited_items().push_back(extension_item);
+  EXPECT_TRUE(instant_service_->IsValidURLForNavigation(extension_item.url));
+
+  // The renderer filters out javascript:// URLs so we should never receive a
+  // request to navigate to one. But if we do, deny it.
+  InstantMostVisitedItem js_item;
+  js_item.url = GURL("javascript:'moo'");
+  most_visited_items().push_back(js_item);
+  EXPECT_FALSE(instant_service_->IsValidURLForNavigation(js_item.url));
+
+  // Normal case: a request for a web safe URL in the most visited items should
+  // be allowed.
+  InstantMostVisitedItem example_item;
+  example_item.url = GURL("https://example.com");
+  most_visited_items().push_back(example_item);
+  EXPECT_TRUE(instant_service_->IsValidURLForNavigation(example_item.url));
+
+  // Normal case: a request for a web safe URL in the most visited items should
+  // be allowed as well.
+  InstantMostVisitedItem chromium_item;
+  chromium_item.url = GURL("https://chromium.org");
+  suggestions_items().push_back(chromium_item);
+  EXPECT_TRUE(instant_service_->IsValidURLForNavigation(chromium_item.url));
+
+  // http://chromium.org is web safe, but not in |suggestions_items_| or
+  // |most_visited_items_|, so it should be denied.
+  EXPECT_FALSE(
+      instant_service_->IsValidURLForNavigation(GURL("http://chromium.org")));
+}
diff --git a/chrome/browser/ui/search/search_tab_helper.cc b/chrome/browser/ui/search/search_tab_helper.cc
index 65a7f91..20901477 100644
--- a/chrome/browser/ui/search/search_tab_helper.cc
+++ b/chrome/browser/ui/search/search_tab_helper.cc
@@ -450,6 +450,15 @@
 void SearchTabHelper::NavigateToURL(const GURL& url,
                                     WindowOpenDisposition disposition,
                                     bool is_most_visited_item_url) {
+  // Make sure the specified URL is actually on the most visited or suggested
+  // items list.
+  // TODO(treib): The |is_most_visited_item_url| is meaningless: the way it's
+  // currently set by the renderer means it can't be used to decide which list
+  // of items (most visited or suggestions) to use for the validation check. Can
+  // it be removed?
+  if (!instant_service_ || !instant_service_->IsValidURLForNavigation(url))
+    return;
+
   if (is_most_visited_item_url) {
     content::RecordAction(
         base::UserMetricsAction("InstantExtended.MostVisitedClicked"));
diff --git a/chrome/renderer/searchbox/searchbox_extension.cc b/chrome/renderer/searchbox/searchbox_extension.cc
index 5dc7ae5..7e3af7a 100644
--- a/chrome/renderer/searchbox/searchbox_extension.cc
+++ b/chrome/renderer/searchbox/searchbox_extension.cc
@@ -1164,7 +1164,9 @@
 
   DVLOG(1) << render_view << " NavigateContentWindow: " << destination_url;
 
-  // Navigate the main frame.
+  // Navigate the main frame. Note that the security checks are enforced by the
+  // browser process in InstantService::IsValidURLForNavigation(), but some
+  // simple checks here are useful for avoiding unnecessary IPCs.
   if (destination_url.is_valid() &&
       !destination_url.SchemeIs(url::kJavaScriptScheme)) {
     WindowOpenDisposition disposition = CURRENT_TAB;