Fixes:

• https://pagure.io/pagure/issue/221
• https://pagure.io/pagure/issue/229
• https://pagure.io/pagure/issue/230
• https://pagure.io/pagure/issue/231
• https://pagure.io/pagure/issue/232

:+1: Looks good!

This method (no_js(...)) feels very error prone to me. It misses a lot of cases, for example when the script tag is capitalized or mixed-case. Or when I write <iframe src="javascript:alert('hi');"></iframe> instead of a plain script tag. Or if I use an <embed> tag which contains a malicious SVG. Or if I use an <img> tag with a malicious onmouseover attribute and make it really large so people mouse over it on accident.

I am still in favor of disabling html input altogether. It is not worth the security risks involved.

Ugh. I thought if I put it in backticks that wouldn't happen. The XSS above is caused by an iframe tag with a src="javascript:alert(...)".

Oh, that's weird. When I submitted, the backticked code triggered an XSS (the iframe one, specifically), but when I refresh, it doesn't, it renders it as a code block. So it sounds like there is another bug where submitting a comment for the first time, vs rendering it in the future, has potentially different output.

``
Oh, that's weird. When I submitted, the backticked code triggered an XSS (the iframe one, specifically), but when I refresh, it doesn't, it renders it as a code block. So it sounds like there is another bug where submitting a comment for the first time, vs rendering it in the future, has potentially different output.

That's because the inline comments code does not run via the same path as the
rest, that's something Patrick worked on to avoid having to reload the page at
every comment (making commenting on PR harder) but that we should still address.

As for the root cause, I'm still interesting to know you thought on allowing
markdown in comments while being secure.
I don't want to take markdown out of the picture, I find it an interesting
feature, but XSS is a problem definitely.
Do you think you could come up with a patch of some sort?

@pingou https://pythonhosted.org/Markdown/release-2.6.html suggests using the bleach library's clean method after converting the markdown to HTML. Maybe give that a shot?

@pingou https://pythonhosted.org/Markdown/release-2.6.html suggests using the bleach library's clean method after converting the markdown to HTML. Maybe give that a shot?

Ok done via: https://pagure.io/pagure/pull-request/235
Let's see in the next release if it helps :)

Thanks for the input!