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

Escape html places #1263

Merged
merged 4 commits into from Nov 26, 2018

Conversation

Projects
None yet
2 participants
@cmdcolin
Copy link
Contributor

cmdcolin commented Nov 18, 2018

@garrettjstevens This is sort of a modification to to the escaping/unescaping approach that was going on.

If you want to test and give feedback let me know!

@wafflebot wafflebot bot added the in progress label Nov 18, 2018

@cmdcolin cmdcolin force-pushed the escape_html_places branch from 2515c2d to 30ae4d6 Nov 18, 2018

@cmdcolin cmdcolin force-pushed the escape_html_places branch 2 times, most recently from a8c5126 to 6ba3cc0 Nov 19, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Nov 19, 2018

I have changed the approach to make it explicit when we want to escape HTML by default on all values but by it enables unsafe HTML rendering if the "fmtDetailValue" or "fmtMetaValue" config is used as this is often used for creating custom HTML in a popup

Therefore this allows the behavior or customization with HTML when users have an advanced config but safe behavior by default

@cmdcolin cmdcolin force-pushed the escape_html_places branch 3 times, most recently from 1e03e13 to 5ff43d2 Nov 19, 2018

@garrettjstevens

This comment has been minimized.

Copy link
Contributor

garrettjstevens commented Nov 24, 2018

@cmdcolin the idea of escaping by default unless one of those configs is present. I think I found a couple places where unescaped HTML might be coming through, though. It looks like it's probably happening in the mouseovers for CanvasFeatures and the description for HTMLFeatures, since the <CN0> and <CN2> are missing:
screenshot from 2018-11-24 07-56-06

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Nov 24, 2018

I think that's true, mouseover tooltips do allow html. Another place would be like the literal htmlfeatures feature labels. There are probably a lot of places and it's a little treacherous....I am imagining that unsafe html will possibly be better managed in react land..

@cmdcolin cmdcolin force-pushed the escape_html_places branch from 5ff43d2 to 7791809 Nov 24, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Nov 24, 2018

I added some changes to allow explicit unsafeMouseover, unsafePopup, and unsafeHTMLFeatures. Some of this is slightly breaking if someone assumed that HTML could be safely displayed in these areas. When fmtDetailValue callbacks are used it is automatically assumed unsafe on a specific field in the popups. Otherwise you have to manually set unsafePopup on the track config.

@garrettjstevens

This comment has been minimized.

Copy link
Contributor

garrettjstevens commented Nov 25, 2018

Looks great to me! I think React just assumes that you need to escape everything. If you try to use unescaped text it triggers an eslint rule (see below). So with JB2 we might just need to say that everything is going to be interpreted as HTML so it should already be escaped. Plus with JSX syntax turned on you pretty much have to escape things like < and > or else you'll get syntax errors.
image

@cmdcolin cmdcolin added this to the 1.16.0 milestone Nov 26, 2018

@cmdcolin cmdcolin merged commit 17b5673 into dev Nov 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 26, 2018

@cmdcolin cmdcolin deleted the escape_html_places branch Nov 26, 2018

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