chrome.settingsPrivate: Check whitelist for getPref / setPref

chrome.settingsPrivate: Check whitelist for getPref / setPref

We shouldn't provide access to prefs that are not in the Settings
whitelist for getPref or setPref.

BUG=707539
For comment change to settings_private.idl:
TBR=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2792163003
Cr-Commit-Position: refs/heads/master@{#461824}
Committed: https://chromium.googlesource.com/chromium/src/+/5679d251c7f59517f9423c1ee6e3c1825b116ead

[stevenjb, 2017-04-03 20:54:03]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[stevenjb, 2017-04-03 21:02:16]

Note: I did a grep for [gs]etPref and verified that every place we call those
directly the keys are already in the whitelist. (Anything using PrefObject would
fail already if not in the whitelist).

[Dan Beam, 2017-04-03 21:32:04]

lgtm

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

lgtm

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/settings_private/prefs_util.cc:378: const auto&
iter = keys.find(pref_name);
nit: keys.count(pref_name) ? : is (IMO) prettier than iterators

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/settings_private/prefs_util.cc:411: LOG(ERROR) <<
"Cros settings pref not found: " << name;
would NOTREACHED be more appropriate than removing the dcheck?

https://codereview.chromium.org/2792163003/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/settings_private.idl (right):

https://codereview.chromium.org/2792163003/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/settings_private.idl:6: // from the settings UI.
Access is restricted to a whitelisted set of user
nit: user-facing

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

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/settings_private/prefs_util.cc:378: const auto&
iter = keys.find(pref_name);
On 2017/04/04 00:37:09, michaelpg wrote:
> nit: keys.count(pref_name) ? : is (IMO) prettier than iterators

We need to return iter->second.

https://codereview.chromium.org/2792163003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/settings_private/prefs_util.cc:411: LOG(ERROR) <<
"Cros settings pref not found: " << name;
On 2017/04/04 00:37:09, michaelpg wrote:
> would NOTREACHED be more appropriate than removing the dcheck?

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.

https://codereview.chromium.org/2792163003/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/settings_private.idl (right):

https://codereview.chromium.org/2792163003/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/settings_private.idl:6: // from the settings UI.
Access is restricted to a whitelisted set of user
On 2017/04/04 00:37:09, michaelpg wrote:
> nit: user-facing

meh

[stevenjb, 2017-04-04 18:18:15]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:18:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 18:30:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:30:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2017-04-04 18:38:51]

Description was changed from

==========
chrome.settingsPrivate: Check whitelist for getPref / setPref

We shouldn't provide access to prefs that are not in the Settings
whitelist for getPref or setPref.

BUG=707539
==========

to

==========
chrome.settingsPrivate: Check whitelist for getPref / setPref

We shouldn't provide access to prefs that are not in the Settings
whitelist for getPref or setPref.

BUG=707539
For comment change to settings_private.idl:
TBR=rdevlin.cronin@chromium.org
==========

[stevenjb, 2017-04-04 18:38:56]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:39:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 20:44:35]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491331136470990, "parent_rev":
"ab450a3c76f7f6a4fa685b3151724623afc7ada1", "commit_rev":
"5679d251c7f59517f9423c1ee6e3c1825b116ead"}

[commit-bot: I haz the power, 2017-04-04 20:45:35]

Description was changed from

==========
chrome.settingsPrivate: Check whitelist for getPref / setPref

We shouldn't provide access to prefs that are not in the Settings
whitelist for getPref or setPref.

BUG=707539
For comment change to settings_private.idl:
TBR=rdevlin.cronin@chromium.org
==========

to

==========
chrome.settingsPrivate: Check whitelist for getPref / setPref

We shouldn't provide access to prefs that are not in the Settings
whitelist for getPref or setPref.

BUG=707539
For comment change to settings_private.idl:
TBR=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2792163003
Cr-Commit-Position: refs/heads/master@{#461824}
Committed:
https://chromium.googlesource.com/chromium/src/+/5679d251c7f59517f9423c1ee6e3...
==========

[commit-bot: I haz the power, 2017-04-04 20:45:40]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5679d251c7f59517f9423c1ee6e3...