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

[LorisForm] Fix quotation display in text elements #7490

Conversation

CamilleBeau
Copy link
Contributor

@CamilleBeau CamilleBeau commented Jun 18, 2021

Brief summary of changes

This PR escape quotations in the text element HTML rendering so that the value will not get cut off, and will display properly.

Testing instructions (if applicable)

  1. Go to an instrument with text element fields (not including text area element since the issue did not apply to those elements)
  2. Outside of this PR, attempt to save a value in the text element that has quotations in it.
  3. Notice that it does not display properly after clicking "save"
  4. check out this PR
  5. Attempt again to save a value with quotations in it

Link(s) to related issue(s)

@ridz1208
Copy link
Collaborator

@CamilleBeau I have only given a very quick look over this but I potentially see a few issues.

  1. how is this issue only applying to " how are <, > and & being transmitted to the front end? coded or no?
  2. why are your " stored as " and not &quot? are they " in the database or are they being decoded before getting there ? i know we used to have the opposite issue but I still dont think it should ever get to that code section without being encoded !! huge security hole.

@CamilleBeau
Copy link
Contributor Author

CamilleBeau commented Jun 18, 2021

@ridz1208

@CamilleBeau I have only given a very quick look over this but I potentially see a few issues.

  1. how is this issue only applying to " how are <, > and & being transmitted to the front end? coded or no?

Yes all of the special chars are in plaintext at this point, so they display fine on the front end. The problem with the quotation mark is that the value is set in quotation marks in the HTML element, so having a plaintext quotation mark in the value tricks it into thinking the value is done, and the rest of the value is not considered (e.g. <input name="pulmonary_issues_specific" type="text" class="form-control input-sm" value="testing mid-"quotes"" > ).

  1. why are your " stored as " and not &quot? are they " in the database or are they being decoded before getting there ? i know we used to have the opposite issue but I still dont think it should ever get to that code section without being encoded !! huge security hole.

They are stored as &quot; and then decoded back to " sometime before this point

@ridz1208
Copy link
Collaborator

ridz1208 commented Jun 18, 2021

Yes all of the special chars are in plaintext at this point, so they display fine on the front end. The problem with the quotation mark is that the value is set in quotation marks in the HTML element, so having a plaintext quotation mark in the value tricks it into thinking the value is done, and the rest of the value is not considered (e.g. <input name="pulmonary_issues_specific" type="text" class="form-control input-sm" value="testing mid-"quotes"" > )

Thats a huge red flag. if someone stores a value like " <script>something_malicious.js</script>... you just lost. so its not fine for the other characters, its a problem for all of them.

They are stored as &quot; and then decoded back to " sometime before this point

I think you should investigate at what point that is instead of adhoc re-encoding them at this level

@CamilleBeau
Copy link
Contributor Author

@ridz1208 Ok yes good point.. I tried looking into where they were being decoded and I'm not sure what's going on here. From what I can tell, when the page is loaded the values are stored into defaultValues of LorisForm where they are encoded, but when getValue is called in rendering the html elements, the values are obtained through $_REQUEST and they are decoded. This is confusing to me because the $_REQUEST is getting the value from defaultValues and there is no other place where defaultValues is modified from what I can tell.

@CamilleBeau
Copy link
Contributor Author

@driusan @ridz1208 PLEASE NOTE: The current changes in this PR are not meant to be the actual solution or merged, but are meant to demonstrate the problem / what is needed for a solution. Whether the values need to be encoded / decoded right now depends on whether it is from $_REQUEST or default values, and whether it is being loaded or saved. What is in the PR changes right now is a very hacky, and unadvised way of fixing this, but that has the intended functionality. A cleaner and safer solution is needed.

@ridz1208
Copy link
Collaborator

ridz1208 commented Nov 9, 2021

closed replaced by linked PRs

@ridz1208 ridz1208 closed this Nov 10, 2021
driusan pushed a commit that referenced this pull request Nov 17, 2021
…#7776)

This fixes the escaping issue that occurs when a text field in an instrument contains an HTML special character. The solution employed here is to redirect on successful save to reload data directly from the database instead of loading it using the _POST data.

This solution is sub-optimal because the escaping issue will still occur when an error is detected on instrument save and the values MUST be reloaded from the _POST array where the reload can not occur to avoid losing the unsaved data.

Alternate to: #7777
Fixes #7489
Replaces #7490
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants