View Issue Details

IDProjectCategoryView StatusLast Update
0003514Composr alpha bug reports[All Projects] General / Uncategorisedpublic2018-02-12 00:02
ReporterChris GrahamAssigned ToSalman 
SeverityFeature-request 
Status resolvedResolutionfixed 
Summary0003514: Tooltip escaping
DescriptionSee screenshot. Ask if it's not clear.
TagsNo tags attached.
Sponsorship open

Activities

Chris Graham

2018-02-01 00:50

administrator  

tooltip-escaping.png (152,532 bytes)
tooltip-escaping.png (152,532 bytes)

Salman

2018-02-09 06:47

reporter   ~0005481

Last edited: 2018-02-09 06:58

View 2 revisions

Alright. I don't think this issue was introduced by me. I spent 6+ hours tracking this down (the menu generation code is extremely complicated, its ->get_node() calls all the way down), it turns out these tooltips are stored in the 'translate' table and their serialized Tempcode in the 'text_parsed' columns has its HTML entities already escaped. So when they're referenced in Tempcode (i.e., {TEMPCODE*}), we get a double escaping issue.

See 'SELECT t.text_parsed FROM cms.cms_cached_comcode_pages p LEFT JOIN cms.cms_translate t ON t.id=p.string_index WHERE id = 830;'

I don't know what populates the translate table with these tooltips so I think you are better qualified to fix this way quickly or please guide me...

Chris Graham

2018-02-09 13:29

administrator   ~0005482

Ok I'll take a look.

The complexity is because I'm a stable genius. ;-)

Chris Graham

2018-02-12 00:00

administrator   ~0005500

Last edited: 2018-02-12 00:09

View 2 revisions

v10...

page_grouping.php:
$child_description = (is_object($link[4])) ? $link[4] : comcode_lang_string($link[4]);

This is HTML encapsulated in Tempcode, and goes into the node structure, which ends up going to the menu branch template.

In the menu branch template:
onmouseover="if (typeof window.activate_tooltip!='undefined') activate_tooltip(this,event,'{TOOLTIP;^*}','auto');"
We want them to be escaped here as we can't put HTML tags (deep) within an HTML escape happening. So apostrophes may be "double escaped". So all is okay so far.

Actually we should really use '=' instead of '*' because we want to force escaping, even for language string output ('*' will not escape language strings, as they are assumed to already be HTML). We are taking something known to be HTML and deliberately escaping it. I'll fix that, but that's a side issue and invisible right now as we're not passing in language strings.


v11...

(page_grouping.php same)

In the menu branch template:
data-mouseover-activate-tooltip="['{TOOLTIP;^=}', 'auto']"

The escape flow is...
(initially) Banning IP addresses is useful to totally remove a user's ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool. -->
(;) Banning IP addresses is useful to totally remove a user\'s ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool. -->
(;^) Banning IP addresses is useful to totally remove a user\'s ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool. [i.e. no change] -->
(;^=) Banning IP addresses is useful to totally remove a user\'s ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool.

Note ";" will slash-escape HTML-escaped apostrophes. I'm not sure if that's needed anymore. It may have been an incorrect response to a bug years ago, and it is triggering the JSON5 parse errors in 0003513, because apparently JSON(5?) is less amiable to unnecessary escaping than normal JS. I'll remove this behaviour. "*"/"=" escaping should always happen after ";" escaping in HTML templates, as that's the logical pipeline.

The browser will do these layer conversions...

Stream layer: Banning IP addresses is useful to totally remove a user\'s ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool.
DOM layer: Banning IP addresses is useful to totally remove a user\'s ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool.

After my change though:

Stream layer: Banning IP addresses is useful to totally remove a user's ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool.
DOM layer: Banning IP addresses is useful to totally remove a user's ability to access the site; unfortunately, users can very easily switch IP addresses so it is not a perfect tool.

Still none of this is anything to do with the apostrophe double escaping. That is coming from an unexpected place:
title="{CAPTION*}{+START,IF_NON_EMPTY,{TOOLTIP}}: {$STRIP_TAGS,{TOOLTIP}}

The 'title' attribute is turned into a tooltip automatically by the JS. This is racing against your own tooltip code. As can be seen in another bug I just found (but didn't add to the tracker), whereby hoving the link directly doesn't even show the tooltip.

This should have been (my fault):
title="{$STRIP_TAGS,{CAPTION}}{+START,IF_NON_EMPTY,{TOOLTIP}}: {$STRIP_TAGS,{TOOLTIP}}
as the {CAPTION} was already "HTML-ready", just we need to not have the HTML-tags, leaving HTML entities in tact.

This was not an issue in v10 because for whatever reason the racing wasn't happening or not causing an issue.

Fixed in https://github.com/ocproducts/composr/commit/7135e556fcc3b9df2539441cd01a06f1ac401590

Issue History

Date Modified Username Field Change
2018-02-01 00:50 Chris Graham New Issue
2018-02-01 00:50 Chris Graham Status non-assigned => assigned
2018-02-01 00:50 Chris Graham Assigned To => Salman
2018-02-01 00:50 Chris Graham File Added: tooltip-escaping.png
2018-02-09 06:47 Salman Note Added: 0005481
2018-02-09 06:58 Salman Note Edited: 0005481 View Revisions
2018-02-09 13:29 Chris Graham Note Added: 0005482
2018-02-12 00:00 Chris Graham Note Added: 0005500
2018-02-12 00:02 Chris Graham Status assigned => resolved
2018-02-12 00:02 Chris Graham Resolution open => fixed
2018-02-12 00:09 Chris Graham Note Edited: 0005500 View Revisions
2023-02-26 18:29 Chris Graham Category General => General / Uncategorised