Fixed Tooltips should be made HTML safe

rellek

rellek

Well-known member
Hi,

I think there's a little bug, at least in the admin panel. There are some tooltips that are filled with a phrase. If that phrase contains quotes, it will break the javascript. As an example:

Code: 
<label class="secondaryContent Tooltip" data-offsetx="3" title="{phrase goes here}">

The phrase to be put there is set_x_as_default_language. If that phrase does not use single quotes like in the original but double quotes ("), this will happen:
http://xenforo.com/community/threads/acp-list-items-disappear-on-hover.57068/

Therefore, at least quotes should be converted automatically to their html entity &quot; so that it will at least not break. If {title} contains a quote, this works too. So I think this is just down to the phrase itself.

For some reason, this tooltip works fine in the node description even if there are quotes. No problem here:
Code: 
<blockquote class="nodeDescription nodeDescriptionTooltip baseHtml" id="nodeDescription-18">Hier könnt ihr eure Projekte, die mit sozialer Software realisiert sind, vorstellen. Um neue Themen starten zu können, braucht ihr 20 Beiträge. Lest bitte die <a href="/threads/regeln-zu-diesem-forum.80/">Regeln</a>.</blockquote>
 
The node tooltip is fine because it's not in an attribute.

You're right in that it only affects the phrase content itself - the phrase variables will still be escaped. Phrases can normally contain HTML (though not that commonly), so this is generally consistent/expected. However, it comes up whenever we potentially use a phrase in a context that requires HTML escaping, likely because it's inside an attribute (or because it's simply nonsensical).

The problem is that the straight forward way to do this likely ends up double escaping content within the phrase. The "correct" way leads to using raw access to all variables within, and then actively escaping the entire content. I'm not happy with this as a workaround as it's easy to make a mistake and create a security issue (via user generated content, rather than the otherwise trusted phrase content). The best fix would likely involve indicating HTML (or another type) of escaping in the phrase context. This is a fairly significant change.

This would have to be done on a case by case basis since, as mentioned, phrases can normally contain HTML. Combined with the approach needed for a proper fix (new template function/tag), I don't think this would be fixed before 1.3 at the earliest.

Workarounds involve using single quotes or, if the HTML usage is known, &quot; or entities.
 
XF2 has a template filter called "for_attr" which essentially ensures that all HTML is escaped within, without double escaping. I've gone through and applied this to all of our templates where it should be necessary, so that should resolve this issue.

If you notice any areas in XF2 where we use a phrase in an attribute and we don't apply this filter, please report it as a bug.
 
Another approach could be to apply HTML escaping by default and have a flag to disable escaping when necessary. This is how Ruby on Rails deals with the HTML safety by default. This approach will ensure that addons or custom template modifications will not cause this issue unless explicitly marked to skip escaping.
 
This issue is mostly specific to phrases where HTML is allowed (intentionally) in most contexts. So it makes more sense to opt out in this specific case. It should roughly be considered similar to using a phrase within JS/JSON which needs different escaping rules.

(Just to confirm, this is caused by changes to the phrase itself, not variables within the phrase; those always have been escaped and still will be.)
 
Back
Top Bottom