fix: contract course asset URLs to /static/ before saving xblock data (LP-704)

Author: djoseph-apphelix

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><jsinput html_file="/{asset_key}" gradefn="getGrade"/></problem>'},
+        )
+        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><jsinput html_file="{foreign_url}" gradefn="getGrade"/></problem>'},
+        )
+        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

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:

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/<name>`` form.
+
+    Handles every form `StaticContent.get_canonicalized_asset_path` can emit:
+    relative (``/asset-v1:...@name`` or ``.../name``), versioned
+    (``/assets/courseware/v1/<digest>/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<quote>\\?['"])"""                          # 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<name>[^'"?]*)"""                           # the asset name
+        r"""(?P<query>\?[^'"]*)?"""                        # 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)

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,61 @@ 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 = '<jsinput html_file="/asset-v1:HarvardX+BDSG+2T2022+type@asset+block@textlog.html" gradefn="getGrade"/>'
+    expected = '<jsinput html_file="/static/textlog.html" gradefn="getGrade"/>'
+    assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected
+
+
+def test_contract_static_urls_versioned_asset_path():
+    text = (
+        '<img src="/assets/courseware/v1/43761bc1cc13b218d267f790ed4313d1/'
+        'asset-v1:HarvardX+BDSG+2T2022+type@asset+block/image.jpg">'
+    )
+    expected = '<img src="/static/image.jpg">'
+    assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected
+
+
+def test_contract_static_urls_host_qualified():
+    text = '<a href="https://courses.edx.org/asset-v1:HarvardX+BDSG+2T2022+type@asset+block@syllabus.pdf">Syllabus</a>'
+    expected = '<a href="/static/syllabus.pdf">Syllabus</a>'
+    assert contract_static_urls(text, SPLIT_COURSE_KEY) == expected
+
+
+def test_contract_static_urls_preserves_query_string():
+    text = '<img src="/asset-v1:HarvardX+BDSG+2T2022+type@asset+block@pic.png?width=100">'
+    expected = '<img src="/static/pic.png?width=100">'
+    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 = '<img src="/asset-v1:OtherX+Other+1T2020+type@asset+block@pic.png">'
+    assert contract_static_urls(text, SPLIT_COURSE_KEY) == text
+
+
+def test_contract_static_urls_leaves_external_and_portable_urls():
+    text = '<a href="https://example.com/asset.png">x</a> <img src="/static/already.png">'
+    assert contract_static_urls(text, SPLIT_COURSE_KEY) == text
+
+
+def test_contract_static_urls_old_style_course_key():
+    text = '<img src="/c4x/org/course/asset/foo.gif">'
+    expected = '<img src="/static/foo.gif">'
+    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