Bug 215444 – Add API to allow CSP by-passing

• Component: WebKit API
• Status: NEW

[Philippe Normand]

For testing purposes it's sometimes needed to disable CSP checks. WebKit already has an API to disable CORS checks. Would a similar API be welcome upstream? On Mac this could be a WebViewConfiguration boolean option. In WPE/GTK it could be a WebContext bool construct-only property.

Any thoughts are welcome.

[Radar WebKit Bug Importer]

<rdar://problem/67467275>

[Philippe Normand]

Created attachment 407284 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

WIP API to disable CSP checks
    
This is exposed in the PageConfiguration currently, but could be moved elsewhere.
I am aware the glib API test could be reworked a bit as well.

[Alex Christensen]

Comment on attachment 407284 [details]
Patch

Ideally this would not use a singleton in the web process, but rather a property on the page.
What use case is this trying to fulfill?  Do people want to do local testing but are running into errors they would not run into in production, or are they trying to make insecure apps?
ChangeLog entries are missing.

[Philippe Normand]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 407284 [details]
> Patch
> 
> Ideally this would not use a singleton in the web process, but rather a
> property on the page.

I used a similar approach as for the CORS disabler.

> What use case is this trying to fulfill?  Do people want to do local testing
> but are running into errors they would not run into in production, or are
> they trying to make insecure apps?

This is useful for testing indeed. CSP can get in the way eg, for instance when a continuous integration bot needs to crawl a generated website.

> ChangeLog entries are missing.

I will add them :)

[Philippe Normand]

Created attachment 409042 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Sam Weinig]

Comment on attachment 409042 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        A new CSP disabler singleton is introduced, using a similar pattern as for CORS disabling.
> +        The disabler is configured on the WebProcess through IPC.

