-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Hide spoilers from desktop notifications #31699
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: develop
Are you sure you want to change the base?
Hide spoilers from desktop notifications #31699
Conversation
| } | ||
|
|
||
| try { | ||
| const parser = new DOMParser(); |
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.
I think this needs to use more of the same rendering stack we use for the timeline to sanitise the HTML before processing
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.
My (admittedly limited) understanding is that the existing rendering stack is quite consolidated: event goes in, rendered html comes out. The problem is that we now need two different output formats:
- The existing format where spoilers are wrapped in
<span data-mx-spoiler>tags containing the actual spoilered text - The new format where the spoilered text is replaced with
[Spoiler]
I see roughly four different solutions:
- Break up the consolidated rendering stack to be more modular, so that call sites have more control over the ouput format. This seems more complex than what I'm comfortable with as an external contributor.
- Add a boolean parameter to the existing rendering function, to switch between behaviors.
- Remove the spoilers from the generated html without actually parsing html, using
replaceAllor (God forbid) some regex-based solution.
None of these are appealing to me, which is why I chose approach I ended up with. It's quite possible I'm missing an obvious solution, though, since I'm not familiar with the code base. What are your thoughts?
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.
I think it could do with some comments around the justification, and that only the textContent is seemingly safe to use, to avoid xss injection attacks given the untrusted input html
When receiving a message containing a spoiler, the desktop notification shows the spoilered text as plain text, defeating the point of the spoiler. This PR aims to fix that.
Fixes #12034
Checklist
public/exportedsymbols have accurate TSDoc documentation.