[webkit-reviews] review denied: [Bug 193439] [GTK][WPE] Add enable-javascript-markup setting : [Attachment 359147] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 193439: [GTK][WPE] Add enable-javascript-markup setting
https://bugs.webkit.org/show_bug.cgi?id=193439

Attachment 359147: Patch

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




--- 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