Add SPI for allowing non-special schemes to have non-null SecurityOrigins after r206165

[Alex Christensen]

Add SPI for allowing non-special schemes to have non-null SecurityOrigins after r206165

[Alex Christensen]

Created attachment 289638 [details]
Patch

[Alex Christensen]

Created attachment 289641 [details]
Patch

[WebKit Commit 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

[Michael Catanzaro]

Comment on attachment 289641 [details]
Patch

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

Thanks for updating our WebKitSecurityManager API. You also need to update webkit2gtk-4.0-sections.txt, that's why EWS is red.

Please also wait for Carlos to approve the GTK+ API changes. Ideally we would also add an API test, maybe he'll help with that?

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:166
> + * This is deprecated. Use webkit_security_manager_register_valid_security_origin_protocol instead.

Use a deprecated tag:

* Deprecated: 2.16. Use webkit_security_manager_register_valid_security_origin_protocol() instead.

Also note I added parentheses to the function name, else it doesn't get formatted correctly in the documentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:177
> + * This is deprecated. Use webkit_security_manager_uri_scheme_is_valid_security_origin_protocol instead.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:179
> + * Returns: %False.

%FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:183
> +    return false;

We prefer to return FALSE (gboolean type) since it's a gboolean and not a bool. Of course either works.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:192
> + * Register @protocol as a valid security origin protocol. 
> + * This means that security origins with this protocol are not always null, which they are by default.

Hm, what does this mean? I have no clue, so I think API users have no chance.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:193
> + */

Use a since tag:

 * Since: 2.16

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:211
> + * Returns: %TRUE if @protocol is a valid security origin or %FALSE otherwise.

Also needs a Since tag

[Michael Catanzaro]

Also, is there any way to keep the old API working? Even if it's just a GTK-layer wrapper around the new API?

[Carlos Garcia Campos]

Comment on attachment 289641 [details]
Patch

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

Thanks for updating the GTK+ API too. We should make sure that we don't change the behavior, so I would add resource unconditionally in the SchemeRegistry, and we should probably add manually the custom uri schemes when registered.

> Source/WebCore/page/SecurityOrigin.cpp:97
> +    if (SchemeRegistry::isValidSecurityOriginProtocol(innerURL.protocol()))

Why valid? Aren't null security origins valid too? Could we use null or unique instead of valid? SchemeRegistry::shouldTreatURLSchemeAsNonNull() or SchemeRegistry::shouldTreatURLSchemeAsNonUnique()?

> Source/WebCore/platform/SchemeRegistry.cpp:77
> -static URLSchemesMap& schemesWithUniqueOrigins()
> +static URLSchemesMap& validSecurityOriginProtocols()

This is the SchemeRegistry and all other methods use Scheme, not Protocol, so I would make this consistent and use Scheme instead. schemesWithNonUniqueOrigins(), for example

> Source/WebCore/platform/SchemeRegistry.cpp:-87
> -    if (schemesWithUniqueOrigins.get().isEmpty()) {
> -        schemesWithUniqueOrigins.get().add("about");
> -        schemesWithUniqueOrigins.get().add("javascript");
> -        // This is a willful violation of HTML5.
> -        // See https://bugs.webkit.org/show_bug.cgi?id=11885
> -        schemesWithUniqueOrigins.get().add("data");
> -    }

So, here I would add resource inside a GTK platform ifdef.

> Source/WebCore/platform/SchemeRegistry.cpp:190
> -void SchemeRegistry::registerURLSchemeAsNoAccess(const String& scheme)
> +void SchemeRegistry::registerValidSecurityOriginProtocol(const String& scheme)

Same here, I would follow the pattern registerURLSchemeAs for consistency.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:87
> -    encoder << urlSchemesRegisteredAsNoAccess;
> +    encoder << validSecurityOriginProtocols;

Ditto, urlSchemesRegisteredAs...

>> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:166
>> + * This is deprecated. Use webkit_security_manager_register_valid_security_origin_protocol instead.
> 
> Use a deprecated tag:
> 
> * Deprecated: 2.16. Use webkit_security_manager_register_valid_security_origin_protocol() instead.
> 
> Also note I added parentheses to the function name, else it doesn't get formatted correctly in the documentation.

I'm not sure about using webkit_security_manager_register_valid_security_origin_protocol() instead, because those a re not equivalent, they do the opposite. If you want to to register a schemes as no-access, now you don't need to do anything, because that scheme is already considered as no-access by WebKit.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:-174
> -    g_return_if_fail(WEBKIT_IS_SECURITY_MANAGER(manager));
> -    g_return_if_fail(scheme);
> -
> -    registerSecurityPolicyForURIScheme(manager, scheme, SecurityPolicyNoAccess);

When we deprecate a function with a change on behvior like this, we normally show a message to let the users know they are using something that does nothing.

g_warning("%s: all non special URI schemes are now considered as no-access by WebKit, this function does nothing.", __func__);

For example.

>> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:177
>> + * This is deprecated. Use webkit_security_manager_uri_scheme_is_valid_security_origin_protocol instead.
> 
> Ditto.

Ditto.

>> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:179
>> - * Returns: %TRUE if @scheme is a no-access scheme or %FALSE otherwise.
>> + * Returns: %False.
> 
> %FALSE

I think we should keep the documentation of the return value, even if it's not going to return TRUE.

>> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:183
>> +    return false;
> 
> We prefer to return FALSE (gboolean type) since it's a gboolean and not a bool. Of course either works.

And add the g_warning here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:187
> + * webkit_security_manager_register_valid_security_origin_protocol:

In the GTK+ public API we want to keep the consistency for sure, so this should be something like:

webkit_security_manager_register_uri_scheme_as_non_unique_security_origin()

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:189
> + * @protocol: a URI protocol

And this should be scheme instead of protocol

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:204
> + * webkit_security_manager_uri_scheme_is_valid_security_origin_protocol:

webkit_security_manager_uri_scheme_is_non_unique_security_origin()

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:206
> + * @protocol: a URI protocol

scheme

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityManager.cpp:219
>  }

We also have to update the documentation files to add the new symbols. In any case, if you don't want to deal with the GTK+ part it's perfectly ok, if you just leave the no-access methods emtpy and we will add the new API in a follow up patch.

[Alex Christensen]

I rolled out the original change.  I think we need to consider this a bit more.