[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

--- Comment #6 from Adrian Perez <aperez at igalia.com> ---
(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Adrian Perez from comment #3)
> > Comment on attachment 359147 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=359147&action=review
> > 
> > Informally reviewing, this would be a r- for my: the description
> > in the API documentation is seriously lacking, and the name of the
> > setting is quite bad (non-descriptive, and difficult to search
> > online for). I don't care what WebKit calls the setting internally,
> > I would rather have a name for it that better indicates what it does
> > *and* that we provide a good description of what the setting does and
> > its intended usage.
> > 
> > > 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.”
> 
> 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.

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)

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

Which other means of execution other than *_run_javascript()? If “ALL
JavaScript related elements and attributes” can't be used (according to
the description, emphasis mine on “all”), how come that other means of
execution whih are NOT under control of the C API can result in JS
being executed?

> > 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.
> 
> I think it's clear enough. I'm open to change the name of the setting,
> though.

To be honest, I am left with even more questions, so definitely not
“clear enough” for me. Not being clear and exhaustive explaining this
kind of things in the API documentation which affect security would
do a disservice, IMO.

My least of concerns is the name of the option, like I wrote already.

:-\

-- 
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/20190115/7c37087c/attachment.html>