Implement ContentSecurityPolicy on the browser-side.

Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When no renderer are used (frame-ancestors with PDF files, ...).

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2612793002
Cr-Commit-Position: refs/heads/master@{#451807}
Committed: https://chromium.googlesource.com/chromium/src/+/7c9cab4db31531feba84b8b8e6e8a8b859afbc5a

[arthursonzogni, 2017-01-04 17:16:56]

Description was changed from

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When there is not renderer (frame-ancestors with PDF files, ...)

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
==========

to

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When there is not renderer (frame-ancestors with PDF files, ...)

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-01-04 17:31:44]

Description was changed from

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When there is not renderer (frame-ancestors with PDF files, ...)

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When no renderer are used (frame-ancestors with PDF files, ...).

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-01-05 10:48:48]

Patchset #1 (id:1) has been deleted

[arthursonzogni, 2017-01-11 14:02:36]

Patchset #1 (id:20001) has been deleted

[arthursonzogni, 2017-01-11 14:53:34]

Patchset #1 (id:40001) has been deleted

[arthursonzogni, 2017-01-11 14:58:22]

Patchset #1 (id:60001) has been deleted

[arthursonzogni, 2017-01-11 15:08:06]

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

[arthursonzogni, 2017-01-11 15:08:07]

Hi Mike,

I would like to have your feeling about this. Its a set of class that could
parse and check CSPs. It's not tied to the renderer process.

Then, my plan would be to implement CSPContext and put it inside each
FrameTreeNode on the browser-side. Then the NavigationThrottles could check and
block urls from several directives we are interested in: 
- frame-ancestors
- form-action
- child-src

I know that you are very busy right know, sorry! Let me know if there is any
problems.

Thanks!

[arthursonzogni, 2017-01-18 10:30:24]

arthursonzogni@chromium.org changed reviewers:
+ nasko@chromium.org

[arthursonzogni, 2017-01-18 10:30:24]

Hi Nasko,

I discuss with Mike yesterday, he think it would be a safer first step to just
check the CSP on the browser-side but parse it on the renderer.

Now I understand better how it works, I don't think it would be difficult to
remove the parser from this patch and turn this set of class into a set of
struct that could be populated by the renderer when the request is launched by
the renderer.

The drawback is that we won't be able check the frame-ancestors without a
renderer (for instance a pdf-loader). But there is no problems with
frame-ancestors related to PlzNavigate.

What do you think? Are you still okay?

[arthursonzogni, 2017-01-18 17:08:11]

Patchset #2 (id:100001) has been deleted

[arthursonzogni, 2017-01-23 10:34:49]

Patchset #3 (id:140001) has been deleted

[nasko, 2017-01-23 22:54:50]

> I discuss with Mike yesterday, he think it would be a safer first step to just
> check the CSP on the browser-side but parse it on the renderer.

If we don't have to do it browser-side, I'm fine with that and even happy :).

> Now I understand better how it works, I don't think it would be difficult to
> remove the parser from this patch and turn this set of class into a set of
> struct that could be populated by the renderer when the request is launched by
> the renderer.
> 
> The drawback is that we won't be able check the frame-ancestors without a
> renderer (for instance a pdf-loader). But there is no problems with
> frame-ancestors related to PlzNavigate.

I'm not familiar with all the details of CSP for navigation unfortunately :(.
I'd defer to mkwst@ and alexmos@ on reasoning whether we have to enforce
frame-ancestors on redirects or not.
 
> What do you think? Are you still okay?

If mkwst@ and alexmos@ say we are ok, I'm ok :).

https://codereview.chromium.org/2612793002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2612793002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3200: //if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
nit: Hopefully not committing this, right? :)

[nasko, 2017-01-23 23:08:07]

nasko@chromium.org changed reviewers:
+ alexmos@chromium.org

[nasko, 2017-01-23 23:08:07]

Adding alexmos@ actually.

[alexmos, 2017-01-24 01:54:09]

On 2017/01/23 22:54:50, nasko wrote:
> > I discuss with Mike yesterday, he think it would be a safer first step to
just
> > check the CSP on the browser-side but parse it on the renderer.
> 
> If we don't have to do it browser-side, I'm fine with that and even happy :).
> 
> > Now I understand better how it works, I don't think it would be difficult to
> > remove the parser from this patch and turn this set of class into a set of
> > struct that could be populated by the renderer when the request is launched
by
> > the renderer.
> > 
> > The drawback is that we won't be able check the frame-ancestors without a
> > renderer (for instance a pdf-loader). But there is no problems with
> > frame-ancestors related to PlzNavigate.
> 
> I'm not familiar with all the details of CSP for navigation unfortunately :(.
> I'd defer to mkwst@ and alexmos@ on reasoning whether we have to enforce
> frame-ancestors on redirects or not.

This (parsing in the renderer, enforcing in the browser) seems reasonable to me
as a first step.  It seems that we ignore the CSP header in redirect responses,
so I don't see a problem there.  (I was thinking whether smth like a 301
response for a subframe could also include a CSP header with frame-ancestors
that could block following the redirect, but that doesn't seem to be the case --
Nasko, did you have some other possibly problematic redirect scenario in mind?)

