WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(10.70 KB, patch)
2012-11-29 18:04 PST
,
Mike West
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mike West
Comment 1
2012-11-29 12:45:20 PST
Created
attachment 176793
[details]
Patch
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
Created
attachment 176873
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug