Extend slash command '/topic' to display the room topic #2532

boeserwolf · 2019-01-30T11:21:17Z

If no topic is provided, the command will display a modal dialog containing the sanitized and linkified room topic. This is just adding some juice to make reading long room topics more convenient.

Signed-off-by: Bastian [email protected]

If no <topic> is provided, the command will display a modal dialog containing the sanitized and linkified room topic. This is just adding some juice to make reading long room topics more convenient.

Signed-off-by: Bastian <[email protected]>

into develop

dbkr

This generally looks excellent, thank you! We should sort out our linkification and sanitisation into one place as it doesn't really make sense to import linkifyMatrix in here, but it's what we do elsewhere. We also seem to vary in our sanitisation of text, ie. in TextualBody we also escape() the text before passing it to sanitizeHtml but in other places we don't.

Also a I think a QuestionDialog with no cancel button probably gives you a dialog that can't be dismissed with the escape key which is not quite what you want, but again it's what we do elsewhere so for now it's better to be consistent.

The one thing I would say is what happens when the room has no topic?

dbkr · 2019-01-31T11:03:26Z

ftr I've tracked our sanitisation / linkification issues in element-hq/element-web#8337

Signed-off-by: Bastian <[email protected]>

Display a default message if no room topic is present Signed-off-by: Bastian <[email protected]>

Signed-off-by: Bastian <[email protected]>

Add HtmlUtils.linkifyElement() Add HtmlUtils.linkifyAndSanitize() Refactor module imports Signed-off-by: Bastian <[email protected]>

Signed-off-by: Bastian <[email protected]>

boeserwolf · 2019-02-01T11:56:00Z

Hi Dave,

This generally looks excellent, thank you! We should sort out our linkification and sanitisation into one place as it doesn't really make sense to import linkifyMatrix in here, but it's what we do elsewhere. We also seem to vary in our sanitisation of text, ie. in TextualBody we also escape() the text before passing it to sanitizeHtml but in other places we don't.

I consolidated the linkification methods in HtmlUtils, but ignored escaping for the moment.

Also a I think a QuestionDialog with no cancel button probably gives you a dialog that can't be dismissed with the escape key which is not quite what you want, but again it's what we do elsewhere so for now it's better to be consistent.

"QuestionDialog" was replaced by a new Modal "InfoDialog", as you suggested. For closing the InfoDialog using the Keyboard, press the [Enter] key.

The one thing I would say is what happens when the room has no topic?

In that case the Modal now shows a message: "There is no room topic."

dbkr

Thanks for going the extra mile on this! Just a couple of fixes to your new stuff I'm afraid. (Also what I should have said is that it might be better to open a separate PR to add InfoDialog and bigger things like that, but let's carry on with this one: I think we're almost there).

dbkr · 2019-02-04T16:30:55Z

src/components/structures/RoomDirectory.js

@@ -438,7 +433,7 @@ module.exports = React.createClass({
            if (topic.length > MAX_TOPIC_LENGTH) {
                topic = `${topic.substring(0, MAX_TOPIC_LENGTH)}...`;
            }
-            topic = linkifyString(sanitizeHtml(topic));
+            topic = linkifyAndSanitizeHtml(topic);


So this was linkifyString(sanitizeHtml(..)) previously but linkifyAndSanitizeHtml() 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?

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. :-)

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 and dangerouslySetInnerHTML, 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).

src/components/views/dialogs/InfoDialog.js

src/components/views/verification/VerificationShowSas.js

src/SlashCommands.js

Signed-off-by: Bastian <[email protected]>

boeserwolf · 2019-02-06T18:59:06Z

Thanks for going the extra mile on this! Just a couple of fixes to your new stuff I'm afraid. (Also what I should have said is that it might be better to open a separate PR to add InfoDialog and bigger things like that, but let's carry on with this one: I think we're almost there).

