[NTP] Ensure that search suggestion hashes are exactly 4 bytes

Hashes for search suggestions should be exactly 4 alphanumeric bytes,
this is explicitly enforced in searchbox_extension but in
search_suggest_service the byte array must be truncated to a length of
4 as the length is lost when passed through via IPC.
Previously the check was that they were *up to* 4 bytes, but this
isn't strict enough. Also remove usage of the banned std::to_string.

Bug: 965715
Change-Id: I687496ccaf225118a8f7b07bbaa2913bba19e6b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1632716
Commit-Queue: Kyle Milka <kmilka@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664345}

chrome/browser/search/search_suggest/search_suggest_service.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
@@ -19,18 +20,13 @@
 
 namespace {
 
-constexpr char kSuggestionHashRegex[] = "[a-z0-9]{1,4}";
+constexpr char kSuggestionHashRegex[] = "[a-z0-9]{4}";
 
-std::string* ValidateHash(const uint8_t hash[4]) {
-  const std::string hash_string = reinterpret_cast<const char*>(hash);
+bool ValidateHash(const uint8_t hash[4], std::string& result) {
+  const std::string hash_string(reinterpret_cast<const char*>(hash), 4);
+  result = hash_string;
 
-  std::string* trimmed_string = new std::string("");
-  // The uint8_t array received via IPC ends in an EOT byte (\4), remove it.
-  base::TrimString(hash_string, "\4", trimmed_string);
-
-  if (!re2::RE2::FullMatch(*trimmed_string, kSuggestionHashRegex))
-    return nullptr;
-  return trimmed_string;
+  return re2::RE2::FullMatch(hash_string, kSuggestionHashRegex);
 }
 
 const char kFirstShownTimeMs[] = "first_shown_time_ms";
@@ -267,7 +263,7 @@ void SearchSuggestService::BlocklistSearchSuggestion(int task_version,
     return;
 
   std::string task_version_id =
-      std::to_string(task_version) + "_" + std::to_string(task_id);
+      base::NumberToString(task_version) + "_" + base::NumberToString(task_id);
   DictionaryPrefUpdate update(profile_->GetPrefs(),
                               prefs::kNtpSearchSuggestionsBlocklist);
   base::DictionaryValue* blocklist = update.Get();
@@ -284,13 +280,12 @@ void SearchSuggestService::BlocklistSearchSuggestionWithHash(
   if (!search::DefaultSearchProviderIsGoogle(profile_))
     return;
 
-  std::string* hash_string = ValidateHash(hash);
-
-  if (!hash_string)
+  std::string hash_string;
+  if (!ValidateHash(hash, hash_string))
     return;
 
   std::string task_version_id =
-      std::to_string(task_version) + "_" + std::to_string(task_id);
+      base::NumberToString(task_version) + "_" + base::NumberToString(task_id);
 
   DictionaryPrefUpdate update(profile_->GetPrefs(),
                               prefs::kNtpSearchSuggestionsBlocklist);
@@ -298,7 +293,7 @@ void SearchSuggestService::BlocklistSearchSuggestionWithHash(
   base::Value* value = blocklist->FindKey(task_version_id);
   if (!value)
     value = blocklist->SetKey(task_version_id, base::ListValue());
-  value->GetList().emplace_back(base::Value(*hash_string));
+  value->GetList().emplace_back(base::Value(hash_string));
 
   search_suggest_data_ = base::nullopt;
   Refresh();
@@ -310,13 +305,13 @@ void SearchSuggestService::SearchSuggestionSelected(int task_version,
   if (!search::DefaultSearchProviderIsGoogle(profile_))
     return;
 
-  std::string* hash_string = ValidateHash(hash);
-
-  if (!hash_string)
+  std::string hash_string;
+  if (!ValidateHash(hash, hash_string))
     return;
 
-  std::string blocklist_item = std::to_string(task_version) + "_" +
-                               std::to_string(task_id) + ":" + *hash_string;
+  std::string blocklist_item = base::NumberToString(task_version) + "_" +
+                               base::NumberToString(task_id) + ":" +
+                               hash_string;
 
   std::string blocklist = GetBlocklistAsString();
   if (!blocklist.empty())

chrome/browser/search/search_suggest/search_suggest_service_unittest.cc

@@ -258,23 +258,34 @@ TEST_F(SearchSuggestServiceTest, BlocklistUnchangedOnInvalidHash) {
   uint8_t hash1[5] = {'a', 'b', '?', 'd', '\0'};
   uint8_t hash2[5] = {'a', '_', 'b', 'm', '\0'};
   uint8_t hash3[5] = {'A', 'B', 'C', 'D', '\0'};
-  uint8_t hash4[6] = {'a', 'b', 'c', 'd', 'e', '\0'};
   std::string expected = std::string();
 
   service()->BlocklistSearchSuggestionWithHash(0, 1234, hash1);
   service()->BlocklistSearchSuggestionWithHash(0, 1234, hash2);
   service()->BlocklistSearchSuggestionWithHash(0, 1234, hash3);
-  service()->BlocklistSearchSuggestionWithHash(0, 1234, hash4);
   ASSERT_EQ(expected, service()->GetBlocklistAsString());
 }
 
-TEST_F(SearchSuggestServiceTest, ShortHashUpdatesBlackist) {
+TEST_F(SearchSuggestServiceTest, ShortHashDoesNotUpdateBlackist) {
   SetUserSelectedDefaultSearchProvider("{google:baseURL}");
   ASSERT_EQ(std::string(), service()->GetBlocklistAsString());
 
   uint8_t hash1[4] = {'a', 'b', 'c', '\0'};
   uint8_t hash2[5] = {'d', 'e', '\0', 'f', '\0'};
-  std::string expected = "0_1234:abc;1_5678:de";
+  std::string expected = std::string();
+
+  service()->BlocklistSearchSuggestionWithHash(0, 1234, hash1);
+  service()->BlocklistSearchSuggestionWithHash(1, 5678, hash2);
+  ASSERT_EQ(expected, service()->GetBlocklistAsString());
+}
+
+TEST_F(SearchSuggestServiceTest, LongHashIsTruncated) {
+  SetUserSelectedDefaultSearchProvider("{google:baseURL}");
+  ASSERT_EQ(std::string(), service()->GetBlocklistAsString());
+
+  uint8_t hash1[6] = {'a', 'b', 'c', 'd', 'e', '\0'};
+  uint8_t hash2[7] = {'d', 'e', 'f', 'g', '\0', 'h', 'i'};
+  std::string expected = "0_1234:abcd;1_5678:defg";
 
   service()->BlocklistSearchSuggestionWithHash(0, 1234, hash1);
   service()->BlocklistSearchSuggestionWithHash(1, 5678, hash2);

chrome/renderer/searchbox/searchbox_extension.cc

@@ -1063,7 +1063,7 @@ void NewTabPageBindings::BlocklistSearchSuggestionWithHash(
     int task_version,
     int task_id,
     const std::string& hash) {
-  if (hash.length() > 4) {
+  if (hash.length() != 4) {
     return;
   }