Display hyperlinks

[hhc97]

Displays links as hyperlinks within clues and student comments and replies.

Also sanitize incoming text before displaying links to prevent against unwanted attacks.

If a potential bad comment is found, it is logged into the debug logger at WARNING level in the following format:

10/07/2020 03:37:16 [WARNING] - HTML detected in comment or reply (student): {'text': '<badtag> test text </badtag>', 'owner': 15, 'instance': 4, 'release': 3}

A set of pre-chosen tags can be allowed through by adding them into the self.allowed attribute of the new HTMLSanitizer class.

[utmandrew]

Consider the use of bleach (https://pypi.org/project/bleach/) instead of a custom Sanitizer class in views.py. The argument: sanitization is a security issue, and a failure to sanitize properly will lead to a security vulnerability.

Your code does look clean -- and I can't see a bug in it -- but that isn't a guarantee. :-)

[hhc97]

For the purposes of logging bad comments bleach doesn't seem to tell you if it found any unwanted tags in the input string, and we can't just do if bleached_text != original_text because an <em > will get bleached to a <em> (notice the space) but they are the same tag and should not be logged. We could perhaps count the number of <'s but again I'm not sure that's reliable.

Lastly, I'd argue that my custom class depends on the builtin HTMLParser so if we depend on the parent class to be able to find all HTML tags I feel like that would be secure enough? Also a plus because its a builtin? (one less dependency) ;)

[utmandrew]

It would be sad to lose precise logging.

I don't think that reducing dependencies is an argument. This is a security-related function. I would prefer that someone who is likely to be paying close attention to threats be doing updates. That's actually a benefit.

I don't have an absolute preference on this since you've already completed the work, but if I were to have done this, I would have used bleach. Writing code to do something when there is an already maintained package that already does it is (a) slower and (b) a risk.

[hhc97]

Agreed on that point, bleach will be maintained for longer than I will be around to maintain this. So switched to bleach.

Although on that point about 'faster', yes its faster to implement bleach, but I did some testing and found that bleach is actually consistently 10-20x slower than a custom parser class when sanitizing. Good thing is that it only needs to be run once, and as long as thousands of comments don't suddenly flood the server it should be fine, we're not at a point where that would be a bottleneck.

[utmandrew]

Note: The whitespace fixes make paragraphs happen but does not fix indentation. That is being left for markdown.