[Bugfix:CourseMaterials] Fix invalid filepath breaking database#11909
[Bugfix:CourseMaterials] Fix invalid filepath breaking database#11909
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11909 +/- ##
============================================
- Coverage 21.74% 21.73% -0.01%
- Complexity 9332 9335 +3
============================================
Files 268 268
Lines 35829 35849 +20
Branches 471 474 +3
============================================
+ Hits 7791 7793 +2
- Misses 27571 27586 +15
- Partials 467 470 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
||
| if (!rename($course_material->getPath(), $new_path)) { | ||
| return JsonResponse::getErrorResponse("Failure to rename filepath, likely due to a folder with the same name as the file."); | ||
| } |
There was a problem hiding this comment.
This line does prevent editing course materials, but it does not give a helpful error message to the instructor on what they're doing wrong unless they go into console.
The problem is because the frontend expects just a jsonresponse but it also receives the function failure warning so it can't parse what is going on. We should explicitly check if the filepath contains the same name as the file and then return this json error response instead.
| if (isset($_POST['title']) && $_POST['title'] != null && str_contains($_POST['title'], '/')) { | ||
| return JsonResponse::getErrorResponse("Slash in file title"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this code is necessary since uploadCourseMaterials prevents any slashes in the title anyway and instructors cannot edit this title.
There was a problem hiding this comment.
That's true, I'll remove it.
There was a problem hiding this comment.
The code for handling malformed database paths is solid and successfully detects if there is a non-existent parent directory while also rendering the page. I still have some issues with editing the course materials, though.
Not sure if we want to fully support adding to the folder structure by editing the filename, given that we do check if the filename is valid but don't prevent the ajax request.
That way, the easy fix would be to just add an else statement and return in the js.
If we do want to support it, we should remove the validFileName check so there isn't an alert but explicitly check changes that can create database inconsistencies (whether breaking or not). One is the issue addressed in this PR, editing the filename to have itself as the parent directory (creating a non-existent parent directory).
Another I found was editing a link filename to be in a different directory and then edit the link filename to something completely different (instructors are unable to remove the file at all).
site/public/js/drag-and-drop.js
Outdated
| } | ||
| else { | ||
| if (title !== null) { | ||
| alert("Invalid file name.") |
There was a problem hiding this comment.
The event listener for the Enter key causes the following to appear, but on clicking Submit, this alert displays. The directories are never created, but it's just a subtle bug on the frontend.
There was a problem hiding this comment.
It might be a bit confusing though. Even though this prevents creating directories through filename edits, it gives the alert but also appends the attempted file name edit to the file path for every path, which is weird. Here, I tried editing the 07-23-notes.txt to be 07-23-notes/07-23-notes.txt.
Though, this might disappear when we re-enable creating directories through file name edits, it is a good thing to note for now if it still persists.
Chriun
left a comment
There was a problem hiding this comment.
We discussed in the meeting today that it would be useful to keep the functionality of creating directories through the file name or file path edits. It seems that the changes made now prevent this, though last time I checked it had still worked (but gives the invalid alert).
|
Creating directories through the filename should work more like a feature now. |
Chriun
left a comment
There was a problem hiding this comment.
Changes look good and there is no breaking functionality to either uploading or editing course materials.
jeffrey-cordero
left a comment
There was a problem hiding this comment.
Changes look good to me. The course materials controller and front-end scripts are now more robust than before, verifying invalid file paths with clear and concise error messages.
Why is this Change Important & Necessary?
A recent issue popped up in a class where having a duplicate folder name to its child filename caused the course materials page to become permanently broken, requiring manual intervention in the database. We have replicated this issue as being one that occurs upon course material edit, but this PR also introduces fixes for other potential ways this error could have happened.
What is the New Behavior?
We now throw errors in more of the places where they should be thrown, which should prevent bad course materials from being uploaded and edited.
What steps should a reviewer take to reproduce or test the bug or new feature?
Upload a course material, edit it to have a parent directory with the same name as the file, i.e. 'check.pdf/check.pdf', and then submit. Two alerts should pop up, but upon page reload you should get an error. To reverse this, you must delete the associated course material database entry and the associated file from /var/local/submitty/courses/f25/sample/uploads/course_materials/. Switch to this branch, submitty_install_site, and try to do the same thing. It should now throw the error alert but not cause an error on reload.
Automated Testing & Documentation
This isn't tested, maybe it should be.
Other information
N/A