From 8ab62b5b993c45f911a1657f276ce69fc8283c77 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 12 Jul 2016 21:13:30 -0400 Subject: [PATCH] [PERF-351] Account for settings.STATIC_URL in XBlock resource URLs when rewriting them. In PERF-341, we adjusted the static_replace middleware to try and exclude static XBlock resource URLs from being interpreted as the marker URLs used to signify course assets in course content. Since they both started with /static, this could, and did, cause issues where linking directly to the assets of an XBlock within, say, one of its templates, would lead to that link being rewritten and ultimately being incorrect. The fix attempted to see if the link started with the prefix that all static XBlock resource URLs start, and if so, it returned them unmodified. We incorrectly assumed that our testing captured all cases, and since we're here, we know that this was wrong. We weren't accounting for cases when the URLs being generated had the STATIC_URL configuration value prefixed -- https://example.com/static/xblock/.... -- and so our direct check of seeing if such a URL started with "/static/xblock" would always fail, leading to the erroneous rewriting and nonsensical output. This fix checks if the link either starts with the prefix OR if it starts with the STATIC_URL value and contains the prefix overall. There is a small overlap between the STATIC_URL and the prefix we check for, so an inconsistency could arise down the line if we changed our STATIC_URL to use a difference base directory, but our tests will at least catch the issue now. --- common/djangoapps/static_replace/__init__.py | 6 +++++- .../static_replace/test/test_static_replace.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index e2a8fe5069..a4a63430f5 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -114,7 +114,11 @@ def process_static_urls(text, replacement_function, data_dir=None): # Don't rewrite XBlock resource links. Probably wasn't a good idea that /static # works for actual static assets and for magical course asset URLs.... full_url = prefix + rest - if full_url.startswith(XBLOCK_STATIC_RESOURCE_PREFIX): + + starts_with_static_url = full_url.startswith(unicode(settings.STATIC_URL)) + starts_with_prefix = full_url.startswith(XBLOCK_STATIC_RESOURCE_PREFIX) + contains_prefix = XBLOCK_STATIC_RESOURCE_PREFIX in full_url + if starts_with_prefix or (starts_with_static_url and contains_prefix): return original return replacement_function(original, prefix, quote, rest) diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index 906ee28c78..1b90e5da1f 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -4,6 +4,7 @@ import ddt import re +from django.test import override_settings from django.utils.http import urlquote, urlencode from urlparse import urlparse, urlunparse, parse_qsl from PIL import Image @@ -211,6 +212,22 @@ def test_static_url_with_xblock_resource(mock_modulestore, mock_storage): assert_equals(post_text, replace_static_urls(pre_text, DATA_DIRECTORY, COURSE_KEY)) +@patch('static_replace.staticfiles_storage', autospec=True) +@patch('static_replace.modulestore', autospec=True) +@override_settings(STATIC_URL='https://example.com/static/') +def test_static_url_with_xblock_resource_on_cdn(mock_modulestore, mock_storage): + """ + Make sure that for URLs with XBlock resource URL, which start with /static/, + we don't rewrite them, even if these are served from an absolute URL like a CDN. + """ + mock_storage.exists.return_value = False + mock_modulestore.return_value = Mock(MongoModuleStore) + + pre_text = 'EMBED src ="https://example.com/static/xblock/resources/tehehe.xblock/public/images/woo.png"' + post_text = pre_text + assert_equals(post_text, replace_static_urls(pre_text, DATA_DIRECTORY, COURSE_KEY)) + + @ddt.ddt class CanonicalContentTest(SharedModuleStoreTestCase): """