Bug 96765 - Implement the "Content-Security-Policy" header.
: Implement the "Content-Security-Policy" header.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
:
: 53572 97190
  Show dependency treegraph
 
Reported: 2012-09-14 06:27 PST by
Modified: 2012-10-31 16:41 PST (History)


Attachments
Patch (95.20 KB, patch)
2012-09-14 07:36 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Better enum names. (99.17 KB, patch)
2012-09-20 03:36 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (98.84 KB, patch)
2012-09-21 02:33 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Rebasing onto ToT. (98.91 KB, patch)
2012-10-23 13:01 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (98.93 KB, patch)
2012-10-31 13:10 PST, Mike West
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 2012-09-14 06:27:52 PST
Once CSP 1.0 is published as a Candidate Recommendation (soon!), we should implement the canonical header.

As Adam suggested on public-webappsec[1], we plan to implement CSP 1.0 behind the "Content-Security-Policy" header and experiment with CSP 1.1 behind the "X-WebKit-CSP" header (but still gated on the CSP_NEXT flag for the moment).

[1]: http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0007.html
------- Comment #1 From 2012-09-14 07:36:32 PST -------
Created an attachment (id=164151) [details]
Patch
------- Comment #2 From 2012-09-14 07:40:33 PST -------
Hey Adam!

Based on our conversation this morning, I don't think we want to land this until both the 1.0 spec has moved to CR, and webkit-dev has been notified. It's worth taking a look at now if you have a few minutes, but I guess we're looking at late next week at the earliest?

I can't really imagine that any port would really, really, really want to opt-out of the canonical header, given that none of the functionality is behind a flag at this point, but it seems like a good idea to give them a heads up.

Once you're happy with this, I'll rebase the paths change on top of it. It would be good to land those as closely together as possible if we'd like to ensure that Content-Security-Policy supports paths out of the box.

Thanks!

-mike
------- Comment #3 From 2012-09-14 07:45:10 PST -------
(From update of attachment 164151 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=164151&action=review

> Source/WebCore/dom/Document.cpp:3077
> +        contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicy::PrefixedEnforcePolicy);

I went back and forth between adding new items to the enum, adding a whole new type to pipe through, and scrapping the enum in favor of a bitmask (e.g. 'ContentSecurityPolicy::Prefixed & ContentSecurityPolicy::EnforcePolicy'). The enum was simplest, and since I can't think of anything we'd really be adding to it, I'm not terribly worried about additional complexity creeping in.

Still, WDYT? I'm happy to change this implementation if there's a better route.

> Source/WebCore/page/ContentSecurityPolicy.cpp:-795
> -    directives->m_header = header;

Moved to CSPDirectiveList::parse.

> Source/WebCore/page/ContentSecurityPolicy.cpp:-799
> -        directives->m_reportOnly = true;

Moved to CSPDirectiveList::CSPDirectiveList.
------- Comment #4 From 2012-09-14 10:35:43 PST -------
(From update of attachment 164151 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=164151&action=review

>> Source/WebCore/dom/Document.cpp:3077
>> +        contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicy::PrefixedEnforcePolicy);
> 
> I went back and forth between adding new items to the enum, adding a whole new type to pipe through, and scrapping the enum in favor of a bitmask (e.g. 'ContentSecurityPolicy::Prefixed & ContentSecurityPolicy::EnforcePolicy'). The enum was simplest, and since I can't think of anything we'd really be adding to it, I'm not terribly worried about additional complexity creeping in.
> 
> Still, WDYT? I'm happy to change this implementation if there's a better route.

We can also just add a third parameter to the function.  EnableAllDirectives / EnableStableDirectives

> Source/WebCore/page/ContentSecurityPolicy.cpp:708
> -    ContentSecurityPolicy::HeaderType headerType() const { return m_reportOnly ? ContentSecurityPolicy::ReportOnly : ContentSecurityPolicy::EnforcePolicy; }
> +    ContentSecurityPolicy::HeaderType headerType() const { return m_headerType; }

Oh, I see.  Adding another parameter would require re-plumbing all that worker code.  I think what you've done is fine, but I might try to make the enum names a bit more semantic:

