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

[Instruments] Escaping issue optimal - Only HTML escape on data retrieval #7777

Closed
wants to merge 4 commits into from

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Nov 4, 2021

Brief summary of changes

This PR fixes the escaping issue that occurs when a text field in an instrument contains an HTML special character. The solution employed here is to avoid HTML escaping special characters before database save and to instead escape them on form load (in the display logic).

Cons:

  • This solution is major and might have wider effects that predicted on any other form in loris still using lorisforms

Pros:

  • optimal permanent solution, not a temporary hack
  • Collateral advantage of preventing scripting in filter fields

Modules using LorisForm affected by changes (other than instruments):

  • examiners
  • help_editor
  • my_preferences
  • user_accounts
  • ...

Modules NOT using LorisForm (directly loading data) affected by changes: (added to https://github.com/aces/Loris/projects/27)

  • configuration
  • publication
    • This will need it's own ticket. Files allow some special characters that it shouldn't. some fields get HTML char decoded, some dont.
  • acknowledgements
  • bvl_feedback
  • candidate_parameters
  • data_release
  • document_repository
  • electrophysiology_browser
  • genomic_browser
  • issue_tracker
  • media (edit function only)

Modules NOT loading any data on forms (LorisForm or otherwise)

  • survey_accounts
  • imaging_uploader
    • Does not seem to allow special characters by constantly validating matches with PSCID,DCCID and visitlabel; no free text allowed

Found using search on

  • createText
  • addBasicText
  • addBasicTextArea
  • addTextAreaGroup (unused)
  • createTextArea (unused)
  • addTextAreaGroup (unused)
  • createDate (unnecessary)
  • addDate (unnecessary)
  • createStatic (unnecessary)
  • addStatic (unnecessary)
  • createTime (unnecessary/unused)
  • addTime (unnecessary/unused)
  • year (unnecessary/unused)
  • file (unnecessary/unused)
  • checkbox (unnecessary/unused)

Alternate to: #7776
Fixes #7489
Replaces #7490

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

Make sure to test other usecases as well

  1. behaviour when the form contains an error (triggering a reload with erroneous fields highlighted)
  2. behaviour when there are multiple ",<,> or & in the string
  3. behaviour when the instrument status sidebar is modified (is it affected by the reload)

Review:

  • Make sure all field types that should be escaped are escaped (select fields?)

Link(s) to related issue(s)

  • Resolves # (Reference the issue this fixes, if any.)

@ridz1208 ridz1208 added 24.0.0-bugs Issues or bug fix PRs that were raised during the testing of release 24.0.0 Category: Bug PR or issue that aims to report or fix a bug Cleanup PR or issue introducing/requiring at least one clean-up operation Critical to release PR or issue is key for the release to which it has been assigned State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed Category: Security PR or issue that aims to improve security labels Nov 4, 2021
@@ -53,6 +53,15 @@
$useObjects =true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating this tool cause it still might be used in this release by some projects and it was broken

@ridz1208 ridz1208 force-pushed the instrument_coding_real_fix branch 3 times, most recently from 713bded to ce6b4c9 Compare November 5, 2021 13:57
@ridz1208 ridz1208 removed the Critical to release PR or issue is key for the release to which it has been assigned label Nov 9, 2021
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Nov 15, 2021

closing for now. will require a substantial amount of work

@ridz1208 ridz1208 closed this Nov 15, 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
@ridz1208 ridz1208 reopened this Dec 1, 2021
@ridz1208 ridz1208 force-pushed the instrument_coding_real_fix branch 2 times, most recently from 7196d88 to b7d7b20 Compare December 1, 2021 18:45
@ridz1208 ridz1208 closed this Dec 2, 2021
@ridz1208 ridz1208 reopened this Dec 2, 2021
@ridz1208 ridz1208 force-pushed the instrument_coding_real_fix branch from b7d7b20 to ed19695 Compare January 27, 2022 20:53
@ridz1208
Copy link
Collaborator Author

@driusan I dont' know if there is a need for a script to remove already encoded data (it seems unlikely that any already exist outside of instruments)

so technically this is ready for review

@ridz1208 ridz1208 removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Jan 28, 2022
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Nov 4, 2022

I just dont think this is gonna happen and no issues have recently poped up about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
24.0.0-bugs Issues or bug fix PRs that were raised during the testing of release 24.0.0 Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security Cleanup PR or issue introducing/requiring at least one clean-up operation State: Needs changelog update PR that needs an update in the CHANGELOG file to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LorisForm] HTML Text Element does not display quotes
2 participants