-
Notifications
You must be signed in to change notification settings - Fork 72
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
3.1.1.8 Resolve Manage Patient Identifier Sources from XSS attack #123
Conversation
@slubwama is this still draft because you are not yet done with it? |
We probably also need to be sure to escape things on the display end too. Otherwise, it remains trivial to, e.g., submit the request via the REST API… |
@ibacher Done |
@@ -6,6 +6,21 @@ | |||
<%@ include file="/WEB-INF/template/header.jsp"%> | |||
<%@ include file="localHeader.jsp"%> | |||
|
|||
<script type="text/javascript"> | |||
function beforeSubmit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect this to be named sanitizeAndSubmit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkayiwa I have just changed the method call to beforeSubmit to allow flexibility in the future. In case there is a need of more client side controls on the form before submission
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original method name was better because it described what exactly the method is doing.
It is not a good practice to sacrifice clarity for a possible future which in most cases turns out not to happen. 😊
If that future comes and we find that we need to rename/refactor, let it happen then, but not now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkayiwa corrected to original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other question is, did you actually test this? The reason i am asking is simply because the method name was different from the one you had for the onSubmit attribute. So i would have expected you to get errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkayiwa I had tested it, and before pushing, I renamed the method to beforeSubmit. That’s how it ended up breaking. I forgot that this wouldn’t rename properly, since the method call in the form tag was being recognized as a string rather than an actual method call in IntelliJ. As a result, the automatic rename didn’t work.
</tr> | ||
<tr> | ||
<th align="right" valign="top"> | ||
<span class="requiredField">*</span> | ||
<spring:message code="idgen.firstIdentifierBase" />: | ||
<spring:message id="baseCharacterSet" code="idgen.firstIdentifierBase" />: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this seems to produce the error:
org.apache.jasper.JasperException: /WEB-INF/view/module/idgen/editIdentifierSource.jsp (line: [92], column: [5]) Attribute [id] invalid for tag [message] according to TLD
No description provided.