Skip to content
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

RA1865 - Fixing reflected XSS in the notes section #39

Merged
merged 5 commits into from Sep 22, 2022

Conversation

varsha5595
Copy link
Contributor

Description of what I changed

@isears
Added input sanitization for Notes section in the Appoint Scheduling Form.

Issue I worked on

This fix ensures iframes are not inserted in the Notes section while scheduling appointments. The bug can be reproduced by following the steps mentioned here.

Link to ticket

https://issues.openmrs.org/browse/RA-1865

Checklist: I completed these to help reviewers :)

  • [x ] My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • [x ] I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • [ x] I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • [x ] All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • [x ] My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@varsha5595
Copy link
Contributor Author

@isears
I looked further into the UiUtils implementation of HTML Encoding and realized that they are directly calling the encodeHtml function. As I encountered problems while using the UiUtils class, I directly used the OWASP standard Encode.forHtml function to handle the XSS issue here.

I also looked into fixing this at the GSP level, but because the textarea has a two-way binding to a variable using ng-model, I was unsure how to go about it.

@varsha5595 varsha5595 changed the title Fixing reflective XSS in the notes section RA1865 - Fixing reflected XSS in the notes section Dec 2, 2020
pom.xml Outdated
@@ -133,6 +133,12 @@
<version>2.2</version>
</dependency>

<dependency>
<groupId>org.owasp.encoder</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't having the same library in multiple modules result into this problem? https://issues.openmrs.org/browse/TRUNK-5344

How about just relying on the one in core? https://github.com/openmrs/openmrs-core/blob/2.0.0/web/pom.xml#L116-L120

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 have changed the implementation to use the escapeHTML() from the already existing WebUtil class from the core.

Copy link
Member

@isears isears left a comment

Choose a reason for hiding this comment

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

In general, pulling escapeHTML from WebUtil is a really nice solution to the importing problem we've been having.

In this specific case, though, it looks like we're double-encoding at /openmrs/appointmentschedulingui/manageAppointments.page:
image

At which pages specifically do these XSS vulnerabilities show up? Appointment dashboard or patient dashboard? Is patching in the corresponding .gsp pages an option?

@brandones
Copy link
Contributor

This looks fine to me. @isears ?

mogoodrich and others added 2 commits September 22, 2022 10:07
…ointmentRequest.java

Co-authored-by: Brandon Istenes <brandonesbox@gmail.com>
…ointmentRequest.java

Co-authored-by: Brandon Istenes <brandonesbox@gmail.com>
@mogoodrich mogoodrich merged commit 2ccbe39 into openmrs:master Sep 22, 2022
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants