Propagating csp attribute changes on frameOwnerPropertiesChanged()

Adding csp to frameOwnerPropertiesChanged

If the iframe is remote and the csp attribute gets updated,
we need to have access to the newest value when sending
Embedding-CSP header and setting browsing context's requiredCSP value.

BUG=647588
Committed: https://crrev.com/e4100ce09bf324e0fb2612ae83c7892f0870c3fe
Cr-Commit-Position: refs/heads/master@{#423468}

[amalika, 2016-09-28 08:41:16]

amalika@google.com changed reviewers:
+ mkwst@chromium.org

[amalika, 2016-09-28 08:44:55]

Description was changed from

==========
Adding csp getter

BUG=647588
==========

to

==========
Adding csp() getter on frame elements

BUG=647588
==========

[dcheng, 2016-09-28 08:52:37]

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

[dcheng, 2016-09-28 08:52:37]

Drive-by, but please make sure the CL description is more... descriptive. Not
just 'what the CL does' but why would be helpful.

Also +site-isolation-reviews, since this might involve more state replication?

[Mike West, 2016-09-28 11:24:03]

On 2016/09/28 at 08:52:37, dcheng wrote:
> Drive-by, but please make sure the CL description is more... descriptive. Not
just 'what the CL does' but why would be helpful.

Indeed! This was just a strawman for Malika and I to chat about; I'm sure she'll
be more verbose when we have something in the right shape to start thinking
about landing.

> Also +site-isolation-reviews, since this might involve more state replication?

Yeah, we certainly planned on looping y'all in. :)

[amalika, 2016-09-28 12:55:42]

Description was changed from

==========
Adding csp() getter on frame elements

BUG=647588
==========

to

==========
Adding csp() getter on frame elements.

If the iframe is remote and the csp attribute gets updated, we need to have
access to the newest value when sending Embedding-CSP header and setting
browsing context's requriedCSP value. 

BUG=647588
==========

[amalika, 2016-09-29 08:12:46]

Description was changed from

==========
Adding csp() getter on frame elements.

If the iframe is remote and the csp attribute gets updated, we need to have
access to the newest value when sending Embedding-CSP header and setting
browsing context's requriedCSP value. 

BUG=647588
==========

to

==========
Adding csp to frameOwnerPropertiesChanged

If the iframe is remote and the csp attribute gets updated, 
we need to have access to the newest value when sending 
Embedding-CSP header and setting browsing context's requiredCSP value. 

BUG=647588
==========

[Mike West, 2016-09-29 08:58:51]

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

[Mike West, 2016-09-29 08:58:53]

This is looking good, Malika! I've left a few comments. And let's loop in
alexmos@, who should have opinions about the replication.

https://codereview.chromium.org/2378643002/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2378643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:556:
switches::kEnableExperimentalWebPlatformFeatures);
Hrm. This might be an argument for pushing the CSP bits out of the
`FrameOwnerPropertiesPropagationMargin` test into their own test (similar to
what's been done for `SitePerProcessHighDPIBrowserTest`). Let's ask the site
isolation folks for their opinion.

https://codereview.chromium.org/2378643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3094: int current_margin_height
= 50;
Why did the margins change?

https://codereview.chromium.org/2378643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3096: // Before each navigation,
we change the marginwidth, marginheight and csp
Nit: Oxford comma (e.g. "x, y, and z")

https://codereview.chromium.org/2378643002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3104: // Change marginwidth,
marginheight, and csp before navigating.
Nit: Move this before the assignments above.

https://codereview.chromium.org/2378643002/diff/60001/content/common/frame_ow...
File content/common/frame_owner_properties.cc (right):

https://codereview.chromium.org/2378643002/diff/60001/content/common/frame_ow...
content/common/frame_owner_properties.cc:14: csp("") {}
I think the default initializer would be fine here.

https://codereview.chromium.org/2378643002/diff/60001/content/common/frame_ow...
content/common/frame_owner_properties.cc:52: csp.compare(other.csp) == 0 &&
`==` should be fine here.

https://codereview.chromium.org/2378643002/diff/60001/content/common/frame_ow...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2378643002/diff/60001/content/common/frame_ow...
content/common/frame_owner_properties.h:37: std::string csp;
`base::string16` would avoid some conversions (we've overridden `=` for
WebString => string16 and vice-versa).

https://codereview.chromium.org/2378643002/diff/60001/content/test/data/frame...
File content/test/data/frame_owner_properties_margin_and_csp.html (right):

https://codereview.chromium.org/2378643002/diff/60001/content/test/data/frame...
content/test/data/frame_owner_properties_margin_and_csp.html:7: csp="object-src
none"
Nit: none => 'none'

https://codereview.chromium.org/2378643002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2378643002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:165: // allowfullscreen or csp.  This
is used when this frame's parent is in
Nit: Oxford comma. :)

