Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(680)

Issue 2853983002: Ensure settings returned from Content Settings providers are valid (Closed)

Created:
3 years, 7 months ago by raymes
Modified:
3 years, 7 months ago
Reviewers:
msramek, Devlin, meacer
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Ensure settings returned from Content Settings providers are valid This CL adds DCHECKs to the providers we control to ensure that the settings returned are valid. It also adds conditional logic to ensure the policy provider never returns invalid settings. In the case of the policy provider the settings come from prefs which can be set by enterprise admins. BUG=711004 TBR=rdevlin.cronin@chromium.org Review-Url: https://codereview.chromium.org/2853983002 Cr-Commit-Position: refs/heads/master@{#470498} Committed: https://chromium.googlesource.com/chromium/src/+/26e1b6d4e1d7e27840055e3091520a43f59dde41

Patch Set 1 #

Patch Set 2 : Ensure settings returned from Content Settings providers are valid #

Patch Set 3 : Ensure settings returned from Content Settings providers are valid #

Patch Set 4 : Ensure settings returned from Content Settings providers are valid #

Patch Set 5 : Ensure settings returned from Content Settings providers are valid #

Patch Set 6 : Ensure settings returned from Content Settings providers are valid #

Patch Set 7 : Ensure settings returned from Content Settings providers are valid #

Total comments: 4

Patch Set 8 : \ #

Patch Set 9 : Ensure settings returned from Content Settings providers are valid #

Patch Set 10 : Ensure settings returned from Content Settings providers are valid #

Unified diffs Side-by-side diffs Delta from patch set Stats (+93 lines, -53 lines) Patch
M chrome/browser/content_settings/content_settings_internal_extension_provider.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/content_settings/content_settings_policy_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +23 lines, -4 lines 0 comments Download
M chrome/browser/content_settings/host_content_settings_map_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/extensions/api/content_settings/content_settings_api.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M components/content_settings/core/browser/content_settings_default_provider.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_info.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_info.cc View 1 2 chunks +23 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_policy_provider.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -1 line 0 comments Download
M components/content_settings/core/browser/content_settings_pref.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_registry.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/content_settings/core/browser/content_settings_registry_unittest.cc View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -35 lines 0 comments Download

Messages

Total messages: 68 (44 generated)
raymes
3 years, 7 months ago (2017-05-03 01:16:36 UTC) #15
msramek
LGTM, I always wanted to have this :) https://codereview.chromium.org/2853983002/diff/120001/components/content_settings/core/browser/content_settings_policy_provider.cc File components/content_settings/core/browser/content_settings_policy_provider.cc (right): https://codereview.chromium.org/2853983002/diff/120001/components/content_settings/core/browser/content_settings_policy_provider.cc#newcode360 components/content_settings/core/browser/content_settings_policy_provider.cc:360: } ...
3 years, 7 months ago (2017-05-03 20:05:42 UTC) #16
raymes
Thanks! https://codereview.chromium.org/2853983002/diff/120001/components/content_settings/core/browser/content_settings_policy_provider.cc File components/content_settings/core/browser/content_settings_policy_provider.cc (right): https://codereview.chromium.org/2853983002/diff/120001/components/content_settings/core/browser/content_settings_policy_provider.cc#newcode360 components/content_settings/core/browser/content_settings_policy_provider.cc:360: } else { On 2017/05/03 20:05:42, msramek (recovering) ...
3 years, 7 months ago (2017-05-04 05:49:15 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-04 05:49:35 UTC) #28
commit-bot: I haz the power
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_presubmit/builds/427176)
3 years, 7 months ago (2017-05-04 05:57:26 UTC) #30
raymes
TBR=meacer@chromium.org for trivial change in chrome/browser/extensions/api/content_settings/content_settings_api.cc
3 years, 7 months ago (2017-05-07 22:02:09 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-07 22:04:38 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/364528)
3 years, 7 months ago (2017-05-07 22:20:07 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-07 22:40:18 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/378290)
3 years, 7 months ago (2017-05-08 00:33:04 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-08 00:40:19 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/447178)
3 years, 7 months ago (2017-05-08 01:58:18 UTC) #45
meacer
lgtm (not an owner though)
3 years, 7 months ago (2017-05-08 17:00:09 UTC) #46
raymes
On 2017/05/08 17:00:09, meacer OOO May 6 - 29 wrote: > lgtm (not an owner ...
3 years, 7 months ago (2017-05-08 21:51:51 UTC) #47
raymes
TBR=rdevlin.cronin@chromium.org for trivial change in chrome/browser/extensions/api/content_settings/content_settings_api.cc
3 years, 7 months ago (2017-05-08 21:53:00 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-08 21:54:24 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/448184)
3 years, 7 months ago (2017-05-08 23:28:04 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-09 23:59:01 UTC) #56
Devlin
extensions lgtm
3 years, 7 months ago (2017-05-10 00:27:58 UTC) #57
commit-bot: I haz the power
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/265770)
3 years, 7 months ago (2017-05-10 00:37:20 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-10 00:41:29 UTC) #61
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; ...
3 years, 7 months ago (2017-05-10 04:05:35 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2853983002/180001
3 years, 7 months ago (2017-05-10 05:03:04 UTC) #65
commit-bot: I haz the power
3 years, 7 months ago (2017-05-10 06:18:15 UTC) #68
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/26e1b6d4e1d7e27840055e309152...

Powered by Google App Engine
This is Rietveld 408576698