Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treatment of html in version 9 #15235

Closed
drmrbrewer opened this issue Feb 26, 2021 · 8 comments · Fixed by #15990
Closed

Treatment of html in version 9 #15235

drmrbrewer opened this issue Feb 26, 2021 · 8 comments · Fixed by #15990

Comments

@drmrbrewer
Copy link

drmrbrewer commented Feb 26, 2021

Hit Run repeatedly with the following jsfiddle:

http://jsfiddle.net/ohmkqzub/

This is meant to simulate that the title is made up of individual components, each of which is subject to different formatting based on user settings, and the word separator is also a user option... and a valid and common choice is just a space character ( ).

Now try it with highcharts version 9:

http://jsfiddle.net/tna93r6x/

Why does the space character get wiped out? Anything to do with the new filtering feature?

@KacperMadej
Copy link

Every whitespace text is filtered (maybe @TorsteinHonsi might be able to provide more info about this?):

// Whitespace text node, don't append it to the AST
if (/^[\s]*$/.test(textContent)) {
return;
}

Workaround: use a blank Braille block (source)
Demo: http://jsfiddle.net/BlackLabel/jt8fL1zq/

@drmrbrewer If you need to disable this AST feature for further testing - the ugly hack continues in the top JS code lines: http://jsfiddle.net/BlackLabel/jt8fL1zq/1/

@drmrbrewer
Copy link
Author

drmrbrewer commented Feb 26, 2021

@KacperMadej thanks... I think the only viable workaround is to disable AST... as mentioned in the other issue I can't really see the value of AST in the first place, particularly if it can so easily just be disabled via a hack!

EDIT in retrospect the replacement of each normal space should be separator.replace(/ /g, '\u2008') not separator.replace(/ /g, '\u0020') since multiple \u0020 chars seem to be treated as just a single white space.

HC 8: http://jsfiddle.net/es54m2jf/ (working)
HC 9: http://jsfiddle.net/wfsjLpkq/ (broken)
HC 9: http://jsfiddle.net/k3hgmauq/ (working with hack to disable AST)

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Mar 4, 2021

I acknowledge this as a bug.

Minimal demo

https://jsfiddle.net/highcharts/gj9uv7wz/

I can't really see the value of AST in the first place, particularly if it can so easily just be disabled via a hack!

@drmrbrewer Our aim with the AST and allow-lists is to stop XSS coming in from string configuration in chart options. When you have full access to scripting in the page, you can insert anything you'd like into the DOM, with or without Highcharts.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented Mar 4, 2021

I think the whitespace filtering is an attempt to port this check from v8:

// Trim to prevent useless/costly process on the spaces
// (#5258)
.replace(/^\s+|\s+$/g, '')

But whitespace in between elements have a meaning in HTML and SVG, so we need to preserve it for cases like this. Perhaps what we need to do instead is to just trim the HTML before parsing. Then the unit and visual tests will reveal unwanted side effects.

@drmrbrewer
Copy link
Author

drmrbrewer commented Aug 18, 2021

@TorsteinHonsi is this really fully fixed in 9.2.0 like it's claimed to be?

Maybe it's fixed for your "minimal demo" above:

HC 9.0.1: https://jsfiddle.net/q3hxut60/ (broken)
HC 9.2.0: https://jsfiddle.net/24ny7krb/ (working)

But here is a continuation of my earlier example series above:

HC 8.2.2: http://jsfiddle.net/es54m2jf/ (working)
HC 9.0.1: http://jsfiddle.net/wfsjLpkq/ (broken)
HC 9.0.1: http://jsfiddle.net/k3hgmauq/ (working with hack to disable AST)
HC 9.2.0: http://jsfiddle.net/tdpz5sfr/ (still broken?)

@TorsteinHonsi
Copy link
Collaborator

Thanks for the additional case! Here's a "minimal demo" of your complex case: https://jsfiddle.net/highcharts/6nLq38eb/

The problem seems to arise with nested structures, so if you remove the outer div, it works as expected.

@drmrbrewer
Copy link
Author

Trouble is that in my actual code I'm building up the title dynamically from a number of different sources, each with different colour/style, as I've tried to simulate in my demo. Perhaps the outer div is itself doing something useful, and can't just be removed:

http://jsfiddle.net/thomq2f1/

Bottom line is that, if you disable AST then it works even with messy, perhaps less-than-optimised html:

http://jsfiddle.net/6Lzayqxr/

So there must be a way in which the behaviour of AST can be modified further to avoid judging a user's sub-optimal use of a complicated nested html structure? :-)

@TorsteinHonsi
Copy link
Collaborator

Yes, I acknowledge the bug, just proposed a workaround. I'm looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants