Check all CSPs rather than exiting early if a resource is blocked

Check all CSPs rather than exiting early if a resource is blocked

Previously, if a single policy blocked a resource load, we would exit
early and not check the load against any other policies that might be in
effect. Checking other policies, however, might have side effects (like
sending reports) that should execute even if the load has already been
blocked.

BUG=503730
Committed: https://crrev.com/c9aac646c5b7be2c1dc2c0db0ac79fcedbaceee9
Cr-Commit-Position: refs/heads/master@{#392398}

[Mike West, 2016-04-07 22:32:32]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2016-04-07 22:32:33]

Two nits that apply throughout, and I'd like to see a few more tests (multiple
enforcements, multiple reportings, and ordering the one before the other and
vice versa) before you land it, but otherwise LGTM! Thanks for taking this... I
wasn't paying attention to the bug at all. :/

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:330:
isAllowed = (policy.get()->*allowed)(reportingStatus) && isAllowed;
Nit: `&=` seems simpler.

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:331: }
Nit: No {} for single-line clauses.

[Mike West, 2016-04-26 08:14:44]

(Ping?)

[estark, 2016-04-26 17:10:11]

On 2016/04/26 08:14:44, Mike West (slow until 25th) wrote:
> (Ping?)

Sorry, I haven't forgotten about this -- I ran into something weird that I
didn't understand when I was writing more tests and didn't have time to look
into it yet. I'll come back to this this week, promise!

[estark, 2016-05-09 18:35:39]

On 2016/04/26 17:10:11, estark wrote:
> On 2016/04/26 08:14:44, Mike West (slow until 25th) wrote:
> > (Ping?)
> 
> Sorry, I haven't forgotten about this -- I ran into something weird that I
> didn't understand when I was writing more tests and didn't have time to look
> into it yet. I'll come back to this this week, promise!

Ok, apparently I lied about coming back to this that week, but I got there
eventually. Addressed your comments, Mike, and added some more tests -- want to
take another look before I land it? Thanks!

[estark, 2016-05-09 18:35:59]

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:330:
isAllowed = (policy.get()->*allowed)(reportingStatus) && isAllowed;
On 2016/04/07 22:32:33, Mike West wrote:
> Nit: `&=` seems simpler.

Done.

https://codereview.chromium.org/1869063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:331: }
On 2016/04/07 22:32:33, Mike West wrote:
> Nit: No {} for single-line clauses.

Done.

[Mike West, 2016-05-09 18:52:21]

Still LGTM.

That said, would you mind filing a bug about the multiple console warnings?
They're all technically accurate, but it would be ideal if we would only send
out one console message for the whole thing. That, of course, would mean a good
deal of refactoring, so... later? :)

[estark, 2016-05-09 19:01:43]

On 2016/05/09 18:52:21, Mike West wrote:
> Still LGTM.
> 
> That said, would you mind filing a bug about the multiple console warnings?
> They're all technically accurate, but it would be ideal if we would only send
> out one console message for the whole thing. That, of course, would mean a
good
> deal of refactoring, so... later? :)

Sure: https://crbug.com/610410

Thanks!

[estark, 2016-05-09 19:01:47]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-05-09 19:02:07]

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

[commit-bot: I haz the power, 2016-05-09 19:38:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-09 19:40:29]

Description was changed from

==========
Check all CSPs rather than exiting early if a resource is blocked

Previously, if a single policy blocked a resource load, we would exit
early and not check the load against any other policies that might be in
effect. Checking other policies, however, might have side effects (like
sending reports) that should execute even if the load has already been
blocked.

BUG=503730
==========

to

==========
Check all CSPs rather than exiting early if a resource is blocked

Previously, if a single policy blocked a resource load, we would exit
early and not check the load against any other policies that might be in
effect. Checking other policies, however, might have side effects (like
sending reports) that should execute even if the load has already been
blocked.

BUG=503730

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

[commit-bot: I haz the power, 2016-05-09 19:40:30]

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