Remove KeywordTableHashVerification featureThis feature was only enabled on Windows, so the code is instead markedas Windows only, along with the tests.No production behavior change is intended by this CL, except that it isno longer possible to disable the hash verification from the commandline.BUG=376303929Change-Id:Ia8d98eafb69c6ea284d038f6695d22503c437049Reviewed-on:https://chromium-review.googlesource.com/c/chromium/src/+/6766501Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>Commit-Queue: Will Harris <wfh@chromium.org>Cr-Commit-Position: refs/heads/main@{#1488997}diff --gita/components/search_engines/keyword_table.ccb/components/search_engines/keyword_table.ccindex ad6b7bd0..b6fa442e6 100644--- a/components/search_engines/keyword_table.cc+++ b/components/search_engines/keyword_table.cc
@@ -37,25 +37,6 @@ using ::base::Time; using ::country_codes::CountryId;-namespace features {-BASE_FEATURE(kKeywordTableHashVerification,- "KeywordTableHashVerification",-// Only enable this hash checking feature on Windows. This because the value of-// OSCrypt::IsEncryptionAvailable can vary and is platform specific. E.g.-// os_crypt_posix.cc historically returned 'false' for IsEncryptionAvailable. On-// Linux, OSCrypt::IsEncryptionAvailable can return `false` if v11 encryption is-// not available, but data could still be encrypted with v10 encryption, and the-// backend can change for various reasons including command line options or-// desktop window manager.-#if BUILDFLAG(IS_WIN)- base::FEATURE_ENABLED_BY_DEFAULT-#else- base::FEATURE_DISABLED_BY_DEFAULT-#endif // BUILDFLAG(IS_WIN)-);--} // namespace features- namespace { // These values are persisted to logs. Entries should not be renumbered and@@ -524,10 +505,12 @@ all_rows_migrated); };+ // See the comment in `GetKeywordDataFromStatement` as to why this code is+ // only enabled for Windows.+#if BUILDFLAG(IS_WIN) // If there is no platform encryption, nothing left to do, since the // `url_hash` column will just be NULL.- if (!base::FeatureList::IsEnabled(features::kKeywordTableHashVerification) ||- !encryptor()->IsEncryptionAvailable()) {+ if (!encryptor()->IsEncryptionAvailable()) { return transaction.Commit(); }@@ -570,7 +553,7 @@ continue; } }-+#endif // BUILDFLAG(IS_WIN) return transaction.Commit(); }@@ -634,9 +617,16 @@ status); };- if (!base::FeatureList::IsEnabled(features::kKeywordTableHashVerification)) {- status = HashValidationStatus::kNotVerifiedFeatureDisabled;- } else if (!encryptor()->IsDecryptionAvailable()) {+// Only enable this hash checking feature on Windows. This because the value of+// `OSCrypt::IsEncryptionAvailable` (exposed via the `Encryptor`+// `IsDecryptionAvailable` API) can vary and is platform specific. E.g.+// os_crypt_posix.cc historically returned 'false' for `IsEncryptionAvailable`.+// On Linux, `IsEncryptionAvailable` can return `false` if v11 encryption is+// not available, but data could still be encrypted with v10 encryption, and the+// backend can change for various reasons including command line options or+// desktop window manager.+#if BUILDFLAG(IS_WIN)+ if (!encryptor()->IsDecryptionAvailable()) { status = HashValidationStatus::kNotVerifiedNoCrypto; } else { const auto hash = encryptor()->DecryptData(s.ColumnBlob(27));@@ -659,7 +649,9 @@ return std::nullopt; } }-+#else+ status = HashValidationStatus::kNotVerifiedFeatureDisabled;+#endif // BUILDFLAG(IS_WIN) return data; }diff --gita/components/search_engines/keyword_table.hb/components/search_engines/keyword_table.hindex 12e73f0..a6008b0e 100644--- a/components/search_engines/keyword_table.h+++ b/components/search_engines/keyword_table.h
@@ -13,7 +13,6 @@ #include <vector> #include "base/compiler_specific.h"-#include "base/feature_list.h" #include "base/gtest_prod_util.h" #include "components/country_codes/country_codes.h" #include "components/search_engines/template_url_id.h"@@ -26,12 +25,6 @@ class Statement; } // namespace sql-namespace features {-// An emergency 'off switch' to disable hash verification.-// TODO(crbug.com/376303929): Remove in M134.-BASE_DECLARE_FEATURE(kKeywordTableHashVerification);-} // namespace features- // This class manages the |keywords| MetaTable within the SQLite database // passed to the constructor. It expects the following schema: //diff --gita/components/search_engines/keyword_table_unittest.ccb/components/search_engines/keyword_table_unittest.ccindex 8254c4d0..e1649dac 100644--- a/components/search_engines/keyword_table_unittest.cc+++ b/components/search_engines/keyword_table_unittest.cc
@@ -127,18 +127,20 @@ TEST_F(KeywordTableTest, Keywords) {- // The feature is tested elsewhere, force enable to make sure expectations- // match.- base::test::ScopedFeatureList enable_verification(- features::kKeywordTableHashVerification);- TemplateURLData keyword(CreateAndAddKeyword()); base::HistogramTester histograms; KeywordTable::Keywords keywords(GetKeywords());+ constexpr base::HistogramBase::Sample32 expected_bucket =+#if BUILDFLAG(IS_WIN)+ 0; // HashValidationStatus::kSuccess;+#else+ 5; // HashValidationStatus::kNotVerifiedFeatureDisabled;+#endif // BUILDFLAG(IS_WIN) histograms.ExpectUniqueSample("Search.KeywordTable.HashValidationStatus",- /*HashValidationStatus::kSuccess*/ 0, 1);+ expected_bucket, 1);+ EXPECT_EQ(1U, keywords.size()); const TemplateURLData& restored_keyword = keywords.front();@@ -285,18 +287,17 @@ } }+#if BUILDFLAG(IS_WIN) namespace { struct TestCase { bool encryption_enabled;- bool feature_enabled; bool tamper; base::HistogramBase::Sample32 expected_histogram_sample; size_t expected_keyword_count; std::string Name() const { return base::StrCat({encryption_enabled ? "Encryption" : "NoEncryption",- feature_enabled ? "FeatureEnabled" : "FeatureDisabled", tamper ? "Tamper" : "NoTamper"}); } };@@ -306,14 +307,6 @@ class KeywordTableTestEncryption : public KeywordTableTest, public ::testing::WithParamInterface<::TestCase> {- public:- KeywordTableTestEncryption() {- feature_.InitWithFeatureState(features::kKeywordTableHashVerification,- GetParam().feature_enabled);- }-- private:- base::test::ScopedFeatureList feature_; }; TEST_P(KeywordTableTestEncryption, KeywordBadHash) {@@ -345,55 +338,25 @@ /*empty*/, KeywordTableTestEncryption, ::testing::Values(- ::TestCase{- .encryption_enabled = false,- .feature_enabled = false,- .tamper = true,- .expected_histogram_sample = /*kNotVerifiedFeatureDisabled*/ 5,- .expected_keyword_count = 1u}, ::TestCase{.encryption_enabled = false,- .feature_enabled = true, .tamper = true, .expected_histogram_sample = /*kNotVerifiedNoCrypto*/ 4, .expected_keyword_count = 1u},- ::TestCase{- .encryption_enabled = true,- .feature_enabled = false,- .tamper = true,- .expected_histogram_sample = /*kNotVerifiedFeatureDisabled*/ 5,- .expected_keyword_count = 1u}, ::TestCase{.encryption_enabled = true,- .feature_enabled = true, .tamper = true, .expected_histogram_sample = /*kIncorrectHash*/ 3, .expected_keyword_count = 0},- ::TestCase{- .encryption_enabled = false,- .feature_enabled = false,- .tamper = false,- .expected_histogram_sample = /*kNotVerifiedFeatureDisabled*/ 5,- .expected_keyword_count = 1u}, ::TestCase{.encryption_enabled = false,- .feature_enabled = true, .tamper = false, .expected_histogram_sample = /*kNotVerifiedNoCrypto*/ 4, .expected_keyword_count = 1u},- ::TestCase{- .encryption_enabled = true,- .feature_enabled = false,- .tamper = false,- .expected_histogram_sample = /*kNotVerifiedFeatureDisabled*/ 5,- .expected_keyword_count = 1u}, ::TestCase{.encryption_enabled = true,- .feature_enabled = true, .tamper = false, .expected_histogram_sample = /*kSuccess*/ 0, .expected_keyword_count = 1u}), [](const auto& info) { return info.param.Name(); }); TEST_F(KeywordTableTest, KeywordBadCrypto) {- base::test::ScopedFeatureList enable_verification(- features::kKeywordTableHashVerification); TemplateURLData keyword(CreateAndAddKeyword()); { KeywordTable::Keywords keywords(GetKeywords());@@ -416,11 +379,9 @@ 1); } }+#endif // BUILDFLAG(IS_WIN) TEST_F(KeywordTableTest, KeywordBadUrl) {- base::test::ScopedFeatureList enable_verification(- features::kKeywordTableHashVerification);- TemplateURLData keyword(CreateAndAddKeyword()); { KeywordTable::Keywords keywords(GetKeywords());diff --gita/components/webdata/common/web_database_migration_unittest.ccb/components/webdata/common/web_database_migration_unittest.ccindex 621d88c..d0b807ac 100644--- a/components/webdata/common/web_database_migration_unittest.cc+++ b/components/webdata/common/web_database_migration_unittest.cc
@@ -1516,6 +1516,7 @@ } }+#if BUILDFLAG(IS_WIN) class WebDatabaseMigrationTestEncryption : public WebDatabaseMigrationTest, public ::testing::WithParamInterface<bool> {@@ -1525,11 +1526,6 @@ // Tests addition of the url_hash column to the keywords table. TEST_P(WebDatabaseMigrationTestEncryption, MigrateVersion136ToCurrent) {- // The feature is tested elsewhere, force enable to make sure expectations- // match.- base::test::ScopedFeatureList enable_verification(- features::kKeywordTableHashVerification);- encryptor_.set_encryption_available_for_testing(IsEncryptionAvailable()); encryptor_.set_decryption_available_for_testing(IsEncryptionAvailable());@@ -1594,13 +1590,10 @@ // Tests migration of a keywords table with an empty url, which is invalid. The // entry should not be migrated, and the test should not crash. The dropping of // the invalid entry takes place upon the first GetKeywords call, and this is-// tested elsewhere in KeywordTableTest.KeywordBadUrl.+// tested elsewhere in KeywordTableTest.KeywordBadUrl. This test is only valid+// on Windows because the bad url detection only happens if encrypted hashing is+// enabled. TEST_F(WebDatabaseMigrationTest, MigrateVersion136ToCurrentBadUrl) {- // The feature is tested elsewhere, force enable to make sure expectations- // match.- base::test::ScopedFeatureList enable_verification(- features::kKeywordTableHashVerification);- ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_136.sql"))); const TemplateURLID kTestId = 99; {@@ -1622,6 +1615,27 @@ false, 1); } }+#else+// On non-Windows the 136 to 137 migration does nothing except update add the+// `url_hash` column and update the database version.+TEST_F(WebDatabaseMigrationTest, MigrateVersion136ToCurrent) {+ ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_136.sql")));+ {+ sql::Database connection(sql::test::kTestTag);+ ASSERT_TRUE(connection.Open(GetDatabasePath()));+ EXPECT_EQ(136, VersionFromConnection(&connection));+ EXPECT_FALSE(connection.DoesColumnExist("keywords", "url_hash"));+ }+ DoMigration();+ {+ sql::Database connection(sql::test::kTestTag);+ ASSERT_TRUE(connection.Open(GetDatabasePath()));+ EXPECT_EQ(WebDatabase::kCurrentVersionNumber,+ VersionFromConnection(&connection));+ EXPECT_TRUE(connection.DoesColumnExist("keywords", "url_hash"));+ }+}+#endif // BUILDFLAG(IS_WIN) TEST_F(WebDatabaseMigrationTest, MigrateVersion137ToCurrent) { ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_137.sql")));