-
-
Notifications
You must be signed in to change notification settings - Fork 833
Fix escaping markdown by rendering plaintext #622
Conversation
Can one of the admins verify this patch? |
We still need to parse "plaintext" messages through the markdown renderer so that escappes are rendered properly. Fixes element-hq/element-web#2870. Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
ddd7467
to
893a5c9
Compare
(This is a WIP at the moment, will take care of the errors) |
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
Should be ready fro reviewing now! |
Oh, and I just realized that I accidentally got c819b43 in there, but it's not directly related to this PR, it just fixes the old messageinput component to use the new markdown render function. I can split that out into its own PR if you want. |
Okay, this isn't quite ready for merging yet, there are some more functions that need to be nulled-out, eg |
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
And now that is fixed! This should probably be tested out for a little while by more than just me before this is merged, to try to catch any other possible edge cases. |
@@ -56,23 +57,47 @@ export default class Markdown { | |||
return is_plain; | |||
} | |||
|
|||
toHTML() { | |||
render(html) { |
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.
Please don't call this render
, as it is confusing and can make readers mistake the Markdown
class as a React class.
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.
Also, it is not at all clear that html
is a boolean
. Please document what it is supposed to do.
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.
What do you want me to rename it to? toHTML
isn't really an apt name anymore with this.
EDIT: I guess I can revert this to be separate functions actually.
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.
Given most of this function is branching based on this conditional, separate functions sounds excellent, as it will also make it clearer at the call site what you're actually doing toHTML
or toPlaintext
or whatever.
real_paragraph.call(this, node, entering); | ||
} else { | ||
renderer.out = function(s) { | ||
this.lit(s); |
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.
Uhh, what is this doing? No comments.
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.
Ah, seems I only added the comment in the commit message. Will fix that. The normal out
function runs the output through a sanitizer that will entity encode things like quotation marks, which is problematic when it happens multiple times.
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.
Thanks for the explanation. What's lit(s)
doing though?
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.
This should probably be tested out for a little while by more than just me before this is merged, to try to catch any other possible edge cases.
Please add some unit tests for this. This should be pretty easy to do given how modular the Markdown
class is.
Unit tests for what exactly? Just the issue I found ( |
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
The point is to test that the
|
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
Split the functions and added some test cases now. It seems like while commonmark doesn't strip |
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.
Other than 2 things, this LGTM!
@@ -56,12 +59,10 @@ export default class Markdown { | |||
return is_plain; | |||
} | |||
|
|||
toHTML() { | |||
const parser = new commonmark.Parser(); | |||
toHTML(html) { |
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.
You don't appear to use html
.
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.
Aah, relics that I missed in my git add -p
!
if (md.isPlainText()) { | ||
contentText = md.toPlaintext(); | ||
} else { | ||
contentHTML = md.toHTML(true); |
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.
Likewise I don't think you need the true
anymore.
@@ -331,6 +331,7 @@ module.exports = React.createClass({ | |||
MatrixClientPeg.get().sendHtmlMessage(this.props.room.roomId, contentText, htmlText); | |||
} | |||
else { | |||
const contentText = mdown.toPlaintext(false); |
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.
And here.
mci.handleReturn(sinon.stub()); | ||
|
||
expect(spy.calledOnce).toEqual(true); | ||
expect(spy.args[0][1]).toEqual('Lorem ipsum dolor sit amet, consectetur adipiscing elit.'); |
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.
Don't you need to assert that spy.args[0][2]
is actually blank to be sure it didn't try to send this as HTML?
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.
If it tried to send it as HTML the calledOnce assertion will fail, because then sendHtmlMessage
will have been used rather than sendTextMessage
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.
Aha! Gotcha.
mci.handleReturn(sinon.stub()); | ||
|
||
expect(spy.calledOnce).toEqual(true); | ||
expect(spy.args[0][1]).toEqual('Lorem ipsum dolor sit amet, consectetur adipiscing elit.\n\nFusce congue sapien sed neque molestie volutpat.'); |
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.
Same here.
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
LGTM! |
We still need to parse "plaintext" messages through the markdown
renderer so that escappes are rendered properly.