Remove URL fragment in referrer chain

Clear URL fragment before recording navigation events and before
matching event url or webcontents url against navigation events.

Bug: 776587
Change-Id: I91ee7b9365dd365a5971b64fee55f4df7d395936
Reviewed-on: https://chromium-review.googlesource.com/745318Reviewed-by: Robert Shield <robertshield@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513214}

chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc

@@ -160,12 +160,11 @@ void SafeBrowsingNavigationObserver::DidStartNavigation(
   // committed navigation.
   if (navigation_handle->IsRendererInitiated() && current_frame_host &&
       current_frame_host->GetLastCommittedURL().is_valid()) {
-    nav_event->source_url =
-        SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+    nav_event->source_url = SafeBrowsingNavigationObserverManager::ClearURLRef(
         current_frame_host->GetLastCommittedURL());
   }
   nav_event->original_request_url =
-      SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+      SafeBrowsingNavigationObserverManager::ClearURLRef(
           navigation_handle->GetURL());
 
   nav_event->source_tab_id =
@@ -175,7 +174,7 @@ void SafeBrowsingNavigationObserver::DidStartNavigation(
     nav_event->source_main_frame_url = nav_event->source_url;
   } else {
     nav_event->source_main_frame_url =
-        SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+        SafeBrowsingNavigationObserverManager::ClearURLRef(
             navigation_handle->GetWebContents()->GetLastCommittedURL());
   }
   navigation_handle_map_[navigation_handle] = std::move(nav_event);
@@ -190,7 +189,7 @@ void SafeBrowsingNavigationObserver::DidRedirectNavigation(
   }
   NavigationEvent* nav_event = navigation_handle_map_[navigation_handle].get();
   nav_event->server_redirect_urls.push_back(
-      SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+      SafeBrowsingNavigationObserverManager::ClearURLRef(
           navigation_handle->GetURL()));
   nav_event->last_updated = base::Time::Now();
 }

chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc

@@ -75,6 +75,9 @@ const char kLandingURL[] =
 const char kLandingReferrerURL[] =
     "/safe_browsing/download_protection/navigation_observer/"
     "landing_referrer.html";
+const char kLandingReferrerURLWithQuery[] =
+    "/safe_browsing/download_protection/navigation_observer/"
+    "landing_referrer.html?bar=foo";
 const char kPageBeforeLandingReferrerURL[] =
     "/safe_browsing/download_protection/navigation_observer/"
     "page_before_landing_referrer.html";
@@ -385,6 +388,14 @@ class SBNavigationObserverBrowserTest : public InProcessBrowserTest {
     }
   }
 
+  void IdentifyReferrerChainForWebContents(content::WebContents* web_contents,
+                                           ReferrerChain* referrer_chain) {
+    observer_manager_->IdentifyReferrerChainByWebContents(
+        web_contents,
+        2,  // kDownloadAttributionUserGestureLimit
+        referrer_chain);
+  }
+
   // Identify referrer chain of a PPAPI download and populate |referrer_chain|.
   void IdentifyReferrerChainForPPAPIDownload(
       const GURL& initiating_frame_url,
@@ -2024,6 +2035,56 @@ IN_PROC_BROWSER_TEST_F(SBNavigationObserverBrowserTest,
                            referrer_chain.Get(0));
 }
 
