chrome.settingsPrivate: Check whitelist for getPref / setPref

chrome/browser/extensions/api/settings_private/prefs_util.cc

Index: chrome/browser/extensions/api/settings_private/prefs_util.cc
diff --git a/chrome/browser/extensions/api/settings_private/prefs_util.cc b/chrome/browser/extensions/api/settings_private/prefs_util.cc
index 0bf1e5e6b49fb30b7c49f3ca0e2b9b60ca95f673..306d39963aa496b3b154cb1feb0847ef62f73d62 100644
--- a/chrome/browser/extensions/api/settings_private/prefs_util.cc
+++ b/chrome/browser/extensions/api/settings_private/prefs_util.cc
@@ -372,6 +372,14 @@ const PrefsUtil::TypedPrefMap& PrefsUtil::GetWhitelistedKeys() {
   return *s_whitelist;
 }
 
+settings_private::PrefType PrefsUtil::GetWhitelistedPrefType(
+    const std::string& pref_name) {
+  const TypedPrefMap& keys = GetWhitelistedKeys();
+  const auto& iter = keys.find(pref_name);

[michaelpg, 2017/04/04 00:37:09]

nit: keys.count(pref_name) ? : is (IMO) prettier than iterators

[stevenjb, 2017/04/04 18:18:11]

We need to return iter->second.

+  return iter != keys.end() ? iter->second
+                            : settings_private::PrefType::PREF_TYPE_NONE;
+}
+
 settings_private::PrefType PrefsUtil::GetType(const std::string& name,
                                               base::Value::Type type) {
   switch (type) {
@@ -399,7 +407,10 @@ std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetCrosSettingsPref(
 
 #if defined(OS_CHROMEOS)
   const base::Value* value = CrosSettings::Get()->GetPref(name);
-  DCHECK(value) << "Pref not found: " << name;
+  if (!value) {
+    LOG(ERROR) << "Cros settings pref not found: " << name;

[michaelpg, 2017/04/04 00:37:09]

would NOTREACHED be more appropriate than removing the dcheck?

[stevenjb, 2017/04/04 18:18:11]

We should only use DCHECK for code that is logically impossible. This can happen
through the JS console.

We could use CHECK here instead, but since it is harmless and since we already
have upstream handlers, I prefer to just log an error. Otherwise if a developer
is debugging and mistypes a setting that they want to query, they crash chrome
instead of getting an error.

+    return nullptr;
+  }
   pref_object->key = name;
   pref_object->type = GetType(name, value->GetType());
   pref_object->value.reset(value->DeepCopy());
@@ -410,10 +421,17 @@ std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetCrosSettingsPref(
 
 std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetPref(
     const std::string& name) {
+  if (GetWhitelistedPrefType(name) ==
+      settings_private::PrefType::PREF_TYPE_NONE) {
+    return nullptr;
+  }
+
   const PrefService::Preference* pref = nullptr;
   std::unique_ptr<settings_private::PrefObject> pref_object;
   if (IsCrosSetting(name)) {
     pref_object = GetCrosSettingsPref(name);
+    if (!pref_object)
+      return nullptr;
   } else {
     PrefService* pref_service = FindServiceForPref(name);
     pref = pref_service->FindPreference(name);
@@ -506,6 +524,11 @@ std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetPref(
 
 PrefsUtil::SetPrefResult PrefsUtil::SetPref(const std::string& pref_name,
                                             const base::Value* value) {
+  if (GetWhitelistedPrefType(pref_name) ==
+      settings_private::PrefType::PREF_TYPE_NONE) {
+    return PREF_NOT_FOUND;
+  }
+
   if (IsCrosSetting(pref_name))
     return SetCrosSettingsPref(pref_name, value);
 
@@ -621,15 +644,8 @@ bool PrefsUtil::RemoveFromListCrosSetting(const std::string& pref_name,
 }
 
 bool PrefsUtil::IsPrefTypeURL(const std::string& pref_name) {
-  settings_private::PrefType pref_type =
-      settings_private::PrefType::PREF_TYPE_NONE;
-
-  const TypedPrefMap keys = GetWhitelistedKeys();
-  const auto& iter = keys.find(pref_name);
-  if (iter != keys.end())
-    pref_type = iter->second;
-
-  return pref_type == settings_private::PrefType::PREF_TYPE_URL;
+  return GetWhitelistedPrefType(pref_name) ==
+         settings_private::PrefType::PREF_TYPE_URL;
 }
 
 #if defined(OS_CHROMEOS)