fix: XSS issues in page descriptions #307
Conversation
There was a problem hiding this comment.
Looks alright to me once you fix the build failure
This did break a test turning
1
2
into 12 rather than 1\n\n2 I think this is an acceptable trade off since paragraphs should probably be on a new line anyway.
I’d agree, though I’d be interested to see how prevalent this is in the wild (e.g. does MyRadio’s TinyMCE put in newlines or not)
| @@ -110,14 +111,6 @@ func RenderTemplate(w http.ResponseWriter, context structs.PageContext, data int | |||
| "formatTime": func(fmt string, t time.Time) string { | |||
| return t.Format(fmt) | |||
| }, | |||
| // TODO(CaptainHayashi): this is temporary | |||
| "stripHTML": func(s string) string { | |||
There was a problem hiding this comment.
I assume you’ve checked this isn’t used anywhere?
There was a problem hiding this comment.
Ah yes I have broken something, will fix that now 🤦🏼♂️, I thought it was re-declaration
There was a problem hiding this comment.
oh bugger this new one doesnt handle ' properly
There was a problem hiding this comment.
Will take another look at this tomorrow, unless someone beats me to it :)
There was a problem hiding this comment.
I look forward to any and all instances of hacky hayashicode being removed :o)
|
Just remembered this never got merged, I will circle back on this soon and lets get it in :) |
One option here is to run the html through a formatter first |
|
No idea who is best to review this these days, but I have rebased and made a few changes to get this into a state where it can be shipped. I have just removed the title tag, not sure its helpful to have the full show description in a little tooltip anyway
RE the above - I have had a cursory glance at the current shows on the schedule and they all look like they are formatted correctly. The value of this seems higher than the cost imo, finally closing out the XSS issue. |
uses bluemonday to sanitize html, since we are loading in that library I have also applied the same to the strip html.
This did break a test turning
<p>1</p><p>2</p>into12rather than1\n\n2I think this is an acceptable trade off since paragraphs should probably be on a new line anyway.