Version bump

changelog.d/20260504184023_djoseph_assets_proxy_paths.md

@@ -0,0 +1 @@
+- [Bugfix] Serve SCORM assets at the exact requested path, fixing blank-page rendering for packages containing duplicate basenames such as Articulate Rise packages with multiple `scormdriver.js` files. Also reject path traversal and absolute paths in asset URLs. (by @djoseph)

openedxscorm/__about__.py

@@ -1 +1 @@
-__version__ = "19.0.3"
+__version__ = "19.0.4"

openedxscorm/scormxblock.py

@@ -230,8 +230,20 @@ def assets_proxy(self, request, suffix):
         -------
         Response object containing the content of the requested file with the appropriate content type.
         """
-        file_name = os.path.basename(suffix)
-        file_path = self.find_file_path(file_name)        
+        try:
+            clean_suffix = self.clean_asset_path(suffix)
+        except ScormError as exc:
+            logger.warning("Invalid asset path %r: %s", suffix, exc)
+            return Response("Invalid asset path", status=400, content_type="text/plain")
+        file_name = os.path.basename(clean_suffix)
+        requested_path = os.path.join(self.extract_folder_path, clean_suffix)
+        if self.storage.exists(requested_path):
+            file_path = requested_path
+        else:
+            # Preserve legacy basename lookup for packages or callers that do
+            # not request the extracted relative path. Exact-path lookup above
+            # avoids this fallback for normal requests with duplicate basenames.
+            file_path = self.find_file_path(file_name)
         file_type, _ = mimetypes.guess_type(file_name)
         with self.storage.open(file_path) as response:
             file_content = response.read()
@@ -419,6 +431,31 @@ def clean_path(self, path):
         Removes query string from a path
         """
         return path.split('?')[0] if path else path
+
+    def clean_asset_path(self, path):
+        """
+        Clean and validate an asset path requested through the proxy.
+        """
+        cleaned = urllib.parse.unquote(self.clean_path(path))
+        if not cleaned or "\x00" in cleaned or cleaned.endswith(("/", OS_PATH_ALT_SEP)):
+            raise ScormError(
+                "Invalid asset path: expected a non-empty relative file path "
+                "without null bytes or trailing separators"
+            )
+        normalized_separators_path = cleaned.replace(OS_PATH_ALT_SEP, os.path.sep)
+        path_parts = normalized_separators_path.split(os.path.sep)
+        cleaned = os.path.normpath(normalized_separators_path)
+        if (
+            os.path.isabs(cleaned)
+            or cleaned == os.curdir
+            or re.match(r"^[A-Za-z]:", cleaned)
+            or os.pardir in path_parts
+        ):
+            raise ScormError(
+                "Invalid asset path: expected a relative path within the package, "
+                "not an absolute path, drive letter, or parent-directory reference"
+            )
+        return cleaned
 
     def path_exists(self, path):
         """

openedxscorm/tests.py

@@ -8,7 +8,7 @@
 import mock
 from xblock.field_data import DictFieldData
 
-from .scormxblock import ScormXBlock
+from .scormxblock import ScormError, ScormXBlock
 
 
 @ddt
@@ -25,6 +25,14 @@ def make_one(**kw):
         )
         return block
 
+    @staticmethod
+    def make_storage_with_file(content=b"content"):
+        storage = mock.Mock()
+        opened_file = mock.MagicMock()
+        opened_file.__enter__.return_value.read.return_value = content
+        storage.open.return_value = opened_file
+        return storage
+
     def test_fields_xblock(self):
         block = self.make_one()
         self.assertEqual(block.display_name, "Scorm")
@@ -124,6 +132,94 @@ def test_build_file_storage_path(self):
 
         self.assertEqual(file_storage_path, "org/course/block_type/block_id/sha1.html")
 
+    @mock.patch.object(
+        ScormXBlock, "extract_folder_path", new_callable=mock.PropertyMock
+    )
+    def test_assets_proxy_serves_exact_requested_path(self, extract_folder_path):
+        block = self.make_one()
+        storage = self.make_storage_with_file(b"exact asset")
+        storage.exists.return_value = True
+        block._storage = storage
+        block.find_file_path = mock.Mock(return_value="scorm/block/sha1/other/app.js")
+        extract_folder_path.return_value = "scorm/block/sha1"
+
+        response = block.assets_proxy(mock.Mock(), "assets/app.js")
+
+        storage.exists.assert_called_once_with("scorm/block/sha1/assets/app.js")
+        block.find_file_path.assert_not_called()
+        storage.open.assert_called_once_with("scorm/block/sha1/assets/app.js")
+        self.assertEqual(response.body, b"exact asset")
+
+    @mock.patch.object(
+        ScormXBlock, "extract_folder_path", new_callable=mock.PropertyMock
+    )
+    def test_assets_proxy_fallback_uses_cleaned_basename(self, extract_folder_path):
+        block = self.make_one()
+        storage = self.make_storage_with_file()
+        storage.exists.return_value = False
+        block._storage = storage
+        block.find_file_path = mock.Mock(return_value="scorm/block/sha1/fallback/app.js")
+        extract_folder_path.return_value = "scorm/block/sha1"
+
+        block.assets_proxy(mock.Mock(), "assets/app.js?v=1")
+
+        storage.exists.assert_called_once_with("scorm/block/sha1/assets/app.js")
+        block.find_file_path.assert_called_once_with("app.js")
+        storage.open.assert_called_once_with("scorm/block/sha1/fallback/app.js")
+
+    @data(
+        "",
+        "?v=1",
+        ".",
+        "/app.js",
+        "%2Fapp.js",
+        "%5Capp.js",
+        r"C:\app.js",
+        "C%3A/app.js",
+        "../app.js",
+        "%2E%2E/app.js",
+        "assets/",
+        "assets/..",
+        "assets/%2E%2E/app.js",
+        "assets/../../app.js",
+        "assets/%2E%2E/%2E%2E/app.js",
+        r"assets\..\app.js",
+        "assets/%00/app.js",
+    )
+    def test_assets_proxy_rejects_invalid_requested_paths(self, suffix):
+        block = self.make_one()
+
+        response = block.assets_proxy(mock.Mock(), suffix)
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.content_type, "text/plain")
+        self.assertEqual(response.body, b"Invalid asset path")
+
+    @data(
+        "",
+        "?v=1",
+        ".",
+        "/app.js",
+        "%2Fapp.js",
+        "%5Capp.js",
+        r"C:\app.js",
+        "C%3A/app.js",
+        "../app.js",
+        "%2E%2E/app.js",
+        "assets/",
+        "assets/..",
+        "assets/%2E%2E/app.js",
+        "assets/../../app.js",
+        "assets/%2E%2E/%2E%2E/app.js",
+        r"assets\..\app.js",
+        "assets/%00/app.js",
+    )
+    def test_clean_asset_path_rejects_invalid_requested_paths(self, suffix):
+        block = self.make_one()
+
+        with self.assertRaises(ScormError):
+            block.clean_asset_path(suffix)
+
     @mock.patch(
         "openedxscorm.ScormXBlock._file_storage_path", return_value="file_storage_path"
     )