[Bugfix:InstructorUI] Fix Course Materials Prefix Collisions#12208
[Bugfix:InstructorUI] Fix Course Materials Prefix Collisions#12208
Conversation
…e materials management.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12208 +/- ##
============================================
- Coverage 21.71% 21.71% -0.01%
- Complexity 9601 9602 +1
============================================
Files 268 268
Lines 36038 36043 +5
Branches 475 475
============================================
Hits 7827 7827
- Misses 27740 27745 +5
Partials 471 471
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Rkoester47
left a comment
There was a problem hiding this comment.
I tested making directories/sub-directories on this branch and it seems the bug has been fixed. The changes to the code look good and are applied in the place where this issue could possibly be seen.
I made Test/Test01 and Tests/Test0. Deleting Test01 and Test0 do not interfere with separate parent directories, as intended. Also, if Test01 or Test0 are the only sub-folder in their parent directory, deleting them will also delete the parent directory. The deletion of these subfolders do not seem to have unintended consequences for other folders.
JManion32
left a comment
There was a problem hiding this comment.
I was able to replicate the issue on main and found that this PR addresses it. This logic eliminates the risk of prefix collisions, preventing instructors from unnecessarily losing course materials when deleting unrelated directories.
Why is this Change Important & Necessary?
Fixes #12203.
As detailed by the issue itself, there is a significant flaw in the implementation of course materials file deletion. In the current production version, if there is a directory which shares the same prefix as another directory, such as
tests/test0andtest/test01, the deletion of thetest01subdirectory would (assuming no other directories intest) cascade to deletetestsand all of its subdirectories. This is due to an improper usage ofstr_starts_within theCourseMaterialsController.What is the New Behavior?
The error has been corrected by enforcing a strict directory boundary to avoid prefix collisions.
What steps should a reviewer take to reproduce or test the bug or new feature?
Checkout the
mainbranch. As an instructor, create directories in a course's course materials such that the exploit is reproducible, as intests/test0andtest/test01, and then try deleting (in separate attempts) the top directory, immediate subdirectory, or file with respect to the directory that has the shorter handle. Notice the error. Repeat this process with this branch (course-materials-edit-bugfix) and notice that the behavior is as intended.