[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 #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Adrian Perez from comment #6)
> (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?

Yes, that happens after parsing.

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

It should work either way.

>  - Will <script> tags inside nested frames or an <iframe> be loaded
>    and executed?

No.

> (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()?

Use scripts, js api in the web extension, user message handlers, etc.

> If “ALL
> JavaScript related elements and attributes” can't be used (according to
> the description, emphasis mine on “all”),

The description doesn't say they can't be used, it says they are removed from the document during parsing.

> how come that other means of
> execution whih are NOT under control of the C API can result in JS
> being executed?

Because other means of execution don't depend on script tags and attributes present in the document during parsing.

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

I'm fine with adding more explanations, but what you propose doesn't look better than the current text IMO.

> My least of concerns is the name of the option, like I wrote already.
> 
> :-\

I agree.

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