Bug 103652 – CSP 1.1: Make the CSP_NEXT flag runtime enabled.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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

[Mike West]

Created attachment 176793 [details]
Patch

[Mike West]

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]

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.

[Adam Barth]

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]

Here's a recent patch that makes a feature runtime named: https://bugs.webkit.org/show_bug.cgi?id=103669

[Adam Barth]

s/named/enabled/

[Mike West]

Created attachment 176873 [details]
Patch

[Mike West]

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]

Looks great.

[WebKit Review Bot]

Comment on attachment 176873 [details]
Patch

Clearing flags on attachment: 176873

Committed r136305: <http://trac.webkit.org/changeset/136305>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.