Bug 162335 – Refactor ContentSecurityPolicy::allow* methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

These methods take a bool to override allow decision.
It might be better to not call this method if the override boolean is true.

[youenn fablet]

Created attachment 289445 [details]
Patch

[youenn fablet]

This is a follow-up of bug 162144 refactoring.

[Darin Adler]

Comment on attachment 289445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289445&action=review

> Source/WebCore/ChangeLog:12
> +        This patch updates the callers of allow* methods to check for the parameter before making the call.

While that change makes sense and is cleaner and more efficient, it does make it clear that policy is spread around in all the different call sites in the code; because of that we may need “why” comments at each call site explaining why this is a good policy.

> Source/WebCore/Modules/fetch/FetchLoader.cpp:90
> +    if (!context.shouldBypassMainWorldContentSecurityPolicy() && !context.contentSecurityPolicy()->allowConnectToSource(fetchRequest.url())) {

Pretty ugly to call context.contentSecurityPolicy()-> both here and in the code above. Might be nicer to put a reference into local variable with a single word name instead.

> Source/WebCore/Modules/websockets/WebSocket.cpp:250
> +    if (!scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy() && !scriptExecutionContext()->contentSecurityPolicy()->allowConnectToSource(m_url)) {

Pretty ugly to call scriptExecutionContext()-> multiple times. Might be nicer to put a reference into local variable with a single word name instead.

> Source/WebCore/html/HTMLMediaElement.cpp:2007
> +    if (!isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url)) {

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

One way to do that would be to make a different function with a name that says something about bypassing content security policy that is an inline that just calls isInUserAgentShadowTree. Then the comment about the reason could go in that one function rather than at every call site.

> Source/WebCore/html/HTMLMediaElement.cpp:6368
> +        if (!trackElement.isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url))

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:778
> +    if (this->isInUserAgentShadowTree())

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

Also, after refactoring, we no longer need "this->" here. It was here before because of the name conflict with a local variable and now there is no conflict.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:793
>      String declaredMimeType = document().isPluginDocument() && document().ownerElement() ?

Type of this doesn’t need to be String, causing a bit of reference count churn. Since this is an attribute value it could just be const AtomicString&, and I think it can can be written as auto&.

> Source/WebCore/html/HTMLTrackElement.cpp:219
> +    if (!isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url)) {

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:225
> +    if (!isAllowedByContentSecurityPolicy(request.url(), redirectResponse.isNull() ? ContentSecurityPolicy::RedirectResponseReceived::No : ContentSecurityPolicy::RedirectResponseReceived::Yes)) {

See my comment in another bug about our enum vs. bool strategy for ideas on how to keep this good going forward.

> Source/WebCore/loader/PolicyChecker.cpp:58
> +    if (ownerElement->isInUserAgentShadowTree())

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

> Source/WebCore/loader/SubframeLoader.cpp:251
> +        if (!element.isInUserAgentShadowTree()

Unclear why isInUserAgentShadowTree is a reason to bypass the security policy; already was unclear but it’s even more obvious now. Needs a why comment.

> Source/WebCore/workers/WorkerGlobalScope.cpp:234
>          bool shouldBypassMainWorldContentSecurityPolicy = scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy();
> -        if (!scriptExecutionContext()->contentSecurityPolicy()->allowScriptFromSource(url, shouldBypassMainWorldContentSecurityPolicy)) {
> +        if (!shouldBypassMainWorldContentSecurityPolicy && !scriptExecutionContext()->contentSecurityPolicy()->allowScriptFromSource(url)) {

Pretty ugly to call scriptExecutionContext()-> multiple times. Might be nicer to put a reference into local variable with a single word name instead.

> Source/WebCore/xml/XMLHttpRequest.cpp:504
> +    if (!scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy() && !scriptExecutionContext()->contentSecurityPolicy()->allowConnectToSource(m_url)) {

Pretty ugly to call scriptExecutionContext()-> multiple times. Might be nicer to put a reference into local variable with a single word name instead.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 289445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289445&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        This patch updates the callers of allow* methods to check for the parameter before making the call.
> 
> While that change makes sense and is cleaner and more efficient, it does
> make it clear that policy is spread around in all the different call sites
> in the code; because of that we may need “why” comments at each call site
> explaining why this is a good policy.

There is https://fetch.spec.whatwg.org/#concept-request-destination that relates CSP with clients.
Basically a fetch request has a destination, which relates to a specific CSP.
Fetch users (img e.g.) specify that destination in the HTTML spec and do not ned to talk about CSP.

If we were to introduce this destination, a single place (CachedResourceLoader) could do those checks.
We could do that progressively although some code paths are pretty long currently.

Also, currently we fail early which might be expected by some code paths.

[youenn fablet]

Created attachment 289534 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

> > Source/WebCore/Modules/fetch/FetchLoader.cpp:90
> > +    if (!context.shouldBypassMainWorldContentSecurityPolicy() && !context.contentSecurityPolicy()->allowConnectToSource(fetchRequest.url())) {
> 
> Pretty ugly to call context.contentSecurityPolicy()-> both here and in the
> code above. Might be nicer to put a reference into local variable with a
> single word name instead.

Done

> > Source/WebCore/Modules/websockets/WebSocket.cpp:250
> > +    if (!scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy() && !scriptExecutionContext()->contentSecurityPolicy()->allowConnectToSource(m_url)) {
> 
> Pretty ugly to call scriptExecutionContext()-> multiple times. Might be
> nicer to put a reference into local variable with a single word name instead.

Done

> > Source/WebCore/html/HTMLMediaElement.cpp:2007
> > +    if (!isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url)) {
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.

My understanding is that a user agent shadow tree (or any built-in element) should behave the same no matter the embedding document CSP.

> One way to do that would be to make a different function with a name that
> says something about bypassing content security policy that is an inline
> that just calls isInUserAgentShadowTree. Then the comment about the reason
> could go in that one function rather than at every call site.

Done

> > Source/WebCore/html/HTMLMediaElement.cpp:6368
> > +        if (!trackElement.isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url))
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.

Done

> > Source/WebCore/html/HTMLPlugInImageElement.cpp:778
> > +    if (this->isInUserAgentShadowTree())
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.

Done

> Also, after refactoring, we no longer need "this->" here. It was here before
> because of the name conflict with a local variable and now there is no
> conflict.

Done

> > Source/WebCore/html/HTMLPlugInImageElement.cpp:793
> >      String declaredMimeType = document().isPluginDocument() && document().ownerElement() ?
> 
> Type of this doesn’t need to be String, causing a bit of reference count
> churn. Since this is an attribute value it could just be const
> AtomicString&, and I think it can can be written as auto&.

Done

> > Source/WebCore/html/HTMLTrackElement.cpp:219
> > +    if (!isInUserAgentShadowTree() && !document().contentSecurityPolicy()->allowMediaFromSource(url)) {
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:225
> > +    if (!isAllowedByContentSecurityPolicy(request.url(), redirectResponse.isNull() ? ContentSecurityPolicy::RedirectResponseReceived::No : ContentSecurityPolicy::RedirectResponseReceived::Yes)) {
> 
> See my comment in another bug about our enum vs. bool strategy for ideas on
> how to keep this good going forward.

OK

> > Source/WebCore/loader/PolicyChecker.cpp:58
> > +    if (ownerElement->isInUserAgentShadowTree())
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.

Done

> > Source/WebCore/loader/SubframeLoader.cpp:251
> > +        if (!element.isInUserAgentShadowTree()
> 
> Unclear why isInUserAgentShadowTree is a reason to bypass the security
> policy; already was unclear but it’s even more obvious now. Needs a why
> comment.

Done

> > Source/WebCore/workers/WorkerGlobalScope.cpp:234
> >          bool shouldBypassMainWorldContentSecurityPolicy = scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy();
> > -        if (!scriptExecutionContext()->contentSecurityPolicy()->allowScriptFromSource(url, shouldBypassMainWorldContentSecurityPolicy)) {
> > +        if (!shouldBypassMainWorldContentSecurityPolicy && !scriptExecutionContext()->contentSecurityPolicy()->allowScriptFromSource(url)) {
> 
> Pretty ugly to call scriptExecutionContext()-> multiple times. Might be
> nicer to put a reference into local variable with a single word name instead.

Done

> > Source/WebCore/xml/XMLHttpRequest.cpp:504
> > +    if (!scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy() && !scriptExecutionContext()->contentSecurityPolicy()->allowConnectToSource(m_url)) {
> 
> Pretty ugly to call scriptExecutionContext()-> multiple times. Might be
> nicer to put a reference into local variable with a single word name instead.

Done

[WebKit Commit Bot]

Comment on attachment 289534 [details]
Patch for landing

Clearing flags on attachment: 289534

Committed r206254: <http://trac.webkit.org/changeset/206254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 289445 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=289445&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        This patch updates the callers of allow* methods to check for the parameter before making the call.
> > 
> > While that change makes sense and is cleaner and more efficient, it does
> > make it clear that policy is spread around in all the different call sites
> > in the code; because of that we may need “why” comments at each call site
> > explaining why this is a good policy.
> 
> There is https://fetch.spec.whatwg.org/#concept-request-destination that
> relates CSP with clients.
> Basically a fetch request has a destination, which relates to a specific CSP.
> Fetch users (img e.g.) specify that destination in the HTTML spec and do not
> ned to talk about CSP.
> 
> If we were to introduce this destination, a single place
> (CachedResourceLoader) could do those checks.

Hmm, some policies require to be done at specific places in the code: https://w3c.github.io/webappsec-csp/2/#directive-connect-src