Bug 154345 - CSP: Implement frame-ancestors directive
Summary: CSP: Implement frame-ancestors directive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2016-02-17 11:12 PST by Daniel Bates
Modified: 2016-05-27 12:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch and Layout Tests (146.44 KB, patch)
2016-03-10 15:07 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and Layout Tests (146.47 KB, patch)
2016-03-10 15:27 PST, Daniel Bates
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>