https://github.com/mozilla/elmo/blob/develop/apps/shipping/templates/shipping/signoff-rows.html#L85 wants bleach. This is not really critical, as the review comments are only done by drivers of our products, and if they're evil, we have bigger fish to fry. Yet, we should bleach those comments to be on the safe side.

(In reply to comment #0) > https://github.com/mozilla/elmo/blob/develop/apps/shipping/templates/ > shipping/signoff-rows.html#L85 wants bleach. > > This is not really critical, as the review comments are only done by drivers > of our products, and if they're evil, we have bigger fish to fry. > > Yet, we should bleach those comments to be on the safe side. Why show it as a '|safe'? Why not let it just auto-escape? I've got a great piece of code that mixes HTML-quoting with making URLs and emails into links if that's of interest.

Also, regarding line 85 (github link above) is there's a reason the '</div>' is missing?

the comments include html markup, like https://l10n-stage-sj.mozilla.org/shipping/signoffs/sr/fx5 has right now. Re the div, there's a hidden span inside the comment, that's used to diff against the signed-off revision.

(In reply to comment #2) > Also, regarding line 85 (github link above) is there's a reason the '</div>' > is missing? Strike that. I counted the ifequals wrong.

(In reply to comment #3) > the comments include html markup, like > https://l10n-stage-sj.mozilla.org/shipping/signoffs/sr/fx5 has right now. > So it's already a '<a>' tag. Then we need jsocol's bleach https://github.com/jsocol/bleach

In retrospect, the change I made to the etag decorator is off-tangent to this patch but I needed it for manually testing. Without it, it becomes very hard to test changes in development. Note that this changes elmo-lib to require html5lib (vendor-local/lib and bleach (submodule in vendor-local/src) but I don't know how to make a diff to show that.

Comment on attachment 537563 [details] [diff] [review] Makes the comment safe using bleach.clean() and bleach.linkify() Review of attachment 537563 [details] [diff] [review]: ----------------------------------------------------------------- r=me, I guess you don't want the changes to signoff.py though. I assume there a change to elmo-lib to have bleach and html5lib? ::: apps/shipping/views/signoff.py @@ +105,5 @@ > > > def etag_signoff(request, locale_code, app_code): > + if settings.DEBUG: > + return Is this supposed to be in this patch?

Landed. New dependencies in vendor-local, namely bleach and html5lib. https://github.com/mozilla/elmo/commit/799d61577b84c48b37e4d6a9cdfc37bbbc081b02 Regarding the `if settings.DEBUG:return` inside the etag decorator, I've removed it. It was a neat trick that I needed to be able to test this patch manually.