FIX: display tables in posts history diff #6032
Conversation
|
You've signed the CLA, OsamaSayegh. Thank you! This pull request is ready for review. |
|
This is fine, but we need 1 test for it 👍 |
|
Added a test ✔️ |
|
@OsamaSayegh Could we also look at adding a test case for the XSS fix? It is unclear to me what the original security fix was trying to prevent at the moment. Also it seems like |
| @@ -252,8 +252,15 @@ export default Ember.Controller.extend(ModalFunctionality, { | |||
| if (viewMode === "side_by_side_markdown") { | |||
| return html; | |||
| } else { | |||
| const whiteLister = new WhiteLister({ features: { editHistory: true } }); | |||
| const tableTags = ["table", "thead", "tbody", "th", "tr", "td"]; | |||
There was a problem hiding this comment.
yeah this is not sitting right ... we have our default markdown whitelister ... why can we not use it.
There was a problem hiding this comment.
There was a problem hiding this comment.
so can't we just ask pretty text for the whitelister it uses ? and simply reuse that?
There was a problem hiding this comment.
I've tried using the same whitelister that pretty-text uses and it did fix this particular issue, however, it introduced a whole new bunch of issues. Mainly it messed up the layout and diff coloring (green and red) because all diff-ins and dif-del classes were stripped from diff HTML.
All this pain is caused by this bit here:
discourse/lib/discourse_diff.rb
Lines 248 to 253 in 9f558d7
Specifically, Nokogiri Parser. It seems to be decoding HTML entities when parsing our HTML and returning HTML that's not absolutely safe and needs to be sanitized again.
Looks like there is a way to disable HTML decoding, but it doesn't work :/ (see this issue).
There was a problem hiding this comment.
re: diff-ins and dif-del that is fine, I would then take the whitelister, clone it and add the 2 new classes you need. I follow about nokogiri making trouble.
There was a problem hiding this comment.
Just to be sure before I start working on this, I'll need to whitelist those two classes with every HTML tag we allow in posts (I don't think I'll have to list all the tags manually in an array, I think I can get them from existing code). Is that OK?
|
This seems like quite a lot of pain for a rather trivial thing to "fix".. are we sure this is all worth it? |
|
Ok, never mind my last response. I took a break from this and came back to get it accidentally to work. I simply added back Regis' code and used our pretty-text whitelister and now it works. ✨ Let me know if it looks OK, and I'll fix tests and add a couple more for the security issue and this bug. |
|
let me know when this is ready to merge |
|
@SamSaffron Ok, I think this is now in good shape. |
|
yay, yes this is exactly what I wanted! great work |
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/markdown-table-missing-from-edit-history/90243/4 |
I'm changing Régis code that he added for this security fix, so I'd like him to review this when he comes back :)