SafeBrowsing DownloadProtectionService: only check final URL against whitelist.

BUG=none

Review URL: https://codereview.chromium.org/166223004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251545 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/safe_browsing/download_protection_service.cc

@@ -570,21 +570,10 @@ class DownloadProtectionService::CheckClientDownloadRequest
     if (!database_manager_.get()) {
       reason = REASON_SB_DISABLED;
     } else {
-      for (size_t i = 0; i < url_chain_.size(); ++i) {
-        const GURL& url = url_chain_[i];
-        if (url.is_valid() &&
-            database_manager_->MatchDownloadWhitelistUrl(url)) {
-          VLOG(2) << url << " is on the download whitelist.";
-          reason = REASON_WHITELISTED_URL;
-          break;
-        }
-      }
-      if (referrer_url_.is_valid() && reason == REASON_MAX &&
-          database_manager_->MatchDownloadWhitelistUrl(
-              referrer_url_)) {
-        VLOG(2) << "Referrer url " << referrer_url_
-                << " is on the download whitelist.";
-        reason = REASON_WHITELISTED_REFERRER;
+      const GURL& url = url_chain_.back();
+      if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
+        VLOG(2) << url << " is on the download whitelist.";
+        reason = REASON_WHITELISTED_URL;
       }
       if (reason != REASON_MAX || signature_info_.trusted()) {
         UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedOrWhitelistedDownload", 1);

chrome/browser/safe_browsing/download_protection_service.h

@@ -138,6 +138,8 @@ class DownloadProtectionService {
  private:
   class CheckClientDownloadRequest;  // Per-request state
   friend class DownloadProtectionServiceTest;
+  FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
+                           CheckClientDownloadWhitelistedUrl);
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
                            CheckClientDownloadValidateRequest);
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -357,38 +357,85 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
+  // Response to any requests will be DANGEROUS.
+  ClientDownloadResponse response;
+  response.set_verdict(ClientDownloadResponse::DANGEROUS);
+  net::FakeURLFetcherFactory factory(NULL);
+  factory.SetFakeResponse(
+      DownloadProtectionService::GetDownloadRequestUrl(),
+      response.SerializeAsString(),
+      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+
+  std::string hash = "hash";
   base::FilePath a_tmp(FILE_PATH_LITERAL("a.tmp"));
   base::FilePath a_exe(FILE_PATH_LITERAL("a.exe"));
   std::vector<GURL> url_chain;
-  url_chain.push_back(GURL("http://www.evil.com/bla.exe"));
-  url_chain.push_back(GURL("http://www.google.com/a.exe"));
-  GURL referrer("http://www.google.com/");
+  GURL referrer;
 
   content::MockDownloadItem item;
-  EXPECT_CALL(item, AddObserver(_)).Times(2);
-  EXPECT_CALL(item, RemoveObserver(_)).Times(2);
+  EXPECT_CALL(item, AddObserver(_)).Times(4);
+  EXPECT_CALL(item, RemoveObserver(_)).Times(4);
   EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
   EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_exe));
   EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
   EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+  EXPECT_CALL(item, GetHash()).WillRepeatedly(ReturnRef(hash));
+  EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(100));
+  EXPECT_CALL(item, HasUserGesture()).WillRepeatedly(Return(true));
+  EXPECT_CALL(item, GetRemoteAddress()).WillRepeatedly(Return(""));
+  EXPECT_CALL(*signature_util_.get(), CheckSignature(a_tmp, _)).Times(4);
+
+  // We should not get whilelist checks for other URLs than specified below.
   EXPECT_CALL(*sb_service_->mock_database_manager(),
-              MatchDownloadWhitelistUrl(_))
+              MatchDownloadWhitelistUrl(_)).Times(0);
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(GURL("http://www.evil.com/bla.exe")))
       .WillRepeatedly(Return(false));
   EXPECT_CALL(*sb_service_->mock_database_manager(),
               MatchDownloadWhitelistUrl(GURL("http://www.google.com/a.exe")))
       .WillRepeatedly(Return(true));
-  EXPECT_CALL(*signature_util_.get(), CheckSignature(a_tmp, _)).Times(2);
 
+  // With no referrer and just the bad url, should be marked DANGEROUS.
+  url_chain.push_back(GURL("http://www.evil.com/bla.exe"));
   download_service_->CheckClientDownload(
       &item,
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
+#if defined(OS_WIN)
+  EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+#else
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#endif
 
-  // Check that the referrer is matched against the whitelist.
-  url_chain.pop_back();
-  referrer = GURL("http://www.google.com/a.exe");
+  // Check that the referrer is not matched against the whitelist.
+  referrer = GURL("http://www.google.com/");
+  download_service_->CheckClientDownload(
+      &item,
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  MessageLoop::current()->Run();
+#if defined(OS_WIN)
+  EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+#else
+  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#endif
+
+  // Redirect from a site shouldn't be checked either.
+  url_chain.insert(url_chain.begin(), GURL("http://www.google.com/redirect"));
+  download_service_->CheckClientDownload(
+      &item,
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  MessageLoop::current()->Run();
+#if defined(OS_WIN)
+  EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+#else
+  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#endif
+
+  // Only if the final url is whitelisted should it be SAFE.
+  url_chain.push_back(GURL("http://www.google.com/a.exe"));
   download_service_->CheckClientDownload(
       &item,
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,