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

Remove some uses of "dangerouslySetInnerHTML" #1407

Merged

Conversation

Antonio-Maranhao
Copy link
Contributor

@Antonio-Maranhao Antonio-Maranhao commented Jul 28, 2023

Overview

The dangerouslySetInnerHTML React property was being used by Notification components to allow formatting of the message by embedding HTML elements directly in the message. This flexibility can become a security issue because messages might include data provided by users (e.g. document IDs), in which case it can be used for HTML injection.

This PR removes uses of dangerouslySetInnerHTML in the Notification components in favor of embedding the input msg as a normal React node, which then is properly sanitized by React.

Testing recommendations

Test notifications: e.g. create or delete a document and validate the notification is still displayed.

GitHub issue number

n/a

Related Pull Requests

n/a

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
  • Update rebar.config.script with the correct tag once a new Fauxton release is made

@pgj
Copy link

pgj commented Jul 28, 2023

Nitpick: the name of the React property misses an 's' in the PR description and title sometimes, i.e. written as dangeroulySetInnerHTML.

@Antonio-Maranhao Antonio-Maranhao changed the title Remove some uses of "dangeroulySetInnerHTML" Remove some uses of "dangerouslySetInnerHTML" Jul 28, 2023
@Antonio-Maranhao
Copy link
Contributor Author

Fixed. Thanks @pgj

@Antonio-Maranhao Antonio-Maranhao merged commit c486e1d into apache:main Jul 28, 2023
3 checks passed
@Antonio-Maranhao Antonio-Maranhao deleted the remove-dangeroulysetinnerhtml branch July 28, 2023 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants