Bug 207384

Summary:

[GTK][WPE] Expose allowTopNavigationToDataURL

Product:

WebKit

Reporter:

Lauro Moura <lmoura>

Component:

WebKitGTK

Assignee:

Lauro Moura <lmoura>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aperez, berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gustavo

Priority:

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

207454

Attachments:

Description	Flags
Patch	none
Patch	none
Updated patch after aperez comments	none

Lauro Moura

Reported 2020-02-07 07:42:44 PST

bug206962/r255961 introduced a new loading behavior blocking by default the top frame from navigating to data URL's. This behavior can be controlled with the `allowTopNavigationToDataURLs` settings. This caused GTK's TestUIClient::allow-modal-dialogs test to timeout.

Attachments
Patch (11.64 KB, patch) 2020-02-07 07:49 PST, Lauro Moura	no flags	Details Formatted Diff Diff
Patch (11.64 KB, patch) 2020-02-07 09:53 PST, Lauro Moura	no flags	Details Formatted Diff Diff
Updated patch after aperez comments (12.01 KB, patch) 2020-02-09 20:02 PST, Lauro Moura	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Lauro Moura

Comment 1 2020-02-07 07:49:05 PST

Created attachment 390084 [details] Patch

EWS Watchlist

Comment 2 2020-02-07 07:50:07 PST

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

Lauro Moura

Comment 3 2020-02-07 07:57:38 PST

Comment on attachment 390084 [details] Patch Removing the review, as it'll have to be updated with correct API versioning.

Lauro Moura

Comment 4 2020-02-07 09:53:35 PST

Created attachment 390093 [details] Patch

Adrian Perez

Comment 5 2020-02-09 12:21:38 PST

Comment on attachment 390093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390093&action=review Patch looks great, thanks for submitting it! I think we should land this, and it only needs a couple of small bits in the API documentation. Feel free to land it after taking care of them. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1449 > + * Whether or not the top frame is allowed to navigate to data URLS. Typo: s/URLS/URLs It would be nicer to have a couple of lines additional lines here in the API documentation explaining the reason for the default (disabled), and telling why some application which embeds a WebKitWebView might want to enable it. For some cues as to why disabling top-level data:// loads be default is good, there's this Mozilla blog post: https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/ As for when to enable it, I think that the main use-case is when an application which is *NOT* a generic web browser embeds a WebKitWebView widget and wants to load data:// URLs using webkit_web_view_load_uri() to provide content to be displayed that is controlled by the application. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1457 > + _("Whether or not top frame navigation is allowed to data URLS"), Typo: s/URLS/URLs

Lauro Moura

Comment 6 2020-02-09 20:02:56 PST

Created attachment 390224 [details] Updated patch after aperez comments

WebKit Commit Bot

Comment 7 2020-02-09 20:47:20 PST

Comment on attachment 390224 [details] Updated patch after aperez comments Clearing flags on attachment: 390224 Committed r256109: <https://trac.webkit.org/changeset/256109>

WebKit Commit Bot

Comment 8 2020-02-09 20:47:22 PST

All reviewed patches have been landed. Closing bug.

Carlos Garcia Campos

Comment 9 2020-02-10 00:19:55 PST

Comment on attachment 390224 [details] Updated patch after aperez comments View in context: https://bugs.webkit.org/attachment.cgi?id=390224&action=review > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > + * Since: 2.28 r255961 hasn't been merged in 2.28 branch. I think it's better to leave this for 2.30, it's probably too late in the cycle for a change in behavior like this. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:537 > + webkit_settings_set_allow_top_navigation_to_data_urls(settings, TRUE); > > test->loadHtml("<html><body onload=\"window.showModalDialog('data:text/html,<html><body/><script>window.close();</script></html>')\"></body></html>", 0); > test->waitUntilMainLoopFinishes(); Maybe it would be better to rework this test not to use a data URL. On the other hand it's good to test that the setting actually works, so I'm not sure.

Adrian Perez

Comment 10 2020-02-10 00:51:38 PST

(In reply to Carlos Garcia Campos from comment #9) > Comment on attachment 390224 [details] > Updated patch after aperez comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390224&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > > + * Since: 2.28 > > r255961 hasn't been merged in 2.28 branch. I think it's better to leave this > for 2.30, it's probably too late in the cycle for a change in behavior like > this. I think we could still merge it to the 2.28 branch with the default value for the property set to TRUE, which would keep the existing behavior for 2.28 and also would allow web browsers (Epiphany, Midori, etc.) to set it to FALSE much sooner without accidentally breaking applications at this point of the development cycle. We will also need a small follow-up patch to always allow data:// URLs in the inspector, which I will be posting soon.

Adrian Perez

Comment 11 2020-02-10 01:28:23 PST

(In reply to Adrian Perez from comment #10) > [...] > > We will also need a small follow-up patch to always allow data:// URLs > in the inspector, which I will be posting soon. Done in bug #207454

Carlos Garcia Campos

Comment 12 2020-02-10 01:32:30 PST

(In reply to Adrian Perez from comment #10) > (In reply to Carlos Garcia Campos from comment #9) > > Comment on attachment 390224 [details] > > Updated patch after aperez comments > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=390224&action=review > > > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > > > + * Since: 2.28 > > > > r255961 hasn't been merged in 2.28 branch. I think it's better to leave this > > for 2.30, it's probably too late in the cycle for a change in behavior like > > this. > > I think we could still merge it to the 2.28 branch with the default > value for the property set to TRUE, which would keep the existing > behavior for 2.28 and also would allow web browsers (Epiphany, Midori, > etc.) to set it to FALSE much sooner without accidentally breaking > applications at this point of the development cycle. Ok, let's do that. > We will also need a small follow-up patch to always allow data:// URLs > in the inspector, which I will be posting soon.

Note You need to log in before you can comment on or make changes to this bug.