From 9cdea8f97e07af8f5907f58f518ac15743cca8c0 Mon Sep 17 00:00:00 2001 From: Devasia Joseph Date: Wed, 20 May 2026 10:08:06 +0530 Subject: [PATCH 1/2] fix: resolve scorm assets at the exact requested path --- ...260504184023_djoseph_assets_proxy_paths.md | 1 + openedxscorm/scormxblock.py | 41 +++++++- openedxscorm/tests.py | 98 ++++++++++++++++++- 3 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 changelog.d/20260504184023_djoseph_assets_proxy_paths.md diff --git a/changelog.d/20260504184023_djoseph_assets_proxy_paths.md b/changelog.d/20260504184023_djoseph_assets_proxy_paths.md new file mode 100644 index 0000000..8ccb3da --- /dev/null +++ b/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) diff --git a/openedxscorm/scormxblock.py b/openedxscorm/scormxblock.py index 6ae123e..42836d8 100644 --- a/openedxscorm/scormxblock.py +++ b/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): """ diff --git a/openedxscorm/tests.py b/openedxscorm/tests.py index 3f025c3..633b169 100644 --- a/openedxscorm/tests.py +++ b/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" ) From e1701b41adece42064e028dffec16e7d5cde6f2d Mon Sep 17 00:00:00 2001 From: Devasia Joseph Date: Wed, 20 May 2026 14:22:31 +0530 Subject: [PATCH 2/2] chore: version bump to 19.0.4 --- openedxscorm/__about__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedxscorm/__about__.py b/openedxscorm/__about__.py index f48c1ca..14126cb 100644 --- a/openedxscorm/__about__.py +++ b/openedxscorm/__about__.py @@ -1 +1 @@ -__version__ = "19.0.3" +__version__ = "19.0.4"