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.

Sign up for GitHub

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

Jump to bottom

[login] Add code sanitizer before dangerouslySetInnerHTML #7491

Merged
merged 2 commits into from
Nov 30, 2022
Merged

[login] Add code sanitizer before dangerouslySetInnerHTML #7491

merged 2 commits into from
Nov 30, 2022

Conversation

CamilleBeau
Copy link
Contributor

Brief summary of changes

This PR adds DOMPurify to sanitize the code that is passed through dangerouslySetInnerHTML to avoid XSS vulnerabilities.

Testing instructions (if applicable)

  1. Run npm install dompurify
  2. Before checking out this PR, edit and save the description field of GUI tab in the configuration module:
    <h3 onclick="alert('Attack message test')">Example Study Description</h3> (or something similar)
  3. Logout and try clicking on "Example Study Description" from the login page. You should see a message pop-up
  4. Checkout this PR and run make dev
  5. Try again to click on "Example Study Description" from the login page. You should now see no pop-up.
  6. Make sure that the intended html is still rendering (bold, titles, etc.)

@maltheism maltheism added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Oct 17, 2022
@maltheism
Copy link
Member

Hi @CamilleBeau, I can approve after the conflicts are fixed.

@maltheism maltheism self-requested a review October 17, 2022 18:43
@maltheism maltheism added Passed manual tests PR has been successfully tested by at least one peer and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Oct 31, 2022
@maltheism maltheism added the Category: Security PR or issue that aims to improve security label Nov 21, 2022
@CamilleBeau
Copy link
Contributor Author

CamilleBeau commented Nov 29, 2022 •
edited
Loading

@driusan @ridz1208 @maltheism Confirmed that benign html such as h4 tags still go through with dompurify. Ready for merge.

@driusan driusan merged commit b932880 into aces:main Nov 30, 2022
@laemtl laemtl mentioned this pull request Nov 30, 2022
Merged
driusan pushed a commit that referenced this pull request Dec 1, 2022
@laemtl
Add missing package in package-lock.json (#8246)
64926ed 
This fix the tests failure introduced by #7491, which was sent before the package-lock.json file was commited.
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
@CamilleBeau CamilleBeau mentioned this pull request Mar 22, 2023
Merged
1 task
driusan pushed a commit that referenced this pull request Mar 28, 2023
@CamilleBeau
[documentation] Add to CHANGELOG (#8473)
1f3bb49 
Add changes for PR #7491 and PR #7772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maltheism maltheism maltheism approved these changes

Assignees
No one assigned
Labels
Category: Security PR or issue that aims to improve security Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@CamilleBeau @maltheism @driusan @ridz1208