From 8af8709c66d7adb08634a862390b3641cdf11e27 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Mon, 5 Oct 2020 11:39:57 +0300 Subject: [PATCH 1/3] feat: Prevent XSS attack from comments - Escaped comment text --- lms/lmsdb/models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index df87b034..c05f4762 100644 --- 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 From 702dcac613e196ea803f7f244db2b4c7af32b29d Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Mon, 5 Oct 2020 11:49:05 +0300 Subject: [PATCH 2/3] feat: Prevent XSS attack from comments - Added html escape in comments text - Fixed SQL Linter test comment text because of escaping characters --- lms/tests/test_sql_linter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/tests/test_sql_linter.py b/lms/tests/test_sql_linter.py index 6d581a16..962607c7 100644 --- a/lms/tests/test_sql_linter.py +++ b/lms/tests/test_sql_linter.py @@ -3,7 +3,7 @@ INVALID_CODE = 's1\n' -INVALID_CODE_MESSAGE = "Found unparsable section: 's1'" +INVALID_CODE_MESSAGE = 'Found unparsable section: 's1'' # Escape VALID_CODE = 'SELECT 1\n' From 50babd74c61ea73ca03acb2da15f9169730e27fa Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Mon, 5 Oct 2020 12:24:20 +0300 Subject: [PATCH 3/3] - Added a unittest for testing escaping html in comment text - Fixed a linter test --- lms/tests/test_flake8_linter.py | 14 +++++++------- lms/tests/test_html_escaping.py | 25 +++++++++++++++++++++++++ lms/tests/test_html_linter.py | 12 ++++++------ lms/tests/test_sql_linter.py | 10 +++++----- 4 files changed, 43 insertions(+), 18 deletions(-) create mode 100644 lms/tests/test_html_escaping.py diff --git a/lms/tests/test_flake8_linter.py b/lms/tests/test_flake8_linter.py index daad0114..172d9716 100644 --- 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 new file mode 100644 index 00000000..c1ab06ef --- /dev/null +++ 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 = '

Welcome "LMS"

' +USER_COMMENT_AFTER_ESCAPING = ( + '<html><body><p>Welcome "LMS"' + '</p></body></html>' +) + + +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 index 246c686a..22472031 100644 --- 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 = '' INVALID_CODE_MESSAGES = { - 'Start tag seen without seeing a doctype first. Expected “”.', + 'Start tag seen without seeing a doctype first. Expected “<!DOCTYPE html>”.', '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 index 962607c7..932c57e2 100644 --- a/lms/tests/test_sql_linter.py +++ b/lms/tests/test_sql_linter.py @@ -1,4 +1,4 @@ -from lms.lmsdb import models +from lms.lmsdb.models import Comment, Solution from lms.lmstests.public.linters import tasks @@ -9,22 +9,22 @@ 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