https://codereview.chromium.org/2378643002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/2378643002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:35: , csp(WebString())
Default constructor is fine.

[amalika, 2016-09-29 09:56:35]

Separating CSP test, adding EmbedderCSPEnforcement command line switch for the
test, addressing comments

[Mike West, 2016-09-29 11:50:14]

Thanks for taking another pass! If alexmos@, et al. are happy with the
structure, I think we're pretty close.

https://codereview.chromium.org/2378643002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2378643002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3125:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
If you're going to split this out, I'd suggest moving it into a separate test
class (see `SitePerProcessHighDPIBrowserTest` for an example) so that the
enabled feature only applies to this test, and doesn't effect the others.

https://codereview.chromium.org/2378643002/diff/80001/content/common/frame_ow...
File content/common/frame_owner_properties.cc (right):

https://codereview.chromium.org/2378643002/diff/80001/content/common/frame_ow...
content/common/frame_owner_properties.cc:51:
std::equal(delegated_permissions.begin(), delegated_permissions.end(),
Is this what `git cl format` gave you?

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
File content/test/data/frame_owner_properties_margin.html (left):

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
content/test/data/frame_owner_properties_margin.html:1: <!DOCTYPE html>
If you move this file, doesn't the existing test start failing, as it relies on
`frame_owner_properties_margin.html` which no longer exists?

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
File content/test/data/frame_owner_properties_margin_and_csp.html (right):

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
content/test/data/frame_owner_properties_margin_and_csp.html:7: csp="object-src
'none'"
If you move this file, doesn't the existing test start failing, as it relies on
`frame_owner_properties_margin.html` which no longer exists?

https://codereview.chromium.org/2378643002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/2378643002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:8: #include
"../platform/WebString.h"
Nit: Replace the leading `../` with `public/`. We use absolute links for
includes.

[amalika, 2016-09-29 12:56:46]

https://codereview.chromium.org/2378643002/diff/80001/content/common/frame_ow...
File content/common/frame_owner_properties.cc (right):

https://codereview.chromium.org/2378643002/diff/80001/content/common/frame_ow...
content/common/frame_owner_properties.cc:51:
std::equal(delegated_permissions.begin(), delegated_permissions.end(),
On 2016/09/29 at 11:50:14, Mike West wrote:
> Is this what `git cl format` gave you?

Yep! I checked again and that's what it seems to be doing...

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
File content/test/data/frame_owner_properties_margin.html (left):

https://codereview.chromium.org/2378643002/diff/80001/content/test/data/frame...
content/test/data/frame_owner_properties_margin.html:1: <!DOCTYPE html>
On 2016/09/29 at 11:50:14, Mike West wrote:
> If you move this file, doesn't the existing test start failing, as it relies
on `frame_owner_properties_margin.html` which no longer exists?

Oops! I meant for both tests to use frame_owner_properties_margin_and_csp.html
I will fix the original test

https://codereview.chromium.org/2378643002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/2378643002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:8: #include
"../platform/WebString.h"
On 2016/09/29 at 11:50:14, Mike West wrote:
> Nit: Replace the leading `../` with `public/`. We use absolute links for
includes.

Ahh. I just followed the examples in third-party/ which use this style. Will fix
it!

[alexmos, 2016-09-30 06:20:42]

A few comments and nits, but overall I think this makes sense, given that we're
still in pre-PlzNavigate and pre-CSP-enforcement-in-browser-process world. :)  I
skimmed the proposal for this - very interesting!

On a high level, I think we need to comment on the distinction between
replicating this attribute and replicating actual CSP headers in effect for a
particular frame (the latter is tracked in
FrameReplicationState::accumulated_csp_headers).  It seems the two are
orthogonal.  If I understand the proposal correctly, the replication here will
allow us to issue a request in an OOPIF with the correct Embedding-CSP header. 
If the response comes back with a matching Content-Security-Policy header, the
other CSP header replication will kick in once the subframe commits, so that the
browser and any proxies know which headers actually took effect.  I pointed to
one possible place for a comment to explain this below.

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:578: // the
EmbedderCSPEnforcement becomes stable
nit: wrap comments at 80 chars (this is fewer than that).

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:579:
command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
Instead of doing this here, let's define a new class which extends
SitePerProcessBrowserTest and adds this flag.  Then put the new test in that
class.  There are some existing examples of this here, such as 
SitePerProcessHighDPIBrowserTest or SitePerProcessIgnoreCertErrorsBrowserTest.

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3123: // elements ropagates to
child frames correctly.
nits: 
s/ropagates/propagates/
Please fix the wrapping in the comment (and similar ones below).

Also, how about including the link to a bug here?

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3127: "a.com",
"/frame_owner_properties_margin_and_csp.html"));
Instead of reusing the file, it might be cleaner to just create a new file,
frame_owner_properties_csp.html, with what you want.  Margin and csp seems like
a weird/unrelated combination. :)  (btw, currently, I don't think the
FrameOwnerPropertiesPropagationMargin test is updated to the new filename).

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3128: NavigateToURL(shell(),
main_url);
nit: EXPECT_TRUE

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3159:
std::vector<base::string16> csp_values = {
why not just std::string and avoid all conversions?

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3175:
base::UTF16ToUTF8(current_csp).c_str())));
nit: could just use csp_values[i] directly (here and below) without the temp
variable.

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3179: EXPECT_EQ(current_csp,
child->frame_owner_properties().csp);
Is it possible to observe that the passed CSP attribute took effect?  The point
of all those navigations is to check all possible ways the out-of-process child
frame can be created and ensure that the CSP value made it to the
RemoteFrameOwner in all cases.  It'd be nice to verify, for example, that you
can't include disallowed scripts/images/etc, according to the test policy for
each URL.  Or, alternatively, and probably easier, you could check that
csp_values[i] made it into
child->current_replication_state()->accumulated_csp_headers (meaning that the
frame committed with the right CSP header from the response, which would only
happen if the request pulled in the correct Embedding-CSP header).  Though I
guess that also requires the test server to support your new header?  If you
aren't that far along in your implementation, maybe add a TODO to validate this
later?

