enforce whitelist for rewrites

Cleanup

more defensive

Addtional tests

quality

further quality fixes

cms/envs/aws.py

@@ -376,6 +376,10 @@ PARSE_KEYS = AUTH_TOKENS.get("PARSE_KEYS", {})
 # Example: {'CN': 'http://api.xuetangx.com/edx/video?s3_url='}
 VIDEO_CDN_URL = ENV_TOKENS.get('VIDEO_CDN_URL', {})
 
+# Whitelist of re-writable sources, only video sources from whitelisted domains
+# will be re-written.  Should be the "netloc" of the URL, for example, www.example.com
+VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS = ENV_TOKENS.get('VIDEO_CDN_REWRITABLE_SOURCES', [])
+
 if FEATURES['ENABLE_COURSEWARE_INDEX'] or FEATURES['ENABLE_LIBRARY_INDEX']:
     # Use ElasticSearch for the search engine
     SEARCH_ENGINE = "search.elastic.ElasticSearchEngine"

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -202,6 +202,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
         # based on user locale.  This exists to support cases where
         # we leverage a geography specific CDN, like China.
         cdn_url = getattr(settings, 'VIDEO_CDN_URL', {}).get(self.system.user_location)
+        cdn_rewritable_source_domains = getattr(settings, 'VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS', [])
 
         # If we have an edx_video_id, we prefer its values over what we store
         # internally for download links (source, html5_sources) and the youtube
@@ -223,7 +224,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
                             sources.append(url)
                         if self.download_video:
                             # function returns None when the url cannot be re-written
-                            rewritten_link = rewrite_video_url(cdn_url, url)
+                            rewritten_link = rewrite_video_url(cdn_url, url, cdn_rewritable_source_domains)
                             if rewritten_link:
                                 download_video_link = rewritten_link
                             else:
@@ -247,7 +248,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
             branding_info = BrandingInfoConfig.get_config().get(self.system.user_location)
 
             for index, source_url in enumerate(sources):
-                new_url = rewrite_video_url(cdn_url, source_url)
+                new_url = rewrite_video_url(cdn_url, source_url, cdn_rewritable_source_domains)
                 if new_url:
                     sources[index] = new_url
 

common/lib/xmodule/xmodule/video_module/video_utils.py

@@ -37,7 +37,7 @@ def create_youtube_string(module):
     ])
 
 
-def rewrite_video_url(cdn_base_url, original_video_url):
+def rewrite_video_url(cdn_base_url, original_video_url, whitelist):
     """
     Returns a re-written video URL for cases when an alternate source
     has been configured and is selected using factors like
@@ -57,6 +57,21 @@ def rewrite_video_url(cdn_base_url, original_video_url):
         return None
 
     parsed = urlparse(original_video_url)
+
+    # malformed input, say not scheme, results in the netloc
+    # being returned as an empty string.
+    if not parsed.netloc:
+        return None
+    
+    # Only re-write URLs if the original netloc is in the whitelist, otherwise
+    # no guarantees can be made that the re-write target can service the request.
+    try:
+        if parsed.netloc not in whitelist:
+            return None
+    except TypeError:
+        log.exception("message")
+        return None
+        
     # Contruction of the rewrite url is intentionally very flexible of input.
     # For example, https://www.edx.org/ + /foo.html will be rewritten to
     # https://www.edx.org/foo.html.

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -814,52 +814,96 @@ class TestVideoCDNRewriting(BaseTestXmodule):
     def setUp(self, *args, **kwargs):
         super(TestVideoCDNRewriting, self).setUp(*args, **kwargs)
         self.original_video_file = "original_video.mp4"
-        self.original_video_url = "http://www.originalvideo.com/" + self.original_video_file
+        self.original_video_host = "www.originalvideo.com"
+        self.original_video_url = "https://" + self.original_video_host + "/" + self.original_video_file
 
-    @patch.dict("django.conf.settings.CDN_VIDEO_URLS",
-                {"CN": "https://chinacdn.cn/"})
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=["www.originalvideo.com"])
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
     def test_rewrite_video_url_success(self):
         """
         Test successful CDN request.
         """
-        cdn_response_video_url = settings.CDN_VIDEO_URLS["CN"] + self.original_video_file
+        cdn_response_video_url = settings.VIDEO_CDN_URL["CN"] + self.original_video_file
 
         self.assertEqual(
-            rewrite_video_url(settings.CDN_VIDEO_URLS["CN"], self.original_video_url),
+            rewrite_video_url(settings.VIDEO_CDN_URL["CN"],
+                              self.original_video_url,
+                              settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS),
             cdn_response_video_url
         )
 
-    @patch.dict("django.conf.settings.CDN_VIDEO_URLS",
-                {"CN": "https://chinacdn.cn/"})
-    def test_rewrite_url_concat(self):
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=["www.originalvideo.com","someother.com"])
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
+    def test_rewrite_url_extended_whitelist_success(self):
         """
