RESOLVED FIXED 103652
CSP 1.1: Make the CSP_NEXT flag runtime enabled.
https://bugs.webkit.org/show_bug.cgi?id=103652
Summary CSP 1.1: Make the CSP_NEXT flag runtime enabled.
Mike West
Reported 2012-11-29 11:16:04 PST
Adam's pointed me to http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebRuntimeFeatures.cpp, which looks like a good place to start digging. :)
Attachments
Patch (16.14 KB, patch)
2012-11-29 12:45 PST, Mike West
no flags
Patch (10.70 KB, patch)
2012-11-29 18:04 PST, Mike West
no flags
Mike West
Comment 1 2012-11-29 12:45:20 PST
Mike West
Comment 2 2012-11-29 12:46:39 PST
Cobbled together from reading other folks' patches, but I think this looks right. Let's see what the bots make of it. :) WDYT, Adam?
WebKit Review Bot
Comment 3 2012-11-29 12:49:56 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.
Adam Barth
Comment 4 2012-11-29 15:36:20 PST
Comment on attachment 176793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176793&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:1721 > +#if ENABLE(CSP_NEXT) > + if (m_scriptExecutionContext->isDocument()) { > + Document* document = static_cast<Document*>(m_scriptExecutionContext); > + return document->settings() && document->settings()->securityPolicyEnabled(); > + } > +#endif Rather than using Settings, we should just call securityPolicyEnabled() from RuntimeEnabledFeatures.h. I know it's kind of confusing to have two different mechanisms for enabling features. We use Settings if we want to vary the feature on a per-Page basis. We us RuntimeEnabledFeatures.h when we want to control the feature globally. For features that have DOM APIs, we need to control them globally, which is why we use RuntimeEnabledFeatures. > Source/WebCore/page/Settings.h:249 > +#if ENABLE(CSP_NEXT) > + void setSecurityPolicyEnabled(bool enabled) { m_securityPolicyEnabled = enabled; } > + bool securityPolicyEnabled() const { return m_securityPolicyEnabled; } > +#else > + void setSecurityPolicyEnabled(bool) { } > + bool securityPolicyEnabled() const { return false; } > +#endif This isn't needed. (If it were needed, you could have put it in Settings.in to get this code autogenerated.) > Source/WebKit/chromium/src/WebSettingsImpl.h:138 > + virtual void setSecurityPolicyEnabled(bool); I would pick a more specific name. This API makes it sound like we're disabling WebKit's security policy. Perhaps setExperimentalContentSecurityPolicyFeaturesEnabled() ? Maybe that's too verbose...
Adam Barth
Comment 5 2012-11-29 15:39:14 PST
Here's a recent patch that makes a feature runtime named: https://bugs.webkit.org/show_bug.cgi?id=103669
Adam Barth
Comment 6 2012-11-29 15:39:25 PST
s/named/enabled/
Mike West
Comment 7 2012-11-29 18:04:21 PST
Mike West
Comment 8 2012-11-29 18:06:38 PST
Ok, thanks for taking the time to explain. The RuntimeEnabledFeatures/Settings distinction makes sense, and in this case it ends up being less code! Hooray! (In reply to comment #4) > > Source/WebKit/chromium/src/WebSettingsImpl.h:138 > > + virtual void setSecurityPolicyEnabled(bool); > > I would pick a more specific name. This API makes it sound like we're disabling WebKit's security policy. Perhaps setExperimentalContentSecurityPolicyFeaturesEnabled() ? Maybe that's too verbose... Eh. We've got more than 80 columns here, we might as well use them. :)
Adam Barth
Comment 9 2012-11-30 13:34:21 PST
Looks great.
WebKit Review Bot
Comment 10 2012-11-30 22:54:17 PST
Comment on attachment 176873 [details] Patch Clearing flags on attachment: 176873 Committed r136305: <http://trac.webkit.org/changeset/136305>
WebKit Review Bot
Comment 11 2012-11-30 22:54:21 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.