SORMAS-Foundation / SORMAS-Project

SORMAS (Surveillance, Outbreak Response Management and Analysis System) is an early warning and management system to fight the spread of infectious diseases.
https://sormas.org
GNU General Public License v3.0
293 stars 143 forks source link

EVENT : List of actions unreadable (code elements displayed in the middle of the information entered) #3193

Closed carolinverset closed 4 years ago

carolinverset commented 4 years ago

Bug Description

In the event part, when we use the editor component to formalize the different stages of the investigation around the event, there is a bug on the screen rendering. Pieces of code appear in the middle of the information entered, affecting the readability and understanding of the formalized actions.

Steps to Reproduce

  1. In event, create an action
  2. Change the font or underline elements
  3. Save the information
  4. Look at the list of actions

Expected Behavior

Screenshots

image

System Details

Additional Information

@bernardsilenou @ftavin @olivierleuci @xavier-calland

xavier-calland commented 4 years ago

it seems to come from https://github.com/hzi-braunschweig/SORMAS-Project/commit/3e619bb5bab8fd6478f7cc4d010ea3cad1d99326

https://github.com/hzi-braunschweig/SORMAS-Project/blob/3e619bb5bab8fd6478f7cc4d010ea3cad1d99326/sormas-ui/src/main/java/de/symeda/sormas/ui/action/ActionListEntry.java#L81-L85

MateStrysewske commented 4 years ago

@xavier-calland You're right. We'll have to discuss how to handle this specific situation in terms of XSS prevention.

DavidBaldsiefen commented 4 years ago

This seems to be related to #3220 . The problem is likely that the html is escaped at multiple levels. A helper class htmlhelper.javahas been added with PR #3240 , which allows the programmer to explicitly unescape specific tags (like <b> and <i>). Calling this would probably be enough to have the html displayed correctly again. I can test this solution.

DavidBaldsiefen commented 4 years ago

I've looked closer at the issue, is there a specific reason why users even get the ability to format Event Action descriptions to such a degree? I haven't seen anything like it anywhere else in SORMAS and don't really understand why selecting fonts, background colour and text size is necessary here. It seems a little out of place. Maybe simple formatting like bold, cursive, underline and unordered lists are enough? @MateStrysewske

ftavin commented 4 years ago

@Seal33 the user story for actions is here : #2434

ftavin commented 4 years ago

@bernardsilenou @MateStrysewske do you still plan to have this bug corrected for 1.51 ? (I'm not sure why you added the "fr" tag to the issue)

DavidBaldsiefen commented 4 years ago

The issue is relatively complicated, as our measures to prevent XSS collide with the RichTextArea used to get the user's input on Actions. You can read more here and here

Especially this:

Notice that the problem applies also to user input from a RichTextArea is transmitted as HTML from the browser to server-side and is not sanitized. As the entire purpose of the RichTextArea component is to allow input of formatted text, you can not just remove all HTML tags. Also many attributes, such as style, should pass through the sanitization.

Thus, I believe it requires a little bit more work than usual to solve this issue. I guess it's unlikely that we can implement this bugfix for 1.51 without using some ugly workarounds

DavidBaldsiefen commented 4 years ago

Maybe we could create a local helper class, which can replace VAADINs RichTextArea wherever needed, and is automatically sanitized to prevent XSS. In that case I would only allow the most common types of formatting: bold, italics, underline, lists But I guess this does require quite some additional work.

MateStrysewske commented 4 years ago

I agree with @Seal33, this will probably have to be moved to the next sprint because it's not that easy to fix.

DavidBaldsiefen commented 4 years ago

I've further investigated this issue and stumbled upon Jsoup.clean. This method allows for very easy sanitizing of html inputs, and the Whitelist class allows to allow custom tags (see here). I've tested it on this issue and it works quite well. I would actually suggest to start using Jsoup.clean(string, Whitelist.basic()) everywhere where we currently use StringEscapeUtils.escapeHtml4. This would also allow us to revert PR #3220 and create a "cleaner" solution to the XSS-Escaping issues we ran into in multiple parts of the code. If you approve, I would open another issue for that and work on this solution from there @MateStrysewske

bernardsilenou commented 4 years ago

@MateStrysewske Can you followup on this please, the users asked about it

MateStrysewske commented 4 years ago

@Seal33 Sounds good to me!

carolinverset commented 4 years ago

@ftavin @bernardsilenou @MateStrysewske I confirm and really stress the importance of having a very quick solution for this problem. thank you really for all you can do! Best regards

DavidBaldsiefen commented 4 years ago

@carolinverset @MateStrysewske I've added a hotfix for this issue under PR #3402 as it seems quite urgent. Further discussion of this topic as a more general issue will take place in #3389 as mentioned earlier.

carolinverset commented 4 years ago

@Seal33 thank you !!!!! @MateStrysewske can I dare to ask if there is a chance that the bug will be fixed in version 1.51?

MateStrysewske commented 4 years ago

@carolinverset Unfortunately the release process has already been started yesterday, so I'm afraid that won't be possible anymore

carolinverset commented 4 years ago

@MateStrysewske @bernardsilenou @olivierleuci @ftavin Ok i understand.. Perhaps it is possible to have it in a possible version 1.51.1 ? You see :-), I am trying everything so much this bug may pose a problem in France where everyone relies heavily on the monitoring of the actions of events.

Sorry for being so insistent, but unfortunately I have no choice :-)

Thank you for all !

MateStrysewske commented 4 years ago

@carolinverset If there's a need for a hotfix, I'll make sure to integrate this fix in it, yes! And I totally understand that you need to be insistent on these things, no problem. Maybe you can instruct the people working with SORMAS to only write plain text for now until this issue has been resolved so that at least new actions created are readable - even if that's not optimal of course.

DavidBaldsiefen commented 4 years ago

@carolinverset For now you can also see the readable, formatted text of any action by clicking on the "edit"-button next to the action.

carolinverset commented 4 years ago

@Seal33 thank you for the tip, I will communicate it to the French teams