Fix bug where a second CSP without script-src would cause failure

Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416, 611652
TBR=mkwst@chromium.org

Committed: https://crrev.com/d9341c818db0c3f07aba8ad98e51eeeb71271506
Cr-Commit-Position: refs/heads/master@{#396104}

[jww, 2016-05-25 00:26:42]

Description was changed from

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
==========

to

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
==========

[jww, 2016-05-25 00:26:42]

jww@chromium.org changed reviewers:
+ estark@chromium.org

[jww, 2016-05-25 00:26:58]

estark@, can you take a look at this? Thanks!

[estark, 2016-05-25 22:20:31]

lgtm

https://codereview.chromium.org/2006653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2006653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:29: NoPolicy = 0,
optional nit: it might make more sense to call this NoDirective instead of
NoPolicy.

[jww, 2016-05-25 22:47:37]

https://codereview.chromium.org/2006653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2006653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:29: NoPolicy = 0,
On 2016/05/25 22:20:31, estark wrote:
> optional nit: it might make more sense to call this NoDirective instead of
> NoPolicy.

Done.

[jww, 2016-05-25 22:48:15]

Description was changed from

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
==========

to

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
TBR=mkwst@chromium.org
==========

[jww, 2016-05-25 22:49:47]

Since this is actively breaking sites, I'm TBR'ing Mike and sending to the
commit queue, but Mike, it would be great if you can take a look when you get
back to sanity check.

[jww, 2016-05-25 22:49:52]

The CQ bit was checked by jww@chromium.org

[jww, 2016-05-25 22:49:53]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2006653005/#ps20001
(title: "Address estark's comments")

[commit-bot: I haz the power, 2016-05-25 22:50:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006653005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2006653005/20001

[commit-bot: I haz the power, 2016-05-26 03:39:24]

Description was changed from

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
TBR=mkwst@chromium.org
==========

to

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
TBR=mkwst@chromium.org
==========

[commit-bot: I haz the power, 2016-05-26 03:39:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-26 03:41:12]

Description was changed from

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
TBR=mkwst@chromium.org
==========

to

==========
Fix bug where a second CSP without script-src would cause failure

After an earlier change to make sure report-only mode did not
erroneously cause a policy failure (see
https://codereview.chromium.org/1980533002), the logic was changed so
that checking the script/style nonce would fail if a policy has no nonce
entry for a directive. Unfortunately, this had the side effect of
disallowing scripts/styles if there are two policies, and one allows
inline scripts via nonce, and the other simply did not mention scripts.

This modifies the nonce logic so that the allow[Script|Style]Nonce no
longer returns a simple bool and instead returns a disposition of Allow,
Deny, or NoPolicy. In the last case, this will not cause a failure in
and of itself, and will allow other policies to be processed before a
decision is made.

BUG=614416,611652
TBR=mkwst@chromium.org

Committed: https://crrev.com/d9341c818db0c3f07aba8ad98e51eeeb71271506
Cr-Commit-Position: refs/heads/master@{#396104}
==========

[commit-bot: I haz the power, 2016-05-26 03:41:13]

Patchset 2 (id:??) landed as
https://crrev.com/d9341c818db0c3f07aba8ad98e51eeeb71271506
Cr-Commit-Position: refs/heads/master@{#396104}

[Mike West, 2016-05-31 11:06:48]

On 2016/05/25 at 22:49:47, jww wrote:
> Since this is actively breaking sites, I'm TBR'ing Mike and sending to the
commit queue, but Mike, it would be great if you can take a look when you get
back to sanity check.

This patch LGTM as a stopgap, but I think we need to significantly refactor
nonce handling in order to correctly handle report-only. We're simply not able
to handle cases in which we violate report-only, but don't violate enforce mode.
Starting to sketch out what that might look like in
https://codereview.chromium.org/2020223002. I'll eventually break that up into
more than one CL.