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] Bugfix multi-escaped special characters #6223

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Apr 1, 2020

Brief summary of changes

Removed duplicate call to HTMLSpecialChars() causing double escaping on any instrument field with one of the following characters & < > "

The removal is justified since these fields are being escaped directly in the database class
https://github.com/aces/Loris/blob/master/php/libraries/Database.class.inc#L538

@ridz1208
Copy link
Collaborator Author

ridz1208 commented Apr 1, 2020

related to #6158

@maltheism
Copy link
Member

maltheism commented Apr 1, 2020

@ridz1208 I'm not a security wizard but I think everywhere that _getFilteredValue used & returning some data. It would have to be verified that removing the sanitization doesn't void potential security. @johnsaigle mentioned this as well.

So far I see getFilteredValue used only in LorisForm.class.inc on lines 578, 584, 604, 607, 615, 667, 689. I'm guessing all those functions that call it may return something that needs to be verified that removing the sanitization doesn't add a vulnerability.

maybe that was already done in last issue?

@ridz1208
Copy link
Collaborator Author

ridz1208 commented Apr 1, 2020

@maltheism
#6158 (review)

@johnsaigle
Copy link
Contributor

Copying what I wrote in #6158 for clarity and easier reference:

After doing some research in earlier PRs and some basic testing, I think we can go ahead with this fix:
The htmlspecialchars() line was added in an earlier phase of LORIS. Right now, the extensive use of strict typing prevents many injection points that this previously addressed. Additionally, the use of ReactJS provides sanitization of output by default on many data points. Finally, HTMLEscapeArray in the DB class ensures that almost all data going into the database is escaped.
Removing this line may slightly increase the risk of XSS attacks but I think the risk is small and outweighed by the potential for data loss that it creates. It was always a bit of a hack and I think it's one that we can probably do without now.

@johnsaigle johnsaigle added Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security labels Apr 1, 2020
@driusan driusan merged commit 8e3f03c into aces:23.0-release Apr 2, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Apr 15, 2020
HenriRabalais pushed a commit to HenriRabalais/Loris that referenced this pull request May 27, 2020
Removed duplicate call to HTMLSpecialChars() causing double escaping on any instrument field with one of the following characters & < > "

The removal is justified since these fields are being escaped directly in the database class
https://github.com/aces/Loris/blob/master/php/libraries/Database.class.inc#L538
spell00 pushed a commit to spell00/Loris that referenced this pull request Jun 2, 2020
Removed duplicate call to HTMLSpecialChars() causing double escaping on any instrument field with one of the following characters & < > "

The removal is justified since these fields are being escaped directly in the database class
https://github.com/aces/Loris/blob/master/php/libraries/Database.class.inc#L538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants