Simplify AreURLsInPageNavigation

The first case guarantees we trust the renderer's definition of an in-page navigation if the urls before and after navigation are on the same origin. An in-page navigation is impossible cross-origin, so this single check is sufficient.

BUG=

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

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

7 files changed, 49 insertions, 41 deletions

diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 6d1c597..a751ccd 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -104,24 +104,30 @@ void ConfigureEntriesForRestore(
   }
 }
 
-// See NavigationController::IsURLInPageNavigation for how this works and why.
+// There are two general cases where a navigation is in page:
+// 1. A fragment navigation, in which the url is kept the same except for the
+//    reference fragment.
+// 2. A history API navigation (pushState and replaceState). This case is
+//    always in-page, but the urls are not guaranteed to match excluding the
+//    fragment. The relevant spec allows pushState/replaceState to any URL on
+//    the same origin.
+// However, due to reloads, even identical urls are *not* guaranteed to be
+// in-page navigations, we have to trust the renderer almost entirely.
+// The one thing we do know is that cross-origin navigations will *never* be
+// in-page. Therefore, trust the renderer if the URLs are on the same origin,
+// and assume the renderer is malicious if a cross-origin navigation claims to
+// be in-page.
 bool AreURLsInPageNavigation(const GURL& existing_url,
                              const GURL& new_url,
                              bool renderer_says_in_page,
-                             NavigationType navigation_type) {
-  if (existing_url.GetOrigin() == new_url.GetOrigin())
-    return renderer_says_in_page;
-
-  if (!new_url.has_ref()) {
-    // When going back from the ref URL to the non ref one the navigation type
-    // is IN_PAGE.
-    return navigation_type == NAVIGATION_TYPE_IN_PAGE;
-  }
-
-  url::Replacements<char> replacements;
-  replacements.ClearRef();
-  return existing_url.ReplaceComponents(replacements) ==
-      new_url.ReplaceComponents(replacements);
+                             RenderFrameHost* rfh) {
+  WebPreferences prefs = rfh->GetRenderViewHost()->GetWebkitPreferences();
+  bool is_same_origin = existing_url.is_empty() ||
+                        existing_url.GetOrigin() == new_url.GetOrigin() ||
+                        !prefs.web_security_enabled;
+  if (!is_same_origin && renderer_says_in_page)
+      rfh->GetProcess()->ReceivedBadMessage();
+  return is_same_origin && renderer_says_in_page;
 }
 
 // Determines whether or not we should be carrying over a user agent override
@@ -766,8 +772,8 @@ bool NavigationControllerImpl::RendererDidNavigate(
   details->type = ClassifyNavigation(rfh, params);
 
   // is_in_page must be computed before the entry gets committed.
-  details->is_in_page = IsURLInPageNavigation(
-      params.url, params.was_within_same_page, details->type);
+  details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(),
+      params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
@@ -986,8 +992,7 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
   // navigations that don't actually navigate, but it can happen when there is
   // an encoding override (it always sends a navigation request).
   if (AreURLsInPageNavigation(existing_entry->GetURL(), params.url,
-                              params.was_within_same_page,
-                              NAVIGATION_TYPE_UNKNOWN)) {
+                              params.was_within_same_page, rfh)) {
     return NAVIGATION_TYPE_IN_PAGE;
   }
 
