diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index c94038a508b5..831ba2d8fe6d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -1948,6 +1948,32 @@ class TestEditItem(TestEditItemSetup): Test xblock update. """ + def test_data_saved_with_portable_static_urls(self): + """ + Absolute asset URLs referencing this course's assets must be contracted + to the portable /static/ form before persisting, so re-runs and + export/import don't break them when an editor fails to contract. + """ + asset_key = self.course.id.make_asset_key('asset', 'textlog.html') + self.client.ajax_post( + self.problem_update_url, + data={'data': f''}, + ) + problem = self.get_item_from_modulestore(self.problem_usage_key) + self.assertIn('html_file="/static/textlog.html"', problem.data) + + def test_data_preserves_other_courses_asset_urls(self): + """ + Asset URLs referencing another course's assets are not ours to rewrite. + """ + foreign_url = '/asset-v1:OtherX+Other+1T2020+type@asset+block@textlog.html' + self.client.ajax_post( + self.problem_update_url, + data={'data': f''}, + ) + problem = self.get_item_from_modulestore(self.problem_usage_key) + self.assertIn(f'html_file="{foreign_url}"', problem.data) + def test_delete_field(self): """ Sending null in for a field 'deletes' it diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index ae7492ddbc91..893d6ad6f316 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -42,7 +42,7 @@ from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink from cms.lib.xblock.upstream_sync_block import sync_from_upstream_block from cms.lib.xblock.upstream_sync_container import sync_from_upstream_container -from common.djangoapps.static_replace import replace_static_urls +from common.djangoapps.static_replace import contract_static_urls, replace_static_urls from common.djangoapps.student.auth import ( has_studio_read_access, has_studio_write_access, @@ -353,6 +353,11 @@ def _save_xblock( old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) if data: + if isinstance(data, str): + # Editors receive `data` with /static/ URLs expanded (see get_block_info) + # and don't all contract them back on save; contract here so URLs pinned + # to this course are never persisted (they break on re-run/export/import). + data = contract_static_urls(data, xblock.location.course_key) # TODO Allow any scope.content fields not just "data" (exactly like the get below this) xblock.data = data else: diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index cfab4dde8375..6d19a521d199 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -2,6 +2,7 @@ import logging import re +import uuid from django.conf import settings from django.contrib.staticfiles import finders @@ -240,3 +241,51 @@ def replace_static_url(original, prefix, quote, rest): return "".join([quote, url, quote]) return process_static_urls(text, replace_static_url, data_dir=static_asset_path or data_directory) + + +def contract_static_urls(text, course_id): + """ + Reverse of `replace_static_urls` for a course's own assets: rewrite absolute + asset URLs referencing ``course_id``'s assets back to the portable + ``/static/`` form. + + Handles every form `StaticContent.get_canonicalized_asset_path` can emit: + relative (``/asset-v1:...@name`` or ``.../name``), versioned + (``/assets/courseware/v1//asset-v1:...``), host-qualified + (``https://host/asset-v1:...``), and old-style ``/c4x/...`` paths. + + URLs referencing other courses' assets, genuinely external URLs, and + already-portable ``/static/`` paths are left untouched. + + text: The source text to do the substitution in + course_id: The course whose asset URLs should be made portable + """ + if not course_id or not isinstance(text, str): + return text + + # Serialize an asset key with a placeholder name, then split it into the + # course-specific prefix and the separator preceding the asset name. + placeholder = uuid.uuid4().hex + serialized = str(course_id.make_asset_key('asset', placeholder)) + name_index = serialized.rfind(placeholder) + key_prefix, separator = serialized[:name_index - 1], serialized[name_index - 1] + # Modern keys serialize with '@' before the name, but canonicalized paths + # may separate the name with '/' instead; accept either. + separator_pattern = '[@/]' if separator == '@' else re.escape(separator) + + asset_url_pattern = re.compile( + r"""(?P\\?['"])""" # the opening quotes + r"""(?:(?:https?:)?//[^'"/]+)?""" # optional scheme and host + r"""(?:/assets/courseware/(?:v\d+/)?[a-f0-9]{32})?""" # optional versioned-asset prefix + r"""/?""" + re.escape(key_prefix) + separator_pattern + + r"""(?P[^'"?]*)""" # the asset name + r"""(?P\?[^'"]*)?""" # optional query string + r"""(?P=quote)""" # the first matching closing quote + ) + + def contract(match): + quote = match.group('quote') + query = match.group('query') or '' + return f"{quote}/static/{match.group('name')}{query}{quote}" + + return asset_url_pattern.sub(contract, text) diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index c71f8fa15c6a..b6b62f26c2b4 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -15,6 +15,7 @@ from common.djangoapps.static_replace import ( _url_replace_regex, + contract_static_urls, make_static_urls_absolute, process_static_urls, replace_course_urls, @@ -62,6 +63,71 @@ def test_multi_replace(): replace_course_urls(replace_course_urls(course_source, COURSE_KEY), COURSE_KEY) +SPLIT_COURSE_KEY = CourseKey.from_string('course-v1:HarvardX+BDSG+2T2022') + + +def test_contract_static_urls_jsinput_html_file(): + text = '' + expected = '' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected + + +def test_contract_static_urls_versioned_asset_path(): + text = ( + '' + ) + expected = '' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected + + +def test_contract_static_urls_versioned_asset_path_without_version_segment(): + # VERSIONED_ASSETS_PATTERN treats the v1/ segment as optional + text = ( + '' + ) + expected = '' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected + + +def test_contract_static_urls_host_qualified(): + text = 'Syllabus' + expected = 'Syllabus' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected + + +def test_contract_static_urls_preserves_query_string(): + text = '' + expected = '' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected + + +def test_contract_static_urls_leaves_foreign_course_urls(): + """URLs pointing at a different course's assets are not this course's to rewrite.""" + text = '' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == text + + +def test_contract_static_urls_leaves_external_and_portable_urls(): + text = 'x ' + assert contract_static_urls(text, SPLIT_COURSE_KEY) == text + + +def test_contract_static_urls_old_style_course_key(): + text = '' + expected = '' + assert contract_static_urls(text, COURSE_KEY) == expected + + +def test_contract_static_urls_non_string_passthrough(): + assert contract_static_urls(None, SPLIT_COURSE_KEY) is None + assert contract_static_urls('', SPLIT_COURSE_KEY) == '' + data = {'key': 'value'} + assert contract_static_urls(data, SPLIT_COURSE_KEY) is data + assert contract_static_urls('text', None) == 'text' + + def test_process_url(): def processor(__, prefix, quote, rest): # pylint: disable=redefined-outer-name return quote + 'test' + prefix + rest + quote