To disable js markup instead of all js execution.
Created attachment 359147 [details] Patch
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
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.” 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.
FWIW, my main issue with this is the lack of explanation in the API documentation. The name is quite “meh”, but that is less of a concern if the others out there prefer to keep it as is :)
(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. 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). > 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.
(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. :-\
(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.
Comment on attachment 359147 [details] 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
(In reply to Michael Catanzaro from comment #8) > Comment on attachment 359147 [details] > 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. I mean the page document. > > 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. Ok, I'm open to suggestions. > But I have another concern. Executing JavaScript is still allowed... how? I think this is pretty clear too... JavaScript execution is not disallowed at all, the script elements and attributes are re moved from the document, that's the only thing. > "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. Yes, that's also desirable. > 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? That's a good question, I'm adding Geoffrey to the CC, since he suggested to use this setting instead of enable-js. > 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? If removing js markup from document is a better approach we could just deprecate the other setting. I find more confusing a setting that disables js, but still allows it for API stuff, than a setting that removes js markup from document (but it seems this is only clear enough for me). > 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
(In reply to Carlos Garcia Campos from comment #9) > I think this is pretty clear too... JavaScript execution is not disallowed > at all, the script elements and attributes are re moved from the document, > that's the only thing. Are there other ways to execute JavaScript, asides from script elements, script attributes, and API requests? I guess not? > > 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? > > That's a good question, I'm adding Geoffrey to the CC, since he suggested to > use this setting instead of enable-js. Basically I think we should fix bug #192753 one way or the other *first* and only *then* decide whether to add this setting. Because as I understand this setting, our solution to that bug might obviate the need for this setting.
(In reply to Michael Catanzaro from comment #10) > (In reply to Carlos Garcia Campos from comment #9) > > I think this is pretty clear too... JavaScript execution is not disallowed > > at all, the script elements and attributes are re moved from the document, > > that's the only thing. > > Are there other ways to execute JavaScript, asides from script elements, > script attributes, and API requests? I guess not? > > > > 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? > > > > That's a good question, I'm adding Geoffrey to the CC, since he suggested to > > use this setting instead of enable-js. > > Basically I think we should fix bug #192753 one way or the other *first* and > only *then* decide whether to add this setting. Because as I understand this > setting, our solution to that bug might obviate the need for this setting. This is not so easy in the end. Maybe run_js should be allowed when js is disabled in settings, but FrameLoader doesn't dispatch didClearWindowObjectInWorld() when scripts are not allowed, so we can't inject our internal js api in the web process. There are more places where canExecuteScripts() is checked. I think it's by far a lot easier to explain and understand that script elements and attributes are removed from the document while parsing.
(In reply to Carlos Garcia Campos from comment #11) > This is not so easy in the end. Maybe run_js should be allowed when js is > disabled in settings, but FrameLoader doesn't dispatch > didClearWindowObjectInWorld() when scripts are not allowed So that means accessibility doesn't work when enable-js is disabled and nobody ever noticed this, right?
r=me with improved documentation, then. Maybe add a sentence or two listing some of the forms of JS execution that are blocked and not blocked by this setting.
(In reply to Michael Catanzaro from comment #12) > (In reply to Carlos Garcia Campos from comment #11) > > This is not so easy in the end. Maybe run_js should be allowed when js is > > disabled in settings, but FrameLoader doesn't dispatch > > didClearWindowObjectInWorld() when scripts are not allowed > > So that means accessibility doesn't work when enable-js is disabled and > nobody ever noticed this, right? Accessibility? why?
(In reply to Carlos Garcia Campos from comment #14) > Accessibility? why? https://bugs.webkit.org/show_bug.cgi?id=182257#c18
(In reply to Michael Catanzaro from comment #15) > (In reply to Carlos Garcia Campos from comment #14) > > Accessibility? why? > > https://bugs.webkit.org/show_bug.cgi?id=182257#c18 Oh, wow, a11y stuff doesn't belong to did clear window object indeed.
I'm going to land this before branching, we can improve the documentation in a follow up patch. Suggestions would be welcome, because current wording is still crystal clear for me.
Committed r241258: <https://trac.webkit.org/changeset/241258>
Comment on attachment 359147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359147&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1458 >>> + * Determines whether or not JavaScript markup is allowed in document. When this setting is disabled, >> >> documents. > > I mean the page document. Still should be: "documents."