chrome.settingsPrivate: Check whitelist for getPref / setPref

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

 // Copyright 2015 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/extensions/api/settings_private/prefs_util.h"
 
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/browser/extensions/settings_api_helpers.h"
@@ skipping 354 matching lines @@
       settings_private::PrefType::PREF_TYPE_DICTIONARY;
 
 #if defined(GOOGLE_CHROME_BUILD)
   (*s_whitelist)[::prefs::kMediaRouterEnableCloudServices] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
 #endif  // defined(GOOGLE_CHROME_BUILD)
 
   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) {
     case base::Value::Type::BOOLEAN:
       return settings_private::PrefType::PREF_TYPE_BOOLEAN;
     case base::Value::Type::INTEGER:
     case base::Value::Type::DOUBLE:
       return settings_private::PrefType::PREF_TYPE_NUMBER;
     case base::Value::Type::STRING:
       return IsPrefTypeURL(name) ? settings_private::PrefType::PREF_TYPE_URL
                                  : settings_private::PrefType::PREF_TYPE_STRING;
     case base::Value::Type::LIST:
       return settings_private::PrefType::PREF_TYPE_LIST;
     case base::Value::Type::DICTIONARY:
       return settings_private::PrefType::PREF_TYPE_DICTIONARY;
     default:
       return settings_private::PrefType::PREF_TYPE_NONE;
   }
 }
 
 std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetCrosSettingsPref(
     const std::string& name) {
   std::unique_ptr<settings_private::PrefObject> pref_object(
       new settings_private::PrefObject());
 
 #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());
 #endif
 
   return pref_object;
 }
 
 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);
     if (!pref)
       return nullptr;
     pref_object.reset(new settings_private::PrefObject());
     pref_object->key = pref->name();
     pref_object->type = GetType(name, pref->GetType());
     pref_object->value.reset(pref->GetValue()->DeepCopy());
   }
@@ skipping 72 matching lines @@
     return pref_object;
   }
 
   // TODO(dbeam): surface !IsUserModifiable or IsPrefSupervisorControlled?
 
   return pref_object;
 }
 
 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);
 
   PrefService* pref_service = FindServiceForPref(pref_name);
 
   if (!IsPrefUserModifiable(pref_name))
     return PREF_NOT_MODIFIABLE;
 
   const PrefService::Preference* pref = pref_service->FindPreference(pref_name);
   if (!pref)
@@ skipping 95 matching lines @@
   }
 
   CrosSettings::Get()->RemoveFromList(pref_name, &value);
   return true;
 #else
   return false;
 #endif
 }
 
 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)
 bool PrefsUtil::IsPrefEnterpriseManaged(const std::string& pref_name) {
   if (IsPrivilegedCrosSetting(pref_name)) {
     policy::BrowserPolicyConnectorChromeOS* connector =
         g_browser_process->platform_part()->browser_policy_connector_chromeos();
     if (connector->IsEnterpriseManaged())
       return true;
   }
@@ skipping 108 matching lines @@
       ExtensionPrefValueMapFactory::GetForBrowserContext(profile_)
           ->GetExtensionControllingPref(pref_object.key);
   if (extension_id.empty())
     return nullptr;
 
   return ExtensionRegistry::Get(profile_)->GetExtensionById(
       extension_id, ExtensionRegistry::ENABLED);
 }
 
 }  // namespace extensions