Hey Dave, thanks for the review! Ok, I will consider in the future and create own PR for such changes. I hope now everything is fine.

Signed-off-by: Bastian <[email protected]>

Bastian added 7 commits January 30, 2019 12:18

Extend slash command '/topic' to display the room topic

bc6d13e

If no <topic> is provided, the command will display a modal dialog containing the sanitized and linkified room topic. This is just adding some juice to make reading long room topics more convenient.

Update i18n

5f90321

Merge branch 'develop' into boeserwolf/get-set-topic

135af40

Fix typo

8940934

Fix typo

b96a4a4

Signed-off-by: Bastian <[email protected]>

Merge branch 'develop' of https://github.com/boeserwolf/matrix-react-sdk

3a40982

into develop

Merge branch 'develop' into boeserwolf/get-set-topic

022dd14

dbkr reviewed Jan 31, 2019

View reviewed changes

Bastian added 5 commits January 31, 2019 17:57

Add InfoDialog

f245fa6

Signed-off-by: Bastian <[email protected]>

Replace QuestionDialog with InfoDialog

8273662

Display a default message if no room topic is present Signed-off-by: Bastian <[email protected]>

Update i18n

3e2bcd1

Signed-off-by: Bastian <[email protected]>

Add HtmlUtils.linkifyString()

9cd13a8

Add HtmlUtils.linkifyElement() Add HtmlUtils.linkifyAndSanitize() Refactor module imports Signed-off-by: Bastian <[email protected]>

Refactor to use HtmlUtils for linkifying and sanitizing

23971b3

Signed-off-by: Bastian <[email protected]>

dbkr reviewed Feb 4, 2019

View reviewed changes

Bastian added 4 commits February 6, 2019 19:27

Update copyright

4dcbb6f

Signed-off-by: Bastian <[email protected]>

Clean up InfoButton

7428e97

Signed-off-by: Bastian <[email protected]>

Reformat

d77f10e

Signed-off-by: Bastian <[email protected]>

Replace QuestionDialog with InfoDialog

aa0ae88

Signed-off-by: Bastian <[email protected]>

Fix error if topicEvents is undefined

951f0fc

Signed-off-by: Bastian <[email protected]>

jryans requested a review from dbkr February 7, 2019 08:57

Use default options from sanitizeHtmlParams for sanitizeHtml()

179f9a1

Signed-off-by: Bastian <[email protected]>

dbkr mentioned this pull request Feb 7, 2019

RoomDirectory tries to treat topics as HTML, but fails element-hq/element-web#8444

Closed

dbkr approved these changes Feb 7, 2019

View reviewed changes

dbkr merged commit b7fd133 into matrix-org:develop Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend slash command '/topic' to display the room topic #2532

Extend slash command '/topic' to display the room topic #2532

boeserwolf commented Jan 30, 2019 •

edited

Loading

dbkr left a comment

dbkr commented Jan 31, 2019

boeserwolf commented Feb 1, 2019 •

edited

Loading

dbkr left a comment

dbkr Feb 4, 2019

boeserwolf Feb 6, 2019

dbkr Feb 7, 2019

boeserwolf commented Feb 6, 2019

Extend slash command '/topic' to display the room topic #2532

Extend slash command '/topic' to display the room topic #2532

Conversation

boeserwolf commented Jan 30, 2019 • edited Loading

dbkr left a comment

Choose a reason for hiding this comment

dbkr commented Jan 31, 2019

boeserwolf commented Feb 1, 2019 • edited Loading

dbkr left a comment

Choose a reason for hiding this comment

dbkr Feb 4, 2019

Choose a reason for hiding this comment

boeserwolf Feb 6, 2019

Choose a reason for hiding this comment

dbkr Feb 7, 2019

Choose a reason for hiding this comment

boeserwolf commented Feb 6, 2019

boeserwolf commented Jan 30, 2019 •

edited

Loading

boeserwolf commented Feb 1, 2019 •

edited

Loading