-        Test that written URLs are returned clean despite input
+        Test that a URL is returned when the whitelist contains the orignial source domain
         """
-        cdn_response_video_url = settings.CDN_VIDEO_URLS["CN"] + "original_video.mp4"
+        cdn_response_video_url = settings.VIDEO_CDN_URL["CN"] + "original_video.mp4"
 
         self.assertEqual(
-            rewrite_video_url(settings.CDN_VIDEO_URLS["CN"] + "///", self.original_video_url),
+            rewrite_video_url(settings.VIDEO_CDN_URL["CN"] + "///",
+                              self.original_video_url,
+                              settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS),
             cdn_response_video_url
         )
 
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=["someother.com","algumaoutra.pt"])
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
+    def test_rewrite_url_extended_whitelist_failure(self):
+        """
+        Test that None is returned if the original URL is not in the whitelist
+        """
+        cdn_response_video_url = settings.VIDEO_CDN_URL["CN"] + "original_video.mp4"
+
+        self.assertIsNone(
+            rewrite_video_url(settings.VIDEO_CDN_URL["CN"] + "///",
+                              self.original_video_url,
+                              settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS),
+            cdn_response_video_url
+        )
+
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=["www.originalvideo.com"])
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
     def test_rewrite_video_url_invalid_url(self):
         """
-        Test if no alternative video in CDN exists.
+        Test that the None is returned for invalid URLs
         """
         invalid_cdn_url = 'http://http://fakecdn.com/'
-        self.assertIsNone(rewrite_video_url(invalid_cdn_url, self.original_video_url))
+        self.assertIsNone(rewrite_video_url(invalid_cdn_url,
+                                            self.original_video_url,
+                                            settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS))
+
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=["oktorewrite.com"])
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
+    def test_rewrite_video_url_whitelist(self):
+        """
+        Test if the original URL is not on the rewrite whitelist
+        """
+        self.assertIsNone(rewrite_video_url(settings.VIDEO_CDN_URL["CN"],
+                                            self.original_video_url,
+                                            settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS))
+
+    @override_settings(VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS=None)
+    @override_settings(VIDEO_CDN_URL={"CN": "https://chinacdn.cn/"})
+    def test_rewrite_video_url_nil_whitelist(self):
+        """
+        Test if the whitelist is None
+        """
+        self.assertIsNone(rewrite_video_url(settings.VIDEO_CDN_URL["CN"],
+                                            self.original_video_url,
+                                            settings.VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS))
 
     def test_none_args(self):
         """
         Ensure None args return None
         """
-        self.assertIsNone(rewrite_video_url(None, None))
+        self.assertIsNone(rewrite_video_url(None, None, None))
 
     def test_emptystring_args(self):
         """
         Ensure emptyrstring args return None
         """
-        self.assertIsNone(rewrite_video_url("", ""))
+        self.assertIsNone(rewrite_video_url("", "", []))
 
 
 @attr('shard_1')

lms/envs/aws.py

@@ -337,6 +337,11 @@ if FEATURES.get('AUTH_USE_CAS'):
 # Example: {'CN': 'http://api.xuetangx.com/edx/video?s3_url='}
 VIDEO_CDN_URL = ENV_TOKENS.get('VIDEO_CDN_URL', {})
 
+# Whitelist of re-writable sources, only video sources from whitelisted domains
+# will be re-written.  Should be the "netloc" of the URL, for example, www.example.com
+VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS = ENV_TOKENS.get('VIDEO_CDN_REWRITABLE_SOURCES', [])
+
+
 # Branded footer
 FOOTER_OPENEDX_URL = ENV_TOKENS.get('FOOTER_OPENEDX_URL', FOOTER_OPENEDX_URL)
 FOOTER_OPENEDX_LOGO_IMAGE = ENV_TOKENS.get('FOOTER_OPENEDX_LOGO_IMAGE', FOOTER_OPENEDX_LOGO_IMAGE)

lms/envs/test.py

@@ -516,6 +516,10 @@ VIDEO_CDN_URL = {
     'CN': 'http://api.xuetangx.com/edx/video?s3_url='
 }
 
+# Whitelist of re-writable sources, only video sources from whitelisted domains
+# will be re-written.  Should be the "netloc" of the URL, for example, www.example.com
+VIDEO_CDN_REWRITABLE_SOURCE_DOMAINS = []
+
 ######### dashboard git log settings #########
 MONGODB_LOG = {
     'host': MONGO_HOST,