Replicate feature policy headers to remote frames

third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp

Index: third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp b/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp
index 9e1171988125dc51966ea8542e0e71df8278d26d..dbe7bbc54fd3132a11a9a3d0e70b4916f870dbeb 100644
--- a/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp
+++ b/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp
@@ -438,6 +438,25 @@ void WebRemoteFrameImpl::setReplicatedName(const WebString& name,
   frame()->tree().setPrecalculatedName(name, uniqueName);
 }
 
+void WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader(

[alexmos, 2016/11/09 01:16:55]

Just to double-check: will this work in the case where we have
A-embed-B-embed-C, A has a feature policy header, B does *not* have the header,
and we're initializing the frame tree in process C?  

We'll first create the root frame's proxy, which will call this and create the
right feature policy.  Next, we'll create the middle child's proxy, and still
call this, but with the default empty header value (since we never called
DidSetFeaturePolicyHeader).  Will this do the right thing with the empty header
value, i.e., inherit the parent's policy?  (Lastly, the bottom child's
LocalFrame will initialize its policy using the middle child's policy.)

It might be worth adding another test to cover this case (remote-remote-local).

[iclelland, 2016/11/09 18:07:59]

Definitely, I'm planning on adding a number of other tests, to cover all of the
cases. That one should work -- a frame with no header should still call
DidSetFeaturePolicyHeader -- the FrameLoader always calls
client()->didSetFeaturePolicyHeader, even if the header is empty. (Maybe it
should be renamed, or dropped in that case)

If I fix that (so that the header is not replicated over IPC if not sent) then
the FrameReplicationState could still be initialized to a correct empty string,
and this method would still be called by RenderFrameProxy::SetReplicatedState,
and would create the correct policy, inheriting everything from the parent.

I'll add an explicit test for that case as well. (And its various permutations)

[alexmos, 2016/11/10 17:50:28]

That sounds great, I missed that DidSetFeaturePolicyHeader was done even for the
empty header.

I see that you've changed it now (which is great, as it benefits the common
case). One issue for that though is that we need to deal properly with the same
frame navigating from a document with a non-empty header to a document with an
empty header.  In that case, we need to reset the header's value in
FrameTreeNode's replication state.  This should've worked when sending the empty
header IPC, but I'm not so sure it does now that we drop it.  We could probably
clear the replication state's feature_policy_header in
NavigatorImpl::DidNavigate, where we update the other replicated properties,
like origin or insecure_request_policy.  Probably another thing worth having a
test for eventually.

[iclelland, 2016/11/16 20:21:32]

Thanks for catching that -- I've fixed it now, and added tests for it. (For both
same and for cross-origin navigations)

+    const WebString& headerValue) const {
+  if (RuntimeEnabledFeatures::featurePolicyEnabled()) {
+    SecurityContext* childSecurityContext = frame()->securityContext();
+    FeaturePolicy* parentFeaturePolicy = nullptr;
+    if (parent()) {
+      Frame* parentFrame = frame()->client()->parent();
+      SecurityContext* parentSecurityContext = parentFrame->securityContext();
+      parentFeaturePolicy = parentSecurityContext->getFeaturePolicy();
+    }
+    std::unique_ptr<FeaturePolicy> childFeaturePolicy(
+        FeaturePolicy::createFromParentPolicy(
+            parentFeaturePolicy, childSecurityContext->getSecurityOrigin()));

[Mike West, 2016/11/08 12:12:23]

It surprised me that you did all this work both here and in FrameLoader. It
seems like you should be able to replicate over a complete policy (including all
the parent policies). That is, the LocalFrame should populate itself in whatever
way it needs to, and then replicate that complete state.

(Note that CSP might not be the best example in it's current implementation:
it's fairly verbose, sending one IPC per header (in
`ContentSecurityPolicy::reportAccumulatedHeaders`)).

[iclelland, 2016/11/08 14:25:45]

I could certainly factor this out into a method on SecurityContext that takes a
parent frame (or the parent SecurityContext) and use it in both places.

It seemed natural to do it this way, using the JSON header itself, rather than
re-serializing the complete policy. I'll give the other way some serious
thought, though, to make sure it can handle all of the cases it needs to.

[alexmos, 2016/11/09 01:16:55]

I'm slightly in favor of the current approach, actually (factor this out into a
helper and use both here and in FrameLoader).  Re-serializing the complete
policy would avoid the duplicate computation here, but it might also mean
replicating more data overall.  E.g., if you have a root frame with the FP
header and 50 different subframes with no header, with this approach you'd only
replicate the root frame's header, but with the other approach each of the
subframes might have a non-null final policy that would also need to be sent
around.

One other thing to think about is whether the enable/disable iframe attributes
would be easier to implement with the current approach or with replicating the
complete policy. (I'm guessing you will work on those in followup CLs?)

+    Vector<String> messages;

[raymes, 2016/11/07 23:10:25]

I think we can ignore messages here because appropriate errors will be displayed
when the header is parsed in its own frame. Is that true? Should we add a
comment to that effect?

[iclelland, 2016/11/08 03:13:41]

Agreed; that's why we don't do anything with the messages var. It's passed by
reference to setHeaderPolicy, though, so I needed to pass *something*. I can
rename it to _, unless it makes more sense to provide a 1-arg version of
setHeaderPolicy just for this case.

[Mike West, 2016/11/08 12:12:23]

1. ContentSecurityPolicy makes itself responsible for sending messages to the
console when it's bound to an execution context. Since it never binds to an
execution context in a remote frame, no messages are duplicated. Perhaps that
kind of model could be useful here? Look at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/csp...
if you're curious.

2. Aesthetically, I think it would be nicer to have a messageless version of
`setHeaderPolicy`, but it's fine either way. If you keep it here, perhaps you
could rename the variable to something like |unused_messages|?

[iclelland, 2016/11/08 14:25:45]

I like that approach, thanks!
I'll make a unary method, and call a private method with nullptr from it (if
it's still necessary, given (1))

[iclelland, 2016/11/09 18:07:59]

I've switched it to taking an optional Vector<String>*, and not logging if the
passed-in pointer is null. I don't think there's any benefit in passing a
reference at this point. The bind-to-execution-context pattern is interesting,
but I'm not sure its worth the additional complexity here. It looks like it is
better at solving the potentially-delayed-side-effect issue than just making the
logging optional.

+    childFeaturePolicy->setHeaderPolicy(headerValue, messages);
+    childSecurityContext->setFeaturePolicy(std::move(childFeaturePolicy));
+  }
+}
+
 void WebRemoteFrameImpl::addReplicatedContentSecurityPolicyHeader(
     const WebString& headerValue,
     WebContentSecurityPolicyType type,