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

Render HTML contained in messages #379

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

kofrezo
Copy link
Contributor

@kofrezo kofrezo commented Sep 11, 2024

For some messages it is useful to render the HTML such as for example if you have multiple messages and want to display them as list.

For some messages it is useful to render the HTML such as for example if
you have multiple messages and want to display them as list.
@kofrezo kofrezo requested a review from zados September 11, 2024 13:23
@kofrezo kofrezo self-assigned this Sep 11, 2024
@kofrezo kofrezo requested a review from lamaral September 17, 2024 15:29
Copy link
Contributor

@lamaral lamaral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It LGTM.
I will wait to hear what @zados says, as I see a potential for XSS.

@zados
Copy link
Member

zados commented Nov 21, 2024

@lamaral is correct, this makes a XSS certainly possible, as we do not escape HTML anymore.
@kofrezo stated that all messages are supplied through the code itself and do not contain user input, therefore this is acceptable to me. However it has to be ensured that no user supplied input can make it's way here.

If this cannot be ensured or we need to put user input into the message, we should not merge this or revert this in the future should user input be required then.

@kofrezo kofrezo merged commit c4d6466 into main Nov 21, 2024
5 checks passed
@kofrezo kofrezo deleted the dk_allow_html_in_messages branch November 21, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants