Drop downloads in portals.

We don't want portals to be an avenue for download spam,
and we aren't presently aware of uses that require
downloads in portals.

This is largely straightforward, but the non-chrome
DownloadManagerDelegate was unfortunately left as a stub
that never calls WebContentsDelegate::CanDownload (like
the chrome/ one does).

Accordingly, this CL expands it to a simplified version
of what the chrome/ one does. The post-task was left
because the TODO comment there insinuates that not doing
so is risky. It would be straightforward to inline the
posted task in the future if one wished to.

This strategy is used instead of a NavigationThrottle,
because NavigationThrottle only covers downloads that are
initiated by navigation, but <a download> and other
renderer-initiated downloads do not fire navigation
throttles.

AwWebContentsDelegate::CanDownload is removed because it
was previously unreachable but is now reachable (because
AwDownloadManagerDelegate does not override
CheckDownloadAllowed), but behavior is otherwise preserved.
Android WebView relies on
AwDownloadManagerDelegate::InterceptDownloadIfApplicable
for its download interception features.

Bug: 967202
Change-Id: Ic019688aab81ebc92728bfd715bb10a5de3f36d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2229463Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775843}

android_webview/browser/aw_web_contents_delegate.cc

@@ -111,16 +111,6 @@ void AwWebContentsDelegate::FindReply(WebContents* web_contents,
       request_id, number_of_matches, active_match_ordinal, final_update);
 }
 
-void AwWebContentsDelegate::CanDownload(
-    const GURL& url,
-    const std::string& request_method,
-    base::OnceCallback<void(bool)> callback) {
-  // Android webview intercepts download in its resource dispatcher host
-  // delegate, so should not reach here.
-  NOTREACHED();
-  std::move(callback).Run(false);
-}
-
 void AwWebContentsDelegate::RunFileChooser(
     content::RenderFrameHost* render_frame_host,
     std::unique_ptr<content::FileSelectListener> listener,

android_webview/browser/aw_web_contents_delegate.h

@@ -35,9 +35,6 @@ class AwWebContentsDelegate
                  const gfx::Rect& selection_rect,
                  int active_match_ordinal,
                  bool final_update) override;
-  void CanDownload(const GURL& url,
-                   const std::string& request_method,
-                   base::OnceCallback<void(bool)> callback) override;
   void RunFileChooser(content::RenderFrameHost* render_frame_host,
                       std::unique_ptr<content::FileSelectListener> listener,
                       const blink::mojom::FileChooserParams& params) override;

content/browser/portal/portal.cc

@@ -556,6 +556,17 @@ void Portal::NavigationStateChanged(WebContents* source,
   outer_contents->GetDelegate()->NavigationStateChanged(source, changed_flags);
 }
 
+void Portal::CanDownload(const GURL& url,
+                         const std::string& request_method,
+                         base::OnceCallback<void(bool)> callback) {
+  // Downloads are not allowed in portals.
+  owner_render_frame_host()->AddMessageToConsole(
+      blink::mojom::ConsoleMessageLevel::kWarning,
+      base::StringPrintf("Download in a portal (from %s) was blocked.",
+                         url.spec().c_str()));
+  std::move(callback).Run(false);
+}
+
 base::UnguessableToken Portal::GetDevToolsFrameToken() const {
   return portal_contents_->GetMainFrame()->GetDevToolsFrameToken();
 }

content/browser/portal/portal.h

@@ -105,6 +105,9 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,
   WebContents* GetResponsibleWebContents(WebContents* web_contents) override;
   void NavigationStateChanged(WebContents* source,
                               InvalidateTypes changed_flags) override;
+  void CanDownload(const GURL& url,
+                   const std::string& request_method,
+                   base::OnceCallback<void(bool)> callback) override;
 
   // Returns the token which uniquely identifies this Portal.
   const base::UnguessableToken& portal_token() const { return portal_token_; }

content/browser/portal/portal_browsertest.cc

@@ -9,11 +9,13 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_timeouts.h"
 #include "build/build_config.h"
+#include "components/download/public/common/download_item.h"
 #include "components/viz/host/host_frame_sink_manager.h"
 #include "content/browser/compositor/surface_utils.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
@@ -27,6 +29,8 @@
 #include "content/browser/renderer_host/render_widget_host_view_child_frame.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame.mojom-test-utils.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/download_manager.h"
 #include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/content_switches.h"
@@ -40,11 +44,14 @@
 #include "content/public/test/navigation_handle_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_browser_context.h"
+#include "content/shell/browser/shell_content_browser_client.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "content/test/portal/portal_activated_observer.h"
 #include "content/test/portal/portal_created_observer.h"
 #include "content/test/portal/portal_interceptor_for_testing.h"
 #include "content/test/test_render_frame_host_factory.h"
+#include "net/base/escape.h"
 #include "net/base/net_errors.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -54,6 +61,7 @@
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/input/focus_type.mojom.h"
 #include "third_party/blink/public/mojom/portal/portal.mojom.h"
+#include "url/gurl.h"
 #include "url/url_constants.h"
 
 using testing::_;
@@ -1949,4 +1957,130 @@ IN_PROC_BROWSER_TEST_F(PortalOOPIFBrowserTest, OOPIFInsidePortal) {
   deleted_observer.WaitUntilDeleted();
 }
 
