Accept invalid chrome-extension:// and chrome:// CSP tokens

Accept invalid chrome-extension:// and chrome:// CSP tokens

Do not refuse to load the extension when the CSP contains
"chrome-extension://", because there are some extensions in the wild
that contains this token in the CSP. It is safe to accept this token
because the invalid CSP token is ignored by Blink (together with an
error message in the console, so the developer can fix the problem if
they bother to look at the console).

BUG=432227
R=kalman@chromium.org

Committed: https://crrev.com/087183913f74ebd1e18964fabd269dfebb4763f3
Cr-Commit-Position: refs/heads/master@{#304922}

[robwu, 2014-11-17 23:41:58]

Ben, could you quickly take a look at this?
If we wait a few more days, M39 will be released and the exclusion of this patch
will break some extensions.
An alternative to merging this patch is to contact some of the affected
extension authors. WDYT?

There is 1 extension with 400k+ users;
There are 2 extensions from 2013 with 6k users each;
There are 3 extensions with 523, 802 and 1046 users that are actively developed.

There are 7 extensions with 146 - 348 users, half of them are dead.
There is 1 extension with 1k users that is already broken in terms of
functionality.
There are 41 extensions with fewer than 100 users (I haven't reviewed their
source code yet).

[not at google - send to devlin, 2014-11-18 01:29:39]

I'd like to see this backed up with tests. But I'd also like to see developers
fix their extensions.

Hm, a lot has happened since these changes went in (to my memory), why don't we
just ignore invalid CSP and apply the default CSP if it doesn't parse?

[robwu, 2014-11-18 01:41:45]

On 2014/11/18 01:29:39, kalman wrote:
> I'd like to see this backed up with tests. But I'd also like to see developers
> fix their extensions.

There's an error in the console when the token is invalid.

> Hm, a lot has happened since these changes went in (to my memory), why don't
we
> just ignore invalid CSP and apply the default CSP if it doesn't parse?

Not refusing to load the extension? Sounds good. To be consistent with CSP, I
would skip invalid tokens, print an error and apply the valid CSP tokens.

[not at google - send to devlin, 2014-11-18 16:11:04]

Skipping invalid tokens sounds even better, hopefully there is already something
which has implemented parsing for you.

[robwu, 2014-11-19 09:19:45]

Aw. 39 shipped and broke an extension with 400k users:
https://chrome.google.com/webstore/detail/advanced-rest-client/hgmloofddffdnp...

kalman, can we ship this simple fix and merge it with stable and beta?

[robwu, 2014-11-19 11:55:19]

Patchset #3 (id:40001) has been deleted

[robwu, 2014-11-19 12:21:26]

Patchset #3 (id:60001) has been deleted

[robwu, 2014-11-19 12:46:24]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-11-19 12:46:48]

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

[commit-bot: I haz the power, 2014-11-19 13:27:45]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2014-11-19 13:28:23]

Patchset 3 (id:??) landed as
https://crrev.com/67244e678279741947c09ba2ab18fbfec1707a97
Cr-Commit-Position: refs/heads/master@{#304799}

[not at google - send to devlin, 2014-11-19 17:27:29]

Wuh, how did this land, there was never an lg?

[not at google - send to devlin, 2014-11-19 17:28:13]

Please revert.

[robwu, 2014-11-19 17:31:27]

On 2014/11/19 17:27:29, kalman wrote:
> Wuh, how did this land, there was never an lg?

I did a TBR, because there is something broken in the wild (assuming that you
would be okay with the change that is backed by a test).

I was planning to submit the ignore-invalid-CSP in a separate CL, but that is
slightly more complex than the current two-line change, so that will probably
require some baking.

[not at google - send to devlin, 2014-11-19 17:36:55]

https://codereview.chromium.org/722233004/diff/80001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/722233004/diff/80001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:101:
EXPECT_TRUE(ContentSecurityPolicyIsSecure(
Explain why this is secure (and link to the bug).

https://codereview.chromium.org/722233004/diff/80001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:170:
EXPECT_TRUE(ContentSecurityPolicyIsSecure(
Ditto.

[robwu, 2014-11-19 17:38:20]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/739133003/ by rob@robwu.nl.

The reason for reverting is: Need some extra comments before relanding.

[not at google - send to devlin, 2014-11-19 17:39:58]

On 2014/11/19 17:31:27, robwu wrote:
> On 2014/11/19 17:27:29, kalman wrote:
> > Wuh, how did this land, there was never an lg?
> 
> I did a TBR, because there is something broken in the wild (assuming that you
> would be okay with the change that is backed by a test).

This isn't what TBR is for. For starters we can't just immediately push out some
change to 39, so TBR doesn't necessarily help it out any more quickly.
Secondly... just never TBR anything other than a revert or changes like renaming
an interface which touches a whole bunch of files (and even that people often
don't TBR).

> 
> I was planning to submit the ignore-invalid-CSP in a separate CL, but that is
> slightly more complex than the current two-line change, so that will probably
> require some baking.

However - I think your instinct is right, that this patch is a mergable quick
fix and probably the right thing to do.

[not at google - send to devlin, 2014-11-19 17:40:37]

If you change me from TBR= to R= then lgtm on this patch.

[not at google - send to devlin, 2014-11-19 17:40:49]

On 2014/11/19 17:40:37, kalman wrote:
> If you change me from TBR= to R= then lgtm on this patch.

(you'll need to re-open it)

[robwu, 2014-11-19 18:04:34]

Added extra comments. Thanks for educating me one the right use of TBR.

[robwu, 2014-11-19 22:01:07]

Ben, does the new comments in PS 4 lg to you?

[not at google - send to devlin, 2014-11-19 22:03:05]

lgtm

[robwu, 2014-11-19 22:03:46]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-11-19 22:05:31]

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

[commit-bot: I haz the power, 2014-11-19 23:34:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-11-19 23:34:43]

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