EnforceAllDirectives / EnforceStableDirectives / ReportAllDirectives / ReportStableDirectives
------- Comment #5 From 2012-09-14 10:36:20 PST -------
We should also consider putting X-WebKit-CSP behind ENABLE(CSP_NEXT).  We probably don't want to do that immediately.  It's better to give folks a chance to move over to the new name.
------- Comment #6 From 2012-09-14 10:36:47 PST -------
(From update of attachment 164151 [details])
This looks great, but I agree that we should hold off landing it slightly longer.
------- Comment #7 From 2012-09-20 03:18:09 PST -------
(In reply to comment #5)
> We should also consider putting X-WebKit-CSP behind ENABLE(CSP_NEXT).  We probably don't want to do that immediately.  It's better to give folks a chance to move over to the new name.

I agree. Filed https://bugs.webkit.org/show_bug.cgi?id=97190 to think about what we need to do here.
------- Comment #8 From 2012-09-20 03:32:47 PST -------
(In reply to comment #4)
> > Source/WebCore/page/ContentSecurityPolicy.cpp:708
> > -    ContentSecurityPolicy::HeaderType headerType() const { return m_reportOnly ? ContentSecurityPolicy::ReportOnly : ContentSecurityPolicy::EnforcePolicy; }
> > +    ContentSecurityPolicy::HeaderType headerType() const { return m_headerType; }
> 
> Oh, I see.  Adding another parameter would require re-plumbing all that worker code.  I think what you've done is fine, but I might try to make the enum names a bit more semantic:
> 
> EnforceAllDirectives / EnforceStableDirectives / ReportAllDirectives / ReportStableDirectives

I like it. Uploading that patch in a bit.

Will you drop a note to webkit-dev when the spec moves to CR?
------- Comment #9 From 2012-09-20 03:36:02 PST -------
Created an attachment (id=164884) [details]
Better enum names.
------- Comment #10 From 2012-09-20 03:37:52 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #11 From 2012-09-20 03:39:30 PST -------
Uploaded.

This required changing some enums in Chromium's WebContentSecurityPolicyType. It doesn't appear to me that we use any of the enum values inside of Chromium (we just pass them through the worker code), but I'd appreciate you verifying that, Adam. :)
------- Comment #12 From 2012-09-20 11:08:42 PST -------
(From update of attachment 164884 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=164884&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:732
> -    explicit CSPDirectiveList(ContentSecurityPolicy*);
> +    explicit CSPDirectiveList(ContentSecurityPolicy*, ContentSecurityPolicy::HeaderType);

"explicit" no longer needed

> Source/WebCore/page/ContentSecurityPolicy.cpp:1233
> -    else if (equalIgnoringCase(name, formAction))
> +    else if (equalIgnoringCase(name, formAction) && m_experimental)

I would check m_experimental first since it's faster.

> Source/WebKit/chromium/src/AssertMatchingEnums.cpp:4
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions are
> - * met:
> - *
> - *     * Redistributions of source code must retain the above copyright
> - * notice, this list of conditions and the following disclaimer.
> - *     * Redistributions in binary form must reproduce the above
> +m

License corruption?
------- Comment #13 From 2012-09-20 11:09:09 PST -------
> This required changing some enums in Chromium's WebContentSecurityPolicyType. It doesn't appear to me that we use any of the enum values inside of Chromium (we just pass them through the worker code), but I'd appreciate you verifying that, Adam. :)

Yeah, it looks like we just pass it around.
------- Comment #14 From 2012-09-21 02:33:59 PST -------
Created an attachment (id=165092) [details]
Patch
------- Comment #15 From 2012-09-21 04:24:54 PST -------
(In reply to comment #14)
> Created an attachment (id=165092) [details] [details]
> Patch

Applied your feedback, Adam, thanks! Whenever CR rolls around, I think this should be ready to land.
------- Comment #16 From 2012-10-23 13:01:40 PST -------
Created an attachment (id=170211) [details]
Rebasing onto ToT.
------- Comment #17 From 2012-10-31 13:05:19 PST -------
(From update of attachment 170211 [details])
CSP 1.0 has been approved by the W3C.  We're just in the publication blackout for TPAC.
------- Comment #18 From 2012-10-31 13:08:24 PST -------
(From update of attachment 170211 [details])
Mike says the patch needs to be rebased.
------- Comment #19 From 2012-10-31 13:09:31 PST -------
(In reply to comment #18)
> (From update of attachment 170211 [details] [details])
> Mike says the patch needs to be rebased.

Well, last time I rebased it was the 23rd. It might apply cleanly. I'm checking that right now. :)
------- Comment #20 From 2012-10-31 13:10:42 PST -------
Created an attachment (id=171708) [details]
Patch for landing
------- Comment #21 From 2012-10-31 13:11:11 PST -------
(From update of attachment 171708 [details])
Hrm. land-safely doesn't like me.
------- Comment #22 From 2012-10-31 14:58:45 PST -------
(From update of attachment 171708 [details])
Bah. This is failing tests locally. Rebase was clean, but something important must have moved around. :/
------- Comment #23 From 2012-10-31 15:15:13 PST -------
(In reply to comment #22)
> (From update of attachment 171708 [details] [details])
> Bah. This is failing tests locally. Rebase was clean, but something important must have moved around. :/

*facepalm* Nothing to see here. Running tests on the chromium port after compiling the mac port is not a recipe for success.
------- Comment #24 From 2012-10-31 16:41:49 PST -------
(From update of attachment 171708 [details])
Clearing flags on attachment: 171708

Committed r133095: <http://trac.webkit.org/changeset/133095>
------- Comment #25 From 2012-10-31 16:41:54 PST -------
All reviewed patches have been landed.  Closing bug.