Fix multiple download protection for <a download> x-origin redirect

The bug: multiple downloads protection is bypassed when there are multiple
<a download> download attempts and they end up triggering a x-origin redirect
to another download.

The cause: Each x-origin redirect following the <a download> is being treated as
a navigation. (See DownloadManagerImpl::InterceptDownload() (NetworkService
enabled), DownloadResourceHandler::OnRequestRedirected() (NetworkService
disabled)). The navigation will hit
DownloadRequestLimiter::TabDownloadState::DidStartNavigation that resets some
state of the limiter, and future downloads won't be prevented.

The solution: plumb |from_download_cross_origin_redirect| to NavigationRequest,
and skip resetting the limiter state when the flag is true.

Bug: 959640
Change-Id: I7d8aca09670be5258e149e34e3e6f2f3107442ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1627209Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665973}

chrome/browser/download/download_browsertest.cc

@@ -121,6 +121,7 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "net/test/embedded_test_server/request_handler_util.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "services/device/public/mojom/constants.mojom.h"
@@ -207,15 +208,14 @@ class CreatedObserver : public content::DownloadManager::Observer {
 
 class OnCanDownloadDecidedObserver {
  public:
-  OnCanDownloadDecidedObserver()
-      : on_decided_called_(false), last_allow_(false) {}
+  OnCanDownloadDecidedObserver() = default;
 
-  void Wait(bool expectation) {
-    if (on_decided_called_) {
-      EXPECT_EQ(last_allow_, expectation);
-      on_decided_called_ = false;
+  void Wait(const std::vector<bool>& expected_decisions) {
+    if (expected_decisions.size() <= decisions_.size()) {
+      EXPECT_TRUE(std::equal(expected_decisions.begin(),
+                             expected_decisions.end(), decisions_.begin()));
     } else {
-      expectation_ = expectation;
+      expected_decisions_ = expected_decisions;
       base::RunLoop run_loop;
       completion_closure_ = run_loop.QuitClosure();
       run_loop.Run();
@@ -223,22 +223,24 @@ class OnCanDownloadDecidedObserver {
   }
 
   void OnCanDownloadDecided(bool allow) {
-    // It is possible this is called before Wait(), so the result needs to
-    // be stored in that case.
-    if (!completion_closure_.is_null()) {
+    decisions_.push_back(allow);
+    if (decisions_.size() == expected_decisions_.size()) {
+      DCHECK(!completion_closure_.is_null());
+      EXPECT_EQ(decisions_, expected_decisions_);
       std::move(completion_closure_).Run();
-      EXPECT_EQ(allow, expectation_);
-    } else {
-      on_decided_called_ = true;
-      last_allow_ = allow;
     }
   }
 
+  void Reset() {
+    decisions_.clear();
+    expected_decisions_.clear();
+    completion_closure_.Reset();
+  }
+
  private:
-  bool expectation_;
+  std::vector<bool> decisions_;
+  std::vector<bool> expected_decisions_;
   base::Closure completion_closure_;
-  bool on_decided_called_;
-  bool last_allow_;
 
   DISALLOW_COPY_AND_ASSIGN(OnCanDownloadDecidedObserver);
 };
@@ -1483,7 +1485,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadResourceThrottleCancels) {
 }
 
 // Test to make sure 'download' attribute in anchor tag doesn't trigger a
-// downloadd if DownloadRequestLimiter disallows it.
+// download if DownloadRequestLimiter disallows it.
 IN_PROC_BROWSER_TEST_F(DownloadTest,
                        DownloadRequestLimiterDisallowsAnchorDownloadTag) {
   OnCanDownloadDecidedObserver can_download_observer;
@@ -1515,7 +1517,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest,
       "window.domAutomationController.send(startDownload1());",
       &download_attempted));
   ASSERT_TRUE(download_attempted);
-  can_download_observer.Wait(false);
+  can_download_observer.Wait({false});
+  can_download_observer.Reset();
 
   // Let the 2nd download to succeed.
   std::unique_ptr<content::DownloadTestObserver> observer(
@@ -1527,7 +1530,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest,
       "window.domAutomationController.send(startDownload2());",
       &download_attempted));
   ASSERT_TRUE(download_attempted);
-  can_download_observer.Wait(true);
+  can_download_observer.Wait({true});
   // Waits for the 2nd download to complete.
   observer->WaitForFinished();
 
@@ -3256,6 +3259,52 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, TestMultipleDownloadsRequests) {
   browser()->tab_strip_model()->GetActiveWebContents()->Close();
 }
 
+// Test the scenario for 3 consecutive <a download> download attempts that all
+// trigger a x-origin redirect to another download. No download is expected to
+// happen.
+IN_PROC_BROWSER_TEST_F(
+    DownloadTest,
+    MultipleAnchorDownloadsRequestsCrossOriginRedirectToAnotherDownload) {
+  std::unique_ptr<content::DownloadTestObserver> downloads_observer(
+      CreateWaiter(browser(), 0u));
+
+  OnCanDownloadDecidedObserver can_download_observer;
+  g_browser_process->download_request_limiter()
+      ->SetOnCanDownloadDecidedCallbackForTesting(base::BindRepeating(
+          &OnCanDownloadDecidedObserver::OnCanDownloadDecided,
+          base::Unretained(&can_download_observer)));
+
+  embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory());
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url = embedded_test_server()->GetURL(
+      "/downloads/multiple_a_download_x_origin_redirect_to_download.html");
+
+  base::StringPairs port_replacement;
+  port_replacement.push_back(std::make_pair(
+      "{{PORT}}", base::NumberToString(embedded_test_server()->port())));
+  std::string download_url = net::test_server::GetFilePathWithReplacements(
+      "redirect_x_origin_download.html", port_replacement);
+
+  url = GURL(url.spec() + "?download_url=" + download_url);
+
+  // Navigate to a page that triggers 3 consecutive <a download> download
+  // attempts that all trigger a x-origin redirect to another download.
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(browser(), url, 1);
+
+  // The 1st <a download> attempt should pass the download limiter check,
+  // and prevent further download attempts from passing. The subsequent 2nd/3rd
+  // <a download> attempts as well as the |download as a result of the x-origin
+  // redirect from the 1st download attempt| should all fail the download
+  // limiter check.
+  can_download_observer.Wait({true, false, false, false});
+
+  // Only the 1st <a download> attempt passed the download limiter check, but it
+  // was still aborted by a x-origin redirect, therefore we expect no download
+  // to happen.
+  EXPECT_EQ(
+      0u, downloads_observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+}
+
 IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_Renaming) {
   embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory());
   ASSERT_TRUE(embedded_test_server()->Start());

chrome/browser/download/download_request_limiter.cc

@@ -151,9 +151,11 @@ void DownloadRequestLimiter::TabDownloadState::DidStartNavigation(
       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 this is a navigation as a result of x-origin redirect from a previous
+    // <a download> download, or if this is a forward/back navigation in a host
+    // already seen, also don't reset a prompting or blocking limiter state.
+    // This prevents a page to use any of those mechanisms to trigger multiple
+    // downloads.
     if (IsNavigationRestricted(navigation_handle))
       return;
   }
@@ -418,6 +420,9 @@ void DownloadRequestLimiter::TabDownloadState::SetDownloadStatusAndNotifyImpl(
 
 bool DownloadRequestLimiter::TabDownloadState::IsNavigationRestricted(
     content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->FromDownloadCrossOriginRedirect())
+    return true;
+
   std::string host = navigation_handle->GetURL().host();
   if (navigation_handle->GetPageTransition() & ui::PAGE_TRANSITION_FORWARD_BACK)
     return restricted_hosts_.find(host) != restricted_hosts_.end();
@@ -585,6 +590,7 @@ void DownloadRequestLimiter::CanDownloadImpl(
         case CONTENT_SETTING_ASK:
           state->PromptUserForDownload(std::move(callback));
           state->increment_download_count();
+          ret = false;
           break;
         case CONTENT_SETTING_SESSION_ONLY:
         case CONTENT_SETTING_NUM_SETTINGS:

chrome/browser/download/download_request_limiter.h

@@ -234,6 +234,9 @@ class DownloadRequestLimiter
   FRIEND_TEST_ALL_PREFIXES(DownloadTest, DownloadResourceThrottleCancels);
   FRIEND_TEST_ALL_PREFIXES(DownloadTest,
                            DownloadRequestLimiterDisallowsAnchorDownloadTag);
+  FRIEND_TEST_ALL_PREFIXES(
+      DownloadTest,
+      MultipleAnchorDownloadsRequestsCrossOriginRedirectToAnotherDownload);
   FRIEND_TEST_ALL_PREFIXES(ContentSettingBubbleControllerTest, Init);
   FRIEND_TEST_ALL_PREFIXES(ContentSettingImageModelBrowserTest,
                            CreateBubbleModel);

chrome/test/data/downloads/multiple_a_download_x_origin_redirect_to_download.html

+<!DOCTYPE html>
+<body></body>
+<script>
+const url_params = new URLSearchParams(location.search);
+let link = document.createElement('a');
+link.download = '';
+link.href = url_params.get('download_url');
+document.body.appendChild(link);
+link.click();
+link.click();
+link.click();
+</script>
\ No newline at end of file

chrome/test/data/downloads/redirect_x_origin_download.html

+redirecting
\ No newline at end of file

chrome/test/data/downloads/redirect_x_origin_download.html.mock-http-headers

+HTTP/1.1 302 Moved
+Location: http://www.a.com:{{PORT}}/downloads/empty.bin
\ No newline at end of file

content/browser/download/download_manager_impl.cc

@@ -549,6 +549,7 @@ bool DownloadManagerImpl::InterceptDownload(
       params.frame_tree_node_id =
           RenderFrameHost::GetFrameTreeNodeIdForRoutingId(
               info.render_process_id, info.render_frame_id);
+      params.from_download_cross_origin_redirect = true;
       web_contents->GetController().LoadURLWithParams(params);
     }
     if (info.request_handle)

content/browser/download/download_resource_handler.cc

@@ -124,6 +124,7 @@ void NavigateOnUIThread(const GURL& url,
                         const std::vector<GURL> url_chain,
                         const Referrer& referrer,
                         bool has_user_gesture,
+                        bool from_download_cross_origin_redirect,
                         const ResourceRequestInfo::WebContentsGetter& wc_getter,
                         int frame_tree_node_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -135,6 +136,8 @@ void NavigateOnUIThread(const GURL& url,
     params.referrer = referrer;
     params.redirect_chain = url_chain;
     params.frame_tree_node_id = frame_tree_node_id;
+    params.from_download_cross_origin_redirect =
+        from_download_cross_origin_redirect;
     web_contents->GetController().LoadURLWithParams(params);
   }
 }
@@ -210,6 +213,7 @@ void DownloadResourceHandler::OnRequestRedirected(
                      Referrer::NetReferrerPolicyToBlinkReferrerPolicy(
                          redirect_info.new_referrer_policy)),
             GetRequestInfo()->HasUserGesture(),
+            true /* from_download_cross_origin_redirect */,
             GetRequestInfo()->GetWebContentsGetterForRequest(),
             GetRequestInfo()->frame_tree_node_id()));
     controller->Cancel();

content/browser/frame_host/navigation_controller_impl.cc

@@ -3141,10 +3141,14 @@ NavigationControllerImpl::CreateNavigationRequestFromLoadParams(
   // extra_headers in params are \n separated; NavigationRequests want \r\n.
   std::string extra_headers_crlf;
   base::ReplaceChars(params.extra_headers, "\n", "\r\n", &extra_headers_crlf);
-  return NavigationRequest::CreateBrowserInitiated(
+
+  auto navigation_request = NavigationRequest::CreateBrowserInitiated(
       node, common_params, commit_params, !params.is_renderer_initiated,
       extra_headers_crlf, *frame_entry, entry, request_body,
       params.navigation_ui_data ? params.navigation_ui_data->Clone() : nullptr);
+  navigation_request->set_from_download_cross_origin_redirect(
+      params.from_download_cross_origin_redirect);
+  return navigation_request;
 }
 
 std::unique_ptr<NavigationRequest>

content/browser/frame_host/navigation_handle_impl.cc

@@ -452,6 +452,10 @@ int NavigationHandleImpl::GetNavigationEntryOffset() {
   return navigation_request_->navigation_entry_offset();
 }
 
+bool NavigationHandleImpl::FromDownloadCrossOriginRedirect() {
+  return navigation_request_->from_download_cross_origin_redirect();
+}
+
 bool NavigationHandleImpl::IsSignedExchangeInnerResponse() {
   return navigation_request_->response()
              ? navigation_request_->response()

content/browser/frame_host/navigation_handle_impl.h

@@ -112,6 +112,7 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   const base::Optional<url::Origin>& GetInitiatorOrigin() override;
   bool IsSameProcess() override;
   int GetNavigationEntryOffset() override;
+  bool FromDownloadCrossOriginRedirect() override;
 
   // Returns the NavigationRequest which owns this NavigationHandle.
   NavigationRequest* navigation_request() { return navigation_request_; }

content/browser/frame_host/navigation_request.h

@@ -406,6 +406,9 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
     return previous_url_;
   }
 
+  bool from_download_cross_origin_redirect() const {
+    return from_download_cross_origin_redirect_;
+  }
 
 #if defined(OS_ANDROID)
   // Returns a reference to |navigation_handle_| Java counterpart. It is used
@@ -421,6 +424,11 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
 
   Referrer& sanitized_referrer() { return sanitized_referrer_; }
 
+  void set_from_download_cross_origin_redirect(
+      bool from_download_cross_origin_redirect) {
+    from_download_cross_origin_redirect_ = from_download_cross_origin_redirect;
+  }
+
   // This should be a private method. The only valid reason to be used
   // outside of the class constructor is in the case of an initial history
   // navigation in a subframe. This allows a browser-initiated NavigationRequest
@@ -833,6 +841,10 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
 
   bool was_redirected_ = false;
 
+  // Whether this navigation was triggered by a x-origin redirect following a
+  // prior (most likely <a download>) download attempt.
+  bool from_download_cross_origin_redirect_ = false;
+
   // Used when SignedExchangeSubresourcePrefetch is enabled to hold the
   // prefetched signed exchanges. This is shared with the navigation initiator's
   // RenderFrameHostImpl. This also means that only the navigations that were

content/public/browser/navigation_controller.cc

@@ -25,6 +25,7 @@ NavigationController::LoadURLParams::LoadURLParams(const GURL& url)
       should_clear_history_list(false),
       started_from_context_menu(false),
       navigation_ui_data(nullptr),
+      from_download_cross_origin_redirect(false),
       was_activated(WasActivatedOption::kUnknown),
       reload_type(ReloadType::NONE) {}
 

content/public/browser/navigation_controller.h

@@ -201,6 +201,10 @@ class NavigationController {
     // ContentBrowserClient::GetNavigationUIData.
     std::unique_ptr<NavigationUIData> navigation_ui_data;
 
+    // Whether this navigation was triggered by a x-origin redirect following a
+    // prior (most likely <a download>) download attempt.
+    bool from_download_cross_origin_redirect;
+
     // Time at which the input leading to this navigation occurred. This field
     // is set for links clicked by the user; the embedder is recommended to set
     // it for navigations it initiates.

content/public/browser/navigation_handle.h

@@ -343,6 +343,10 @@ class CONTENT_EXPORT NavigationHandle {
 
   // Returns whether this navigation is currently deferred.
   virtual bool IsDeferredForTesting() = 0;
+
+  // Whether this navigation was triggered by a x-origin redirect following a
+  // prior (most likely <a download>) download attempt.
+  virtual bool FromDownloadCrossOriginRedirect() = 0;
 };
 
 }  // namespace content

content/public/test/mock_navigation_handle.h

@@ -97,6 +97,7 @@ class MockNavigationHandle : public NavigationHandle {
   MOCK_METHOD0(IsDeferredForTesting, bool());
   MOCK_METHOD1(RegisterSubresourceOverride,
                void(mojom::TransferrableURLLoaderPtr));
+  MOCK_METHOD0(FromDownloadCrossOriginRedirect, bool());
   MOCK_METHOD0(IsSameProcess, bool());
   MOCK_METHOD0(GetNavigationEntryOffset, int());