Update DOMPurify Sanitizer#1591
Conversation
|
@Zach-O-Bates, @Zachary-Squires & @Quillen09 I wanted to check that I'm treating this correctly. This PR replaces PR #1544 and addresses any remaining comments in that PR, and also replaces PR #1488 (which the other PR already replaced and is closed). |
Yes that is correct, it should have all changes including the last comment from PR #1544. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zach-O-Bates, @Zachary-Squires & @Quillen09 Thank you for this contribution. I looked over PR #1544 and determined all items in that are addressed and the code is in this PR. Thus, I closed that PR. I made one comment for something I found during final testing. Two other comments:
- I found two places with success/failure methods that are not covered the way you sanitized in src/server/routes/obvius.js and src/server/services/csvPipeline/failure.js. They seem to use escapeHtml. Can you please look into this and determine/use a common methodology across the code. Also, it may be a good idea to test these functions similarly to what you did for the other functions.
- There are two failed tests on GitHub. They seem to relate to the sanitizing changes. "should handle null comment" may be simply how empty strings are handled. "should handle special characters in comments" may be more complex. Both made me a little nervous since some responses (such as from CVS upload via script) have been publicly documented so changing them may cause issues for sites. Can you please look into this and then we can talk to see if any changes in response need to be rethought.
Please let me know if anything is not clear or you have any thoughts.
| }); | ||
| expect(res).to.have.status(401); | ||
|
|
||
| xssIndicators.forEach(indicator => { |
There was a problem hiding this comment.
I looked at the res.text and it would never have the username as it only indicates with "Not authorized". Thus, this isn't a good test of sanitization. I think a different route should be used.
|
@Zach-O-Bates, @Zachary-Squires & @Quillen09 Thank you for your work on this issue. I know it is after the time you originally intended to work on OED. Given OED wants to finish this up & it has been several months since my last comment, I'm going to get someone else to complete this work unless you indicate by 6/15/26 that you intend to work on it and it is completed in the near future. |
2 participants
Description
Merges changes from PR #1488 and PR #1544. This PR fixes the issue where the update to DOMPurify and jsdom was not reflected in the package-lock.json file. It also addresses the modification issues mentioned in the previous PR. Now that DOMPurify is properly implemented the cross-site scripting vulnerability when users upload meter data should be remediated.
Developed and Implemented by:
Zachary Squires - https://github.com/Zachary-Squires
Zachary Bates - https://github.com/Zach-O-Bates
Krista Clanton - https://github.com/Quillen09
Type of change
Checklist
Limitations
N/A