https://codereview.chromium.org/2378643002/diff/100001/content/common/frame_o...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2378643002/diff/100001/content/common/frame_o...
content/common/frame_owner_properties.h:37: base::string16 csp;
Can this just be a std::string, like other replicated properties in
FrameReplicationState?

https://codereview.chromium.org/2378643002/diff/100001/content/common/frame_o...
content/common/frame_owner_properties.h:37: base::string16 csp;
Also, how about adding a comment on top of this, just to explain that this is an
experimental attribute, which is different from replicated CSP headers kept in
FrameReplicationState.  And explain the difference: the former is used by a
parent frame to define a CSP for a subframe, whereas the latter tracks the
(cumulative) CSP headers currently in effect for a frame.  It's probably also
usefulещ link to the bug here as well.

https://codereview.chromium.org/2378643002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2378643002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:165: // allowfullscreen, or csp.  This
is used when this frame's parent is in
Feel free to not update this, given that it already doesn't include
delegated_permissions.  (This is just meant to provide a couple of examples.) 
Mentioning it here might also need an explanation of how this is an attribute
which is different from regular CSP headers, which are replicated in another
way, etc.

[amalika, 2016-09-30 07:47:53]

Patchset #7 (id:120001) has been deleted

[amalika, 2016-09-30 07:48:51]

Thank you for the suggestions! 
Updated adding comments, string16->string, separating the test

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2378643002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3179: EXPECT_EQ(current_csp,
child->frame_owner_properties().csp);
On 2016/09/30 at 06:20:42, alexmos wrote:
> Is it possible to observe that the passed CSP attribute took effect?  The
point of all those navigations is to check all possible ways the out-of-process
child frame can be created and ensure that the CSP value made it to the
RemoteFrameOwner in all cases.  It'd be nice to verify, for example, that you
can't include disallowed scripts/images/etc, according to the test policy for
each URL.  Or, alternatively, and probably easier, you could check that
csp_values[i] made it into
child->current_replication_state()->accumulated_csp_headers (meaning that the
frame committed with the right CSP header from the response, which would only
happen if the request pulled in the correct Embedding-CSP header).  Though I
guess that also requires the test server to support your new header?  If you
aren't that far along in your implementation, maybe add a TODO to validate this
later?

