Don't reset TabDownloadState on history back/forward

Currently performing forward/backward on a tab will reset the TabDownloadState.
Which allows javascript code to do trigger multiple downloads.
This CL disables that behavior by not resetting the TabDownloadState on
forward/back.
It is still possible to reset the TabDownloadState through user gesture
or using browser initiated download.

BUG=848535

Change-Id: I7f9bf6e8fb759b4dcddf5ac0c214e8c6c9f48863
Reviewed-on: https://chromium-review.googlesource.com/1108959
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574437}

chrome/browser/download/download_request_limiter.cc

@@ -135,14 +135,23 @@ void DownloadRequestLimiter::TabDownloadState::DidStartNavigation(
   download_seen_ = false;
   ui_status_ = DOWNLOAD_UI_DEFAULT;
 
-  // If the navigation is renderer-initiated (but not user-initiated), ensure
-  // that a prompting or blocking limiter state is not reset, so
-  // window.location.href or meta refresh can't be abused to avoid the limiter.
-  // User-initiated navigations will trigger DidGetUserInteraction, which resets
-  // the limiter before the navigation starts.
-  if (navigation_handle->IsRendererInitiated() &&
-      (status_ == PROMPT_BEFORE_DOWNLOAD || status_ == DOWNLOADS_NOT_ALLOWED)) {
-    return;
+  if (status_ == PROMPT_BEFORE_DOWNLOAD || status_ == DOWNLOADS_NOT_ALLOWED) {
+    std::string host = navigation_handle->GetURL().host();
+    // If the navigation is renderer-initiated (but not user-initiated), ensure
+    // that a prompting or blocking limiter state is not reset, so
+    // window.location.href or meta refresh can't be abused to avoid the
+    // limiter.
+    if (navigation_handle->IsRendererInitiated()) {
+      if (!host.empty())
+        restricted_hosts_.emplace(host);
+      return;
+    }
+
+    // If this is a forward/back navigation, also don't reset a prompting or
+    // blocking limiter state unless a new host is encounted. This prevents a
+    // page to use history forward/backward to trigger multiple downloads.
+    if (IsNavigationRestricted(navigation_handle))
+      return;
   }
 
   if (status_ == DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS ||
@@ -174,7 +183,8 @@ void DownloadRequestLimiter::TabDownloadState::DidFinishNavigation(
   // DidStartNavigation.
   if (status_ == ALLOW_ONE_DOWNLOAD ||
       (status_ == PROMPT_BEFORE_DOWNLOAD &&
-       !navigation_handle->IsRendererInitiated())) {
+       !navigation_handle->IsRendererInitiated() &&
+       !IsNavigationRestricted(navigation_handle))) {
     // When the user reloads the page without responding to the infobar,
     // they are expecting DownloadRequestLimiter to behave as if they had
     // just initially navigated to this page. See http://crbug.com/171372.
@@ -383,6 +393,11 @@ void DownloadRequestLimiter::TabDownloadState::SetDownloadStatusAndNotifyImpl(
   if (!web_contents())
     return;
 
+  if (status_ == PROMPT_BEFORE_DOWNLOAD || status_ == DOWNLOADS_NOT_ALLOWED) {
+    if (!initial_page_host_.empty())
+      restricted_hosts_.emplace(initial_page_host_);
+  }
+
   // We want to send a notification if the UI status has changed to ensure that
   // the omnibox decoration updates appropriately. This is effectively the same
   // as other permissions which might be in an allow state, but do not show UI
@@ -396,6 +411,14 @@ void DownloadRequestLimiter::TabDownloadState::SetDownloadStatusAndNotifyImpl(
       content::NotificationService::NoDetails());
 }
 
+bool DownloadRequestLimiter::TabDownloadState::IsNavigationRestricted(
+    content::NavigationHandle* navigation_handle) {
+  std::string host = navigation_handle->GetURL().host();
+  if (navigation_handle->GetPageTransition() & ui::PAGE_TRANSITION_FORWARD_BACK)
+    return restricted_hosts_.find(host) != restricted_hosts_.end();
+  return false;
+}
+
 // DownloadRequestLimiter ------------------------------------------------------
 
 DownloadRequestLimiter::DownloadRequestLimiter() : factory_(this) {}

chrome/browser/download/download_request_limiter.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -171,6 +172,10 @@ class DownloadRequestLimiter
     void SetDownloadStatusAndNotifyImpl(DownloadStatus status,
                                         ContentSetting setting);
 
+    // Check if download is restricted (either requires prompting or is blocked)
+    // for the |navigation_handle|.
+    bool IsNavigationRestricted(content::NavigationHandle* navigation_handle);
+
     content::WebContents* web_contents_;
 
     DownloadRequestLimiter* host_;
@@ -192,6 +197,10 @@ class DownloadRequestLimiter
     // callbacks.
     std::vector<DownloadRequestLimiter::Callback> callbacks_;
 
+    // A list of hosts that won't cause tab's download state to change if the
+    // state is PROMPT_BEFORE_DOWNLOAD or DOWNLOADS_NOT_ALLOWED.
+    std::set<std::string> restricted_hosts_;
+
     ScopedObserver<HostContentSettingsMap, content_settings::Observer>
         observer_;
 

chrome/browser/download/download_request_limiter_unittest.cc

@@ -453,6 +453,125 @@ TEST_F(DownloadRequestLimiterTest, RendererInitiated) {
             download_request_limiter_->GetDownloadUiStatus(web_contents()));
 }
 
+// Test that history back will not change the tab download state if all the
+// previous navigations are renderer-initiated.
+TEST_F(DownloadRequestLimiterTest, HistoryBack) {
+  NavigateAndCommit(GURL("http://foo.com/bar"));
+  LoadCompleted();
+
+  // Do one download so we end up in PROMPT.
+  CanDownload();
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // Renderer-initiated navigation to a different host shouldn't reset the
+  // state.
+  content::NavigationSimulator::NavigateAndCommitFromDocument(
+      GURL("http://foobar.com/bar"), web_contents()->GetMainFrame());
+  LoadCompleted();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // History back shouldn't reset the state, either.
+  auto backward_navigation =
+      content::NavigationSimulator::CreateHistoryNavigation(-1 /* Offset */,
+                                                            web_contents());
+  backward_navigation->Start();
+  backward_navigation->Commit();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // Browser-initiated navigation to a different host, which should reset the
+  // state.
+  NavigateAndCommit(GURL("http://foobar.com"));
+  LoadCompleted();
+  EXPECT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+  CanDownload();
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // History back should reset the state as it is going to a different host.
+  backward_navigation = content::NavigationSimulator::CreateHistoryNavigation(
+      -1 /* Offset */, web_contents());
+  backward_navigation->Start();
+  backward_navigation->Commit();
+  EXPECT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+}
+
+// Tab download state shouldn't change when forward/back between to a
+// renderer-initiated page.
+TEST_F(DownloadRequestLimiterTest, HistoryForwardBack) {
+  NavigateAndCommit(GURL("http://foo.com/bar"));
+  LoadCompleted();
+
+  // Do one download so we end up in PROMPT.
+  CanDownload();
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // Renderer-initiated navigation to a different host shouldn't reset the
+  // state.
+  content::NavigationSimulator::NavigateAndCommitFromDocument(
+      GURL("http://foobar.com/bar"), web_contents()->GetMainFrame());
+  LoadCompleted();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // History back shouldn't reset the state, either.
+  auto backward_navigation =
+      content::NavigationSimulator::CreateHistoryNavigation(-1 /* Offset */,
+                                                            web_contents());
+  backward_navigation->Start();
+  backward_navigation->Commit();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // History forward shouldn't reset the state, as the host is encountered
+  // before.
+  auto forward_navigation =
+      content::NavigationSimulator::CreateHistoryNavigation(1 /* Offset */,
+                                                            web_contents());
+  forward_navigation->Start();
+  forward_navigation->Commit();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // History backward again, nothing should change.
+  backward_navigation = content::NavigationSimulator::CreateHistoryNavigation(
+      -1 /* Offset */, web_contents());
+  backward_navigation->Start();
+  backward_navigation->Commit();
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+}
+
 TEST_F(DownloadRequestLimiterTest, DownloadRequestLimiter_ResetOnUserGesture) {
   NavigateAndCommit(GURL("http://foo.com/bar"));
   LoadCompleted();