Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Basic UI: escape HTML characters #3749

Merged
merged 1 commit into from Jul 3, 2017
Merged

Basic UI: escape HTML characters #3749

merged 1 commit into from Jul 3, 2017

Conversation

lolodomo
Copy link
Contributor

Fix #3744

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo
Copy link
Contributor Author

@resetnow : this one is for you ,)
All my tests are positive.

Copy link
Contributor

@vlad-ivanov-name vlad-ivanov-name left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my remark, otherwise LGTM

@@ -82,7 +82,8 @@ public boolean canRender(Widget w) {
String button = getSnippet("button");
button = StringUtils.replace(button, "%item%", w.getItem());
button = StringUtils.replace(button, "%cmd%", mapping.getCmd());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need escape here as well. Test case:

Switch item=DemoString mappings=["value__1"="description_1", "value_\"_2"="description_2"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, using StringEscapeUtils.escapeHTML here is probably not the right choice — only quotes and backslashes need to be prepended with \ so resulting HTML attribute has correct value when it's being read from Javascript.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about mappings from number or ON/OFF/OPEN/CLOSE... for example but not from a general string.
You're right, it has to be tested and fixed for general string commands too.

Fix #3744

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 2, 2017

Fixed.

Copy link
Contributor

@vlad-ivanov-name vlad-ivanov-name left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sjsf sjsf merged commit becd5fe into eclipse-archived:master Jul 3, 2017
@sjsf
Copy link
Contributor

sjsf commented Jul 3, 2017

Thanks!

@lolodomo lolodomo deleted the basic_escape branch July 3, 2017 15:03
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
@kaikreuzer kaikreuzer added the bug label Dec 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants