Prevent user comments xss (#225)

* feat: Prevent XSS attack from comments - Escaped comment text - Fixed SQL Linter test comment text because of escaping characters - Added a unittest for testing escaping html in comment text - Fixed a linter test
PythonFreeCourse · Oct 5, 2020 · 04cae7a · 04cae7a
1 parent 573d654
commit 04cae7a
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 20 deletions.
diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py
@@ -1,5 +1,6 @@
 from collections import Counter
 import enum
+import html
 import secrets
 import string
 from datetime import datetime
@@ -708,7 +709,7 @@ def create_comment(
         cls, text: str, flake_key: Optional[str] = None,
     ) -> 'CommentText':
         instance, _ = CommentText.get_or_create(
-            **{CommentText.text.name: text},
+            **{CommentText.text.name: html.escape(text)},
             defaults={CommentText.flake8_key.name: flake_key},
         )
         return instance

diff --git a/lms/tests/test_flake8_linter.py b/lms/tests/test_flake8_linter.py
@@ -2,7 +2,7 @@
 import shutil
 import tempfile
 
-from lms.lmsdb import models
+from lms.lmsdb.models import Comment, Solution
 from lms.lmstests.public.linters import tasks
 from lms.models import notifications
 
@@ -30,23 +30,23 @@ def teardown_class(cls):
         if cls.test_directory is not None:
             shutil.rmtree(cls.test_directory)
 
-    def test_pyflake_wont_execute_code(self, solution: models.Solution):
+    def test_pyflake_wont_execute_code(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.code = self.execute_script
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert not os.listdir(self.test_directory)
         assert len(comments) == 2
         exec(compile(self.execute_script, '', 'exec'))  # noqa S102
         assert os.listdir(self.test_directory) == ['some-file']
 
-    def test_invalid_solution(self, solution: models.Solution):
+    def test_invalid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.code = INVALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert comments
         assert len(comments) == 1
         comment = comments[0].comment
@@ -59,10 +59,10 @@ def test_invalid_solution(self, solution: models.Solution):
         assert solution.exercise.subject in subject
         assert '1' in subject
 
-    def test_valid_solution(self, solution: models.Solution):
+    def test_valid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.code = VALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert not comments
diff --git a/lms/tests/test_html_escaping.py b/lms/tests/test_html_escaping.py
@@ -0,0 +1,25 @@
+from flask import json
+
+from lms.lmsdb.models import Solution, User
+from lms.tests import conftest
+
+
+USER_COMMENT_BEFORE_ESCAPING = '<html><body><p>Welcome "LMS"</p></body></html>'
+USER_COMMENT_AFTER_ESCAPING = (
+    '&lt;html&gt;&lt;body&gt;&lt;p&gt;Welcome &quot;LMS&quot;'
+    '&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;'
+)
+
+
+class TestHtmlEscaping:
+    @staticmethod
+    def test_comment_text_escaping(student_user: User, solution: Solution):
+        client = conftest.get_logged_user(student_user.username)
+
+        # Creating a comment
+        comment_response = client.post('/comments', data=json.dumps(dict(
+            fileId=solution.files[0].id, act='create', kind='text',
+            comment=USER_COMMENT_BEFORE_ESCAPING, line=1,
+        )), content_type='application/json')
+        assert comment_response.status_code == 200
+        assert solution.comments[0].comment.text == USER_COMMENT_AFTER_ESCAPING
diff --git a/lms/tests/test_html_linter.py b/lms/tests/test_html_linter.py
@@ -2,13 +2,13 @@
 
 import pytest
 
-from lms.lmsdb import models
+from lms.lmsdb.models import Comment, Solution
 from lms.lmstests.public.linters import tasks
 
 
 INVALID_CODE = '<html>'
 INVALID_CODE_MESSAGES = {
-    'Start tag seen without seeing a doctype first. Expected “<!DOCTYPE html>”.',
+    'Start tag seen without seeing a doctype first. Expected “&lt;!DOCTYPE html&gt;”.',
     'Element “head” is missing a required instance of child element “title”.',
     'Consider adding a “lang” attribute to the “html” start tag to declare the language of this document.',
 }
@@ -31,23 +31,23 @@
     reason='should run with VNU linter in path. see VNULinter class for more information',
 )
 class TestHTMLLinter:
-    def test_invalid_solution(self, solution: models.Solution):
+    def test_invalid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.path = 'index.html'
         solution_file.code = INVALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert comments
         assert len(comments) == 3
         comment_texts = {comment.comment.text for comment in comments}
         assert comment_texts == INVALID_CODE_MESSAGES
 
-    def test_valid_solution(self, solution: models.Solution):
+    def test_valid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.path = 'index.html'
         solution_file.code = VALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert not comments
diff --git a/lms/tests/test_sql_linter.py b/lms/tests/test_sql_linter.py
@@ -1,30 +1,30 @@
-from lms.lmsdb import models
+from lms.lmsdb.models import Comment, Solution
 from lms.lmstests.public.linters import tasks
 
 
 INVALID_CODE = 's1\n'
-INVALID_CODE_MESSAGE = "Found unparsable section: 's1'"
+INVALID_CODE_MESSAGE = 'Found unparsable section: &#x27;s1&#x27;'  # Escape
 
 VALID_CODE = 'SELECT 1\n'
 
 
 class TestSQLLinter:
-    def test_invalid_solution(self, solution: models.Solution):
+    def test_invalid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.path = 'sql.sql'
         solution_file.code = INVALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert comments
         assert len(comments) == 1
         assert comments[0].comment.text == INVALID_CODE_MESSAGE
 
-    def test_valid_solution(self, solution: models.Solution):
+    def test_valid_solution(self, solution: Solution):
         solution_file = solution.solution_files.get()
         solution_file.path = 'sql.sql'
         solution_file.code = VALID_CODE
         solution_file.save()
         tasks.run_linter_on_solution(solution.id)
-        comments = tuple(models.Comment.by_solution(solution))
+        comments = tuple(Comment.by_solution(solution))
         assert not comments