[Webkit-unassigned] [Bug 193439] [GTK][WPE] Add enable-javascript-markup setting

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193439

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #359147|review?                     |review-
              Flags|                            |

--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 359147
  --> https://bugs.webkit.org/attachment.cgi?id=359147
Patch

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

Nice test.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1458
> +     * Determines whether or not JavaScript markup is allowed in document. When this setting is disabled,

documents.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1459
> +     * all JavaScript related elements and attributes are removed from the document during parsing. Note that

JavaScript-related

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1460
> +     * all JavaScript related elements and attributes are removed from the document during parsing. Note that
> +     * executing JavaScript is still allowed if #WebKitSettings:enable-javascript is %TRUE.

I agree with Adrian that this documentation in insufficient. Basically all of his questions should be clarified by better documentation.

But I have another concern. Executing JavaScript is still allowed... how? "Use scripts, js api in the web extension, user message handlers, etc." OK, but those are API requests, and I've separately suggests that API requests should always ignore #WebKitSettings:enable-javascript and execute anyway. This looked like something we could change on a cross-platform basis. So then maybe this new setting is not needed after all, if it, in practice, just blocks all JS except API requests? I really don't like mentioning that executing JavaScript is still allowed via API if #WebKitSettings:enable-javascript is %TRUE here, because I want to change that! Basically my fear is that "enable-javascript-markup" is the setting that most applications will want to use, and "enable-javascript" will be a wrong trap choice. So I'd rather make that usable first. Would this setting still be useful after that change?

r- to improve the docs and continue this discussion. The code is fine, of course.

>>>>> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3582
>>>>> + * Returns: %TRUE If JavaScript markup is enabled or %FALSE otherwise.
>>>> 
>>>> What is “JavaScript Markup”? A specification, a WebKit specific concept?
>>>> Something widely understood among Web developers?... I think the documentation
>>>> for this setting needs an explanation of what the option does, as it is nearly
>>>> impossible to find anything about this topic that would seem relevant when
>>>> implementing an application which makes use of WebKit.
>>>> 
>>>> After some digging in the repository history I arrived at bug #112999 and
>>>> bug #113122 and it took me wading through a pile of comments and reading
>>>> some bits of the code to guess what the setting does. So please let's make
>>>> the API documentation better by adding something in the lines of:
>>>> 
>>>>   “Enabling this setting will strip <script> tags from loaded HTML
>>>>    content, but other forms of JavaScript execution e.g. using
>>>>    webkit_web_view_run_javascript() are still allowed. This setting
>>>>    is intended for applications which display HTML content but are
>>>>    not full browsers, and which want to avoid the risk of script
>>>>    injection attacks, as is the case of applications like e-mail
>>>>    and news readers.”
>>>> 
>>>> If possible it should be more concrete that the above explaining what
>>>> gets restricted and what not, because very often we programmers end up
>>>> introducing accidental security vulnerabilities due to assumptions we
>>>> make caused by incomplete documentation of third party code used.
>>> 
>>> The explanation in the patch is even more accurate and complete than this one, I would say:
>>> 
>>> +     * Determines whether or not JavaScript markup is allowed in document. When this setting is disabled,
>>> +     * all JavaScript related elements and attributes are removed from the document during parsing. Note that
>>> +     * executing JavaScript is still allowed if #WebKitSettings:enable-javascript is %TRUE.
>>> 
>>> It's not only about script tags but also even listener attributes like onload and other js related attributes. That's clear in the current explanation. It also says that elements are removed while parsing and that js execution is still possible (not only run_js, but any js execution).
>> 
>> TBH, with this writing it is not completely clear to me what the
>> setting does. What does “all JavaScript related elements and attributes”
>> mean? For example it does not answer question like:
>> 
>>  - Can the JS DOM API be used to insert a new <script> tag?
>>  - If a <script> tag can be inserted using the DOM, will it work for
>>    <script src="..."> or only for elements with inline JS code in them? 
>>  - Will <script> tags inside nested frames or an <iframe> be loaded
>>    and executed?
>> 
>> (and that's only from the top of my head, I could come up with more)
> 
> Yes, that happens after parsing.

"%TRUE If" -> "%TRUE if"

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:437
> +        " <body onload='document.title = \"JavaScript allowed from body onload attribute\"'>"

JavaScript improperly allowed

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:439
> +        "  <script>document.title = 'JavaScript allowed from body script'</script>"

JavaScript improperly allowed

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:447
> +    g_assert(jsResult);

g_assert_nonnull

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190121/5aa85497/attachment-0001.html>