Hi,
I am a newbie to openmrs and would like to contribute. I found this [1] at issue-tracker.
The org.apache.commons.lang.StringEscapeUtils is also a solution for this. But there are some issues which are introduce in [1] when used this.
OWASP Java Encoder [2] is recently used widely to prevent XSS attacks. I am implementing it in openmrs at the moment to give it a try. This has worked for in my work so far.
Will it be a problem if I introduce new package in to code?
Any known facts, information and advices are welcome on this.
I found out that there is the org.openmrs.web.WebUtil class to help with HTML encoding for jsps. But the escapeHtml method is the only one related to the context.
If we have the intention of keeping everything within our code, we can implement the effective methods in WebUtil.
But considering the fact that OWASP Encoder is opensource, isn’t it better to move on to OWASP Encoder.
What is preffered?
Rewrite the relevant WebUtil methods to delegate to OWASP’s version
Assuming that the OWASP encoder’s methods have convenient and intuitive method signatures, I would deprecate our WebUtil methods and point to the OWASP versions. But if those methods aren’t intuitive, we can have “easier” versions of them in WebUtil that delegate.
I have sent a PR for reporting module at [1]. But it was before getting the advices from you guys. You can review it and let me know if any thing is wrong with that. Also you can update the jira [2]
I will rewrite the relevant WebUtil.escapeHtml method to delegate to OWASP’s version. Also I will add the other methods delegating to the OWASP’s version.
I think OWASP encoder’s method signatures are convenient and intuitive. But I am just a newbie. So we can discuss. FYI the Api for OWASP Encoder is at [3]
I am thinking about adding the same functionalities for a javascript module to use to encode JavaScript variables more efficiently and contextually. How about that?
I have sent a PR at [1] for openmrs-core, introducing contextual encoding methods in WebUtil class, delegating to OWASP encoder.
The PR at [2] was sent before implementing [1]. If [1] is ok to be merged, then we can use those methods in code. But as @darius suggested, this is needed only if OWASP encoder’s methods are not that much convenient and intuitive. I think [2] is a good example using reporting module to elaborate the usage of OWASP Encoder methods in openmrs code.
Based on the above decision, we can move on with OWASP Encoder methods or WebUtil methods to prevent XSS attacks in openmrs.
Hi darius,
I have added the OWASP encoder to our codebase. I have introduced new methods in WebUtil [1] methods to delegate to OWASP’s version. Now anyone can use those methods to encode the variables contextually.
I am wondering whether we have to update the Wiki page. Is [2] needed to be updated or are there any other suitable place?
Yes go ahead and update the wiki. If you mention the WebUtil methods, then you also need to mention that they are available from platform 2.0 and above. Else one would need to include the owasp jars.
I feel explaining all the methods in the [1] is not appropriate. Is there any particular wiki where WebUtil methods are explained or shall I create new one?
I updated the Wiki. Shall we deprecate the method escapeHtml() method in there or we keep it as it is? This method is named as escapeHtml to preserve backward compatibility. But I hope there is no issue of keeping that name. At the same time there may be a slight misunderstanding with the name being different from others while the functionality is delegated to OWASP Encoder same like other “encodeForSomeContext” type methods.
So we can discuss. FYI the Api for OWASP Encoder is at [3]