fix: resolve scorm assets at the exact requested path#114
Open
djoseph-apphelix wants to merge 1 commit into
Open
fix: resolve scorm assets at the exact requested path#114djoseph-apphelix wants to merge 1 commit into
djoseph-apphelix wants to merge 1 commit into
Conversation
| """ | ||
| 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") |
Contributor
There was a problem hiding this comment.
I think with the error, we should log what kind of path scorm is expecting. Similarly below.
| try: | ||
| clean_suffix = self.clean_asset_path(suffix) | ||
| except ScormError: | ||
| logger.warning("Invalid asset path: %r", suffix) |
Contributor
There was a problem hiding this comment.
Suggested change
| logger.warning("Invalid asset path: %r", suffix) | |
| logger.error("Invalid asset path: %r", suffix) |
| ) | ||
| return block | ||
|
|
||
| @staticmethod |
Contributor
There was a problem hiding this comment.
I love the fact that you added tests. Thank you so much for this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fix SCORM asset proxy resolution so asset requests prefer the exact requested path instead of resolving by basename only.
Previously,
assets_proxydiscarded the directory portion of the requested URL and recursively searched for the first file with the same basename. Packages containing duplicate filenames at different paths could therefore serve the wrong asset, causing runtime failures such as blank-page rendering in Articulate Rise exports with multiplescormdriver.jsfiles.Reported issue
Issue No:112
Details
..%2E%2E400 Bad Requestfor invalid asset URLs instead of surfacing an unhandled exception.Notes
The basename fallback is intentionally preserved for backward compatibility, but exact-path lookup now handles normal requests first to avoid duplicate-basename collisions.
Invalid asset URLs now return
400 Bad Request. The pre-existingScormErrorbehavior for missing files found viafind_file_pathis unchanged in this PR.Testing
python -m pytest openedxscorm/tests.py -k 'assets_proxy or clean_asset_path'Result:
36 passed, 19 deselected