[webkit-reviews] review denied: [Bug 53685] Add CSP to Frame and add a stub method that checks the response for CSPs : [Attachment 81064] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 53685: Add CSP to Frame and add a stub method that checks the response for
CSPs
https://bugs.webkit.org/show_bug.cgi?id=53685

Attachment 81064: Patch
https://bugs.webkit.org/attachment.cgi?id=81064&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81064&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:38
> -ContentSecurityPolicy::ContentSecurityPolicy()
> +ContentSecurityPolicy::ContentSecurityPolicy(Frame* frame)
> +    : m_frame(frame)
> +{
> +}

The policy should be on the Document because the policy is scoped to a given
document.  The Frame survives navigation between documents, but the policy
doesn't.

> Source/WebCore/page/ContentSecurityPolicy.cpp:49
> +bool ContentSecurityPolicy::isEnabled() const
>  {
> +    DEFINE_STATIC_LOCAL(String, CSPHeader, ("X-WebKit-CSP"));
> +
> +    Frame* frame = m_frame;
> +    if (frame->document()->url() == blankURL())
> +	   frame = m_frame->tree()->parent();
> +
> +    return
!frame->loader()->documentLoader()->response().httpHeaderField(CSPHeader).isEmp
ty();
>  }

It looks like you copied this logic from XSSAuditor, but that logic was
somewhat specific to that case (especially the blankURL bit).  This design
won't scale well when we have to do more complex parsing of the policy header. 
Instead, we should have WebCore call into this object when the header is
available, so that it can parse the header once and remember what it said.  You
can look at how this works for X-Frame-Options.