Bug 154345

Summary: CSP: Implement frame-ancestors directive
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, bfulgham, cdumez, commit-queue, japhet, mkwst, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=94836
Attachments:
Description Flags
Patch and Layout Tests
none
Patch and Layout Tests bfulgham: review+

Description Daniel Bates 2016-02-17 11:12:27 PST
We should implement the Content Security Policy frame-ancestors directive to allow a web developer to whitelist the resources that are allowed to be embedded in a web page. In an HTML document such embedding may be via the use of a frame, iframe, object, embed, or applet element. See <https://w3c.github.io/webappsec-csp/2/#directive-frame-ancestors> for more details.
Comment 1 Radar WebKit Bug Importer 2016-02-17 11:13:10 PST
<rdar://problem/24702161>
Comment 2 Daniel Bates 2016-03-10 15:07:56 PST
Created attachment 273630 [details]
Patch and Layout Tests
Comment 3 Daniel Bates 2016-03-10 15:27:24 PST
Created attachment 273636 [details]
Patch and Layout Tests
Comment 4 Brent Fulgham 2016-03-10 15:55:55 PST
Comment on attachment 273636 [details]
Patch and Layout Tests

View in context: https://bugs.webkit.org/attachment.cgi?id=273636&action=review

r=me.

> Source/WebCore/loader/FrameLoader.cpp:691
> +        m_frame.document()->contentSecurityPolicy()->didReceiveHeaders(m_documentLoader->response(), ContentSecurityPolicy::ReportParsingErrors::No);

So we are using an implicit cast here? I'm not sure that is an improvement.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:165
> +    // We can never have both a script execution context and a frame.

Seems like we should ASSERT on this invariant, perhaps in places where we set one of these values. That is not needed for the current patch, but it's something to consider in the future.
Comment 5 Daniel Bates 2016-03-10 16:41:13 PST
(In reply to comment #4)
> > Source/WebCore/loader/FrameLoader.cpp:691
> > +        m_frame.document()->contentSecurityPolicy()->didReceiveHeaders(m_documentLoader->response(), ContentSecurityPolicy::ReportParsingErrors::No);
> 
> So we are using an implicit cast here? I'm not sure that is an improvement.
> 

Will revert. I will also mark the constructor of ContentSecurityPolicyResponseHeaders explicit and update all other callers.

I take it you feel that it is makes the code more understandable to see the explicit construction of a ContentSecurityPolicyResponseHeaders object from a ResourceResponse object.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:165
> > +    // We can never have both a script execution context and a frame.
> 
> Seems like we should ASSERT on this invariant, perhaps in places where we
> set one of these values.

No need to use a run-time assert. This invariant is enforced by the public interface of ContentSecurityPolicy. It is not possible to create a ContentSecurityPolicy object that has both a script execution context and a frame because you can only instantiate one or the other via two different constructors and ContentSecurityPolicy does not expose a setter for either ContentSecurityPolicy::m_scriptExecutionContext or ContentSecurityPolicy::m_frame.
Comment 6 Daniel Bates 2016-03-10 16:45:07 PST
Committed r197972: <http://trac.webkit.org/changeset/197972>