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

EMPT-74. Fix XSS vulnerability on the manage privilege page #58

Merged
merged 3 commits into from Apr 1, 2021

Conversation

jmmacdo4
Copy link
Contributor

Used HTML encoding to fix XSS vulnerability on privileges name field

@jmmacdo4
Copy link
Contributor Author

@isears pretty simple XSS fix here let me know what you think.

@@ -71,8 +71,8 @@
initialValue : ui.encodeHtmlContent(privilege.privilege)
])}
<% } else{ %>
<b>${ui.message("general.name")}:</b> ${privilege.privilege}
<input type="hidden" name="privilegeName" value="${privilege.privilege}" />
<b>${ui.message("general.name")}:</b> ${ui.encodeHtmlContent(privilege.privilege)}

Choose a reason for hiding this comment

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

hi @isears does this bring much difference than using <c:out value='${privilege.privileget}'/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question Herbert, I can do either.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can use <c:out> tags outside of .jsp files

@@ -71,8 +71,8 @@
initialValue : ui.encodeHtmlContent(privilege.privilege)
])}
<% } else{ %>
<b>${ui.message("general.name")}:</b> ${privilege.privilege}
<input type="hidden" name="privilegeName" value="${privilege.privilege}" />
<b>${ui.message("general.name")}:</b> ${ui.encodeHtmlContent(privilege.privilege)}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can use <c:out> tags outside of .jsp files

<b>${ui.message("general.name")}:</b> ${privilege.privilege}
<input type="hidden" name="privilegeName" value="${privilege.privilege}" />
<b>${ui.message("general.name")}:</b> ${ui.encodeHtmlContent(privilege.privilege)}
<input type="hidden" name="privilegeName" value="${ui.encodeHtmlContent(privilege.privilege)}" />
Copy link
Member

Choose a reason for hiding this comment

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

ui.encodeHtmlAttribute is better suited here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@isears Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, should have been a little more clear. encodeHtmlContent is fine on line 74, but encodeHtmlAttribute is better suited for line 75 (because it's an HTML attribute rather than raw HTML).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, fixing now

Copy link
Contributor Author

@jmmacdo4 jmmacdo4 Apr 1, 2021

Choose a reason for hiding this comment

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

Great that should now be fixed. Thanks for explaining that @isears definitely makes sense.

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.

Great, LGTM!

@isears isears merged commit 4f85654 into openmrs:master Apr 1, 2021
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
3 participants