+namespace {
+
+class DownloadObserver : public DownloadManager::Observer {
+ public:
+  DownloadObserver()
+      : manager_(BrowserContext::GetDownloadManager(
+            ShellContentBrowserClient::Get()->browser_context())) {
+    manager_->AddObserver(this);
+  }
+
+  ~DownloadObserver() override {
+    if (manager_)
+      manager_->RemoveObserver(this);
+  }
+
+  ::testing::AssertionResult DownloadObserved() {
+    if (download_url_.is_empty())
+      return ::testing::AssertionFailure() << "no download observed";
+    return ::testing::AssertionSuccess()
+           << "download observed: " << download_url_;
+  }
+
+  ::testing::AssertionResult AwaitDownload() {
+    if (download_url_.is_empty() && !dropped_download_) {
+      base::RunLoop run_loop;
+      quit_closure_ = run_loop.QuitClosure();
+      run_loop.Run();
+      quit_closure_.Reset();
+    }
+    return DownloadObserved();
+  }
+
+  // DownloadManager::Observer
+
+  void ManagerGoingDown(DownloadManager* manager) override {
+    DCHECK_EQ(manager_, manager);
+    manager_->RemoveObserver(this);
+    manager_ = nullptr;
+  }
+
+  void OnDownloadCreated(DownloadManager* manager,
+                         download::DownloadItem* item) override {
+    DCHECK_EQ(manager_, manager);
+    if (download_url_.is_empty()) {
+      download_url_ = item->GetURL();
+      if (!quit_closure_.is_null())
+        std::move(quit_closure_).Run();
+    }
+  }
+
+  void OnDownloadDropped(DownloadManager* manager) override {
+    DCHECK_EQ(manager_, manager);
+    dropped_download_ = true;
+    if (!quit_closure_.is_null())
+      std::move(quit_closure_).Run();
+  }
+
+ private:
+  DownloadManager* manager_;
+  bool dropped_download_ = false;
+  GURL download_url_;
+  base::OnceClosure quit_closure_;
+};
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(PortalBrowserTest, DownloadsBlockedInMainFrame) {
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  ASSERT_TRUE(NavigateToURL(
+      web_contents_impl,
+      embedded_test_server()->GetURL("portal.test", "/title1.html")));
+  CreatePortalToUrl(web_contents_impl, embedded_test_server()->GetURL(
+                                           "portal.test", "/title2.html"));
+
+  GURL download_url = embedded_test_server()->GetURL(
+      "portal.test", "/set-header?Content-Disposition: attachment");
+
+  DownloadObserver download_observer;
+  EXPECT_TRUE(ExecJs(
+      web_contents_impl,
+      JsReplace("document.querySelector('portal').src = $1", download_url)));
+  EXPECT_FALSE(download_observer.AwaitDownload());
+}
+
+IN_PROC_BROWSER_TEST_F(PortalBrowserTest, DownloadsBlockedInSubframe) {
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  ASSERT_TRUE(NavigateToURL(
+      web_contents_impl,
+      embedded_test_server()->GetURL("portal.test", "/title1.html")));
+  CreatePortalToUrl(web_contents_impl, embedded_test_server()->GetURL(
+                                           "portal.test", "/title2.html"));
+
+  GURL download_url = embedded_test_server()->GetURL(
+      "portal.test", "/set-header?Content-Disposition: attachment");
+  GURL iframe_url = embedded_test_server()->GetURL(
+      "portal.test", "/iframe?" + net::EscapeQueryParamValue(
+                                      download_url.spec(), /*use_plus=*/false));
+
+  DownloadObserver download_observer;
+  EXPECT_TRUE(ExecJs(
+      web_contents_impl,
+      JsReplace("document.querySelector('portal').src = $1", iframe_url)));
+  EXPECT_FALSE(download_observer.AwaitDownload());
+}
+
+IN_PROC_BROWSER_TEST_F(PortalBrowserTest, DownloadsBlockedViaDownloadLink) {
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  ASSERT_TRUE(NavigateToURL(
+      web_contents_impl,
+      embedded_test_server()->GetURL("portal.test", "/title1.html")));
+  Portal* portal = CreatePortalToUrl(
+      web_contents_impl,
+      embedded_test_server()->GetURL("portal.test", "/title2.html"));
+
+  DownloadObserver download_observer;
+  EXPECT_TRUE(ExecJs(portal->GetPortalContents(),
+                     "let a = document.createElement('a');\n"
+                     "a.download = 'download.html';\n"
+                     "a.href = '/title3.html';\n"
+                     "a.click();\n"));
+  EXPECT_FALSE(download_observer.AwaitDownload());
+}
+
 }  // namespace content

content/public/browser/download_manager_delegate.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/download/public/common/download_item.h"
+#include "content/public/browser/web_contents_delegate.h"
 
 namespace content {
 
@@ -69,11 +70,33 @@ void DownloadManagerDelegate::CheckDownloadAllowed(
     bool from_download_cross_origin_redirect,
     bool content_initiated,
     CheckDownloadAllowedCallback check_download_allowed_cb) {
-  // TODO: once hook up delegate callback, make sure sync run of it doesn't
-  // crash and test it
+  // TODO: Do this directly, if it doesn't crash.
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(check_download_allowed_cb), true));
+      FROM_HERE,
+      base::BindOnce(
+          [](const WebContents::Getter& web_contents_getter, const GURL& url,
+             const std::string& request_method,
+             CheckDownloadAllowedCallback check_download_allowed_cb) {
+            WebContents* contents = web_contents_getter.Run();
+            if (!contents) {
+              // The contents was closed.
+              std::move(check_download_allowed_cb).Run(false);
+              return;
+            }
+
+            WebContentsDelegate* delegate = contents->GetDelegate();
+            if (!delegate) {
+              // The default behavior is to allow it.
+              std::move(check_download_allowed_cb).Run(true);
+              return;
+            }
+
+            delegate->CanDownload(url, request_method,
+                                  std::move(check_download_allowed_cb));
+          },
+          web_contents_getter, url, request_method,
+          std::move(check_download_allowed_cb)));
 }
 
 download::QuarantineConnectionCallback