+// Verify referrer chain when there are URL fragments.
+IN_PROC_BROWSER_TEST_F(SBNavigationObserverBrowserTest,
+                       DownloadAttributionWithURLFragment) {
+  GURL initial_url = embedded_test_server()->GetURL(kSingleFrameTestURL);
+  // Clicks on link and navigates to ".../page_before_landing_referrer.html".
+  ClickTestLink("attribution_should_ignore_url_fragments", 1, initial_url);
+  GURL expected_page_before_landing_referrer_url =
+      embedded_test_server()->GetURL(kPageBeforeLandingReferrerURL);
+  // Clicks on link and navigates to ".../landing_referrer.html?bar=foo#baz".
+  ClickTestLink("link_to_landing_referrer_with_query_and_fragment", 1,
+                expected_page_before_landing_referrer_url);
+  GURL expected_landing_referrer_url_with_query =
+      embedded_test_server()->GetURL(kLandingReferrerURLWithQuery);
+  // Clicks on link and navigates to ".../landing.html#".
+  ClickTestLink("link_to_landing_with_empty_fragment", 1,
+                expected_landing_referrer_url_with_query);
+  GURL expected_landing_url = embedded_test_server()->GetURL(kLandingURL);
+
+  std::string test_server_ip(embedded_test_server()->host_port_pair().host());
+  auto* nav_list = navigation_event_list();
+  ASSERT_EQ(4U, nav_list->Size());
+
+  ReferrerChain referrer_chain;
+  SimulateUserGesture();
+  IdentifyReferrerChainForWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents(), &referrer_chain);
+  ASSERT_EQ(2, referrer_chain.size());
+
+  // Verify url fragment is cleared in referrer chain.
+  VerifyReferrerChainEntry(expected_landing_url,              // url
+                           GURL(),                            // main_frame_url
+                           ReferrerChainEntry::LANDING_PAGE,  // type
+                           test_server_ip,                    // ip_address
+                           expected_landing_referrer_url_with_query,
+                           GURL(),               // referrer_main_frame_url
+                           false,                // is_retargeting
+                           std::vector<GURL>(),  // server redirects
+                           referrer_chain.Get(0));
+  VerifyReferrerChainEntry(
+      expected_landing_referrer_url_with_query,  // url
+      GURL(),                                    // main_frame_url
+      ReferrerChainEntry::LANDING_REFERRER,      // type
+      test_server_ip,                            // ip_address
+      GURL(),  // referrer_url is empty since this beyonds 2 clicks.
+      GURL(),  // referrer_main_frame_url is empty for the same reason.
+      false,   // is_retargeting
+      std::vector<GURL>(),  // server redirects
+      referrer_chain.Get(1));
+}
+
 IN_PROC_BROWSER_TEST_F(SBNavigationObserverBrowserTest,
                        VerifySanitizeReferrerChain) {
   GURL initial_url = embedded_test_server()->GetURL(kSingleFrameTestURL);

chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc

@@ -175,7 +175,7 @@ NavigationEvent* NavigationEventList::FindRetargetingNavigationEvent(
 
 void NavigationEventList::RecordNavigationEvent(
     std::unique_ptr<NavigationEvent> nav_event) {
-  // Skip page refresh.
+  // Skip page refresh and in-page navigation.
   if (nav_event->source_url == nav_event->GetDestinationUrl() &&
       nav_event->source_tab_id == nav_event->target_tab_id)
     return;
@@ -206,8 +206,8 @@ bool SafeBrowsingNavigationObserverManager::IsUserGestureExpired(
 }
 
 // static
-GURL SafeBrowsingNavigationObserverManager::ClearEmptyRef(const GURL& url) {
-  if (url.has_ref() && url.ref().empty()) {
+GURL SafeBrowsingNavigationObserverManager::ClearURLRef(const GURL& url) {
+  if (url.has_ref()) {
     url::Replacements<char> replacements;
     replacements.ClearRef();
     return url.ReplaceComponents(replacements);
@@ -341,7 +341,7 @@ SafeBrowsingNavigationObserverManager::IdentifyReferrerChainByEventURL(
     return INVALID_URL;
 
   NavigationEvent* nav_event = navigation_event_list_.FindNavigationEvent(
-      event_url, GURL(), event_tab_id);
+      ClearURLRef(event_url), GURL(), event_tab_id);
   if (!nav_event) {
     // We cannot find a single navigation event related to this event.
     return NAVIGATION_EVENT_NOT_FOUND;
@@ -364,12 +364,13 @@ SafeBrowsingNavigationObserverManager::IdentifyReferrerChainByWebContents(
     content::WebContents* web_contents,
     int user_gesture_count_limit,
     ReferrerChain* out_referrer_chain) {
-  if (!web_contents || !web_contents->GetLastCommittedURL().is_valid())
+  GURL last_committed_url = web_contents->GetLastCommittedURL();
+  if (!web_contents || !last_committed_url.is_valid())
     return INVALID_URL;
   bool has_user_gesture = HasUserGesture(web_contents);
   int tab_id = SessionTabHelper::IdForTab(web_contents);
   return IdentifyReferrerChainByHostingPage(
-      web_contents->GetLastCommittedURL(), GURL(), tab_id, has_user_gesture,
+      ClearURLRef(last_committed_url), GURL(), tab_id, has_user_gesture,
       user_gesture_count_limit, out_referrer_chain);
 }
 
@@ -385,7 +386,8 @@ SafeBrowsingNavigationObserverManager::IdentifyReferrerChainByHostingPage(
     return INVALID_URL;
 
   NavigationEvent* nav_event = navigation_event_list_.FindNavigationEvent(
-      initiating_frame_url, initiating_main_frame_url, tab_id);
+      ClearURLRef(initiating_frame_url), ClearURLRef(initiating_main_frame_url),
+      tab_id);
   if (!nav_event) {
     // We cannot find a single navigation event related to this hosting page.
     return NAVIGATION_EVENT_NOT_FOUND;
@@ -434,18 +436,17 @@ void SafeBrowsingNavigationObserverManager::RecordNewWebContents(
   // Remove the "#" at the end of URL, since it does not point to any actual
   // page fragment ID.
   GURL cleaned_target_url =
-      SafeBrowsingNavigationObserverManager::ClearEmptyRef(target_url);
+      SafeBrowsingNavigationObserverManager::ClearURLRef(target_url);
 
   std::unique_ptr<NavigationEvent> nav_event =
       base::MakeUnique<NavigationEvent>();
   if (rfh) {
-    nav_event->source_url =
-        SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+    nav_event->source_url = SafeBrowsingNavigationObserverManager::ClearURLRef(
         rfh->GetLastCommittedURL());
   }
   nav_event->source_tab_id = SessionTabHelper::IdForTab(source_web_contents);
   nav_event->source_main_frame_url =
-      SafeBrowsingNavigationObserverManager::ClearEmptyRef(
+      SafeBrowsingNavigationObserverManager::ClearURLRef(
           source_web_contents->GetLastCommittedURL());
   nav_event->original_request_url = cleaned_target_url;
   nav_event->target_tab_id = SessionTabHelper::IdForTab(target_web_contents);

chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h

@@ -111,11 +111,12 @@ class SafeBrowsingNavigationObserverManager
   // kUserGestureTTLInSecond.
   static bool IsUserGestureExpired(const base::Time& timestamp);
 
-  // Helper function to strip empty ref fragment from a URL. Many pages
-  // end up with a "#" at the end of their URLs due to navigation triggered by
+  // Helper function to strip ref fragment from a URL. Many pages end up with a
+  // fragment (e.g. http://bar.com/index.html#foo) at the end due to in-page
+  // navigation or a single "#" at the end due to navigation triggered by
   // href="#" and javascript onclick function. We don't want to have separate
   // entries for these cases in the maps.
-  static GURL ClearEmptyRef(const GURL& url);
+  static GURL ClearURLRef(const GURL& url);
 
   // Checks if we should enable observing navigations for safe browsing purpose.
   // Return true if the safe browsing safe browsing service is enabled and

chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html

@@ -13,5 +13,8 @@
     <a id="link_to_landing" href="landing.html">
       Link to landing
     </a><br>
+    <a id="link_to_landing_with_empty_fragment" href="landing.html#">
+      Link to landing with empty fragment
+    </a><br>
   </body>
 </html>

chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html

@@ -145,5 +145,9 @@
       Download via HTML5 file system API without trigger navigation
     </button><br>
 
+    <a id="attribution_should_ignore_url_fragments" href="page_before_landing_referrer.html">
+      Attribution should ignore url fragments.
+    </a><br>
+
   </body>
 </html>

chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html

@@ -15,5 +15,8 @@
     <a id="link_to_landing_referrer" href="landing_referrer.html">
       Link to landing referrer
     </a><br>
+    <a id="link_to_landing_referrer_with_query_and_fragment" href="landing_referrer.html?bar=foo#baz">
+      Link to landing referrer with query and fragment
+    </a><br>
   </body>
 </html>