From 63ed16e118bcacc385b8670ae0e71d9a721306db Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Jul 2013 12:57:52 -0400 Subject: [PATCH 01/14] add a 'can_execute_unsafe_code' callback method (ala LMS) to allow for whitelisting of courses with respect to codejailing --- cms/djangoapps/contentstore/views/preview.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index deef6a27c9..4eb118e031 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -1,7 +1,9 @@ import logging import sys from functools import partial +import re +from django.conf import settings from django.http import HttpResponse, Http404, HttpResponseBadRequest, HttpResponseForbidden from django.core.urlresolvers import reverse from django.contrib.auth.decorators import login_required @@ -21,6 +23,7 @@ import static_replace from .session_kv_store import SessionKeyValueStore from .requests import render_from_lms from .access import has_access +from ..utils import get_course_for_item __all__ = ['preview_dispatch', 'preview_component'] @@ -93,6 +96,20 @@ def preview_module_system(request, preview_id, descriptor): MongoUsage(preview_id, descriptor.location.url()), ) + # unfortunately this is duplicate code from module_render.py (LMS) + # refactoring this to be more DRY means having the change the call signature + # to pass along a course_id, however, deep in the code where the call is actually made, we don't always have + # access to the course_id + course_id = get_course_for_item(descriptor.location).location.course_id + + def can_execute_unsafe_code(): + # To decide if we can run unsafe code, we check the course id against + # a list of regexes configured on the server. + for regex in settings.COURSES_WITH_UNSAFE_CODE: + if re.match(regex, course_id): + return True + return False + return ModuleSystem( ajax_url=reverse('preview_dispatch', args=[preview_id, descriptor.location.url(), '']).rstrip('/'), # TODO (cpennington): Do we want to track how instructors are using the preview problems? @@ -104,6 +121,7 @@ def preview_module_system(request, preview_id, descriptor): replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_namespace=descriptor.location), user=request.user, xblock_model_data=preview_model_data, + can_execute_unsafe_code=can_execute_unsafe_code, ) From 8d858ecb49940ec6b8c178659bc3b733c82f9bd4 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Jul 2013 14:53:34 -0400 Subject: [PATCH 02/14] add a unit test to exercise the preview_component AJAX callback in the CMS --- .../contentstore/tests/test_contentstore.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index b946aac6bb..85666e0483 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -344,6 +344,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): err_cnt = perform_xlint('common/test/data', ['full']) self.assertGreater(err_cnt, 0) + def test_module_preview(self): + ''' + Tests the ajax callback to render an XModule + ''' + direct_store = modulestore('direct') + import_from_xml(direct_store, 'common/test/data/', ['full']) + + html_module_location = Location(['i4x', 'edX', 'full', 'html', 'html_90', None]) + + url = reverse('preview_component', kwargs={'location': html_module_location.url()}) + + resp = self.client.get(url) + self.assertEqual(resp.status_code, 200) + self.assertIn('Inline content', resp.content) + def test_delete(self): direct_store = modulestore('direct') import_from_xml(direct_store, 'common/test/data/', ['full']) From 0e0dfd22cf37ae6691ea9f4c09461fccebb526d2 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 1 Jul 2013 16:15:26 -0400 Subject: [PATCH 03/14] add test for customresponse problem type --- cms/djangoapps/contentstore/tests/test_contentstore.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 85666e0483..3c2cae99c1 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -359,6 +359,12 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(resp.status_code, 200) self.assertIn('Inline content', resp.content) + # also try a custom response + problem_module_location = Location(['i4x', 'edX', 'full', 'problem', 'H1P1_Energy', None]) + url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) + resp = self.client.get(url) + self.assertEqual(resp.status_code, 200) + def test_delete(self): direct_store = modulestore('direct') import_from_xml(direct_store, 'common/test/data/', ['full']) From 9a8c5857e6930c6807db80e4416b9e29fa2ef318 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 10:22:03 -0400 Subject: [PATCH 04/14] change settings to add test course to sandbox whitelist to try to increase diff code coverage --- cms/djangoapps/contentstore/tests/test_contentstore.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 3c2cae99c1..e369c9bc45 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -344,6 +344,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): err_cnt = perform_xlint('common/test/data', ['full']) self.assertGreater(err_cnt, 0) + @override_settings(COURSES_WITH_UNSAFE_CODE=['edx/full/.*']) def test_module_preview(self): ''' Tests the ajax callback to render an XModule @@ -359,7 +360,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(resp.status_code, 200) self.assertIn('Inline content', resp.content) - # also try a custom response + # also try a custom response which will trigger the 'is this course in whitelist' logic problem_module_location = Location(['i4x', 'edX', 'full', 'problem', 'H1P1_Energy', None]) url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) resp = self.client.get(url) From 11ef3961bcc380e23234d05a66e1abaead4be7c0 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 11:06:45 -0400 Subject: [PATCH 05/14] try to add some additional diff coverage which should get us to 100% --- .../contentstore/tests/test_contentstore.py | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index e369c9bc45..ef5da56299 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -344,8 +344,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): err_cnt = perform_xlint('common/test/data', ['full']) self.assertGreater(err_cnt, 0) - @override_settings(COURSES_WITH_UNSAFE_CODE=['edx/full/.*']) - def test_module_preview(self): + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) + def test_module_preview_in_whitelist(self): ''' Tests the ajax callback to render an XModule ''' @@ -366,6 +366,25 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get(url) self.assertEqual(resp.status_code, 200) + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/dummycourse/.*']) + def test_module_preview_not_in_whitelist(self): + ''' + Tests the ajax callback to render an XModule problem but we don't have the course listed in + our whitelist. + ''' + direct_store = modulestore('direct') + import_from_xml(direct_store, 'common/test/data/', ['full']) + + html_module_location = Location(['i4x', 'edX', 'full', 'html', 'html_90', None]) + + url = reverse('preview_component', kwargs={'location': html_module_location.url()}) + + # also try a custom response which will trigger the 'is this course in whitelist' logic + problem_module_location = Location(['i4x', 'edX', 'full', 'problem', 'H1P1_Energy', None]) + url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) + resp = self.client.get(url) + self.assertEqual(resp.status_code, 200) + def test_delete(self): direct_store = modulestore('direct') import_from_xml(direct_store, 'common/test/data/', ['full']) From bf3a7287f0dc935f4a4f6b08ad08386d8dfc043b Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 13:30:15 -0400 Subject: [PATCH 06/14] do some lamda magic to refactor out the whitelist checking code to be shared between LMS and CMS --- cms/djangoapps/contentstore/views/preview.py | 14 +++----------- common/djangoapps/util/sandboxing.py | 11 +++++++++++ lms/djangoapps/courseware/module_render.py | 12 ++---------- 3 files changed, 16 insertions(+), 21 deletions(-) create mode 100644 common/djangoapps/util/sandboxing.py diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 4eb118e031..56000f2f65 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -1,9 +1,7 @@ import logging import sys from functools import partial -import re -from django.conf import settings from django.http import HttpResponse, Http404, HttpResponseBadRequest, HttpResponseForbidden from django.core.urlresolvers import reverse from django.contrib.auth.decorators import login_required @@ -19,6 +17,8 @@ from xmodule.modulestore.mongo import MongoUsage from xmodule.x_module import ModuleSystem from xblock.runtime import DbModel +from util.sandboxing import can_execute_unsafe_code + import static_replace from .session_kv_store import SessionKeyValueStore from .requests import render_from_lms @@ -102,14 +102,6 @@ def preview_module_system(request, preview_id, descriptor): # access to the course_id course_id = get_course_for_item(descriptor.location).location.course_id - def can_execute_unsafe_code(): - # To decide if we can run unsafe code, we check the course id against - # a list of regexes configured on the server. - for regex in settings.COURSES_WITH_UNSAFE_CODE: - if re.match(regex, course_id): - return True - return False - return ModuleSystem( ajax_url=reverse('preview_dispatch', args=[preview_id, descriptor.location.url(), '']).rstrip('/'), # TODO (cpennington): Do we want to track how instructors are using the preview problems? @@ -121,7 +113,7 @@ def preview_module_system(request, preview_id, descriptor): replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_namespace=descriptor.location), user=request.user, xblock_model_data=preview_model_data, - can_execute_unsafe_code=can_execute_unsafe_code, + can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)), ) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py new file mode 100644 index 0000000000..1e82b90a69 --- /dev/null +++ b/common/djangoapps/util/sandboxing.py @@ -0,0 +1,11 @@ +import re +from django.conf import settings + + +def can_execute_unsafe_code(course_id): + # To decide if we can run unsafe code, we check the course id against + # a list of regexes configured on the server. + for regex in settings.COURSES_WITH_UNSAFE_CODE: + if re.match(regex, course_id): + return True + return False diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 4cafb0979d..66fb907cd3 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -37,7 +37,7 @@ from courseware.access import has_access from courseware.masquerade import setup_masquerade from courseware.model_data import LmsKeyValueStore, LmsUsage, ModelDataCache from courseware.models import StudentModule - +from util.sandboxing import can_execute_unsafe_code log = logging.getLogger(__name__) @@ -313,14 +313,6 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours statsd.increment("lms.courseware.question_answered", tags=tags) - def can_execute_unsafe_code(): - # To decide if we can run unsafe code, we check the course id against - # a list of regexes configured on the server. - for regex in settings.COURSES_WITH_UNSAFE_CODE: - if re.match(regex, course_id): - return True - return False - # TODO (cpennington): When modules are shared between courses, the static # prefix is going to have to be specific to the module, not the directory # that the xml was loaded from @@ -348,7 +340,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours open_ended_grading_interface=open_ended_grading_interface, s3_interface=s3_interface, cache=cache, - can_execute_unsafe_code=can_execute_unsafe_code, + can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)), ) # pass position specified in URL to module through ModuleSystem system.set('position', position) From 7962ad7dd0bcdb1a3f5a3a86686d035635ce1374 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 13:32:28 -0400 Subject: [PATCH 07/14] remove incorrect comment --- cms/djangoapps/contentstore/views/preview.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 56000f2f65..fd2188a734 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -96,10 +96,6 @@ def preview_module_system(request, preview_id, descriptor): MongoUsage(preview_id, descriptor.location.url()), ) - # unfortunately this is duplicate code from module_render.py (LMS) - # refactoring this to be more DRY means having the change the call signature - # to pass along a course_id, however, deep in the code where the call is actually made, we don't always have - # access to the course_id course_id = get_course_for_item(descriptor.location).location.course_id return ModuleSystem( From bcbac3aaa458c8d0cac2b26bf6376752df819d9c Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 16:55:07 -0400 Subject: [PATCH 08/14] add docstring to newly refactored method --- common/djangoapps/util/sandboxing.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index 1e82b90a69..7d1c1da06f 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -3,6 +3,15 @@ from django.conf import settings def can_execute_unsafe_code(course_id): + """ + Determine if this course is allowed to run unsafe code. + + For use from the ModuleStore. Checks the `course_id` against a list of whitelisted + regexes. + + Returns a boolean, true if the course can run outside the sandbox. + + """ # To decide if we can run unsafe code, we check the course id against # a list of regexes configured on the server. for regex in settings.COURSES_WITH_UNSAFE_CODE: From aa7f1f867d7736603113d73d218feac39eee6376 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 16:56:27 -0400 Subject: [PATCH 09/14] actually, I think we use single quotes for docstrings --- common/djangoapps/util/sandboxing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index 7d1c1da06f..f3c7146148 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -3,7 +3,7 @@ from django.conf import settings def can_execute_unsafe_code(course_id): - """ + ''' Determine if this course is allowed to run unsafe code. For use from the ModuleStore. Checks the `course_id` against a list of whitelisted @@ -11,7 +11,7 @@ def can_execute_unsafe_code(course_id): Returns a boolean, true if the course can run outside the sandbox. - """ + ''' # To decide if we can run unsafe code, we check the course id against # a list of regexes configured on the server. for regex in settings.COURSES_WITH_UNSAFE_CODE: From b32c4aaa569e9fdbe9b58d81e14d677b26281faa Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 21:52:48 -0400 Subject: [PATCH 10/14] actually, seems like we should be using double quotes for docstrings --- common/djangoapps/util/sandboxing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index f3c7146148..7d1c1da06f 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -3,7 +3,7 @@ from django.conf import settings def can_execute_unsafe_code(course_id): - ''' + """ Determine if this course is allowed to run unsafe code. For use from the ModuleStore. Checks the `course_id` against a list of whitelisted @@ -11,7 +11,7 @@ def can_execute_unsafe_code(course_id): Returns a boolean, true if the course can run outside the sandbox. - ''' + """ # To decide if we can run unsafe code, we check the course id against # a list of regexes configured on the server. for regex in settings.COURSES_WITH_UNSAFE_CODE: From 5088450ed347e41f0815826d44a4a138f4f44dd2 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 22:09:51 -0400 Subject: [PATCH 11/14] add another unit test to explicitly exercise the can_execute_unsafe_code() method --- .../djangoapps/util/tests/test_sandboxing.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 common/djangoapps/util/tests/test_sandboxing.py diff --git a/common/djangoapps/util/tests/test_sandboxing.py b/common/djangoapps/util/tests/test_sandboxing.py new file mode 100644 index 0000000000..96547cc1eb --- /dev/null +++ b/common/djangoapps/util/tests/test_sandboxing.py @@ -0,0 +1,28 @@ +""" +Tests for sandboxing.py in util app +""" + +from django.test import TestCase +from util.sandboxing import can_execute_unsafe_code +from django.test.utils import override_settings + + +class SandboxingTest(TestCase): + """ + Test sandbox whitelisting + """ + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) + def test_sandbox_exclusion(self): + """ + Test to make sure that a non-match returns false + """ + self.assertFalse(can_execute_unsafe_code('edX/notful/empty')) + + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) + def test_sandbox_inclusion(self): + """ + Test to make sure that a match works across course runs + """ + self.assertTrue(can_execute_unsafe_code('edX/full/2012_Fall')) + self.assertTrue(can_execute_unsafe_code('edX/full/2013_Spring')) + From ccfc4fc01207e4efab00e01baa5eb282cf7ab6c6 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 22:10:33 -0400 Subject: [PATCH 12/14] remove extra line space at end of file --- common/djangoapps/util/tests/test_sandboxing.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/djangoapps/util/tests/test_sandboxing.py b/common/djangoapps/util/tests/test_sandboxing.py index 96547cc1eb..4bccac707f 100644 --- a/common/djangoapps/util/tests/test_sandboxing.py +++ b/common/djangoapps/util/tests/test_sandboxing.py @@ -25,4 +25,3 @@ class SandboxingTest(TestCase): """ self.assertTrue(can_execute_unsafe_code('edX/full/2012_Fall')) self.assertTrue(can_execute_unsafe_code('edX/full/2013_Spring')) - From 656f2b15fd056356782629c66a3dfd66a6c5be42 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 2 Jul 2013 22:16:06 -0400 Subject: [PATCH 13/14] remove redundent test until we get clarity as to if unit tests are actually using the codejail functionality or not --- .../contentstore/tests/test_contentstore.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index ef5da56299..a59394aa85 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -366,25 +366,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get(url) self.assertEqual(resp.status_code, 200) - @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/dummycourse/.*']) - def test_module_preview_not_in_whitelist(self): - ''' - Tests the ajax callback to render an XModule problem but we don't have the course listed in - our whitelist. - ''' - direct_store = modulestore('direct') - import_from_xml(direct_store, 'common/test/data/', ['full']) - - html_module_location = Location(['i4x', 'edX', 'full', 'html', 'html_90', None]) - - url = reverse('preview_component', kwargs={'location': html_module_location.url()}) - - # also try a custom response which will trigger the 'is this course in whitelist' logic - problem_module_location = Location(['i4x', 'edX', 'full', 'problem', 'H1P1_Energy', None]) - url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) - resp = self.client.get(url) - self.assertEqual(resp.status_code, 200) - def test_delete(self): direct_store = modulestore('direct') import_from_xml(direct_store, 'common/test/data/', ['full']) From a224c1e8a3bcaa4e9b6f91dcff6d61a14f0c34c4 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 3 Jul 2013 14:01:33 -0400 Subject: [PATCH 14/14] need to change aws.py to take the COURSES_WITH_UNSAFE_CODE from the envs.json file --- cms/envs/aws.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index c6a383211f..3b8baabc0a 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -105,6 +105,8 @@ ADMINS = ENV_TOKENS.get('ADMINS', ADMINS) SERVER_EMAIL = ENV_TOKENS.get('SERVER_EMAIL', SERVER_EMAIL) MKTG_URLS = ENV_TOKENS.get('MKTG_URLS', MKTG_URLS) +COURSES_WITH_UNSAFE_CODE = ENV_TOKENS.get("COURSES_WITH_UNSAFE_CODE", []) + #Timezone overrides TIME_ZONE = ENV_TOKENS.get('TIME_ZONE', TIME_ZONE)