Bug 53685 - Add CSP to Document and pass the X-WebKit-CSP header from the MainResourceLoader.
: Add CSP to Document and pass the X-WebKit-CSP header from the MainResourceLoa...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 53572
  Show dependency treegraph
 
Reported: 2011-02-03 08:33 PST by
Modified: 2011-02-06 03:13 PST (History)


Attachments
Patch (4.44 KB, patch)
2011-02-03 08:34 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2011-02-04 13:48 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2011-02-05 01:45 PST, jochen@chromium.org
abarth: review+
jochen: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2011-02-05 02:27 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-02-03 08:33:51 PST
Add CSP to Frame and add a stub method that checks the response for CSPs
------- Comment #1 From 2011-02-03 08:34:59 PST -------
Created an attachment (id=81064) [details]
Patch
------- Comment #2 From 2011-02-03 08:36:02 PST -------
Adam, not sure this is the right place to add the policy?
------- Comment #3 From 2011-02-03 09:32:35 PST -------
(From update of attachment 81064 [details])
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).isEmpty();
>  }

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.
------- Comment #4 From 2011-02-03 11:42:51 PST -------
Attachment 81064 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7693384
------- Comment #5 From 2011-02-03 13:41:59 PST -------
@jochen: We should also mark these bugs as blocking the meta bug so folks can follow our progress via the meta bug.
------- Comment #6 From 2011-02-04 13:48:54 PST -------
Created an attachment (id=81279) [details]
Patch
------- Comment #7 From 2011-02-04 14:39:44 PST -------
(From update of attachment 81279 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=81279&action=review

Mostly organizational comments.  We want to put as much of the "guts" inside ContentSecurityPolicy as possible.  The outside code should be as oblivious as possible.

> Source/WebCore/dom/Document.cpp:4622
> +void Document::parseContentSecurityPolicyHeader(const String& header)
> +{
> +    // FIXME: Actually parse the header.
> +    m_contentSecurityPolicy.setEnabled(!header.isEmpty());
> +}

parse should be a method on ContentSecurityPolicy.  It's the one that's going to do the heavy lifting.

> Source/WebCore/dom/Document.h:1093
> +    void parseContentSecurityPolicyHeader(const String&);

We should just have an accessor for m_contentSecurityPolicy

> Source/WebCore/loader/MainResourceLoader.cpp:362
> +        m_frame->loader()->didReceiveContentSecurityPolicyHeader(content);

I'd just change this to call through to the document->contentSecurityPolicy()->didReceiveHeader(content), which can then call parse internally.
------- Comment #8 From 2011-02-04 16:53:38 PST -------
Attachment 81279 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7704048
------- Comment #9 From 2011-02-05 01:45:22 PST -------
Created an attachment (id=81349) [details]
Patch
------- Comment #10 From 2011-02-05 01:52:03 PST -------
(From update of attachment 81349 [details])
Thanks!
------- Comment #11 From 2011-02-05 02:27:57 PST -------
Created an attachment (id=81350) [details]
Patch
------- Comment #12 From 2011-02-05 02:30:06 PST -------
Fixing the header attributes so it compiles on mac.
------- Comment #13 From 2011-02-05 03:52:33 PST -------
(From update of attachment 81350 [details])
Clearing flags on attachment: 81350

Committed r77742: <http://trac.webkit.org/changeset/77742>
------- Comment #14 From 2011-02-05 04:05:35 PST -------
Attachment 81349 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7702330
------- Comment #15 From 2011-02-05 05:25:32 PST -------
The commit-queue encountered the following flaky tests while processing attachment 81350 [details]:

http/tests/websocket/tests/handshake-error.html bug 53851 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.