Bug 96497 – [GTK] Add API to get/set the security policy of a given URI scheme to WebKit2 GTK+

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

This is useful to set the security policy for custom URI schemes, for example, to be treated as local or secure. See bug #95549 for the WebKit1 API.

[Carlos Garcia Campos]

Created attachment 163591 [details]
Patch

Same API than WebKit1.

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 163591 [details]
Patch

Attachment 163591 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13825600

[Carlos Garcia Campos]

Created attachment 163597 [details]
Try to fix the mac build

[Martin Robinson]

Comment on attachment 163597 [details]
Try to fix the mac build

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:648
> +    // We keep the WebCore::SchemeRegistry of the UI process in sync with the
> +    // web process one, so that we can return the WebKitSecurityPolicy for
> +    // a given URI scheme synchronously without blocking.
> +    if (policy & WEBKIT_SECURITY_POLICY_LOCAL) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsLocal(urlScheme);
> +        webContext->registerURLSchemeAsLocal(urlScheme);
> +    }
> +    if (policy & WEBKIT_SECURITY_POLICY_NO_ACCESS_TO_OTHER_SCHEME) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsNoAccess(urlScheme);
> +        webContext->registerURLSchemeAsNoAccess(urlScheme);
> +    }
> +    if (policy & WEBKIT_SECURITY_POLICY_DISPLAY_ISOLATED) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsDisplayIsolated(urlScheme);
> +        webContext->registerURLSchemeAsDisplayIsolated(urlScheme);
> +    }
> +    if (policy & WEBKIT_SECURITY_POLICY_SECURE) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsSecure(urlScheme);
> +        webContext->registerURLSchemeAsSecure(urlScheme);
> +    }
> +    if (policy & WEBKIT_SECURITY_POLICY_CORS_ENABLED) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsCORSEnabled(urlScheme);
> +        webContext->registerURLSchemeAsCORSEnabled(urlScheme);
> +    }
> +    if (policy & WEBKIT_SECURITY_POLICY_EMPTY_DOCUMENT) {
> +        WebCore::SchemeRegistry::registerURLSchemeAsEmptyDocument(urlScheme);
> +        webContext->registerURLSchemeAsEmptyDocument(urlScheme);
> +    }
> +}
> +

One thing I notice here is that if you say, activate WEBKIT_SECURITY_POLICY_LOCAL, and then call this method again with the same scheme without WEBKIT_SECURITY_POLICY_LOCAL removeURLSchemeRegisteredAsLocal is never called. It seems that you can never undo any of the actions you take. On the other hand there doesn't seem to be a way in WebCore to undo registration for all other types of schemes. Perhaps that means this API doesn't map to the one in WebCore. Perhaps you need to add a method in WebCore to remove all registrations for a scheme and call that first here.

[Carlos Garcia Campos]

This is not designed to remove uri schemes

[Martin Robinson]

(In reply to comment #6)
> This is not designed to remove uri schemes

If that's not a usecase you want to support it makes sense to match the WebCore API, I think. Only expose the register* methods. That way it's unambiguous. I can see this being very confusing if you try to change the security policy of a scheme (calling webkit_web_context_set_security_policy_for_uri_scheme more than once for the same scheme).

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > This is not designed to remove uri schemes
> 
> If that's not a usecase you want to support it makes sense to match the WebCore API, I think. Only expose the register* methods. That way it's unambiguous. I can see this being very confusing if you try to change the security policy of a scheme (calling webkit_web_context_set_security_policy_for_uri_scheme more than once for the same scheme).

You are right, it can be confusing, maybe we could add a WebKitSecurityPolicy object with methods to register uri schemes:

webkit_secutiry_policy_register_uri_scheme_as_local()
webkit_secutiry_policy_register_uri_scheme_as_secure()
....

And also methods to query:

webkit_secutiry_policy_is_uri_scheme_local()
webkit_secutiry_policy_is_uri_scheme_secure()
....

And method to get the security policy object (or boxed type) from the context

WebKitSecutiryPolicy* webkit_web_context_get_secutory_policy().

I used the flags to reduce the api, but I agree it can be confusing, we should do the same in wk1.

[Martin Robinson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This is not designed to remove uri schemes
> > 
> > If that's not a usecase you want to support it makes sense to match the WebCore API, I think. Only expose the register* methods. That way it's unambiguous. I can see this being very confusing if you try to change the security policy of a scheme (calling webkit_web_context_set_security_policy_for_uri_scheme more than once for the same scheme).
> 
> You are right, it can be confusing, maybe we could add a WebKitSecurityPolicy object with methods to register uri schemes:
> 
> webkit_secutiry_policy_register_uri_scheme_as_local()
> webkit_secutiry_policy_register_uri_scheme_as_secure()
> ....
> 
> And also methods to query:
> 
> webkit_secutiry_policy_is_uri_scheme_local()
> webkit_secutiry_policy_is_uri_scheme_secure()
> ....
> 
> And method to get the security policy object (or boxed type) from the context
> 
> WebKitSecutiryPolicy* webkit_web_context_get_secutory_policy().
> 
> I used the flags to reduce the api, but I agree it can be confusing, we should do the same in wk1.

You're suggestion makes sense to me. Sorry that I didn't notice this when you did the version for WebKti1!

[Carlos Garcia Campos]

Created attachment 164548 [details]
New patch using a different API

[Martin Robinson]

Comment on attachment 164548 [details]
New patch using a different API

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

Looks good. Couple small suggestions before landing...

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:59
> +static void webkit_security_manager_class_init(WebKitSecurityManagerClass* findClass)

Small nit here: findClass could just be klass or securityManagerClass. I guess this is just a copy-paste thing?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:318
> +    void check(const char* scheme, unsigned policy)

Maybe a name like verifyThatSchemeMatchesPolicy?

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 164548 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164548&action=review
> 
> Looks good. Couple small suggestions before landing...
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:59
> > +static void webkit_security_manager_class_init(WebKitSecurityManagerClass* findClass)
> 
> Small nit here: findClass could just be klass or securityManagerClass. I guess this is just a copy-paste thing?

hehe, yes, I copied this from the CookieManager that was copied from the FindCrontroller. The CookieManager still has findClass :-P

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:318
> > +    void check(const char* scheme, unsigned policy)
> 
> Maybe a name like verifyThatSchemeMatchesPolicy?

Sure.

[Carlos Garcia Campos]

Committed r128989: <http://trac.webkit.org/changeset/128989>