Perhaps a bit related, we similarly parse feature policy in the renderer and
send it off to the browser process, where it lives in the frame replication
state.  I don't think that's used to enforce anything in the browser process
yet, but that was the plan (see https://codereview.chromium.org/2520223002/).

[arthursonzogni, 2017-01-24 15:04:00]

Patchset #4 (id:180001) has been deleted

[arthursonzogni, 2017-01-24 15:27:07]

Patchset #5 (id:220001) has been deleted

[arthursonzogni, 2017-01-24 15:27:17]

Patchset #4 (id:200001) has been deleted

[arthursonzogni, 2017-01-24 15:50:48]

Patchset #4 (id:240001) has been deleted

[arthursonzogni, 2017-01-24 16:16:23]

Hi there,

I think I have reached the point where this patch can be reviewed. Please, could
you take a look at it?

mkwst: I need you for the code that check the CSP on the browser-side and the
one that expose blink's CSP.
* content/common/content_security_policy/*
* third_party/WebKit/Source/core/frame/csp/*

nasko: Can you check the code related to IPC?

Thanks!

---

Since I removed the parser from the browser and rely on the renderer parser, we
can't parse and check the CSP from a redirected response to decide if the
navigation should continue. I assumed that it isn't something used at all, or at
least not for the 2 CSP we want to enforce with PlzNavigate ('frame-src' and
'form-action').

I will have to check what is the current behavior for 'frame-ancestors' if there
is 2 redirections (A -> B -> C) and
B frame-ancestors = 'none '
C frame-ancestors = '*'
If C is not displayed without PlzNavigate, it might be a problem.

[alexmos, 2017-01-24 18:14:04]

> Since I removed the parser from the browser and rely on the renderer parser,
we
> can't parse and check the CSP from a redirected response to decide if the
> navigation should continue. I assumed that it isn't something used at all, or
at
> least not for the 2 CSP we want to enforce with PlzNavigate ('frame-src' and
> 'form-action').
> 
> I will have to check what is the current behavior for 'frame-ancestors' if
there
> is 2 redirections (A -> B -> C) and
> B frame-ancestors = 'none '
> C frame-ancestors = '*'
> If C is not displayed without PlzNavigate, it might be a problem.

Yes, this is exactly what I tried out manually yesterday, and it appears that a
CSP header in a 30x response is ignored.  I.e., B's frame-ancestors 'none' isn't
stopping the redirect to C today.  (I've also peeked at the redirect code a
bit.)  So I don't think there's a problem there, but please double-check me. :) 


frame-src shouldn't be a problem since we'll already have the parent's policy in
the browser for its child frame redirects and can apply it accordingly.  Same
for form-action.

[arthursonzogni, 2017-01-25 10:06:20]

Thanks @alex@ for these confirmations.

I double check what we talk about with 'frame-ancestors'. Yes, the navigation is
not blocked by B's response.

[Mike West, 2017-01-25 10:48:02]

On 2017/01/24 at 18:14:04, alexmos wrote:
> > Since I removed the parser from the browser and rely on the renderer parser,
we
> > can't parse and check the CSP from a redirected response to decide if the
> > navigation should continue. I assumed that it isn't something used at all,
or at
> > least not for the 2 CSP we want to enforce with PlzNavigate ('frame-src' and
> > 'form-action').
> > 
> > I will have to check what is the current behavior for 'frame-ancestors' if
there
> > is 2 redirections (A -> B -> C) and
> > B frame-ancestors = 'none '
> > C frame-ancestors = '*'
> > If C is not displayed without PlzNavigate, it might be a problem.
> 
> Yes, this is exactly what I tried out manually yesterday, and it appears that
a CSP header in a 30x response is ignored.  I.e., B's frame-ancestors 'none'
isn't stopping the redirect to C today.  (I've also peeked at the redirect code
a bit.)  So I don't think there's a problem there, but please double-check me.
:)  

According to the spec, we should be checking `frame-ancestors` on redirects. We
parse the policy in step 13 of https://fetch.spec.whatwg.org/#http-network-fetch
for all responses (including redirects). That redirect response eventually makes
its way back to step 10 of
https://html.spec.whatwg.org/multipage/browsers.html#process-a-navigate-fetch,
which reruns that algorithm, which means that the CSP check in step 6 applies.

We don't currently check CSP on redirects because we have no mechanism to do so
(they're handled in the network stack and don't actually generate a document in
which we could check policy). That's one of the reasons that we moved
X-Frame-Options up to the browser via AncestorThrottle, and I would expect that
we'd follow that example for `frame-ancestors`. To the extent possible, those
two mechanisms should have equivalent behavior.

Is integrating this into AncestorThrottle part of your plan?

> frame-src shouldn't be a problem since we'll already have the parent's policy
in the browser for its child frame redirects and can apply it accordingly.  Same
for form-action.

Agreed.

[arthursonzogni, 2017-01-25 12:27:28]

On 2017/01/25 10:48:02, Mike West (sloooooow) wrote:
> On 2017/01/24 at 18:14:04, alexmos wrote:
> > > Since I removed the parser from the browser and rely on the renderer
parser,
> we
> > > can't parse and check the CSP from a redirected response to decide if the
> > > navigation should continue. I assumed that it isn't something used at all,
> or at
> > > least not for the 2 CSP we want to enforce with PlzNavigate ('frame-src'
and
> > > 'form-action').
> > > 
> > > I will have to check what is the current behavior for 'frame-ancestors' if
> there
> > > is 2 redirections (A -> B -> C) and
> > > B frame-ancestors = 'none '
> > > C frame-ancestors = '*'
> > > If C is not displayed without PlzNavigate, it might be a problem.
> > 
> > Yes, this is exactly what I tried out manually yesterday, and it appears
that
> a CSP header in a 30x response is ignored.  I.e., B's frame-ancestors 'none'
> isn't stopping the redirect to C today.  (I've also peeked at the redirect
code
> a bit.)  So I don't think there's a problem there, but please double-check me.
> :)  
> 
> According to the spec, we should be checking `frame-ancestors` on redirects.
We
> parse the policy in step 13 of
https://fetch.spec.whatwg.org/#http-network-fetch
> for all responses (including redirects). That redirect response eventually
makes
> its way back to step 10 of
> https://html.spec.whatwg.org/multipage/browsers.html#process-a-navigate-fetch,
> which reruns that algorithm, which means that the CSP check in step 6 applies.
> 
> We don't currently check CSP on redirects because we have no mechanism to do
so
> (they're handled in the network stack and don't actually generate a document
in
> which we could check policy). That's one of the reasons that we moved
> X-Frame-Options up to the browser via AncestorThrottle, and I would expect
that
> we'd follow that example for `frame-ancestors`. To the extent possible, those
> two mechanisms should have equivalent behavior.
> 
> Is integrating this into AncestorThrottle part of your plan?
> 
> > frame-src shouldn't be a problem since we'll already have the parent's
policy
> in the browser for its child frame redirects and can apply it accordingly. 
Same
> for form-action.
> 
> Agreed.

Thanks for the clarification about the specification.
So, in the future, we will have to parse the CSP in the browser process whatever
happens.

For shipping PlzNavigate, we just need to make 'frame-src' and 'form-action'
working on redirect. It doesn't need to parse the CSP in the browser.
Yes, my plan is to create some NavigationThrottles that will enforce these two
CSP. Maybe the AncestorThrottle can be reused for checking 'frame-src' too.
I agree that not parsing the CSP in the browser is a safer first step. I am okay
to reuse blink's CSP.

Then in the future, if we are able to parse the CSP in the browser (was
implemented in patch #3, removed in #4), we could be able to check
'frame-ancestor' inside the AncestorThrottle.

[clamy, 2017-01-25 12:40:30]

On 2017/01/25 12:27:28, arthursonzogni wrote:
> On 2017/01/25 10:48:02, Mike West (sloooooow) wrote:
> > On 2017/01/24 at 18:14:04, alexmos wrote:
> > > > Since I removed the parser from the browser and rely on the renderer
> parser,
> > we
> > > > can't parse and check the CSP from a redirected response to decide if
the
> > > > navigation should continue. I assumed that it isn't something used at
all,
> > or at
> > > > least not for the 2 CSP we want to enforce with PlzNavigate ('frame-src'
> and
> > > > 'form-action').
> > > > 
> > > > I will have to check what is the current behavior for 'frame-ancestors'
if
> > there
> > > > is 2 redirections (A -> B -> C) and
> > > > B frame-ancestors = 'none '
> > > > C frame-ancestors = '*'
> > > > If C is not displayed without PlzNavigate, it might be a problem.
> > > 
> > > Yes, this is exactly what I tried out manually yesterday, and it appears
> that
> > a CSP header in a 30x response is ignored.  I.e., B's frame-ancestors 'none'
> > isn't stopping the redirect to C today.  (I've also peeked at the redirect
> code
> > a bit.)  So I don't think there's a problem there, but please double-check
me.
> > :)  
> > 
> > According to the spec, we should be checking `frame-ancestors` on redirects.
> We
> > parse the policy in step 13 of
> https://fetch.spec.whatwg.org/#http-network-fetch
> > for all responses (including redirects). That redirect response eventually
> makes
> > its way back to step 10 of
> >
https://html.spec.whatwg.org/multipage/browsers.html#process-a-navigate-fetch,
> > which reruns that algorithm, which means that the CSP check in step 6
applies.
> > 
> > We don't currently check CSP on redirects because we have no mechanism to do
> so
> > (they're handled in the network stack and don't actually generate a document
> in
> > which we could check policy). That's one of the reasons that we moved
> > X-Frame-Options up to the browser via AncestorThrottle, and I would expect
> that
> > we'd follow that example for `frame-ancestors`. To the extent possible,
those
> > two mechanisms should have equivalent behavior.
> > 
> > Is integrating this into AncestorThrottle part of your plan?
> > 
> > > frame-src shouldn't be a problem since we'll already have the parent's
> policy
> > in the browser for its child frame redirects and can apply it accordingly. 
> Same
> > for form-action.
> > 
> > Agreed.
> 
> Thanks for the clarification about the specification.
> So, in the future, we will have to parse the CSP in the browser process
whatever
> happens.
> 
> For shipping PlzNavigate, we just need to make 'frame-src' and 'form-action'
> working on redirect. It doesn't need to parse the CSP in the browser.
> Yes, my plan is to create some NavigationThrottles that will enforce these two
> CSP. Maybe the AncestorThrottle can be reused for checking 'frame-src' too.
> I agree that not parsing the CSP in the browser is a safer first step. I am
okay
> to reuse blink's CSP.
> 
> Then in the future, if we are able to parse the CSP in the browser (was
> implemented in patch #3, removed in #4), we could be able to check
> 'frame-ancestor' inside the AncestorThrottle.

Can we clarify things a bit here? Sorry I'm not sure I'm following all of the
discussion. This is my understanding so far.

- For shipping PlzNavigate: we don't need to enforce 'frame-ancestor' on
redirects, since the current code doesn't currently enforce them. Is that
correct?
- Long term: we want to enforce 'frame-ancestor' on redirects, in particular if
we receive a 30x response with a CSP directive of frame-ancestor none, we should
stop the navigation. Is that correct?

If that's the case, it's going to be difficult to enforce 'frame-ancestor' on
redirects without parsing the CSP header in the browser. At the time the 30x
response is received by the browser, we simply don't have a renderer that can
parse the header for us and tell us that it contained a directive of
frame-ancestor none. If we absolutely want to keep everything renderer-side, we
can still parse and check CSP renderer-side for every redirect response when we
try to commit the navigation. We would be able to block the frame, but we would
have made several extra-requests. This is wasteful, and has potential side
effects on the cookies in the browser that we may want to avoid.

[Mike West, 2017-01-25 12:43:45]

On 2017/01/25 at 12:40:30, clamy wrote:
> - For shipping PlzNavigate: we don't need to enforce 'frame-ancestor' on
redirects, since the current code doesn't currently enforce them. Is that
correct?

Correct.

> - Long term: we want to enforce 'frame-ancestor' on redirects, in particular
if we receive a 30x response with a CSP directive of frame-ancestor none, we
should stop the navigation. Is that correct?

Correct.

> If that's the case, it's going to be difficult to enforce 'frame-ancestor' on
redirects without parsing the CSP header in the browser.

I agree. Maybe that's an argument for going back to the original proposal of
parsing a subset of the policy in the browser... :(

[arthursonzogni, 2017-01-25 13:38:22]

On 2017/01/25 12:43:45, Mike West (sloooooow) wrote:
> On 2017/01/25 at 12:40:30, clamy wrote:
> > - For shipping PlzNavigate: we don't need to enforce 'frame-ancestor' on
> redirects, since the current code doesn't currently enforce them. Is that
> correct?
> 
> Correct.
> 
> > - Long term: we want to enforce 'frame-ancestor' on redirects, in particular
> if we receive a 30x response with a CSP directive of frame-ancestor none, we
> should stop the navigation. Is that correct?
> 
> Correct.
> 
> > If that's the case, it's going to be difficult to enforce 'frame-ancestor'
on
> redirects without parsing the CSP header in the browser.
> 
> I agree. Maybe that's an argument for going back to the original proposal of
> parsing a subset of the policy in the browser... :(

What do we decide to do now? Should I add the parser again such that the CSP
could be parsed outside of a renderer?
I know that you don't like this solution because there is some code duplication.
Two parsers to maintain... But I don't imagine an easy way to avoid it for the
moment.

[nasko, 2017-01-25 14:49:55]

On 2017/01/25 13:38:22, arthursonzogni wrote:
> On 2017/01/25 12:43:45, Mike West (sloooooow) wrote:
> > On 2017/01/25 at 12:40:30, clamy wrote:
> > > - For shipping PlzNavigate: we don't need to enforce 'frame-ancestor' on
> > redirects, since the current code doesn't currently enforce them. Is that
> > correct?
> > 
> > Correct.
> > 
> > > - Long term: we want to enforce 'frame-ancestor' on redirects, in
particular
> > if we receive a 30x response with a CSP directive of frame-ancestor none, we
> > should stop the navigation. Is that correct?
> > 
> > Correct.
> > 
> > > If that's the case, it's going to be difficult to enforce 'frame-ancestor'
> on
> > redirects without parsing the CSP header in the browser.
> > 
> > I agree. Maybe that's an argument for going back to the original proposal of
> > parsing a subset of the policy in the browser... :(
> 
> What do we decide to do now? Should I add the parser again such that the CSP
> could be parsed outside of a renderer?

Since current behavior doesn't enforce CSP on redirects, we should not add any
new parsing code. The higher priority bit now is to ship PlzNavigate, which
means we can ignore CSP on redirects for now.

> I know that you don't like this solution because there is some code
duplication.
> Two parsers to maintain... But I don't imagine an easy way to avoid it for the
> moment.

Let's tackle this down the road.

[arthursonzogni, 2017-02-03 21:30:59]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 21:31:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 21:36:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 21:36:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[arthursonzogni, 2017-02-03 22:16:10]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 22:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 22:39:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 22:39:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[arthursonzogni, 2017-02-03 23:33:36]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 23:34:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 00:01:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 00:01:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-02-04 00:06:21]

Patchset #10 (id:370001) has been deleted

[arthursonzogni, 2017-02-06 09:23:06]

Patchset #9 (id:350001) has been deleted

[arthursonzogni, 2017-02-06 09:23:47]

Patchset #8 (id:330001) has been deleted

[arthursonzogni, 2017-02-06 09:34:53]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 09:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 10:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 10:00:44]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[arthursonzogni, 2017-02-06 10:19:40]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 10:20:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-02-06 10:27:38]

Patchset #8 (id:390001) has been deleted

[commit-bot: I haz the power, 2017-02-06 11:02:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 11:02:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-02-06 11:05:23]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 11:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 12:39:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 12:39:44]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-02-09 14:01:06]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 14:01:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 16:40:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 16:40:32]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-02-09 16:52:36]

On 2017/01/25 14:49:55, nasko (very slow) wrote:
> On 2017/01/25 13:38:22, arthursonzogni wrote:
> > On 2017/01/25 12:43:45, Mike West (sloooooow) wrote:
> > > On 2017/01/25 at 12:40:30, clamy wrote:
> > > > - For shipping PlzNavigate: we don't need to enforce 'frame-ancestor' on
> > > redirects, since the current code doesn't currently enforce them. Is that
> > > correct?
> > > 
> > > Correct.
> > > 
> > > > - Long term: we want to enforce 'frame-ancestor' on redirects, in
> particular
> > > if we receive a 30x response with a CSP directive of frame-ancestor none,
we
> > > should stop the navigation. Is that correct?
> > > 
> > > Correct.
> > > 
> > > > If that's the case, it's going to be difficult to enforce
'frame-ancestor'
> > on
> > > redirects without parsing the CSP header in the browser.
> > > 
> > > I agree. Maybe that's an argument for going back to the original proposal
of
> > > parsing a subset of the policy in the browser... :(
> > 
> > What do we decide to do now? Should I add the parser again such that the CSP
> > could be parsed outside of a renderer?
> 
> Since current behavior doesn't enforce CSP on redirects, we should not add any
> new parsing code. The higher priority bit now is to ship PlzNavigate, which
> means we can ignore CSP on redirects for now.
> 
> > I know that you don't like this solution because there is some code
> duplication.
> > Two parsers to maintain... But I don't imagine an easy way to avoid it for
the
> > moment.
> 
> Let's tackle this down the road.

Hi Mike, do you have made some progress for the review?
Maybe alexmos@ could help for the review if you want?

Here is a depending patch that enforce frame-src on the browser-side.
It is useful if you want to see how CSPContext will be implemented. 
https://codereview.chromium.org/2655463006/
It almost works, there is still 2 tests that are broken because we are loading
an error page instead of loading nothing and one of the two tests is relying on
the fact that the previous frame content is still there.

[jam, 2017-02-09 17:50:37]

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

[jam, 2017-02-09 17:50:38]

The classes in content/common have constructors that take in Blink types that
are non-POD/enum. Since content/common is linked by browser and we only use
pod/enum there, the approach we solve this is to create helper methods in
content/renderer that create the content/common classes from the blink types.

Also, to ensure that we don't use WebString etc in browser/, ideally the headers
in webkit/public are split between those that contain only pod/enum and which
browser/ depends on, and others that include WebString and which only renderer/
depends on.

[arthursonzogni, 2017-02-10 12:48:29]

Patchset #12 (id:490001) has been deleted

[arthursonzogni, 2017-02-10 12:50:47]

Patchset #12 (id:510001) has been deleted

[arthursonzogni, 2017-02-10 12:53:30]

Patchset #12 (id:530001) has been deleted

[arthursonzogni, 2017-02-10 12:53:58]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-02-10 12:54:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 12:54:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 12:54:21]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[arthursonzogni, 2017-02-10 12:56:35]

On 2017/02/09 17:50:38, jam wrote:
> The classes in content/common have constructors that take in Blink types that
> are non-POD/enum. Since content/common is linked by browser and we only use
> pod/enum there, the approach we solve this is to create helper methods in
> content/renderer that create the content/common classes from the blink types.
> 
> Also, to ensure that we don't use WebString etc in browser/, ideally the
headers
> in webkit/public are split between those that contain only pod/enum and which
> browser/ depends on, and others that include WebString and which only
renderer/
> depends on.

Thanks! I moved the code that convert blink-types into content-types in the
content/renderer instead of in content/common.

[arthursonzogni, 2017-02-10 14:26:40]

Patchset #13 (id:570001) has been deleted

[arthursonzogni, 2017-02-10 14:29:35]

Patchset #13 (id:590001) has been deleted

[Mike West, 2017-02-13 14:10:51]

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source.h:17: struct CONTENT_EXPORT
CSPSource {
If your struct has methods and private members, it's probably not a struct. :)

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source_list_unittest.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_list_unittest.cc:49:
EXPECT_TRUE(source_list.Allow(&context, GURL("https://no-example.com")));
Nit: Please use `not-example.com` throughout.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_list_unittest.cc:98:
CSPSourceList source_list(true,                       // allow_self
Please add a test verifying `'self' blob:` and `'self' filesystem:`.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:13: CSPSource
source("http", "example.com", false, 8000, false, "/foo/");
Can you add a check for a source without a trailing `/`? That is, the URL
`/foo/bar` should not match the CSPSource of `/foo`.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:17:
EXPECT_TRUE(source.Allow(&context, GURL("HTTP://EXAMPLE.com:8000/foo/BAR")));
Can you add a check that `https://example.com:8000` matches
`http://example.com:8000`? (Upgrades are always allowed)

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:89:
EXPECT_FALSE(source.Allow(&context, GURL("https-so://a.com")));
This seems wrong. We should be treating `https-so` as `https` here, as we're
inside fetch, which does not care about the origin's serialization.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:100:
EXPECT_FALSE(source.Allow(&context, GURL("non-standard-scheme://a.com")));
This seems wrong too. This should match.

That said, I'm not sure we can ever cause it to happen for a navigation, as
anything on a non-standard scheme is going to be rendering in something other
than Blink.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:126:
EXPECT_TRUE(source.Allow(&context, GURL("http://.foo.bar")));
Doesn't the `.` get normalized away?

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:135:
EXPECT_FALSE(source.Allow(&context, GURL("http://.foo.bar")));
This surprises me.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:153:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com")));
Can you add `https://a.com` as well?

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:181:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com:443")));
Seems weird. Does this work in Blink today?

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:200:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com/path/to/file")));
Please add an expectation for `/path/to/file/with/subpath`

https://codereview.chromium.org/2612793002/diff/670001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3214: std::vector<CSPPolicy>
content_policies;
CSPPolicy => "Content Security Policy Policy". :(

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:166:
WebContentSecurityPolicyPolicy expose() const;
Please add a comment here regarding the directives this actually exposes in the
exported policy.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:212:
sourceExpression.isHostWildcard = (m_hostWildcard == HasWildcard);
Nit: It seems like we should either use booleans on both sides of the platform
divide, or enums on both sides. Is there a good reason to change this to enums?
Blink usually prefers more descriptive names...

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h
(right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:52:
WebVector<WebContentSecurityPolicySourceExpression> sourceList;
Nit: `sourceList` seems wrong, given the name of the struct. Perhaps
`expressionList`? Or `expressions`?

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:58: };
Since you're only exporting source-list directives, it's not clear to me that
you need the extra struct. Perhaps you could drop the source list, and expand
`WebContentSecurityPolicyDirective` to include a list of expressions?

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:60: struct
WebContentSecurityPolicyPolicy {
Nit: ContentSecurityPolicyPolicy sounds super-weird. How about dropping the last
`Policy`?

[arthursonzogni, 2017-02-14 17:07:04]

Thank Mike for the review!

I think I addressed the majority of the issues.

For reference, here is some tests I port to the renderer-side implementation. 
- https://codereview.chromium.org/2691063003/
- https://codereview.chromium.org/2694233002/
- https://codereview.chromium.org/2697853002/
- https://codereview.chromium.org/2689363003/
Concerning the test expectations that looks wrong/weird, the behavior is
identical on the renderer-side.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source.h:17: struct CONTENT_EXPORT
CSPSource {
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> If your struct has methods and private members, it's probably not a struct. :)

Yes, but I really want this struct to be a struct :)
I want to keep it simple and since it is transmitted through IPC, a struct is
preferable.
BTW, every methods are const.

What do you prefer?
1) I can move methods from private to public.
2) I can move methods out of the struct.
3) I can turn the struct into a class.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source_list_unittest.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_list_unittest.cc:49:
EXPECT_TRUE(source_list.Allow(&context, GURL("https://no-example.com")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Nit: Please use `not-example.com` throughout.

Done.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_list_unittest.cc:98:
CSPSourceList source_list(true,                       // allow_self
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Please add a test verifying `'self' blob:` and `'self' filesystem:`.

I am not sure to understand what is the purpose. Can you explain what do you
want to test?
I will add a test I think is useful, I hope it will be what you were thinking
of.

Please see:
https://codereview.chromium.org/2691063003/

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:13: CSPSource
source("http", "example.com", false, 8000, false, "/foo/");
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Can you add a check for a source without a trailing `/`? That is, the URL
> `/foo/bar` should not match the CSPSource of `/foo`.

This is done in CSPSourceTest.AllowPath, section "path to a file" and "path to a
directory"

I copied this test as-is from its blink equivalent.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:17:
EXPECT_TRUE(source.Allow(&context, GURL("HTTP://EXAMPLE.com:8000/foo/BAR")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Can you add a check that `https://example.com:8000` matches
> `http://example.com:8000`? (Upgrades are always allowed)

It is the same here, it is done in CSPSourceTest.AllowScheme section "http ->
{http, https}"

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:89:
EXPECT_FALSE(source.Allow(&context, GURL("https-so://a.com")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> This seems wrong. We should be treating `https-so` as `https` here, as we're
> inside fetch, which does not care about the origin's serialization.

Same results on the renderer-side:
https://codereview.chromium.org/2694233002/

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:100:
EXPECT_FALSE(source.Allow(&context, GURL("non-standard-scheme://a.com")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> This seems wrong too. This should match.
> 
> That said, I'm not sure we can ever cause it to happen for a navigation, as
> anything on a non-standard scheme is going to be rendering in something other
> than Blink.

Same results on the renderer-side.
https://codereview.chromium.org/2694233002/

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:126:
EXPECT_TRUE(source.Allow(&context, GURL("http://.foo.bar")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Doesn't the `.` get normalized away?

No, I tried and got:
GURL("http://foo.bar") != GURL("http://.foo.bar")

Same results on the renderer-side:
https://codereview.chromium.org/2697853002/

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:135:
EXPECT_FALSE(source.Allow(&context, GURL("http://.foo.bar")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> This surprises me.

Same result on the renderer-side:
https://codereview.chromium.org/2697853002/

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:153:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Can you add `https://a.com` as well?

Done.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:181:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com:443")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Seems weird. Does this work in Blink today?

Yes it works on blink today.
Please see: https://codereview.chromium.org/2689363003

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:200:
EXPECT_TRUE(source.Allow(&context, GURL("http://a.com/path/to/file")));
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Please add an expectation for `/path/to/file/with/subpath`

Nice catch. Done!

https://codereview.chromium.org/2612793002/diff/670001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3214: std::vector<CSPPolicy>
content_policies;
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> CSPPolicy => "Content Security Policy Policy". :(

I like thinking that Content-Security-Policy is {the concept, the feature} and a
Policy is the object.
A policy is a the set of directive, which are a set of source.

Do you really want me to rename this? CSPPolicy => CSP. I still prefer
CSPPolicy.

I saw that it was difficult in blink to name the classes.
Here is roughly what are the corresponding classes between blink and content.
ContentSecurityPolicy <=> a set of CSPPolicy ( + a CSPContext + misc )
CSPDirectiveList <=> CSPPolicy
SourceListDirective <=> CSPDirective + CSPSourceList
CSPSource <=> CSPSource.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:166:
WebContentSecurityPolicyPolicy expose() const;
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Please add a comment here regarding the directives this actually exposes in
the
> exported policy.

Yes, it is very important. Done.
I also added some comments for the classes I added.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:212:
sourceExpression.isHostWildcard = (m_hostWildcard == HasWildcard);
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Nit: It seems like we should either use booleans on both sides of the platform
> divide, or enums on both sides. Is there a good reason to change this to
enums?
> Blink usually prefers more descriptive names...

Okay, let's use an enum in blink.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h
(right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:52:
WebVector<WebContentSecurityPolicySourceExpression> sourceList;
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Nit: `sourceList` seems wrong, given the name of the struct. Perhaps
> `expressionList`? Or `expressions`?

I agree. What about sources?

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:58: };
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Since you're only exporting source-list directives, it's not clear to me that
> you need the extra struct. Perhaps you could drop the source list, and expand
> `WebContentSecurityPolicyDirective` to include a list of expressions?

I prefer to store a list of CSPDirective inside a CSPPolicy instead of storing a
list of Pair<DirectiveName, CSPSourceList>.

I don't know why I didn't used a map<DirectiveName, CSPSourceList>, maybe
because these structs are transmitted through IPC.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:60: struct
WebContentSecurityPolicyPolicy {
On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> Nit: ContentSecurityPolicyPolicy sounds super-weird. How about dropping the
last
> `Policy`?

Inside blink, a ContentSecurityPolicy doesn't represent a policy, but a set of
policies (+ a ton of other things). So maybe dropping the Policy could cause
confusion. The real CSPPolicy class in blink is the CSPDirectiveList.

I would like to abbreviate ContentSecurityPolicy into CSP. But I think it is
against blink coding style.

So, yes maybe dropping the Policy if we don't have a better idea.

[Mike West, 2017-02-15 16:18:18]

Thanks for taking another pass! More comments. :)

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source.h:17: struct CONTENT_EXPORT
CSPSource {
On 2017/02/14 at 17:07:03, arthursonzogni wrote:
> On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> > If your struct has methods and private members, it's probably not a struct.
:)
> 
> Yes, but I really want this struct to be a struct :)
> I want to keep it simple and since it is transmitted through IPC, a struct is
preferable.
> BTW, every methods are const.
> 
> What do you prefer?
> 1) I can move methods from private to public.
> 2) I can move methods out of the struct.
> 3) I can turn the struct into a class.

I'll defer to //content/common OWNERS, I suppose, but it seems very strange to
hang all of this functionality on a struct.

Did you consider refactoring the matching functions into static utility methods?
That's still ugly, but at least the struct wouldn't have magical private
functionality.

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2612793002/diff/670001/content/common/content...
content/common/content_security_policy/csp_source_unittest.cc:89:
EXPECT_FALSE(source.Allow(&context, GURL("https-so://a.com")));
On 2017/02/14 17:07:03, arthursonzogni wrote:
> On 2017/02/13 14:10:51, Mike West (sloooooow) wrote:
> > This seems wrong. We should be treating `https-so` as `https` here, as we're
> > inside fetch, which does not care about the origin's serialization.
> 
> Same results on the renderer-side:
> https://codereview.chromium.org/2694233002/

Actually, this makes sense now that I think about it. The suborigin
serialization treats the leftmost DNS label as the suborigin. So this is really
`https://com` with a suborigin of `a`, which doesn't (and shouldn't) match
`a.com`. Sorry for my confusion.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h
(right):

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:52:
WebVector<WebContentSecurityPolicySourceExpression> sourceList;
On 2017/02/14 17:07:03, arthursonzogni wrote:
> I agree. What about sources?

SGTM.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:58: };
On 2017/02/14 17:07:03, arthursonzogni wrote:
> I prefer to store a list of CSPDirective inside a CSPPolicy instead of storing
a
> list of Pair<DirectiveName, CSPSourceList>.

I think you're adding complexity for little benefit. *shrug* I won't block on
this, but I don't see much value in the indirection.

https://codereview.chromium.org/2612793002/diff/670001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:60: struct
WebContentSecurityPolicyPolicy {
On 2017/02/14 17:07:03, arthursonzogni wrote:
> Inside blink, a ContentSecurityPolicy doesn't represent a policy, but a set of
> policies (+ a ton of other things). So maybe dropping the Policy could cause
> confusion. The real CSPPolicy class in blink is the CSPDirectiveList.

Blink is serving a few masters with the naming: we wrap up all the CSP-related
functionality into a `ContentSecurityPolicy` class, which more or less maps to
what the spec now calls a "CSP List"
(https://html.spec.whatwg.org/multipage/dom.html#concept-document-csp-list). I
think that naming makes sense, given the way it's used (we call into certain
algorithms which operate on the document context).

I'd be fine with you replicating that structure in the browser, but that doesn't
seem to be what you're doing. *shrug*

> I would like to abbreviate ContentSecurityPolicy into CSP. But I think it is
> against blink coding style.

I think it made sense to spell everything out when we first added the feature,
since no one could be expected to know what CSP meant. I suspect we could change
that today without much argument.

> So, yes maybe dropping the Policy if we don't have a better idea.

Please do. Or call it a "directive list" to match Blink. Whatever works for you.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_context.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_context.h:18: // A CSPContext define
every dependencies that have to be implemented to be
I don't really understand what this means. What dependencies? Implemented by
whom?

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_directive.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_directive.h:14: // A CSPDirective is
a set of allowed sources for a given usage.
How about something like:

"""
CSPDirective contains a set of allowed sources for a given Content Security
Policy directive.

For example, the Content Security Policy `default-src img.cdn.com example.com`
would produce a CSPDirective object whose 'name'
is 'DefaultSrc', and whose 'source_list' contains two CSPSourceExpressions
representing 'img.cdn.com' and 'example.com' respectively.

https://w3c.github.io/webappsec-csp/#framework-directives
"""

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_policy.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_policy.h:22: // A CSPPolicy is a set
of |directives| that needs to be enforced.
1. "CSPPolicy". :(

2. Perhaps "... is a collection of CSPDirectives which will be enforced upon
requests."?

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_policy.h:26: //                      
    frame-src 'self';"
I'm not sure this helps much unless you explain how this turns into an object
with three CSPDirective objects in 'directives'.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_source.h:17: // A CSPSource
represents an expression that matches a set of urls.
Perhaps:

"""
CSPSource represents a single Content Security Policy source expression, which
can be evaluated against a GURL to determine whether or not a given request
should be allowed.

https://w3c.github.io/webappsec-csp/#source-expression
"""

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_source.h:51: bool Allow(CSPContext*
context,
Or perhaps this could move to `CSPContext` entirely?

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:166: // Export a
subset of the Policy. The primary goal of this method if to make
Nit: s/if/is/

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:167: // the
embedders able to enforce the directives that are not stricly internal
How about "the embedder aware of the directives that affect navigation, as the
embedder is responsible for navigational enforcement."?

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:175: // of blink.
For instance it doesn't contains 'unsafe-inline' or 'unsafe-eval'
Nit: s/that can be checked outside of blink/that affect navigation./

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/SourceListDirective.h (right):

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:56: // Export a
subset of the source list that can be checked outside of blink.
Again, I'd suggest making the link to navigational checks clear.

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:59:
WebContentSecurityPolicySourceList expose() const;
Maybe we should rename these to something like `exposeForNavigationalChecks`?

[arthursonzogni, 2017-02-16 13:30:26]

Thanks Mike!

I applied your suggestions. I also have removed the methods from the structs.
Tell me if you think it's better now.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_context.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_context.h:18: // A CSPContext define
every dependencies that have to be implemented to be
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> I don't really understand what this means. What dependencies? Implemented by
> whom?
What dependencies?
The functionality defined here that are behind a virtual method :
{
  LogToConsole,
  ReportViolation,
  SchemeShouldByPassCSP
}

Whom?
The one that want to use any class inside this directory.
In https://codereview.chromium.org/2655463006/, CSPContext_impl is one possible
implementation of CSPContext.

In:
* csp_policy_unittest
* csp_source_list_unittest
* csp_source
it is implemented in another way.

But yes, I will improve this comment.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_directive.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_directive.h:14: // A CSPDirective is
a set of allowed sources for a given usage.
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> How about something like:
> 
> """
> CSPDirective contains a set of allowed sources for a given Content Security
> Policy directive.
> 
> For example, the Content Security Policy `default-src http://img.cdn.com
example.com`
> would produce a CSPDirective object whose 'name'
> is 'DefaultSrc', and whose 'source_list' contains two CSPSourceExpressions
> representing 'img.cdn.com' and 'example.com' respectively.
> 
> https://w3c.github.io/webappsec-csp/#framework-directives
> """

I really like, thanks!

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_policy.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_policy.h:22: // A CSPPolicy is a set
of |directives| that needs to be enforced.
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> 1. "CSPPolicy". :(
Okay, it breaks my heart, but I think I will replace all CSPPolicy with CSP, or
maybe ContentSecurityPolicy.

> 2. Perhaps "... is a collection of CSPDirectives which will be enforced upon
> requests."?

I applied you suggestion.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_policy.h:26: //                      
    frame-src 'self';"
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> I'm not sure this helps much unless you explain how this turns into an object
> with three CSPDirective objects in 'directives'.

Okay, I removed it.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_source.h:17: // A CSPSource
represents an expression that matches a set of urls.
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> Perhaps:
> 
> """
> CSPSource represents a single Content Security Policy source expression, which
> can be evaluated against a GURL to determine whether or not a given request
> should be allowed.
> 
> https://w3c.github.io/webappsec-csp/#source-expression
> """

Perfect.

https://codereview.chromium.org/2612793002/diff/710001/content/common/content...
content/common/content_security_policy/csp_source.h:51: bool Allow(CSPContext*
context,
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> Or perhaps this could move to `CSPContext` entirely?

Why do you think it should be moved to CSPContext?

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:166: // Export a
subset of the Policy. The primary goal of this method if to make
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> Nit: s/if/is/

Done.

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:167: // the
embedders able to enforce the directives that are not stricly internal
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> How about "the embedder aware of the directives that affect navigation, as the
> embedder is responsible for navigational enforcement."?

Done.

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h:175: // of blink.
For instance it doesn't contains 'unsafe-inline' or 'unsafe-eval'
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> Nit: s/that can be checked outside of blink/that affect navigation./

Done.

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/SourceListDirective.h (right):

https://codereview.chromium.org/2612793002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:59:
WebContentSecurityPolicySourceList expose() const;
On 2017/02/15 16:18:18, Mike West (sloooooow) wrote:
> Maybe we should rename these to something like `exposeForNavigationalChecks`?

I agree. Done.

[arthursonzogni, 2017-02-16 13:52:06]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 13:52:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 15:02:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 15:02:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2017-02-17 01:03:20]

Some preliminary comments, haven't gone through most of the code yet.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
File content/common/content_security_policy/csp_context.cc (right):

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:11: self_scheme_(""),
nit: No need to initialize it to empty string explicitly, as that is the
implicit construction.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:12: self_source_("", "",
false, -1, false, "") {}
s/""/std::string()/, use url::PORT_UNSPECIFIED as you did below for the port
number.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:19: if
(this->SchemeShouldBypassCSP(url.scheme_piece()))
nit: Chromium code avoids using "this->".

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:37: if (origin.scheme() ==
"file") {
nit: Please use symbolic constants, kFileScheme in this case.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:60: if (self_scheme_ ==
url::kHttpScheme)
What about httpS?

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:76: return
SchemeShouldBypassCSP(self_scheme_);
This method isn't virtual, so it cannot be overriden, yet it returns a constant.
Why not have this method always return false?

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_source.h:26: struct CONTENT_EXPORT
CSPSource {
Why are these structs instead of classes?

[arthursonzogni, 2017-02-17 09:30:22]

Thanks Nasko!

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
File content/common/content_security_policy/csp_context.cc (right):

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:11: self_scheme_(""),
On 2017/02/17 01:03:15, nasko (out until Feb 27th) wrote:
> nit: No need to initialize it to empty string explicitly, as that is the
> implicit construction.

Done.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:12: self_source_("", "",
false, -1, false, "") {}
On 2017/02/17 01:03:16, nasko (out until Feb 27th) wrote:
> s/""/std::string()/, use url::PORT_UNSPECIFIED as you did below for the port
> number.

I have a default constructor that already does the job, I will use it instead.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:19: if
(this->SchemeShouldBypassCSP(url.scheme_piece()))
On 2017/02/17 01:03:16, nasko (out until Feb 27th) wrote:
> nit: Chromium code avoids using "this->".

Done.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:37: if (origin.scheme() ==
"file") {
On 2017/02/17 01:03:17, nasko (out until Feb 27th) wrote:
> nit: Please use symbolic constants, kFileScheme in this case.

Done.

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:60: if (self_scheme_ ==
url::kHttpScheme)
On 2017/02/17 01:03:17, nasko (out until Feb 27th) wrote:
> What about httpS?

I don't know what is the correct behavior, but at least, the browser-side one
and the renderer-side one are the same.

This is the renderer-side code:
bool ContentSecurityPolicy::protocolMatchesSelf(const KURL& url) const {
  if (equalIgnoringCase("http", m_selfProtocol))
    return url.protocolIsInHTTPFamily();
  return equalIgnoringCase(url.protocol(), m_selfProtocol);
}

I reported a bug and made some tests about this there:
https://crbug.com/692442
https://crrev.com/2694233002

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_context.cc:76: return
SchemeShouldBypassCSP(self_scheme_);
On 2017/02/17 01:03:16, nasko (out until Feb 27th) wrote:
> This method isn't virtual, so it cannot be overriden, yet it returns a
constant.
> Why not have this method always return false?

SchemeShouldBypassCSP is virtual :)

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2612793002/diff/770001/content/common/content...
content/common/content_security_policy/csp_source.h:26: struct CONTENT_EXPORT
CSPSource {
On 2017/02/17 01:03:17, nasko (out until Feb 27th) wrote:
> Why are these structs instead of classes? 

Why not?
ContentSecurityPolicy/CSPDirective/CSPSourceList/CSPSource are structs.
They are transmitted through IPC, I prefer having structs for this. Is it even
possible to pass private attributes to the IPC macros?
Moreover they have blink doppelganger in
third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h.

[arthursonzogni, 2017-02-20 14:20:01]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 14:20:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 15:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 15:45:59]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[arthursonzogni, 2017-02-21 08:43:32]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 08:43:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 09:53:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 09:53:08]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-02-21 10:07:31]

Sorry for the OOO delay.

LGTM. Let's land this to get out of y'all's way for PlzNavigate, but looking at
it again, I'm not really thrilled with where we've ended up. This adds a lot of
complexity and duplication for very little benefit, and I'm a bit concerned
about how it's going to be maintained in the future.

But, for the moment, it doesn't regress any tests, and will give us the correct
behavior in PlzNavigate-world once you land the integration with throttles, etc.
Good enough.

[arthursonzogni, 2017-02-21 13:03:25]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-21 13:03:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2612793002/#ps850001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-21 13:03:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-02-21 13:09:33]

The CQ bit was unchecked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-21 13:21:41]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 13:21:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-02-21 13:27:52]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org

[arthursonzogni, 2017-02-21 13:27:54]

Thanks Mike!

Yes, it is not good thing to have CSP checked on the two processes by two
different sets of classes. We will have to think about how to unify the two
implementations.

Since Nasko is OOO until the 27th, I need other reviewers.
* clamy@ Please could you review the content/ part? The main part in
content/common/content_security_policy/* was already reviewed by mkwst@ BTW.
* mkwst@ Do you still LGTM as a ipc/SECURITY_OWNERS point of view? (
content/common/frame_messages.h)

[clamy, 2017-02-21 14:10:59]

Thanks! content/ lgtm.

[Mike West, 2017-02-21 14:16:22]

IPC still LGTM.

[commit-bot: I haz the power, 2017-02-21 15:04:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 15:04:20]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[arthursonzogni, 2017-02-21 15:29:43]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-21 15:29:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2612793002/#ps870001
(title: "Add the TODO and bug ids that was forgotten.")

[commit-bot: I haz the power, 2017-02-21 15:30:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 18:58:18]

CQ is committing da patch.

Bot data: {"patchset_id": 870001, "attempt_start_ts": 1487690983736790,
"parent_rev": "0436f3c4b78022306843c1e901f0cf4c576f1506", "commit_rev":
"7c9cab4db31531feba84b8b8e6e8a8b859afbc5a"}

[commit-bot: I haz the power, 2017-02-21 18:59:05]

Description was changed from

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When no renderer are used (frame-ancestors with PDF files, ...).

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement ContentSecurityPolicy on the browser-side.

Content Security Policy is currently parsed and checked in the renderer
process inside Blink. We want to do it on the browser-side as well for
a few reasons:
- When the navigation occurs on the browser-side with PlzNavigate.
- When using OOPIF.
- When no renderer are used (frame-ancestors with PDF files, ...).

This patch adds the base classes to make this feature works. It is
self-contained and doesn't interact with anything. Only tests are using
it for the moment.

BUG=376522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2612793002
Cr-Commit-Position: refs/heads/master@{#451807}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c9cab4db31531feba84b8b8e6e8...
==========

[commit-bot: I haz the power, 2017-02-21 18:59:07]

Committed patchset #26 (id:870001) as
https://chromium.googlesource.com/chromium/src/+/7c9cab4db31531feba84b8b8e6e8...