NEW 162475
Add SPI for allowing non-special schemes to have non-null SecurityOrigins after r206165
https://bugs.webkit.org/show_bug.cgi?id=162475
Summary Add SPI for allowing non-special schemes to have non-null SecurityOrigins aft...
Alex Christensen
Reported 2016-09-22 17:54:00 PDT
Add SPI for allowing non-special schemes to have non-null SecurityOrigins after r206165
Attachments
Patch (17.77 KB, patch)
2016-09-22 17:55 PDT, Alex Christensen
no flags
Patch (29.14 KB, patch)
2016-09-22 18:29 PDT, Alex Christensen
no flags
Alex Christensen
Comment 1 2016-09-22 17:55:29 PDT
Alex Christensen
Comment 2 2016-09-22 18:29:57 PDT
WebKit Commit Bot
Comment 3 2016-09-22 18:33:05 PDT
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 4 2016-09-22 20:10:11 PDT
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
Comment 5 2016-09-22 21:10:38 PDT
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 6 2016-09-23 00:57:05 PDT
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
Comment 7 2016-09-23 09:03:55 PDT
I rolled out the original change. I think we need to consider this a bit more.
Note You need to log in before you can comment on or make changes to this bug.