Fix duplicate file naming for "Download All" submissions#2312
Open
Fix duplicate file naming for "Download All" submissions#2312
Conversation
Contributor
📝 WalkthroughWalkthroughRefactored ZIP packaging logic in the submissions download feature to pre-filter invalid submissions and replace per-entry duplicate tracking with a Set-based collision-avoidance approach that appends incremental suffixes for duplicate filenames. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/users_controller.rb (1)
174-185: Eager-load associations to avoid N+1 queries during export.Line 181-184 and Line 206-207 access nested associations per submission; this can get expensive for large download batches. Consider eager loading
assessmentandcourse_user_datum.courseup front.♻️ Suggested refactor
- submissions = if params[:final] - Submission.latest.where(course_user_datum: CourseUserDatum.where(user_id: user)) - else - Submission.where(course_user_datum: CourseUserDatum.where(user_id: user)) - end + base_scope = params[:final] ? Submission.latest : Submission.all + submissions = base_scope + .where(course_user_datum: CourseUserDatum.where(user_id: user)) + .includes(:assessment, course_user_datum: :course)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/users_controller.rb` around lines 174 - 185, The submission query is causing N+1s when later accessing s.assessment and s.course_user_datum.course; modify both branches where you build submissions (the Submission.latest.where(...) and Submission.where(...)) to eager-load those associations (e.g. add includes(:assessment, course_user_datum: :course) or preload(...) ) so assessment and course_user_datum.course are fetched in bulk before the subsequent filtering and file checks on s.handin_file_path and s.course_user_datum.course.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/users_controller.rb`:
- Around line 210-223: The code builds ZIP entry paths using raw course_name,
assignment_name and the filename derived in download_filename(p,
assignment_name), which allows `"/"`, `"\"` or `".."` to create unsafe paths;
add a private helper sanitize_zip_component(value) that replaces slashes and
backslashes and collapses `".."` (e.g. gsub %r{[\\\/]} with "_" and
gsub("..","_")), then call it for course_name, assignment_name and the basename
used to generate new_entry_name before composing assignment_directory and
final_entry_path (update uses of assignment_directory, final_entry_path and the
basename variable to use the sanitized values).
---
Nitpick comments:
In `@app/controllers/users_controller.rb`:
- Around line 174-185: The submission query is causing N+1s when later accessing
s.assessment and s.course_user_datum.course; modify both branches where you
build submissions (the Submission.latest.where(...) and Submission.where(...))
to eager-load those associations (e.g. add includes(:assessment,
course_user_datum: :course) or preload(...) ) so assessment and
course_user_datum.course are fetched in bulk before the subsequent filtering and
file checks on s.handin_file_path and s.course_user_datum.course.
Comment on lines
+210
to
+223
| assignment_directory = "#{filename}/#{course_name}/#{assignment_name}" | ||
| original_entry_name = download_filename(p, assignment_name) | ||
|
|
||
| extname = File.extname(original_entry_name) | ||
| basename = File.basename(original_entry_name, extname) | ||
|
|
||
| # Find unique file name for assignment to be saved as | ||
| final_entry_path = File.join(assignment_directory, original_entry_name) | ||
| # If file name duplicate exists, add "_#" suffix | ||
| version_counter = 2 | ||
| while used_paths.include?(final_entry_path) | ||
| new_entry_name = "#{basename}_#{version_counter}#{extname}" | ||
| final_entry_path = File.join(assignment_directory, new_entry_name) | ||
| version_counter += 1 |
Contributor
There was a problem hiding this comment.
Sanitize ZIP entry path components before composing final_entry_path.
Line 210-217 uses raw course_name, assignment_name, and generated filename in zip paths. If any contains /, \, or .., archive entries can become unsafe or malformed.
🔒 Suggested hardening
- assignment_directory = "#{filename}/#{course_name}/#{assignment_name}"
- original_entry_name = download_filename(p, assignment_name)
+ safe_course_name = sanitize_zip_component(course_name)
+ safe_assignment_name = sanitize_zip_component(assignment_name)
+ original_entry_name = sanitize_zip_component(download_filename(p, assignment_name))
+ assignment_directory = File.join(filename, safe_course_name, safe_assignment_name)# add under private
def sanitize_zip_component(value)
value.to_s.gsub(%r{[\\\/]}, "_").gsub("..", "_")
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/users_controller.rb` around lines 210 - 223, The code builds
ZIP entry paths using raw course_name, assignment_name and the filename derived
in download_filename(p, assignment_name), which allows `"/"`, `"\"` or `".."` to
create unsafe paths; add a private helper sanitize_zip_component(value) that
replaces slashes and backslashes and collapses `".."` (e.g. gsub %r{[\\\/]} with
"_" and gsub("..","_")), then call it for course_name, assignment_name and the
basename used to generate new_entry_name before composing assignment_directory
and final_entry_path (update uses of assignment_directory, final_entry_path and
the basename variable to use the sanitized values).
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
1 participant
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.
Description
Previously, this issue came up in production, and it was addressed with a double pass here: #2244. The idea was to check if a file name is duplicated and, if so, to append a version number to the file name to prevent issues when saving it in a zip file.
This new implementation checks this concern more safely by directly checking when saving if a rename is needed, and if so, to rename the file accordingly.
Motivation and Context
Failure in production.
How Has This Been Tested?
This issue seems to come up for 15213's bomb and attack labs, as those labs are submitted through the terminal in "phases". Typically, Autolab will automatically append submission version numbers to the files.
To emulate this for testing, I made "submissions" store multiple versions of the same submission to check that naming works as expected.
Types of changes