wiki: unsafe() diff html on conflict. #765

Closed
wants to merge 2 commits into
from

3 participants

@andre-d

Fixes #755, was caused by a safety net put in place with 102ef36

@spladug
reddit member

I'm not sure I understand a) what this is fixing, or b) how this doesn't just reintroduce the XSS?

@andre-d

The xss was on error messages displayed to the user, Ie. css errors. This fixes diff output (an html table generated in the backend) displaying the raw html rather than rendering. I can show you tomorrow if you wish.

@andre-d

Additionally, the css error xss was fixed by using .text(), so we have two layers of safety there.

https://github.com/reddit/reddit/blob/master/r2/r2/public/static/js/wiki.js#L117

@andre-d

Errrr, maybe it was in error 415, will look at that later. Either way, a different place, not the diff output, the diff output IS html.

@andre-d

💇

Added a safety net fix for the xss.

@chromakode chromakode commented on an outdated diff May 9, 2013
r2/r2/public/static/js/wiki.js
@@ -106,7 +107,7 @@ r.wiki = {
,specials = special.children('#specials')
specials.empty()
for(i in errors) {
- specials.append(errors[i]+'<br/>')
+ specials.append($('<p/>').text(errors[i]))
@chromakode
chromakode added a line comment May 9, 2013

No need for the closing slash in <p>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chromakode

💅 re: diff XSS, if this is getting generated by the same renderer as the template, it should be an equivalent risk to including it on the html diff pages. As long as that is safe, looks ok to me.

@andre-d

💇 Changed to just < p >

@chromakode

🐟

@andre-d andre-d closed this May 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment