Evict caches that are stored for more than 7 days.

Cache duration may be misconfigured. We should evict these caches to
prevent false positive.

Bug: 1156896
Change-Id: I4bc464113dc460319d7256919fb377f5d89bedf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583467Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837707}

components/safe_browsing/core/verdict_cache_manager.cc

@@ -42,6 +42,10 @@ const int kCleanUpIntervalInitSecond = 120;
 // The interval between every cleanup task.
 const int kCleanUpIntervalSecond = 1800;
 
+// The longest duration that a cache can be stored. If a cache is stored
+// longer than the upper bound, it will be evicted.
+const int kCacheDurationUpperBoundSecond = 7 * 24 * 60 * 60;  // 7 days
+
 // A helper class to include all match params. It is used as a centralized
 // place to determine if the current cache entry should be considered as a
 // match.
@@ -179,6 +183,12 @@ bool IsCacheExpired(int cache_creation_time, int cache_duration) {
          static_cast<double>(cache_creation_time + cache_duration);
 }
 
+bool IsCacheOlderThanUpperBound(int cache_creation_time) {
+  return base::Time::Now().ToDoubleT() >
+         static_cast<double>(cache_creation_time +
+                             kCacheDurationUpperBoundSecond);
+}
+
 template <class T>
 size_t RemoveExpiredEntries(base::Value* verdict_dictionary,
                             const char* proto_name) {
@@ -189,7 +199,8 @@ size_t RemoveExpiredEntries(base::Value* verdict_dictionary,
     T verdict;
     if (!ParseVerdictEntry<T>(&item.second, &verdict_received_time, &verdict,
                               proto_name) ||
-        IsCacheExpired(verdict_received_time, verdict.cache_duration_sec())) {
+        IsCacheExpired(verdict_received_time, verdict.cache_duration_sec()) ||
+        IsCacheOlderThanUpperBound(verdict_received_time)) {
       expired_keys.push_back(item.first);
     }
   }

components/safe_browsing/core/verdict_cache_manager.h

@@ -115,6 +115,8 @@ class VerdictCacheManager : public history::HistoryServiceObserver,
       TestRemoveRealTimeUrlCheckCachedVerdictOnURLsDeleted);
   FRIEND_TEST_ALL_PREFIXES(VerdictCacheManagerTest,
                            TestCleanUpExpiredVerdictInBackground);
+  FRIEND_TEST_ALL_PREFIXES(VerdictCacheManagerTest,
+                           TestCleanUpVerdictOlderThanUpperBound);
 
   void ScheduleNextCleanUpAfterInterval(base::TimeDelta interval);
 

components/safe_browsing/core/verdict_cache_manager_unittest.cc

@@ -750,4 +750,25 @@ TEST_F(VerdictCacheManagerTest, TestCleanUpExpiredVerdictInBackground) {
   ASSERT_EQ(0u, cache_manager_->GetStoredRealTimeUrlCheckVerdictCount());
 }
 
+TEST_F(VerdictCacheManagerTest, TestCleanUpVerdictOlderThanUpperBound) {
+  RTLookupResponse response;
+  // Set the cache duration to 20 days.
+  AddThreatInfoToResponse(response, RTLookupResponse::ThreatInfo::DANGEROUS,
+                          RTLookupResponse::ThreatInfo::SOCIAL_ENGINEERING,
+                          /* cache_duration_sec */ 20 * 24 * 60 * 60,
+                          "www.example.com/",
+                          RTLookupResponse::ThreatInfo::EXACT_MATCH);
+
+  cache_manager_->CacheRealTimeUrlVerdict(GURL("https://www.example.com/"),
+                                          response, base::Time::Now(),
+                                          /* store_old_cache */ false);
+  ASSERT_EQ(1u, cache_manager_->GetStoredRealTimeUrlCheckVerdictCount());
+  // Fast forward by 8 days.
+  task_environment_->FastForwardBy(
+      base::TimeDelta::FromSeconds(8 * 24 * 60 * 60));
+  // Although the cache duration is set to 20 days, it is stored longer than the
+  // upper bound(7 days). The cache should be cleaned up.
+  ASSERT_EQ(0u, cache_manager_->GetStoredRealTimeUrlCheckVerdictCount());
+}
+
 }  // namespace safe_browsing