Both have parameters documented as taking text. But, those parameters also accept and use html content in practice.
This put the burden of making sure the content is correctly sanitized on the APIs users, which can be cumbersome.
No mention of HTML in the documentation. The parameter is explicitly named “text”.
XWiki.widgets.ConfirmationBox
Parameters:
confirmationText
yesButtonText
noButtonText
No explicit documentation of the parameters, but the example provided in the documentation uses plain text messages. No mention of HTML in the documentation. The parameters all explicitly contain the “text” suffix.
Usage
This usage analysis only covers code I have access to, namely xwiki-platform, xwiki-contrib, and xwikisas projects.
I also searched use on GitHub more globally.
My search could not be exhaustive given the amount of results, but I couldn’t find any example where something else than plain text was used for the parameters.
Proposal
I can think of three options:
Option 1: Change the implementations to use the parameters a plain text
Pros:
Back to what I consider to be the original intent of those APIs.
Cons:
Breaking change in case of intentional use of HTML for the parameters
No way to fix the issue if we discover a legitimate use of HTML
Option 2: Make plain text the default but offer a way to switch to HTML
Pros/Cons: same as option 1 but with a way to fix potential places where HTML is expected
Option 3: Keep HTML the default but offer a way to switch to plain text
Pros:
No backward compatibility issue
Cons:
Involves a lot of work to handle all the places where plain text is passed as HTML for no good reason
Possibly confusing API where HTML content is passed to “text” parameters, and plain text parameters passed to new “plainText” parameters.
Conclusion
I did not mention implementation idea for options 2 and 3, I’ll enrich the discussion if those choices emerge as viable options.
Feel free to pick different options for Notification and ConfirmationBox if you think they should be handled differently.
My vote goes for:
+1 for option 1
-0 for option 2 since given my current understanding, the additional work to handle HTML wouldn’t be used in practice
-1 for option 3 as this would be promoting bad usage of the APIs
It does seem that 1 would do a lot more good than 3 and no evidence that anything would be broken in practice (but clear evidence that it would fix various escaping problems), so +1 for 1.
2 does not really help with retro compatibility for extensions anyway so 1 is not much worse (and it does not prevent adding support for HTML later if really needed I guess).
You mean properly deal with escaping in all those places I guess ?
Note that there is no API in XWiki for performing this HTML-escaping client-side. There is no native API, and we don’t have any library easily available that provides this escaping. So it’s much more difficult than server-side to actually perform the escaping.
I’m also +1 for option 1. I think what we should do, however, is to add a way for JavaScript code to find out how the parameter is treated by, e.g., adding some constant whose presence can be checked. That way, extensions that, e.g., actually added escaping, can adapt their behavior depending on the version.
After a closer inspection, yesButtonText, noButtonText, and cancelButtonText (that I did not mention earlier), are already used as plain next, for instance
I just imagined having a constant XWiki.widgets.ConfirmationBox.confirmationFormat that we set to plain. That way, the caller can be sure that the confirmation text will be treated as plain text and, e.g., omit cleaning or escaping of the confirmation text. Same for XWiki.widgets.Notification.textFormat.
I found out that XWiki.widgets.Notifications is actually used with HTML content for the text in a few places:
action buttons errors
dataeditor errors
attachments suggestion (suggestAttachments.js)
attachment mimetype validation
I don’t think it is possible to easily change those places to stop relying on HTML.
The usage is also relevant:
bold to highlight relevant piece of text
use of newlines for readability
Option 1: revert to HTML as default
Option 1.1: let each caller handle the escaping
Option 1.2: let text be html by default but introduce a parameter to
Option 2: keep plain text as the default
Introduce a parameter to allow html for the text parameter.
Add a new textHtml boolean parameter (default: false)
Use the text as HTML only whe textHtml is true
Document that the use of this parameter is a risk and can lead to XSS.
Example:
new XWiki.widgets.Notification("text with <strong>html</strong>", "error",
{textHtml: true});
I think we need to handle things differently on the stable branch and on the lts branch.
For stable:
I’m -1 for option 1.1 and 1.2 as they are not secure by default.
I’m +1 for option 2, with implementation details opened for discussion.
For lts:
* I'm +1 for option 1.2 which would be equivalent to option 2 but with `textHtml` `false` by default (and we'll need to fix locally the places where we know text is not HTML)
* I'm -1 for option 2.1 because of the regression risks
Edit: After further discussions I agree that we can take the risk of regressions that are lower than the drawbacks caused by inconsistency between branches.
Note that currently the fix is applied on stable-17.10.x, but we could decide to revert it there because of the regression risk, and
