This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend slash command '/topic' to display the room topic #2532
Extend slash command '/topic' to display the room topic #2532
Changes from all commits
bc6d13e
5f90321
135af40
8940934
b96a4a4
3a40982
022dd14
f245fa6
8273662
3e2bcd1
9cd13a8
23971b3
4dcbb6f
7428e97
d77f10e
aa0ae88
951f0fc
179f9a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
So this was
linkifyString(sanitizeHtml(..))
previously butlinkifyAndSanitizeHtml()
does it the other way around:sanitizeHtml(linkifyString(...))
: I suspect this is probably fine but obviously if it's wrong it's a very big deal. Is there a reason to change it?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.
For security reasons, the specified (insecure) string should always be sanitized last. F. e., someone could figure out how to do nasty things during linkification, thus bypassing the HTML sanitizer completely. Anyways, linkifyString() should always produce valid HTML that passes the sanitizer.
Of course, I could be on the wrong track here, so please feel free to correct me. :-)
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.
Good thought, although in practice
linkifyString
happens to escape HTML as it's intended for parsing plaintext strings, so this will end up escaping all HTML attributes.So Riot, by doing it the other way around, was also sanitising the HTML and then completely escaping it, meaning no HTML could actually be used, so you found a bug there.
In a further plot twist, the topic is not actually HTML at all: I don't know why RoomDirectory was using
sanitizeHtml
anddangerouslySetInnerHTML
, since in RoomHeader, it's just escaped normally as a plain text string and linkified. However, because of the above bug, RoomDirectory was escaping the sanitised HTML and ended up being consistent with the room header...Sorry you've unwittingly got tangled up in this mess: If we were to fix this, I would probably make a
LinkifedText
component which takes the linkification out of https://github.com/matrix-org/matrix-react-sdk/blob/master/src/components/views/rooms/RoomHeader.js#L307 / https://github.com/matrix-org/matrix-react-sdk/blob/master/src/components/views/rooms/RoomHeader.js#L81 ie. using React's normal string escaping. and then pointing linkify at the DOM element.That said, this PR has the same behaviour as before so we could also merge this and then fix it later: you've put more than enough work in to this PR!
One more thing: your parameter to
linkifyAndSanitizeHtml
isn't used in either of the calls: I would remove it and always use our sanitisation options (it's also a bug in Riot that it was using sanitize-html's default rather than our own, but it didn't matter anyway because the HTML all got escaped anyway).