The deserialization and the algorithms have not been implemented yet as right
now we are doing the project in small patches but the intention was to indeed
expand this test. I will add a todo.

[amalika, 2016-10-04 13:27:02]

Update after addressing comments

[Mike West, 2016-10-04 14:55:54]

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

[Mike West, 2016-10-04 14:55:55]

Thanks for taking another pass at this: //third_party/WebKit LGTM, as does the
addition of the property to IPC. Please wait for Alex to sign off on the rest.

https://codereview.chromium.org/2378643002/diff/160001/content/common/frame_o...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2378643002/diff/160001/content/common/frame_o...
content/common/frame_owner_properties.h:41: std::string csp;
Nit: To clarify further, let's rename this to something like `required_csp`, and
add a link to https://www.w3.org/TR/csp-embedded-enforcement/#required-csp, both
here and in the subsequent patch on `FrameLoader`.

[alexmos, 2016-10-04 23:50:21]

Thanks! LGTM once Mike's nit to rename 'csp' to 'required_csp' is addressed,
plus two other nits below.

https://codereview.chromium.org/2378643002/diff/140001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2378643002/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3135: // Verify that "csp"
property on frame elements propogates to child frames
nit: s/propogates/propagates/

https://codereview.chromium.org/2378643002/diff/140001/content/common/frame_o...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2378643002/diff/140001/content/common/frame_o...
content/common/frame_owner_properties.h:41: std::string csp;
Thanks for adding the comment.  Perhaps also add blank lines before comment and
after |csp|, to separate it out more from other attributes?

https://codereview.chromium.org/2378643002/diff/160001/content/common/frame_o...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2378643002/diff/160001/content/common/frame_o...
content/common/frame_owner_properties.h:41: std::string csp;
On 2016/10/04 14:55:55, Mike West wrote:
> Nit: To clarify further, let's rename this to something like `required_csp`,
and
> add a link to https://www.w3.org/TR/csp-embedded-enforcement/#required-csp,
both
> here and in the subsequent patch on `FrameLoader`.

+1, I think this is a good idea.

[amalika, 2016-10-05 08:25:51]

Patchset #9 (id:180001) has been deleted

[amalika, 2016-10-05 08:26:26]

The CQ bit was checked by amalika@google.com

[amalika, 2016-10-05 08:26:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2378643002/#ps200001
(title: "Renaming to required_csp")

[commit-bot: I haz the power, 2016-10-05 08:26:36]

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, 2016-10-05 08:35:48]

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

[commit-bot: I haz the power, 2016-10-05 08:35:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[amalika, 2016-10-05 12:24:11]

amalika@google.com changed reviewers:
+ jochen@chromium.org

[amalika, 2016-10-05 12:24:11]

Missing owners for content/browser and content/common
Adding Jochen!

[amalika, 2016-10-05 12:24:44]

The CQ bit was checked by amalika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 12:24:53]

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

[jochen (gone - plz use gerrit), 2016-10-05 12:26:04]

lgtm

[commit-bot: I haz the power, 2016-10-05 15:51:34]

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

[commit-bot: I haz the power, 2016-10-05 15:51:35]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-10-06 07:21:47]

The CQ bit was checked by amalika@google.com

[amalika, 2016-10-06 07:21:47]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2378643002/#ps220001
(title: "rebasing on master")

[commit-bot: I haz the power, 2016-10-06 07:22:04]

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, 2016-10-06 07:28:20]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-10-06 07:29:38]

Description was changed from

==========
Adding csp to frameOwnerPropertiesChanged

If the iframe is remote and the csp attribute gets updated, 
we need to have access to the newest value when sending 
Embedding-CSP header and setting browsing context's requiredCSP value. 

BUG=647588
==========

to

==========
Adding csp to frameOwnerPropertiesChanged

If the iframe is remote and the csp attribute gets updated,
we need to have access to the newest value when sending
Embedding-CSP header and setting browsing context's requiredCSP value.

BUG=647588

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

[commit-bot: I haz the power, 2016-10-06 07:29:39]

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