Bug 96765 – Implement the "Content-Security-Policy" header.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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

[Mike West]

Created attachment 164151 [details]
Patch

[Mike West]

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

[Mike West]

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.

[Adam Barth]

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

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 164151 [details]
Patch

This looks great, but I agree that we should hold off landing it slightly longer.

[Mike West]

(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.

[Mike West]

(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?

[Mike West]

Created attachment 164884 [details]
Better enum names.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Mike West]

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. :)

[Adam Barth]

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?

[Adam Barth]

> 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.

[Mike West]

Created attachment 165092 [details]
Patch

[Mike West]

(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.

[Mike West]

Created attachment 170211 [details]
Rebasing onto ToT.

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 170211 [details]
Rebasing onto ToT.

Mike says the patch needs to be rebased.

[Mike West]

(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. :)

[Mike West]

Created attachment 171708 [details]
Patch for landing

[Mike West]

Comment on attachment 171708 [details]
Patch for landing

Hrm. land-safely doesn't like me.

[Mike West]

Comment on attachment 171708 [details]
Patch for landing

Bah. This is failing tests locally. Rebase was clean, but something important must have moved around. :/

[Mike West]

(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.

[WebKit Review Bot]

Comment on attachment 171708 [details]
Patch for landing

Clearing flags on attachment: 171708

Committed r133095: <http://trac.webkit.org/changeset/133095>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.