Sanitize comment content on edit (mozilla#4090)

Also:
- Add tests
- Drop useless error representation complexity

Author: mathjazz

pontoon/base/forms.py

@@ -400,6 +400,15 @@ class AddCommentForm(forms.Form):
     translation = forms.IntegerField(required=False)
 
 
+class EditCommentForm(forms.Form):
+    """
+    Form for parameters to the `edit_comment` view.
+    """
+
+    comment_id = forms.IntegerField()
+    content = HtmlField()
+
+
 class CreateTokenForm(forms.ModelForm):
     """
     Form for creating Personal Access Tokens.

pontoon/base/tests/views/test_comment.py

@@ -7,6 +7,51 @@
 from pontoon.test.factories import TranslationCommentFactory
 
 
+@pytest.mark.django_db
+def test_add_comment(member, translation_a):
+    url = reverse("pontoon.add_comment")
+
+    response = member.client.post(
+        url,
+        {
+            "translation": translation_a.pk,
+            "entity": translation_a.entity.pk,
+            "locale": translation_a.locale.code,
+            "comment": "new comment",
+        },
+        HTTP_X_REQUESTED_WITH="XMLHttpRequest",
+    )
+
+    assert response.status_code == 200
+
+    comment = Comment.objects.get(translation=translation_a, author=member.user)
+    assert comment.content == "new comment"
+
+
+@pytest.mark.django_db
+def test_add_comment_sanitizes_html(member, entity_a, locale_a, project_locale_a):
+    url = reverse("pontoon.add_comment")
+
+    payload = "<svg><script>alert(1)</script>safe"
+
+    response = member.client.post(
+        url,
+        {
+            "entity": entity_a.pk,
+            "locale": locale_a.code,
+            "comment": payload,
+        },
+        HTTP_X_REQUESTED_WITH="XMLHttpRequest",
+    )
+
+    assert response.status_code == 200
+
+    comment = Comment.objects.latest("id")
+
+    assert "<script>" not in comment.content
+    assert "safe" in comment.content
+
+
 @pytest.mark.django_db
 def test_pin_comment(member, client, comment_a):
     url = reverse("pontoon.pin_comment")
@@ -66,10 +111,10 @@ def test_unpin_comment(member, client, team_comment_a):
 
 
 @pytest.mark.django_db
-def test_edit_comment(member, client, comment_a):
+def test_edit_comment(member, comment_a):
     url = reverse("pontoon.edit_comment")
 
-    # a user cannot edit someone else's comment
+    # A user cannot edit someone else's comment
     response = member.client.post(
         url,
         {"comment_id": comment_a.pk, "content": "edited"},
@@ -93,6 +138,34 @@ def test_edit_comment(member, client, comment_a):
     assert comment_a.content == "edited content"
 
 
+@pytest.mark.django_db
+def test_edit_comment_sanitizes_html(member, comment_a):
+    url = reverse("pontoon.edit_comment")
+
+    # Make member the author
+    comment_a.author = member.user
+    comment_a.save()
+
+    payload = "<img src=x onerror=alert(1)>safe"
+
+    response = member.client.post(
+        url,
+        {"comment_id": comment_a.pk, "content": payload},
+        HTTP_X_REQUESTED_WITH="XMLHttpRequest",
+    )
+
+    assert response.status_code == 200
+
+    comment_a.refresh_from_db()
+
+    # Dangerous part should be removed
+    assert "onerror" not in comment_a.content
+    assert "<img" not in comment_a.content
+
+    # Safe content should remain
+    assert "safe" in comment_a.content
+
+
 @pytest.mark.django_db
 def test_delete_comment(member, client, comment_a):
     url = reverse("pontoon.delete_comment")

pontoon/base/views.py

@@ -245,9 +245,7 @@ def entities(request):
         return JsonResponse(
             {
                 "status": False,
-                "message": "{error}".format(
-                    error=form.errors.as_json(escape_html=True)
-                ),
+                "message": form.errors,
             },
             status=400,
         )
@@ -666,9 +664,7 @@ def add_comment(request):
         return JsonResponse(
             {
                 "status": False,
-                "message": "{error}".format(
-                    error=form.errors.as_json(escape_html=True)
-                ),
+                "message": form.errors,
             },
             status=400,
         )
@@ -753,33 +749,29 @@ def unpin_comment(request):
 
 @login_required(redirect_field_name="", login_url="/403")
 @require_POST
+@utils.require_AJAX
 @transaction.atomic
 def edit_comment(request):
-    """Edit a comment"""
-    comment_id = request.POST.get("comment_id", None)
-
-    if not comment_id:
-        return JsonResponse({"status": False, "message": "Bad Request"}, status=400)
+    """Edit a comment."""
+    form = forms.EditCommentForm(request.POST)
+    if not form.is_valid():
+        return JsonResponse(
+            {
+                "status": False,
+                "message": form.errors,
+            },
+            status=400,
+        )
 
-    comment = get_object_or_404(Comment, id=comment_id)
+    comment = get_object_or_404(Comment, id=form.cleaned_data["comment_id"])
 
     if request.user != comment.author:
         return JsonResponse(
             {"status": False, "message": "Forbidden: You can't edit this comment"},
             status=403,
         )
 
-    content = request.POST.get("content", None)
-
-    if not content:
-        return JsonResponse(
-            {
-                "status": False,
-                "message": "Bad Request",
-            },
-            status=400,
-        )
-    comment.content = content
+    comment.content = form.cleaned_data["content"]
     comment.save()
 
     return JsonResponse({"status": True})