-
Notifications
You must be signed in to change notification settings - Fork 435
Centralize DOMPurify sanitizer #1488
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
base: development
Are you sure you want to change the base?
Centralize DOMPurify sanitizer #1488
Conversation
…ss/failure responses
huss
left a comment
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.
Thanks to @tnigro45 & @Landon-Wivell for this contribution. I've made a number of comments to address. I'm going to delay full testing until these are addressed. Please let me know if you need anything.
huss
left a comment
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.
Thanks to @Landon-Wivell & @tnigro45 for addressing the comments. Review found it addressed most comments. I updated a couple of comments and added a new one. I will be able to do full testing once the install issue is resolved. Please let me know if you need anything.
| "csv": "~5.3.2", | ||
| "csv-stringify": "~5.6.5", | ||
| "d3": "~7.8.5", | ||
| "dompurify": "~3.2.6", |
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 update to dompurify and jsdom is not reflected in the package-lock.json file. Since OED does not automatically update package-lock.json on install, it must be done as part of the changes. When I tried to do docker compose up with this version it failed due to this. This had been done for the previous addition in this PR. Thus, can you carefully update package-lock.json where it only does what is needed for the two updated packages (not a general npm install). Please let me know if you have questions or need help with this.
|
@Landon-Wivell & @tnigro45 It has been a few weeks and I have not seen any updates to the code. Thus, I wanted to touch base about this and see if you are working on it or I missed something. OED would like to integrate this work so we would appreciate knowing the status. If there is no communication by 8/15 then OED may move forward on this work. Thanks for your efforts to date. |
Description
Implemented DOMPurify to sanitize HTML responses from response.js and success.js. Created sanitizer.js as a centralized, pre-configured instance of DOMPurify on the server. This also allows for implementation of sanitization elsewhere if needed in the future. These changes remediate a cross-site scripting vulnerability when users upload meter data.
Developed and implemented by:
Thomas Nigro - https://github.com/tnigro45
Landon Wivell - https://github.com/Landon-Wivell
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
N/A