DownloadProtection: Add more graceful handling of verdict=UNKNOWN

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 453 matching lines @@
       std::string data;
       bool got_data = source->GetResponseAsString(&data);
       DCHECK(got_data);
       if (!response.ParseFromString(data)) {
         reason = REASON_INVALID_RESPONSE_PROTO;
         result = UNKNOWN;
       } else if (type_ == ClientDownloadRequest::SAMPLED_UNSUPPORTED_FILE) {
         // Ignore the verdict because we were just reporting a sampled file.
         reason = REASON_SAMPLED_UNSUPPORTED_FILE;
         result = UNKNOWN;
-      } else if (response.verdict() == ClientDownloadResponse::SAFE) {
-        reason = REASON_DOWNLOAD_SAFE;
-        result = SAFE;
-      } else if (service_ && !service_->IsSupportedDownload(
-          *item_, item_->GetTargetFilePath())) {
-        // TODO(nparker): Remove this check since it should be impossible.
-        reason = REASON_DOWNLOAD_NOT_SUPPORTED;
-        result = UNKNOWN;
-      } else if (response.verdict() == ClientDownloadResponse::DANGEROUS) {
-        reason = REASON_DOWNLOAD_DANGEROUS;
-        result = DANGEROUS;
-        token = response.token();
-      } else if (response.verdict() == ClientDownloadResponse::UNCOMMON) {
-        reason = REASON_DOWNLOAD_UNCOMMON;
-        result = UNCOMMON;
-        token = response.token();
-      } else if (response.verdict() == ClientDownloadResponse::DANGEROUS_HOST) {
-        reason = REASON_DOWNLOAD_DANGEROUS_HOST;
-        result = DANGEROUS_HOST;
-        token = response.token();
-      } else if (
-          response.verdict() == ClientDownloadResponse::POTENTIALLY_UNWANTED) {
-        reason = REASON_DOWNLOAD_POTENTIALLY_UNWANTED;
-        result = POTENTIALLY_UNWANTED;
-        token = response.token();
       } else {
-        LOG(DFATAL) << "Unknown download response verdict: "
-                    << response.verdict();
-        reason = REASON_INVALID_RESPONSE_VERDICT;
-        result = UNKNOWN;
+        switch (response.verdict()) {
+          case ClientDownloadResponse::SAFE:
+            reason = REASON_DOWNLOAD_SAFE;
+            result = SAFE;
+            break;
+          case ClientDownloadResponse::DANGEROUS:
+            reason = REASON_DOWNLOAD_DANGEROUS;
+            result = DANGEROUS;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::UNCOMMON:
+            reason = REASON_DOWNLOAD_UNCOMMON;
+            result = UNCOMMON;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::DANGEROUS_HOST:
+            reason = REASON_DOWNLOAD_DANGEROUS_HOST;
+            result = DANGEROUS_HOST;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::POTENTIALLY_UNWANTED:
+            reason = REASON_DOWNLOAD_POTENTIALLY_UNWANTED;
+            result = POTENTIALLY_UNWANTED;
+            token = response.token();
+            break;
+          case ClientDownloadResponse::UNKNOWN:
+            reason = REASON_VERDICT_UNKNOWN;
+            result = UNKNOWN;
+            break;
+          default:

[asanka, 2016/07/07 20:46:59]

Nit: Shall we go the route of eliminating the default label? That way the
compiler will throw an error if a new verdict is added that's not handled here.
If the incoming proto has an unrecognized verdict, it should fail to parse,
right?

[Nathan Parker, 2016/07/07 21:02:55]

I don't think it'd fail to parse... hence I'm catching it here. Though I can't
find docs to that effect.

I think the no-default style works well only when the value is set solely within
the same C++ binary, so the compiler can enforce the type.

I was relying on it _not_ failing to parse, so existing chrome binaries will be
able to handle the new "UNKNOWN" value with the old code that doesn't know about
it.

[asanka, 2016/07/07 21:15:11]

Ah. TIL. Acknowledged.

[Nathan Parker, 2016/07/11 20:26:22]

Apparently I was wrong.  The old code apparently can't parse the proto with the
new enum value, so I'm seeing a histogram value of INVALID_RESPONSE_PROTO.  The
result (unknown) is the same.

+            LOG(DFATAL) << "Unknown download response verdict: "
+                        << response.verdict();
+            reason = REASON_INVALID_RESPONSE_VERDICT;
+            result = UNKNOWN;
+        }
       }
+
       if (!token.empty())
         SetDownloadPingToken(item_, token);
 
       DownloadFeedbackService::MaybeStorePingsForDownload(
           result, item_, client_download_request_data_, data);
     }
     // We don't need the fetcher anymore.
     fetcher_.reset();
     UMA_HISTOGRAM_TIMES("SBClientDownload.DownloadRequestDuration",
                         base::TimeTicks::Now() - start_time_);
@@ skipping 872 matching lines @@
       case ClientDownloadResponse::SAFE:
         return SAFE;
       case ClientDownloadResponse::UNCOMMON:
         return UNCOMMON;
       case ClientDownloadResponse::POTENTIALLY_UNWANTED:
         return POTENTIALLY_UNWANTED;
       case ClientDownloadResponse::DANGEROUS:
         return DANGEROUS;
       case ClientDownloadResponse::DANGEROUS_HOST:
         return DANGEROUS_HOST;
+      case ClientDownloadResponse::UNKNOWN:
+        return UNKNOWN;
     }
     return UNKNOWN;
   }
 
   // Given a |default_file_path| and a list of |alternate_extensions|,
   // constructs a FilePath with each possible extension and returns one that
   // satisfies IsCheckedBinaryFile(). If none are supported, returns an
   // empty FilePath.
   static base::FilePath GetSupportedFilePath(
       const base::FilePath& default_file_path,
@@ skipping 311 matching lines @@
 GURL DownloadProtectionService::GetDownloadRequestUrl() {
   GURL url(kDownloadRequestUrl);
   std::string api_key = google_apis::GetAPIKey();
   if (!api_key.empty())
     url = url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 
   return url;
 }
 
 }  // namespace safe_browsing