Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent user comments xss #225

Merged
merged 3 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lms/lmsdb/models.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from collections import Counter
import enum
import html
import secrets
import string
from datetime import datetime
Expand Down Expand Up @@ -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)},
yammesicka marked this conversation as resolved.
Show resolved Hide resolved
defaults={CommentText.flake8_key.name: flake_key},
)
return instance
Expand Down
14 changes: 7 additions & 7 deletions lms/tests/test_flake8_linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
25 changes: 25 additions & 0 deletions lms/tests/test_html_escaping.py
Original file line number Diff line number Diff line change
@@ -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
12 changes: 6 additions & 6 deletions lms/tests/test_html_linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
}
Expand All @@ -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
12 changes: 6 additions & 6 deletions lms/tests/test_sql_linter.py
Original file line number Diff line number Diff line change
@@ -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