Please do not use singletons for settings in new code. This is incompatible with using WebKit as a framework (as you might want different pages to have different settings). There is an undergoing effort to remove uses of these singletons from the code base (see https://bugs.webkit.org/show_bug.cgi?id=216182 and https://bugs.webkit.org/show_bug.cgi?id=215962).

A model to follow is to add new settings via Settings.yaml, which can be accessed from the Page/Frame/Document, etc.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:131
> +@property (nonatomic, setter=_setContentSecurityPolicyByPassEnabled:) BOOL _contentSecurityPolicyByPassEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Unless someone is actually asking for this SPI, I don't see any reason to add it to the cocoa API.

[Philippe Normand]

Hi Sam,

Thanks for the review!
This new Cocoa API would be used by Playwright's MiniBrowser they are using for automated browsing as part of their framework.

[Sam Weinig]

(In reply to Philippe Normand from comment #10)
> Hi Sam,
> 
> Thanks for the review!
> This new Cocoa API would be used by Playwright's MiniBrowser they are using
> for automated browsing as part of their framework.

If it is needed for a third party (I assume that is what Playwright is, I am not familiar with it), it needs to be API, not SPI (e.g. not in. *Private.h header and not starting with an underscore) and needs to go through Apple's formal API review process.

[Philippe Normand]

Created attachment 411111 [details]
Patch

[Philippe Normand]

Created attachment 411117 [details]
Patch

[Philippe Normand]

Created attachment 411119 [details]
Patch

[Sam Weinig]

Comment on attachment 411119 [details]
Patch

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

> Source/WebCore/page/Settings.yaml:442
> +contentSecurityPolicyByPassEnabled:

This is not needed if you remove the “webcoreBinding: none” in the WebPreferences yaml file.

[Philippe Normand]

Created attachment 411123 [details]
Patch

[Sam Weinig]

Comment on attachment 411123 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6504
> +    if (page())
> +        return page()->settings().contentSecurityPolicyByPassEnabled();
> +
> +    return false;

This can just be m_settings->contentSecurityPolicyByPassEnabled(); Document has its own reference to Settings, though honestly, it's probably better to just remove this, and have the caller get the document's settings instead.

> Source/WebCore/loader/DocumentLoader.cpp:2257
> +    return m_frame->document()->shouldByPassCSPValidation();

This can just do m_frame->settings().shouldByPassCSPValidation(). What ensures m_frame is valid here?

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:51
> +#include "RuntimeApplicationChecks.h"

This seems unrelated.

> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:357
> +// Defaults to false.
> +WK_EXPORT bool WKPreferencesGetContentSecurityPolicyByPassEnabled(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetContentSecurityPolicyByPassEnabled(WKPreferencesRef preferencesRef, bool enabled);

I don't think there is any need to expose this in the C-SPI.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:164
> +@property (nonatomic, setter=_setContentSecurityPolicyByPassEnabled:) BOOL _contentSecurityPolicyByPassEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I don't think there is any need to expose this as cocoa SPI.

[Darin Adler]

Comment on attachment 411123 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferences.yaml:416
> +ContentSecurityPolicyByPassEnabled:

"bypass" is one word, not two words "by pass", so this should say "Bypass" not "ByPass"

[Philippe Normand]

Comment on attachment 411123 [details]
Patch

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

>> Source/WTF/Scripts/Preferences/WebPreferences.yaml:416
>> +ContentSecurityPolicyByPassEnabled:
> 
> "bypass" is one word, not two words "by pass", so this should say "Bypass" not "ByPass"

OK, I'll rename this then. Thanks Darin.

>> Source/WebCore/dom/Document.cpp:6504
>> +    return false;
> 
> This can just be m_settings->contentSecurityPolicyByPassEnabled(); Document has its own reference to Settings, though honestly, it's probably better to just remove this, and have the caller get the document's settings instead.

Good point, I'll remove this.

>> Source/WebCore/loader/DocumentLoader.cpp:2257
>> +    return m_frame->document()->shouldByPassCSPValidation();
> 
> This can just do m_frame->settings().shouldByPassCSPValidation(). What ensures m_frame is valid here?

I will had a test on m_frame validity.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:51
>> +#include "RuntimeApplicationChecks.h"
> 
> This seems unrelated.

I had to add this for a previous version of the patch, but indeed it doesn't seem needed anymore.

>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:357
>> +WK_EXPORT void WKPreferencesSetContentSecurityPolicyByPassEnabled(WKPreferencesRef preferencesRef, bool enabled);
> 
> I don't think there is any need to expose this in the C-SPI.

How would a windows app set this preference then?

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:164
>> +@property (nonatomic, setter=_setContentSecurityPolicyByPassEnabled:) BOOL _contentSecurityPolicyByPassEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I don't think there is any need to expose this as cocoa SPI.

So this would become a public property in WKPreferences instead? I'll update the patch but I would welcome some guidance if the new approach is incorrect again. I'm not very familiar on how to handle public API for the Apple ports.

[Philippe Normand]

Created attachment 411211 [details]
Patch

[Philippe Normand]

Created attachment 411213 [details]
Patch

[Philippe Normand]

I will check the layout test failures, clearly related to this patch :)

[Sam Weinig]

(In reply to Philippe Normand from comment #19)
> Comment on attachment 411123 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411123&action=review
> 
> >> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:357
> >> +WK_EXPORT void WKPreferencesSetContentSecurityPolicyByPassEnabled(WKPreferencesRef preferencesRef, bool enabled);
> > 
> > I don't think there is any need to expose this in the C-SPI.
> 
> How would a windows app set this preference then?

I didn't realize any port actually used this API. I was under the impression it was only used by legacy Apple content and the shared test infrastrucure. Which port is that uses it?

[Darin Adler]

I don’t understand the rationale behind making this API. If it’s for testing, then we don’t want it as API so apps don’t accidentally configure WebKit in an insecure mode. We want a special testing interface.

[Philippe Normand]

Comment on attachment 411123 [details]
Patch

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

>>>> Source/WebKit/UIProcess/API/C/WKPreferencesRef.h:357
>>>> +WK_EXPORT void WKPreferencesSetContentSecurityPolicyByPassEnabled(WKPreferencesRef preferencesRef, bool enabled);
>>> 
>>> I don't think there is any need to expose this in the C-SPI.
>> 
>> How would a windows app set this preference then?
> 
> I didn't realize any port actually used this API. I was under the impression it was only used by legacy Apple content and the shared test infrastrucure. Which port is that uses it?

WinCairo

[Philippe Normand]

(In reply to Darin Adler from comment #24)
> I don’t understand the rationale behind making this API. If it’s for
> testing, then we don’t want it as API so apps don’t accidentally configure
> WebKit in an insecure mode. We want a special testing interface.

What do you mean by special testing interface?

As a WebKit app, the Playwright browser needs to optionally disable CSP checks. A preference suits this purpose, and it is disabled by default. I'm not sure what else we can do. Do you mean to use a new "testing" namespace for the preference?

[Philippe Normand]

Created attachment 411310 [details]
Patch