@@ -1253,10 +1258,10 @@ int NavigationControllerImpl::GetIndexOfEntry(
 bool NavigationControllerImpl::IsURLInPageNavigation(
     const GURL& url,
     bool renderer_says_in_page,
-    NavigationType navigation_type) const {
+    RenderFrameHost* rfh) const {
   NavigationEntry* last_committed = GetLastCommittedEntry();
   return last_committed && AreURLsInPageNavigation(
-      last_committed->GetURL(), url, renderer_says_in_page, navigation_type);
+      last_committed->GetURL(), url, renderer_says_in_page, rfh);
 }
 
 void NavigationControllerImpl::CopyStateFrom(
diff --git a/content/browser/frame_host/navigation_controller_impl.h b/content/browser/frame_host/navigation_controller_impl.h
index 7a06ba66..82fac02 100644
--- a/content/browser/frame_host/navigation_controller_impl.h
+++ b/content/browser/frame_host/navigation_controller_impl.h
@@ -167,7 +167,7 @@ class CONTENT_EXPORT NavigationControllerImpl
   bool IsURLInPageNavigation(
       const GURL& url,
       bool renderer_says_in_page,
-      NavigationType navigation_type) const;
+      RenderFrameHost* rfh) const;
 
   // Sets the SessionStorageNamespace for the given |partition_id|. This is
   // used during initialization of a new NavigationController to allow
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index 956288c..c6b4f37 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -3080,48 +3080,52 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) {
 
   // Reloading the page is not an in-page navigation.
   EXPECT_FALSE(controller.IsURLInPageNavigation(url, false,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
   const GURL other_url("http://www.google.com/add.html");
   EXPECT_FALSE(controller.IsURLInPageNavigation(other_url, false,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
   const GURL url_with_ref("http://www.google.com/home.html#my_ref");
   EXPECT_TRUE(controller.IsURLInPageNavigation(url_with_ref, true,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
 
   // Navigate to URL with refs.
   main_test_rfh()->SendNavigate(1, url_with_ref);
 
   // Reloading the page is not an in-page navigation.
   EXPECT_FALSE(controller.IsURLInPageNavigation(url_with_ref, false,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
   EXPECT_FALSE(controller.IsURLInPageNavigation(url, false,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
   EXPECT_FALSE(controller.IsURLInPageNavigation(other_url, false,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
   const GURL other_url_with_ref("http://www.google.com/home.html#my_other_ref");
   EXPECT_TRUE(controller.IsURLInPageNavigation(other_url_with_ref, true,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
 
   // Going to the same url again will be considered in-page
   // if the renderer says it is even if the navigation type isn't IN_PAGE.
   EXPECT_TRUE(controller.IsURLInPageNavigation(url_with_ref, true,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
 
   // Going back to the non ref url will be considered in-page if the navigation
   // type is IN_PAGE.
   EXPECT_TRUE(controller.IsURLInPageNavigation(url, true,
-      NAVIGATION_TYPE_IN_PAGE));
+      main_test_rfh()));
 
   // If the renderer says this is a same-origin in-page navigation, believe it.
   // This is the pushState/replaceState case.
   EXPECT_TRUE(controller.IsURLInPageNavigation(other_url, true,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
 
   // Don't believe the renderer if it claims a cross-origin navigation is
   // in-page.
   const GURL different_origin_url("http://www.example.com");
+  MockRenderProcessHost* rph =
+      static_cast<MockRenderProcessHost*>(main_test_rfh()->GetProcess());
+  EXPECT_EQ(0, rph->bad_msg_count());
   EXPECT_FALSE(controller.IsURLInPageNavigation(different_origin_url, true,
-      NAVIGATION_TYPE_UNKNOWN));
+      main_test_rfh()));
+  EXPECT_EQ(1, rph->bad_msg_count());
 }
 
 // Some pages can have subframes with the same base URL (minus the reference) as
diff --git a/content/browser/frame_host/navigator_delegate.h b/content/browser/frame_host/navigator_delegate.h
index 0835fe0..70c7619 100644
--- a/content/browser/frame_host/navigator_delegate.h
+++ b/content/browser/frame_host/navigator_delegate.h
@@ -61,8 +61,7 @@ class CONTENT_EXPORT NavigatorDelegate {
 
   // Handles post-navigation tasks in navigation BEFORE the entry has been
   // committed to the NavigationController.
-  virtual void DidNavigateMainFramePreCommit(
-      const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {}
+  virtual void DidNavigateMainFramePreCommit(bool navigation_is_within_page) {}
 
   // Handles post-navigation tasks in navigation AFTER the entry has been
   // committed to the NavigationController. Note that the NavigationEntry is
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index a6b00f7..1af57c7 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -447,7 +447,9 @@ void NavigatorImpl::DidNavigate(
       }
 
       // Run tasks that must execute just before the commit.
-      delegate_->DidNavigateMainFramePreCommit(params);
+      bool is_navigation_within_page = controller_->IsURLInPageNavigation(
+          params.url, params.was_within_same_page, render_frame_host);
+      delegate_->DidNavigateMainFramePreCommit(is_navigation_within_page);
     }
 
     if (!use_site_per_process)
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 064052e..62260ff 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2433,13 +2433,11 @@ void WebContentsImpl::DidCommitProvisionalLoad(
 }
 
 void WebContentsImpl::DidNavigateMainFramePreCommit(
-    const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
+    bool navigation_is_within_page) {
   // Ensure fullscreen mode is exited before committing the navigation to a
   // different page.  The next page will not start out assuming it is in
   // fullscreen mode.
-  if (controller_.IsURLInPageNavigation(params.url,
-                                        params.was_within_same_page,
-                                        NAVIGATION_TYPE_UNKNOWN)) {
+  if (navigation_is_within_page) {
     // No page change?  Then, the renderer and browser can remain in fullscreen.
     return;
   }
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 9050689..f7a3c7b 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -472,7 +472,7 @@ class CONTENT_EXPORT WebContentsImpl
       const GURL& url,
       PageTransition transition_type) OVERRIDE;
   virtual void DidNavigateMainFramePreCommit(
-      const FrameHostMsg_DidCommitProvisionalLoad_Params& params) OVERRIDE;
+      bool navigation_is_within_page) OVERRIDE;
   virtual void DidNavigateMainFramePostCommit(
       const LoadCommittedDetails& details,
       const FrameHostMsg_DidCommitProvisionalLoad_Params& params) OVERRIDE;