Whitelist themes in CSP of chrome://newtab

Whitelist themes in CSP of chrome://newtab

BUG=617499
Committed: https://crrev.com/4d6c38c4ea96577313566d1f0ba4b41d86242b6e
Cr-Commit-Position: refs/heads/master@{#398321}

[wychen, 2016-06-06 07:40:38]

wychen@chromium.org changed reviewers:
+ dbeam@chromium.org

[wychen, 2016-06-06 07:40:38]

PTAL

[Dan Beam, 2016-06-06 23:31:17]

lgtm

[Dan Beam, 2016-06-06 23:31:36]

if you can think of any useful way to test this, please do...

[wychen, 2016-06-07 00:25:24]

On 2016/06/06 23:31:36, Dan Beam wrote:
> if you can think of any useful way to test this, please do...

Originally (before https://codereview.chromium.org/2003963004) chrome://newtab
doesn't have CSP enabled. Then I enabled it, and also added img-src, which is
not restricted by default.
Therefore, one valid alternative is to get rid of img-src altogether, instead of
whitelisting all the allowed ones. With this approach, we probably don't have to
test images specifically. Having img-src would only marginally help in security,
and properly testing this requires adding one more scenario (installing a theme
with background image, and use it in incognito mode) in browsertest.

As for the test, I do have a CL about catching CSP violations here:
https://codereview.chromium.org/2013573007/
We don't have consensus about whether to use log parsing or proper notification
yet.

What do you think?

[wychen, 2016-06-07 16:29:59]

On 2016/06/07 00:25:24, wychen wrote:
> On 2016/06/06 23:31:36, Dan Beam wrote:
> > if you can think of any useful way to test this, please do...
> 
> Originally (before https://codereview.chromium.org/2003963004) chrome://newtab
> doesn't have CSP enabled. Then I enabled it, and also added img-src, which is
> not restricted by default.
> Therefore, one valid alternative is to get rid of img-src altogether, instead
of
> whitelisting all the allowed ones. With this approach, we probably don't have
to
> test images specifically. Having img-src would only marginally help in
security,
> and properly testing this requires adding one more scenario (installing a
theme
> with background image, and use it in incognito mode) in browsertest.
> 
> As for the test, I do have a CL about catching CSP violations here:
> https://codereview.chromium.org/2013573007/
> We don't have consensus about whether to use log parsing or proper
notification
> yet.
> 
> What do you think?

I'll address this (add tests, remove img-src altogether, or else) in another CL.
Let's land this first, since it's a release blocker.

[wychen, 2016-06-07 16:30:45]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2016-06-07 16:32:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2040903002/1

[commit-bot: I haz the power, 2016-06-07 17:21:50]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-07 17:23:21]

Description was changed from

==========
Whitelist themes in CSP of chrome://newtab

BUG=617499
==========

to

==========
Whitelist themes in CSP of chrome://newtab

BUG=617499

Committed: https://crrev.com/4d6c38c4ea96577313566d1f0ba4b41d86242b6e
Cr-Commit-Position: refs/heads/master@{#398321}
==========

[commit-bot: I haz the power, 2016-06-07 17:23:22]

Patchset 1 (id:??) landed as
https://crrev.com/4d6c38c4ea96577313566d1f0ba4b41d86242b6e
Cr-Commit-Position: refs/heads/master@{#398321}