Implement HTML sanitization for student and teacher messages

Fixes #132

Author: ihalaij1

dynamic_forms/static/dynamic_forms.js

@@ -13,7 +13,13 @@ function dynamic_forms_textarea() {
     }
     textbox.hide();
     span = $('<span class="textarea"></span>');
-    span.html(base.val() || blankResponse);
+    var rawVal = base.val();
+    var sanitizerFnName = base.data('html-sanitizer');
+    if (sanitizerFnName && typeof window[sanitizerFnName] === 'function' && rawVal) {
+      span.html(window[sanitizerFnName](rawVal));
+    } else {
+      span.html(rawVal || blankResponse);
+    }
     if (base.data('spantarget')) {
       spanbox = $(base.data('spantarget')).first();
       spanbox.show();

feedback/static/feedback.js

@@ -91,6 +91,63 @@ $(function() {
     });
   }
 
+  /* Sanitize response message HTML: keep allowed tags, escape everything else.
+   * Must mirror the server-side sanitize_response_message() helper in feedback/utils.py. */
+  const _ALLOWED_RESPONSE_TAGS = new Set(['B', 'I', 'U', 'STRONG', 'EM', 'CODE', 'A', 'BR']);
+  const _ALLOWED_RESPONSE_ATTRS = new Map([['A', new Set(['href', 'title'])]]);
+  const _SAFE_URL_SCHEMES = new Set(['http:', 'https:', 'mailto:']);
+
+  function _sanitizeResponseNode(source, target) {
+    for (const child of source.childNodes) {
+      if (child.nodeType === Node.TEXT_NODE) {
+        target.appendChild(document.createTextNode(child.data));
+      } else if (child.nodeType === Node.ELEMENT_NODE) {
+        const tag = child.tagName; // uppercase in HTML documents
+        if (_ALLOWED_RESPONSE_TAGS.has(tag)) {
+          const el = document.createElement(tag.toLowerCase());
+          const allowedAttrs = _ALLOWED_RESPONSE_ATTRS.get(tag) || new Set();
+          for (const attrName of allowedAttrs) {
+            const val = child.getAttribute(attrName);
+            if (val === null) continue;
+            if (attrName === 'href') {
+              try {
+                const parsed = new URL(val, window.location.href);
+                if (!_SAFE_URL_SCHEMES.has(parsed.protocol)) continue;
+              } catch (_) { continue; }
+            }
+            el.setAttribute(attrName, val);
+          }
+          _sanitizeResponseNode(child, el);
+          target.appendChild(el);
+        } else {
+          // Render the unknown tag as literal text so it is visible
+          let openTag = '<' + child.tagName.toLowerCase();
+          for (const attr of child.attributes) {
+            openTag += ' ' + attr.name + '="' + attr.value
+              .replace(/&/g, '&amp;').replace(/"/g, '&quot;') + '"';
+          }
+          openTag += '>';
+          target.appendChild(document.createTextNode(openTag));
+          // Still process children rather than discarding them
+          _sanitizeResponseNode(child, target);
+          if (child.childNodes.length > 0) {
+            target.appendChild(document.createTextNode('</' + child.tagName.toLowerCase() + '>'));
+          }
+        }
+      }
+    }
+  }
+
+  function sanitizeResponseHTML(text) {
+    const tmp = document.createElement('div');
+    tmp.innerHTML = text;
+    const out = document.createElement('div');
+    _sanitizeResponseNode(tmp, out);
+    return out.innerHTML; // safe HTML string with disallowed tags escaped
+  }
+  // Expose globally so dynamic_forms.js can look it up via data-html-sanitizer
+  window.sanitizeResponseHTML = sanitizeResponseHTML;
+
   /* Buttons for toggling preview state */
   let sStart, sEnd;
   function on_preview_button(e) {
@@ -101,7 +158,10 @@ $(function() {
       sStart = this.selectionStart;
       sEnd = this.selectionEnd;
       ta.hide();
-      ta.after('<span class="textarea preview">' + ta.val() + '</span>');
+      const previewSpan = document.createElement('span');
+      previewSpan.className = 'textarea preview';
+      previewSpan.innerHTML = sanitizeResponseHTML(ta.val());
+      ta.after(previewSpan);
     });
     me.hide();
     me.siblings('.unpreview-button').show().focus();
@@ -717,11 +777,11 @@ async function studentDiscussionPreview(btn) {
     /* clean up response message, inject text into div rather than embedding form */
     const response_msgs = contentDiv.querySelectorAll('.response-message');
     for (const rsp_msg of response_msgs) {
-      const text_content = rsp_msg.querySelector('textarea').innerText;
+      const text_content = rsp_msg.querySelector('textarea').value;
       if (text_content) {
         const textDiv = document.createElement('div');
         textDiv.className = 'display-response';
-        textDiv.innerText = text_content;
+        textDiv.innerHTML = sanitizeResponseHTML(text_content);
         rsp_msg.firstElementChild.replaceWith(textDiv); // replace form with text div
       } else { // no text content, so don't display anything
         rsp_msg.remove();

feedback/templates/manage/_response_message.html

@@ -60,6 +60,7 @@
 						name="response_msg"
 						data-texttarget="#{{ form.auto_id|fill_format_string:'edit_box' }}"
 						data-spantarget="#{{ form.auto_id|fill_format_string:'span_box' }}"
+						data-html-sanitizer="sanitizeResponseHTML"
 						placeholder="{{ f.label }}"
 						autocomplete="off"
 						{{ form.disabled|yesno:'disabled,' }}

feedback/templates_j2/feedback/_form.html

@@ -35,7 +35,7 @@
 	{% if message %}
 		<li class="list-group-item list-group-item-info staff{% if index > 2 %} initially-hidden{% endif %}">
 			{{ glyph("mortarboard-fill", _("teacher")) }}
-			<span class="text">{{ message|safe }}</span>
+			<span class="text">{{ message|sanitize_response }}</span>
 		</li>
 	{% endif %}
 {%- endmacro -%}
@@ -49,7 +49,7 @@
 		{%- set val = pair[1] -%}
 		{%- if k == f_key and val -%}
 			<li class="list-group-item student{% if index > 2 %} initially-hidden{% endif %}">
-				<span class="text">{{ val|safe }}</span>
+				<span class="text">{{ val }}</span>
 			</li>
 		{%- endif -%}
 	{%- endfor -%}

feedback/templatetags/jinja_tests.py

@@ -2,6 +2,17 @@
 from jinja2 import pass_context
 
 from dynamic_forms.fields import EnchantedBoundField
+from feedback.utils import sanitize_response_message
+
+
+@library.filter(name="sanitize_response")
+def sanitize_response_filter(value):
+    """Sanitize a teacher response message for safe HTML rendering.
+
+    Allows formatting tags (b, i, u, strong, em, code, a, br) and
+    escapes everything else so that text like ``<foo>`` renders literally.
+    """
+    return sanitize_response_message(value)
 
 
 @library.test(name="has_textfields")

feedback/utils.py

@@ -1,13 +1,99 @@
 import logging
-from urllib.parse import urljoin, urlencode
+from html import escape as html_escape
+from html.parser import HTMLParser
+from urllib.parse import urljoin, urlencode, urlparse
+
 from django.conf import settings
 from django.urls import reverse
 from django.template.loader import get_template
 from django.utils import translation
+from django.utils.safestring import mark_safe
 
 from aplus_client.client import AplusGraderClient
 
 
+# HTML tags allowed in teacher response messages (inserted by styling buttons)
+_ALLOWED_RESPONSE_TAGS = frozenset(['b', 'i', 'u', 'strong', 'em', 'code', 'a', 'br'])
+# Allowed attributes per tag; only 'href' and 'title' on <a>
+_ALLOWED_RESPONSE_ATTRS = {
+    'a': frozenset(['href', 'title']),
+}
+# URL schemes permitted in href values
+_SAFE_URL_SCHEMES = frozenset(['http', 'https', 'mailto'])
+
+
+class _ResponseMessageSanitizer(HTMLParser):
+    """HTML parser that passes through a whitelist of tags/attributes and
+    HTML-escapes everything else, so that text like ``<foo>`` is rendered
+    literally rather than silently disappearing in the browser."""
+
+    def __init__(self):
+        super().__init__(convert_charrefs=False)
+        self._output = []
+
+    def handle_starttag(self, tag, attrs):
+        if tag in _ALLOWED_RESPONSE_TAGS:
+            allowed_attr_names = _ALLOWED_RESPONSE_ATTRS.get(tag, frozenset())
+            safe_attrs = []
+            for name, value in attrs:
+                if name not in allowed_attr_names:
+                    continue
+                if value is None:
+                    safe_attrs.append(html_escape(name))
+                    continue
+                if name == 'href':
+                    scheme = urlparse(value).scheme.lower()
+                    if scheme and scheme not in _SAFE_URL_SCHEMES:
+                        continue
+                safe_attrs.append('{}="{}"'.format(html_escape(name), html_escape(value)))
+            attr_str = (' ' + ' '.join(safe_attrs)) if safe_attrs else ''
+            self._output.append('<{}{}>'.format(tag, attr_str))
+        else:
+            attrs_str = ''
+            for name, value in attrs:
+                if value is None:
+                    attrs_str += ' {}'.format(name)
+                else:
+                    attrs_str += ' {}="{}"'.format(name, value)
+            self._output.append(html_escape('<{}{}>'.format(tag, attrs_str)))
+
+    def handle_endtag(self, tag):
+        if tag in _ALLOWED_RESPONSE_TAGS:
+            self._output.append('</{}>'.format(tag))
+        else:
+            self._output.append(html_escape('</{}>'.format(tag)))
+
+    def handle_data(self, data):
+        self._output.append(html_escape(data))
+
+    def handle_entityref(self, name):
+        self._output.append('&{};'.format(name))
+
+    def handle_charref(self, name):
+        self._output.append('&#{};'.format(name))
+
+    def get_output(self):
+        return ''.join(self._output)
+
+
+def sanitize_response_message(message):
+    """Sanitize an HTML teacher response message.
+
+    Allows a small whitelist of safe formatting tags (``<b>``, ``<i>``,
+    ``<u>``, ``<strong>``, ``<em>``, ``<code>``, ``<a>``, ``<br>``) and
+    HTML-escapes everything else so that text like ``<foo>`` is displayed
+    as the literal characters ``<foo>`` instead of vanishing.
+
+    Returns a :class:`~django.utils.safestring.SafeString` ready for
+    insertion into a template without further escaping.
+    """
+    if not message:
+        return mark_safe('')
+    parser = _ResponseMessageSanitizer()
+    parser.feed(message)
+    return mark_safe(parser.get_output())
+
+
 logger = logging.getLogger("feedback.utils")