Created attachment 164151 [details] Patch

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

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 on attachment 164151 [details] Patch This looks great, but I agree that we should hold off landing it slightly longer.

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

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

Created attachment 164884 [details] Better enum names.

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.

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

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

Created attachment 165092 [details] Patch

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

Created attachment 170211 [details] Rebasing onto ToT.

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 on attachment 170211 [details] Rebasing onto ToT. Mike says the patch needs to be rebased.

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

Created attachment 171708 [details] Patch for landing

Comment on attachment 171708 [details] Patch for landing Hrm. land-safely doesn't like me.

Comment on attachment 171708 [details] Patch for landing Bah. This is failing tests locally. Rebase was clean, but something important must have moved around. :/

(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 on attachment 171708 [details] Patch for landing Clearing flags on attachment: 171708 Committed r133095: <http://trac.webkit.org/changeset/133095>

All reviewed patches have been landed. Closing bug.