Skip to content
This repository has been archived by the owner. 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

@lolodomo lolodomo commented Jun 26, 2017

Fix #3744

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

@lolodomo
Copy link
Contributor Author

@lolodomo lolodomo commented Jun 26, 2017

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

Copy link
Contributor

@resetnow resetnow left a comment

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

@resetnow resetnow Jun 27, 2017

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

@resetnow resetnow Jun 27, 2017

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

@lolodomo lolodomo Jun 28, 2017

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 lolodomo commented Jul 2, 2017

Fixed.

Copy link
Contributor

@resetnow resetnow left a comment

LGTM

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

@sjsf sjsf commented Jul 3, 2017

Thanks!

@lolodomo lolodomo deleted the basic_escape branch Jul 3, 2017
@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants