Don't prompt to save malicious downloads on exit

chrome/browser/lifetime/browser_close_manager_browsertest.cc

Index: chrome/browser/lifetime/browser_close_manager_browsertest.cc
diff --git a/chrome/browser/lifetime/browser_close_manager_browsertest.cc b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
index ce61b244028c4b7e4ea4a2f1fbdc7fe02d91fae3..f8fb49148fb53dea1514ca82b8c08c2b70d9b77d 100644
--- a/chrome/browser/lifetime/browser_close_manager_browsertest.cc
+++ b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
@@ -31,6 +31,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "content/test/net/url_request_mock_http_job.h"
 #include "content/test/net/url_request_slow_download_job.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
@@ -121,6 +122,7 @@ class TestBrowserCloseManager : public BrowserCloseManager {
   enum UserChoice {
     USER_CHOICE_USER_CANCELS_CLOSE,
     USER_CHOICE_USER_ALLOWS_CLOSE,
+    NO_USER_CHOICE
   };
 
   static void AttemptClose(UserChoice user_choice) {
@@ -135,7 +137,9 @@ class TestBrowserCloseManager : public BrowserCloseManager {
   virtual void ConfirmCloseWithPendingDownloads(
       int download_count,
       const base::Callback<void(bool)>& callback) OVERRIDE {
+    EXPECT_NE(NO_USER_CHOICE, user_choice_);
     switch (user_choice_) {
+      case NO_USER_CHOICE:
       case USER_CHOICE_USER_CANCELS_CLOSE: {
         callback.Run(false);
         break;
@@ -619,6 +623,52 @@ IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
   EXPECT_TRUE(chrome::BrowserIterator().done());
 }
 
+// Test shutdown with a DANGEROUS_URL download undecided.
+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+    TestWithDangerousUrlDownload) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  // Run a dangerous download, but the user doesn't make a decision.
+  // This test uses a .swf because it will trigger a dangerous download
+  // warning on every platform.
+  base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
+  GURL download_url(content::URLRequestMockHTTPJob::GetMockUrl(file));
+  content::DownloadTestObserverInterrupted observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      1,
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT);
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      GURL(download_url),
+      NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_NONE);
+  observer.WaitForFinished();
+
+  // Change the type of the dangerous download to DANGEROUS_URL, since the
+  // browser still prompts for DANGEROUS_FILE.
+  std::vector<content::DownloadItem*> download_vector;
+  content::BrowserContext::GetDownloadManager(browser()->profile())->
+      GetAllDownloads(&download_vector);
+  EXPECT_EQ(1u, download_vector.size());
+  download_vector.at(0)->SetDangerType(
+      content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL);

[asanka, 2013/10/15 21:39:08]

It would be great if we didn't add a public interface to DownloadItem for this
test. A SetDangerType() did exist once and we removed it because allowing the
danger type to change at arbitrary points in time made it difficult to reason
about the state transitions for a download. So we now allow the danger type to
be set at two points in time: during file name determination using
content::DownloadTargetCallback, and after all the data has been downloaded
using DownloadItem::OnContentCheckCompleted().

It's more code, but the cleanest way to test this, IMO, is to inject a
ChromeDownloadManagerDelegate subclass that overrides
DownloadManagerDelegate::DetermineDownloadTarget() to:
- Wrap the incoming DownloadTargetCallback with one that sets the danger type to
DOWNLOAD_DANGER_TYPE_DANGEROUS_URL.
- invokes CDMD::DetermineDownloadTarget() with the wrapped callback.

You can then set this CDMD subclass as the DownloadManagerDelegate to use using
DownloadService::SetDownloadManagerDelegateForTesting().

This way, the download will behave the same as one which was flagged by
SafeBrowsing as DANGEROUS_URL.

Failing that, the SetDangerType could be renamed to SetDangerTypeForTesting().
However, calling it on all platforms in this case is only safe because the
download was already marked as DANGEROUS_FILE. Setting it to DANGEROUS_URL
doesn't change the IsDangerous() state.

+
+  // Check that the download manager has the expected state.
+  EXPECT_EQ(1, content::BrowserContext::GetDownloadManager(
+      browser()->profile())->InProgressCount());
+  EXPECT_EQ(0, content::BrowserContext::GetDownloadManager(
+      browser()->profile())->NonDangerousInProgressCount());
+
+  // Close the browser with no user action.
+  RepeatedNotificationObserver close_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSED, 1);
+  TestBrowserCloseManager::AttemptClose(
+      TestBrowserCloseManager::NO_USER_CHOICE);
+  close_observer.Wait();
+  EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(chrome::BrowserIterator().done());
+}
+
 // Test shutdown with a download in progress.
 IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest, TestWithDownloads) {
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());