Bug 96765 - Implement the "Content-Security-Policy" header.
: Implement the "Content-Security-Policy" header.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mike West
: WebExposed
Depends on:
Blocks: 53572 97190
  Show dependency treegraph
 
Reported: 2012-09-14 06:27 PDT by Mike West
Modified: 2012-10-31 16:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (95.20 KB, patch)
2012-09-14 07:36 PDT, Mike West
no flags Details | Formatted Diff | Diff
Better enum names. (99.17 KB, patch)
2012-09-20 03:36 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (98.84 KB, patch)
2012-09-21 02:33 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebasing onto ToT. (98.91 KB, patch)
2012-10-23 13:01 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (98.93 KB, patch)
2012-10-31 13:10 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-09-14 06:27:52 PDT
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 Mike West 2012-09-14 07:36:32 PDT
Created attachment 164151 [details]
Patch
Comment 2 Mike West 2012-09-14 07:40:33 PDT
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 Mike West 2012-09-14 07:45:10 PDT
Comment on attachment 164151 [details]
Patch

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 Adam Barth 2012-09-14 10:35:43 PDT
Comment on attachment 164151 [details]
Patch

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 Adam Barth 2012-09-14 10:36:20 PDT
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 Adam Barth 2012-09-14 10:36:47 PDT
Comment on attachment 164151 [details]
Patch

This looks great, but I agree that we should hold off landing it slightly longer.
Comment 7 Mike West 2012-09-20 03:18:09 PDT
(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 Mike West 2012-09-20 03:32:47 PDT
(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 Mike West 2012-09-20 03:36:02 PDT
Created attachment 164884 [details]
Better enum names.
Comment 10 WebKit Review Bot 2012-09-20 03:37:52 PDT
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 Mike West 2012-09-20 03:39:30 PDT
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 Adam Barth 2012-09-20 11:08:42 PDT
Comment on attachment 164884 [details]
Better enum names.

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 Adam Barth 2012-09-20 11:09:09 PDT
> 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 Mike West 2012-09-21 02:33:59 PDT
Created attachment 165092 [details]
Patch
Comment 15 Mike West 2012-09-21 04:24:54 PDT
(In reply to comment #14)
> Created an attachment (id=165092) [details]
> Patch

Applied your feedback, Adam, thanks! Whenever CR rolls around, I think this should be ready to land.
Comment 16 Mike West 2012-10-23 13:01:40 PDT
Created attachment 170211 [details]
Rebasing onto ToT.
Comment 17 Adam Barth 2012-10-31 13:05:19 PDT
Comment on attachment 170211 [details]
Rebasing onto ToT.

CSP 1.0 has been approved by the W3C.  We're just in the publication blackout for TPAC.
Comment 18 Adam Barth 2012-10-31 13:08:24 PDT
Comment on attachment 170211 [details]
Rebasing onto ToT.

Mike says the patch needs to be rebased.
Comment 19 Mike West 2012-10-31 13:09:31 PDT
(In reply to comment #18)
> (From update of attachment 170211 [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 Mike West 2012-10-31 13:10:42 PDT
Created attachment 171708 [details]
Patch for landing
Comment 21 Mike West 2012-10-31 13:11:11 PDT
Comment on attachment 171708 [details]
Patch for landing

Hrm. land-safely doesn't like me.
Comment 22 Mike West 2012-10-31 14:58:45 PDT
Comment on attachment 171708 [details]
Patch for landing

Bah. This is failing tests locally. Rebase was clean, but something important must have moved around. :/
Comment 23 Mike West 2012-10-31 15:15:13 PDT
(In reply to comment #22)
> (From update of attachment 171708 [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 WebKit Review Bot 2012-10-31 16:41:49 PDT
Comment on attachment 171708 [details]
Patch for landing

Clearing flags on attachment: 171708

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