File manager fixes #2297
File manager fixes #2297
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds tar archive support (preview, list contents, view single entry, stream/generate tars) and permissions display in listings; introduces multiple controller helpers for path mapping and tar handling, a new tar viewing/preview UI, a permissions column, a standalone import_submissions.rb script, and minor JS/CSS formatting and a spec update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as FileManager UI
participant FM as FileManagerController
participant FS as Filesystem
Note over UI,FM: Permissions display and tar actions available in listing
User->>UI: Click "Preview Contents" (tar)
UI->>FM: GET /file_manager?path=...&preview=true
FM->>FM: is_tar_file? -> list_tar_contents(path)
FM->>FM: build preview model (entries with permissions)
FM-->>UI: Render tar_preview (list of entries)
sequenceDiagram
autonumber
actor User
participant UI as FileManager UI
participant FM as FileManagerController
participant Tar as Tar/Gzip libs
participant FS as Filesystem
Note over UI,FM: View single file inside tar
User->>UI: Click entry link (view_tar_file)
UI->>FM: GET file_manager/view_tar_file?tar_path=...&file_path=...
FM->>FM: authorize + validate tar format
FM->>Tar: extract_single_file_from_tar(tar_path, file_path)
Tar-->>FM: file payload or error
alt success (text)
FM-->>UI: render tar_file_view with file contents
else success (binary/attachment)
FM-->>UI: send_data as attachment
else error
FM-->>UI: redirect with flash error
end
sequenceDiagram
autonumber
actor User
participant UI as FileManager UI
participant FM as FileManagerController
participant Tar as Tar writer
participant FS as Filesystem
User->>UI: Click "Download" on directory/course/assessment
UI->>FM: GET download (target)
alt single file
FM->>FS: read file
FM-->>UI: send file
else directory / assessment / course
FM->>Tar: stream tar via add_directory_to_tar / tar_writer
FM->>FS: read entries recursively
Tar-->>FM: tar stream
FM-->>UI: stream tar download
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Files/areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
import_submissions.rb (3)
18-18: Hardcoded course name requires manual editing.The hardcoded course name
"18213-m25"on line 18 requires developers to manually edit the script for each use. This reduces maintainability and increases the risk of errors.Accept the course name as a CLI argument:
-zip_path = ARGV[0] -assessment_name = ARGV[1] +zip_path = ARGV[0] +course_name = ARGV[1] +assessment_name = ARGV[2] unless zip_path && assessment_name - puts "Usage: rails runner import_submissions.rb path/to/zipfile.zip \"Assessment Name\"" + puts "Usage: rails runner import_submissions.rb path/to/zipfile.zip \"Course Name\" \"Assessment Name\"" exit 1 end -# Replace this with your actual course ID -course_name = "18213-m25" # <-- change this to your course name course = Course.find_by(name: course_name)
37-37: Regex could be more precise.The regex pattern on line 37 uses
\d+?(non-greedy) for the ID field, but the?quantifier is unnecessary here since\d+will match the digits before the underscore delimiter naturally due to the fixed pattern structure.Simplify to:
-next unless entry.name =~ /\A(.+?)_\d+?_#{Regexp.escape(assessment_name)}-handin\.tar\z/ +next unless entry.name =~ /\A(.+?)_\d+_#{Regexp.escape(assessment_name)}-handin\.tar\z/
66-73: Error handling doesn't distinguish error types.The generic
rescue => eon line 71 catches all exceptions, making it difficult to diagnose specific failure modes (database errors, file system errors, validation errors, etc.).Add more specific rescue clauses:
begin submission.save! file_param = { "file" => Rack::Test::UploadedFile.new(temp_tar_path, "application/x-tar") } submission.save_file(file_param) puts "Successfully created submission for #{username}" -rescue => e - puts "Failed to create submission for #{username}: #{e.message}" +rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e + puts "Failed to save submission for #{username}: #{e.message}" +rescue Errno::ENOENT, Errno::EACCES => e + puts "File system error for #{username}: #{e.message}" +rescue StandardError => e + puts "Unexpected error for #{username}: #{e.class} - #{e.message}" endapp/assets/stylesheets/file_manager.scss (1)
137-142: Commented-out justify-content suggests uncertainty.The commented-out
justify-content: center;on line 141 indicates potential indecision about the layout alignment.Either enable the centering or remove the commented line:
.permissions-container { display: flex; align-items: center; gap: 8px; - // justify-content: center; }app/views/file_manager/index.html.erb (2)
64-75: Extract confirmation message could be more informative.The confirmation dialog on line 73 says "This will extract the tar file to a new subdirectory" but doesn't specify which subdirectory will be created, making it harder for users to predict the outcome.
Include the target directory name in the confirmation:
In the controller, pass the extraction directory name to the view, or show it in JavaScript:
<%= link_to "Extract", "#{entry[:relative]}?extract=true", onclick: "return confirm('This will extract the contents to: #{File.basename(entry[:entry], '.*').gsub(/\.tar$/, '')}/ - Continue?');", style: "margin-right: 10px; font-size: 12px; color: #4CAF50;" %>
94-95: Maxlength and pattern mismatch.The
maxlength="4"on line 94 allows 4 characters, but the pattern[0-7]{3,4}also accepts 3-digit permissions. A user entering755would see it as valid (per pattern) even though they could add a 4th digit.Clarify if 3 or 4 digits are expected. For typical Unix permissions, 3 digits suffice:
-maxlength="4" -pattern="[0-7]{3,4}" -title="Enter 3-4 digit octal permissions (e.g., 755, 644)"> +maxlength="3" +pattern="[0-7]{3}" +title="Enter 3 digit octal permissions (e.g., 755, 644)">If 4-digit permissions (with setuid/setgid/sticky bits) are needed, keep
maxlength="4"but update the pattern to require exactly 4 when a 4th digit is present:pattern="[0-7]{3,4}"is already correct in that case, but ensure backend validation also accepts 4-digit input.
app/assets/javascripts/file_manager.js (1)
244-256: Real-time validation overrides CSS classes.The
inputevent handler on lines 244-256 directly setsstyle.borderColorandstyle.backgroundColor, which will override any CSS classes and make the styling harder to maintain.Use CSS classes instead of inline styles:
-input.addEventListener('input', function(e) { - const value = this.value; - if (value && !/^[0-7]{0,4}$/.test(value)) { - this.style.borderColor = '#dc3545'; - this.style.backgroundColor = '#ffe6e6'; - } else if (value && /^[0-7]{3,4}$/.test(value)) { - this.style.borderColor = '#28a745'; - this.style.backgroundColor = '#e6ffe6'; - } else { - this.style.borderColor = '#ddd'; - this.style.backgroundColor = '#f9f9f9'; - } -}); +input.addEventListener('input', function(e) { + const value = this.value; + this.classList.remove('permissions-invalid', 'permissions-valid', 'permissions-neutral'); + if (value && !/^[0-7]{0,4}$/.test(value)) { + this.classList.add('permissions-invalid'); + } else if (value && /^[0-7]{3,4}$/.test(value)) { + this.classList.add('permissions-valid'); + } else { + this.classList.add('permissions-neutral'); + } +});Then define these classes in
file_manager.scss:input[type=text].permissions-input { // ... existing styles ... &.permissions-invalid { border-color: #dc3545; background-color: #ffe6e6; } &.permissions-valid { border-color: #28a745; background-color: #e6ffe6; } &.permissions-neutral { border-color: #ddd; background-color: #f9f9f9; } }app/controllers/file_manager_controller.rb (6)
61-61: Directory creation could fail silently.
FileUtils.mkdir_pon line 61 can fail due to permission issues, but there's no explicit error handling until the rescue blocks later. If directory creation fails, subsequent operations will fail with less clear error messages.Add explicit check:
# Create the extraction directory -FileUtils.mkdir_p(extract_dir) +begin + FileUtils.mkdir_p(extract_dir) +rescue Errno::EACCES, Errno::ENOSPC => e + flash[:error] = "Cannot create extraction directory: #{e.message}" + redirect_to file_manager_index_path(path: File.dirname(path)) + return +end
88-107: Rescue blocks should avoid returning nil.Lines 87, 94, 100, and 106 explicitly return
nilafter setting flash and redirecting. This is unnecessary and can be confusing since Rails automatically handles the response after redirect.Remove the explicit
nilreturns:rescue Gem::Package::TarInvalidError => e # Clean up partially extracted directory FileUtils.rm_rf(extract_dir) if File.exist?(extract_dir) flash[:error] = "Corrupted tar file: #{e.message}. The file appears to be damaged or invalid." redirect_to file_manager_index_path(path: File.dirname(path)) - nil rescue Zlib::GzipFile::Error => e # ... - nil rescue StandardError => e # ... - nil end
177-226: Chmod action has good validation but could improve error responses.The chmod action properly validates octal format and permission ranges, but the error responses (lines 207-225) use both flash messages and JSON responses. This dual approach is good for supporting different client types.
However, consider adding a success JSON response for consistency:
# Apply the permission change File.chmod(permission_mode, absolute_path) -flash[:success] = "Successfully changed permissions to #{params[:permissions]}" +flash[:success] = "Successfully changed permissions to #{params[:permissions]}" +respond_to do |format| + format.html { redirect_back(fallback_location: file_manager_index_path) } + format.json { render json: { success: true, permissions: params[:permissions] }, status: :ok } +end
482-483: Path traversal checks are good but could be more explicit.Lines 482-483 check for
..and absolute paths in entry names, which prevents basic path traversal. However, consider using a more robust normalization approach.Add explicit path normalization:
tar_reader.each do |entry| - # Skip entries with invalid names or paths that try to escape the extraction directory - next if entry.full_name.include?('..') - next if entry.full_name.start_with?('/') + # Skip entries with invalid names or paths that try to escape the extraction directory + next if entry.full_name.include?('..') + next if entry.full_name.start_with?('/') + + # Additional normalization to catch other edge cases + normalized_name = Pathname.new(entry.full_name).cleanpath.to_s + next if normalized_name.start_with?('../') || normalized_name.start_with?('/') # Handle entries that might be at the root level or in subdirectories target_path = File.join(extract_dir, entry.full_name) + + # Verify the target path is within extract_dir after joining + unless target_path.start_with?(extract_dir) + Rails.logger.warn "Skipping entry #{entry.full_name}: path traversal detected" + next + end
501-527: Chunked reading with fallback is good, but logging could improve.The chunked reading implementation (lines 501-527) handles corruption well with a fallback to full read. The warning log at line 515 is helpful.
Consider adding more context to the log:
-Rails.logger.warn "Chunked reading failed for #{entry.full_name}: - #{e.message}, trying full read" +Rails.logger.warn "Chunked reading failed for #{entry.full_name} (expected size: #{expected_size}): #{e.message}, trying full read"
576-633:is_likely_executable?is comprehensive but could be simpler.The method thoroughly checks for executables using extensions, names, content signatures, and file headers (ELF, Mach-O, PE, etc.). However, it's complex and may over-detect executables.
Consider if this level of detection is necessary. For most course materials, checking the original tar entry's executable bit (which is already done in
extract_tar_entries) may be sufficient. If you need this level of detection, the current implementation is good.If simplification is desired:
def is_likely_executable?(file_path, original_name) # Simplified: check common script extensions and shebang executable_extensions = ['.sh', '.bash', '.zsh', '.py', '.pl', '.rb'] extension = File.extname(original_name).downcase return true if executable_extensions.include?(extension) # Check for shebang if File.exist?(file_path) && File.readable?(file_path) && File.size(file_path) > 0 first_bytes = File.read(file_path, 2) return true if first_bytes == "#!" end false end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/assets/javascripts/file_manager.js(9 hunks)app/assets/stylesheets/file_manager.scss(1 hunks)app/controllers/file_manager_controller.rb(4 hunks)app/views/file_manager/index.html.erb(2 hunks)config/routes.rb(1 hunks)import_submissions.rb(1 hunks)spec/controllers/file_manager_controller_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/file_manager_controller.rb (2)
app/models/assessment.rb (3)
path(505-507)folder_path(93-95)load_dir_to_tar(467-490)app/models/course.rb (2)
directory_path(52-54)generate_tar(340-378)
import_submissions.rb (1)
app/models/submission.rb (1)
save_file(82-116)
🪛 ast-grep (0.39.5)
app/controllers/file_manager_controller.rb
[warning] 301-301: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: assessment.load_dir_to_tar(absolute_path.to_s, "", tar_assessment, filter, "")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Brakeman (7.1.0)
app/controllers/file_manager_controller.rb
[weak] 61-61: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
[weak] 77-77: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
[weak] 103-103: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
[weak] 90-90: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
[weak] 97-97: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
[weak] 312-312: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
spec/controllers/file_manager_controller_spec.rb (1)
16-16: LGTM! Test coverage for new Permissions column.The added expectation correctly validates the presence of the "Permissions" table header in the file manager index view, aligning with the new permissions management feature introduced in this PR.
config/routes.rb (1)
84-84: LGTM! Route definition is correct.The new chmod route is properly defined with appropriate HTTP method (PATCH), path constraint, and routing target. The pattern aligns with existing file_manager routes and enables the new permissions management feature.
app/assets/stylesheets/file_manager.scss (2)
93-111: LGTM! Input styling with good UX.The permissions input styling provides clear visual feedback through focus, invalid, and valid states. The monospace font is appropriate for octal permissions display.
113-135: LGTM! Button styling is accessible.The permissions button has appropriate sizing, color states, and uses flexbox for icon alignment. Hover and active states provide good user feedback.
app/controllers/file_manager_controller.rb (3)
478-574: Extract_tar_entries has proper safety checks.The
extract_tar_entriesmethod includes good security measures:
- Path traversal prevention (line 482)
- Absolute path check (line 483)
- Size validation during extraction (lines 509-511)
- Fallback reading for corrupted entries (lines 517-526)
- Permission handling with fallbacks (lines 537-564)
636-676: Tar validation is thorough and secure.The
validate_tar_filemethod properly validates tar files before extraction, checking readability, size, and format integrity for both gzipped and regular tar files. This prevents malformed tar files from causing issues during extraction.
415-419: Permissions field addition in populate_directory.The addition of the permissions field (lines 415-419) correctly formats Unix permissions as octal strings and includes error handling for cases where permissions can't be retrieved.
| return; | ||
| } | ||
|
|
||
| let rel_path = decodeURIComponent(path.split("/file_manager/")[1]); |
There was a problem hiding this comment.
Path manipulation could fail on edge cases.
Line 213 splits the path by "/file_manager/" and assumes the relevant path is after the first occurrence. This could break if the path itself contains the string "/file_manager/".
Use a more robust approach. If path is already the relative path as used elsewhere in the file, you might not need this splitting at all:
-let rel_path = decodeURIComponent(path.split("/file_manager/")[1]);
+// Assuming path is already the relative path from entry[:relative]
+let rel_path = decodeURIComponent(path);Alternatively, if you must extract from a full URL:
// Remove the /file_manager/ prefix if present
let rel_path = path.replace(/^\/file_manager\//, '');
rel_path = decodeURIComponent(rel_path);🤖 Prompt for AI Agents
In app/assets/javascripts/file_manager.js around line 213, the current code
splits the path on "/file_manager/" and takes the second segment which fails if
the path contains that string multiple times; instead remove only a leading
"/file_manager/" prefix if present (or skip any splitting entirely if path is
already relative), then decodeURIComponent the resulting string; implement this
by checking and stripping the prefix at the start of the string and then calling
decodeURIComponent on the result.
| if params[:extract] && is_tar_file?(absolute_path) | ||
| # Skip extraction if the tar file itself is an executable | ||
| if is_likely_executable?(absolute_path, File.basename(absolute_path)) | ||
| flash[:error] = "Cannot extract tar files that are not directories" | ||
| redirect_to file_manager_index_path(path: File.dirname(path)) | ||
| return | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Verify tar extraction is properly authorized.
The tar extraction feature checks check_instructor(absolute_path) but proceeds to extract to a sibling directory in the parent. Ensure the authorization check covers write permissions to the parent directory, not just read access to the tar file.
Run this script to verify how check_instructor determines authorization and whether it validates write access:
🏁 Script executed:
#!/bin/bash
# Check the implementation of check_instructor and related authorization logic
ast-grep --pattern $'def check_instructor($PATH)
$$$
end'
# Also check if there are any other authorization checks for file operations
rg -A 10 'def check_instructor'Length of output: 1846
Validate write authorization on extraction directory
check_instructor currently only confirms access to the tar file’s path, but does not authorize write operations in the parent/sibling directory where files are extracted. Add a check (e.g. check_instructor(destination_path)) or explicitly verify the user has write permission on the target directory before extraction.
🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 36 to 42, the code
validates access to the tar file but does not verify that the user is authorized
to write into the extraction destination; add an explicit authorization check
for the target directory before performing extraction (for example call
check_instructor on the destination_path or otherwise verify the current user
has write permission on File.dirname(absolute_path) or the intended extraction
directory), and if the check fails set an error flash and redirect back without
extracting.
| def find_by_directory_path(absolute_path) | ||
| # Normalize the path to ensure consistent comparison | ||
| normalized_path = Pathname.new(absolute_path).cleanpath.to_s | ||
| # Find course where the directory_path matches the given absolute path | ||
| Course.find do |course| | ||
| course.directory_path.to_s == normalized_path | ||
| end | ||
| end | ||
|
|
||
| def find_by_folder_path(absolute_path) | ||
| # Normalize the path to ensure consistent comparison | ||
| normalized_path = Pathname.new(absolute_path).cleanpath.to_s | ||
|
|
||
| # Find assessment where the folder_path matches the given absolute path | ||
| Assessment.find do |assessment| | ||
| assessment.folder_path.to_s == normalized_path | ||
| end | ||
| end | ||
|
|
||
| def path_belongs_to_assessment?(absolute_path) | ||
| find_by_folder_path(absolute_path) | ||
| end | ||
|
|
||
| def path_belongs_to_course?(absolute_path) | ||
| find_by_directory_path(absolute_path) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Path lookup methods use inefficient iteration.
Methods find_by_directory_path and find_by_folder_path (lines 228-245) use Course.find and Assessment.find which iterate through all records. This is inefficient for large datasets.
Use database queries instead:
-def find_by_directory_path(absolute_path)
- # Normalize the path to ensure consistent comparison
- normalized_path = Pathname.new(absolute_path).cleanpath.to_s
- # Find course where the directory_path matches the given absolute path
- Course.find do |course|
- course.directory_path.to_s == normalized_path
- end
-end
+def find_by_directory_path(absolute_path)
+ normalized_path = Pathname.new(absolute_path).cleanpath.to_s
+ # Extract course name from path (assuming structure: BASE_DIRECTORY/course_name/...)
+ course_name = normalized_path.sub(BASE_DIRECTORY.to_s + '/', '').split('/').first
+ Course.find_by(name: course_name) if course_name
+endSimilarly for find_by_folder_path:
-def find_by_folder_path(absolute_path)
- # Normalize the path to ensure consistent comparison
- normalized_path = Pathname.new(absolute_path).cleanpath.to_s
-
- # Find assessment where the folder_path matches the given absolute path
- Assessment.find do |assessment|
- assessment.folder_path.to_s == normalized_path
- end
-end
+def find_by_folder_path(absolute_path)
+ normalized_path = Pathname.new(absolute_path).cleanpath.to_s
+ # Extract course and assessment names from path
+ path_parts = normalized_path.sub(BASE_DIRECTORY.to_s + '/', '').split('/')
+ if path_parts.length >= 2
+ course = Course.find_by(name: path_parts[0])
+ course&.assessments&.find_by(name: path_parts[1])
+ end
+endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 228-253, the current
methods iterate all records using Course.find and Assessment.find which is
inefficient; replace them with ActiveRecord queries that use the normalized_path
in the WHERE clause (e.g. Course.find_by(directory_path: normalized_path) and
Assessment.find_by(folder_path: normalized_path)) to let the DB do the lookup,
and update path_belongs_to_assessment? and path_belongs_to_course? to return a
boolean (e.g. use .present? on the result) if intended.
| course = path_belongs_to_course?(absolute_path) | ||
| assessment = path_belongs_to_assessment?(absolute_path) | ||
|
|
||
| if course.present? | ||
| tar_course = generate_tar(course) | ||
| send_data tar_course.string.force_encoding("binary"), | ||
| filename: "#{course.name}.tar", | ||
| type: "application/x-tar", | ||
| disposition: "attachment" | ||
| elsif assessment.present? | ||
| tar_stream = StringIO.new("") | ||
| Gem::Package::TarWriter.new(tar_stream) do |tar| | ||
| Dir[File.join(absolute_path.to_s, '**', '**')].each do |file| | ||
| mode = File.stat(file).mode | ||
| relative_path = file.sub(%r{^#{Regexp.escape(absolute_path.to_s)}/?}, '') | ||
| if File.directory?(file) | ||
| tar.mkdir relative_path, mode | ||
| else | ||
| tar.add_file relative_path, mode do |tar_file| | ||
| File.open(file, "rb") { |f| tar_file.write f.read } | ||
| end | ||
| end | ||
| end | ||
| Gem::Package::TarWriter.new(tar_stream) do |tar_assessment| | ||
| assessment.name | ||
| assessment.dump_yaml | ||
| filter = [] | ||
| assessment.load_dir_to_tar(absolute_path.to_s, "", tar_assessment, filter, "") | ||
| end | ||
| tar_stream.rewind | ||
| tar_stream.close | ||
| send_data tar_stream.string.force_encoding("binary"), | ||
| filename: "file_manager.tar", | ||
| filename: "#{assessment.name}.tar", | ||
| type: "application/x-tar", | ||
| disposition: "attachment" |
There was a problem hiding this comment.
Tar generation logic duplicates course export.
The download_tar method's handling of course tar generation (lines 290-295) appears to duplicate logic from the Course model's generate_tar method. However, line 291 calls generate_tar(course) passing the course object, but the Course#generate_tar method (from relevant code snippets) expects export_configs, not a course.
Fix the method call or clarify the parameter:
if course.present?
- tar_course = generate_tar(course)
+ # The generate_tar method should be called on the course instance
+ tar_course = course.generate_tar(['assessments', 'metrics_config'])
send_data tar_course.string.force_encoding("binary"),
filename: "#{course.name}.tar",
type: "application/x-tar",
disposition: "attachment"🧰 Tools
🪛 ast-grep (0.39.5)
[warning] 301-301: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: assessment.load_dir_to_tar(absolute_path.to_s, "", tar_assessment, filter, "")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 287 to 309, the
controller calls generate_tar(course) which duplicates course export logic and
passes a Course object to a method that expects export_configs; change the call
to use the Course instance method or supply the expected export_configs: either
call course.generate_tar(...) if generate_tar is an instance method (passing any
required export_configs), or call the module function with course.export_configs
(or appropriate config array) so the correct parameter type is provided and the
duplicate tar-building logic is removed.
| <% | ||
| # Check if file is a tar archive | ||
| file_ext = File.extname(entry[:entry]).downcase | ||
| is_tar = file_ext == '.tar' || file_ext == '.tgz' || | ||
| (file_ext == '.gz' && entry[:entry].downcase.end_with?('.tar.gz')) | ||
| %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tar detection duplicates controller logic.
The tar detection logic on lines 58-63 is duplicated between the view and controller (controller's is_tar_file? method). This violates DRY and can lead to inconsistencies.
Move tar detection to a helper method or use the controller's detection result:
In the controller's populate_directory method:
{
# ... existing fields ...
is_tar: is_tar_file?(abs_path_str),
# ...
}Then in the view:
-<%
- # Check if file is a tar archive
- file_ext = File.extname(entry[:entry]).downcase
- is_tar = file_ext == '.tar' || file_ext == '.tgz' ||
- (file_ext == '.gz' && entry[:entry].downcase.end_with?('.tar.gz'))
-%>
-<% if entry[:type] == :file && is_tar %>
+<% if entry[:type] == :file && entry[:is_tar] %>🤖 Prompt for AI Agents
In app/views/file_manager/index.html.erb around lines 58-63, the tar-detection
logic is duplicated; remove the inline File.extname-based detection and use a
single source of truth. Update the controller method that builds directory
entries (populate_directory) to set an is_tar boolean on each entry (e.g., using
the existing is_tar_file? helper), and then in the view replace the removed
logic by reading entry[:is_tar] (or entry["is_tar"] depending on structure).
Ensure the controller populates that key for every entry so the view no longer
computes tar detection itself.
| require 'zip' | ||
| require 'fileutils' | ||
| require 'tempfile' | ||
| require 'rack/test' | ||
| include Rack::Test::Methods |
There was a problem hiding this comment.
Include statement at module scope can cause side effects.
The include Rack::Test::Methods at line 5 pollutes the global namespace, making all of Rack::Test's methods available in the top-level scope. This can lead to naming conflicts and is not a Rails best practice for scripts.
Move the include inside a module or class to limit its scope:
-require 'zip'
-require 'fileutils'
-require 'tempfile'
-require 'rack/test'
-include Rack::Test::Methods
+require 'zip'
+require 'fileutils'
+require 'tempfile'
+require 'rack/test'
+
+module BulkImport
+ include Rack::Test::Methods
+
+ def self.run
+ # move script logic here
+ end
+end
+
+BulkImport.runCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In import_submissions.rb around lines 1 to 5, the top-level `include
Rack::Test::Methods` pollutes the global namespace; move the `include` into a
narrower scope by placing it inside a module or class that encapsulates the
script's functionality (for example a ImportSubmissions class or a module used
only for test helpers), so only that class/module mixes in Rack::Test::Methods;
keep the `require 'rack/test'` at top but remove the top-level include and add
it as `include Rack::Test::Methods` inside the chosen class/module to limit
scope and avoid global method leakage.
|
|
||
| # Extract tar to temporary path | ||
| temp_tar_path = File.join(temp_dir, entry.name) | ||
| entry.extract(temp_tar_path) { true } |
There was a problem hiding this comment.
extract overwrite behavior could cause issues.
The { true } block at line 56 forces overwrite of existing files during extraction. If the temp directory is reused or not properly cleaned up between runs, this could lead to unexpected behavior.
Consider using a unique subdirectory per entry or verifying the extraction path doesn't exist:
# Extract tar to temporary path
-temp_tar_path = File.join(temp_dir, entry.name)
-entry.extract(temp_tar_path) { true }
+unique_subdir = File.join(temp_dir, SecureRandom.hex(8))
+FileUtils.mkdir_p(unique_subdir)
+temp_tar_path = File.join(unique_subdir, entry.name)
+entry.extract(temp_tar_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| entry.extract(temp_tar_path) { true } | |
| # Extract tar to temporary path | |
| - temp_tar_path = File.join(temp_dir, entry.name) | |
| unique_subdir = File.join(temp_dir, SecureRandom.hex(8)) | |
| FileUtils.mkdir_p(unique_subdir) | |
| temp_tar_path = File.join(unique_subdir, entry.name) | |
| entry.extract(temp_tar_path) |
🤖 Prompt for AI Agents
In import_submissions.rb around line 56, the use of entry.extract(temp_tar_path)
{ true } forces overwriting existing files; change this to avoid unconditional
overwrite by creating a unique extraction path per entry (e.g., use Dir.mktmpdir
or append a UUID/timestamp to temp_tar_path) or check whether the target path
exists before extracting and skip or error if it does; remove the unconditional
block that returns true and instead ensure the extraction target is unique or
validated to prevent accidental replacement of files.
KesterTan
requested review from
a team and
coder6583
and removed request for
a team
| end | ||
| end | ||
| else | ||
| File.open(file_path, 'rb') do |file| |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To remediate this issue, we should strictly validate user-provided path input before any file access occurs. The best approach is to reject filenames or relative paths containing:
- Multiple consecutive dots (e.g.,
..,...), - Directory separators (
/or\), - Absolute paths (starting with
/), - Anything not matching a known whitelist of allowed patterns (e.g., only alphanumeric, underscore, dash, and dot for extensions).
We should implement a method, e.g., sanitize_path, that performs this validation, and invoke it just before safe_expand_path(path) (in check_path_exist). If the path is invalid, we return an error. Place the method in the private section of the controller, and update all code paths where user-provided input (params[:path] or path) is received to sanitize and reject bad values early (e.g., before calling any path joining or file access logic).
Required changes:
- Add a new
sanitize_pathmethod to validate input path according to the rules mentioned. - Update the
check_path_existmethod so that it first callssanitize_pathand raises/redirects if invalid. - Optionally, sanitize
params[:path]in theindexmethod before proceeding.
| @@ -371,6 +371,9 @@ | ||
| end | ||
|
|
||
| def check_path_exist(path) | ||
| unless sanitize_path(path) | ||
| raise ActionController::RoutingError, 'Invalid or unsafe path' | ||
| end | ||
| @absolute_path = safe_expand_path(path) | ||
| @relative_path = path | ||
| raise ActionController::RoutingError, 'Not Found' unless File.exist?(@absolute_path) | ||
| @@ -378,6 +381,20 @@ | ||
| @absolute_path | ||
| end | ||
|
|
||
|
|
||
| # Only allow relative paths with safe filename components (no traversal, no separators, no multiple dots) | ||
| def sanitize_path(path) | ||
| return true if path.nil? || path == "" | ||
| # Disallow absolute paths | ||
| return false if path.start_with?('/', '\\') | ||
| # Disallow directory traversal, multiple dots, or dir separators | ||
| return false if path.include?("..") || path.include?("/") || path.include?("\\") | ||
| # Whitelist: only allow simple filenames or relative paths without any dangerous characters | ||
| allowed_pattern = /\A[\w\-\.]+\z/ | ||
| return false unless path.match?(allowed_pattern) | ||
| true | ||
| end | ||
|
|
||
| def is_instructor_of_any_course | ||
| current_user_id = current_user.id | ||
| cuds = CourseUserDatum.where(user_id: current_user_id, instructor: true) |
| end | ||
| end | ||
| else | ||
| File.open(tar_path, 'rb') do |file| |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix this vulnerability, explicit validation and sanitization should be applied directly to user-supplied path values before they are used in any filesystem operations. For archives, both the tar file itself (params[:tar_path]) and the internal file path to extract from the archive (params[:file_path]) must be validated.
Steps to fix:
- Define a whitelist pattern for names allowed (disallowing "..", "/", "", or leading ".").
- Create a helper method (e.g.,
sanitize_path_component) to validate/sanitize a path component. - Use this method to validate both the tar file path and the file inside the tar before calling
check_path_existand before passing the paths toextract_single_file_from_tar. - Raise an error or return an appropriate response if the file path fails validation.
Specifics:
- Edit the relevant controller action to validate/sanitize user input from
params[:tar_path]andparams[:file_path]before using them. - Add a new method to sanitize filenames.
- Call this method before any file operation that uses a user-supplied filename or path.
- No changes needed to any business logic; only the validation of inputs before use.
- Add a test for sanitization to avoid using dangerous filenames.
| @@ -265,8 +265,8 @@ | ||
| end | ||
|
|
||
| def view_tar_file | ||
| tar_path = params[:tar_path] | ||
| file_path = params[:file_path] | ||
| tar_path = sanitize_path_component(params[:tar_path]) | ||
| file_path = sanitize_path_component(params[:file_path]) | ||
|
|
||
| absolute_tar_path = check_path_exist(tar_path) | ||
|
|
||
| @@ -315,6 +315,23 @@ | ||
|
|
||
| private | ||
|
|
||
| # Sanitizes a user-supplied file name/path fragment to prevent traversal or absolute paths. | ||
| # Allow only safe names: alphanumerics, hyphens, underscores, dots (not at start), but not "..", "/", "\", or leading dot. | ||
| def sanitize_path_component(name) | ||
| if name.nil? || name.empty? | ||
| raise ActionController::RoutingError, 'Invalid filename' | ||
| end | ||
| # Disallow directory separators or parent references | ||
| if name.include?('/') || name.include?('\\') || name.include?('..') || name[0] == '.' | ||
| raise ActionController::RoutingError, 'Invalid filename' | ||
| end | ||
| # Only allow whitelisted characters | ||
| unless name =~ /\A[\w\-.]+\z/ | ||
| raise ActionController::RoutingError, 'Invalid filename' | ||
| end | ||
| name | ||
| end | ||
|
|
||
| def populate_directory(current_directory, current_url) | ||
| directory = Dir.entries(current_directory) | ||
| new_url = current_url == '/' ? '' : current_url |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/controllers/file_manager_controller.rb (2)
114-131: Critical performance issue: Inefficient record iteration.Both
find_by_directory_pathandfind_by_folder_pathuseCourse.findandAssessment.find, which load all records into memory and iterate through them in Ruby. This is highly inefficient and will cause performance issues with many courses/assessments.Use database queries instead:
def find_by_directory_path(absolute_path) - # Normalize the path to ensure consistent comparison normalized_path = Pathname.new(absolute_path).cleanpath.to_s - # Find course where the directory_path matches the given absolute path - Course.find do |course| - course.directory_path.to_s == normalized_path - end + # Extract course name from path structure + course_name = normalized_path.sub("#{BASE_DIRECTORY}/", '').split('/').first + Course.find_by(name: course_name) if course_name end def find_by_folder_path(absolute_path) normalized_path = Pathname.new(absolute_path).cleanpath.to_s - - # Find assessment where the folder_path matches the given absolute path - Assessment.find do |assessment| - assessment.folder_path.to_s == normalized_path - end + # Extract course and assessment names from path + parts = normalized_path.sub("#{BASE_DIRECTORY}/", '').split('/') + return nil unless parts.length >= 2 + + course = Course.find_by(name: parts[0]) + course&.assessments&.find_by(name: parts[1]) end
176-181: Incorrect method signature for tar generation.Line 177 calls
generate_tar(course), but based on the code snippets,Course#generate_taris an instance method that expects anexport_configsarray parameter, not a course object.Fix the method call:
if course.present? - tar_course = generate_tar(course) + tar_course = course.generate_tar(['assessments', 'metrics_config']) send_data tar_course.string.force_encoding("binary"), filename: "#{course.name}.tar", type: "application/x-tar", disposition: "attachment"
🧹 Nitpick comments (3)
app/views/file_manager/tar_preview.html.erb (1)
77-127: Consider extracting inline styles to the stylesheet.While inline styles work, consider moving these to
app/assets/stylesheets/file_manager.scssfor better maintainability and to avoid style duplication across views.app/controllers/file_manager_controller.rb (2)
133-139: Consider explicit boolean return values.These methods are named as predicates (with
?) but return the record ornil. For clarity and consistency with Ruby conventions, consider returning explicit booleans.def path_belongs_to_assessment?(absolute_path) - find_by_folder_path(absolute_path) + find_by_folder_path(absolute_path).present? end def path_belongs_to_course?(absolute_path) - find_by_directory_path(absolute_path) + find_by_directory_path(absolute_path).present? end
413-491: LGTM! Tar content listing with proper safety checks.The method includes crucial path traversal protection (skipping entries with
..or starting with/) and handles both compressed and uncompressed tar formats with appropriate error handling.Optional: Consider extracting the common entry-processing logic to reduce duplication between the gzip and non-gzip branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/assets/javascripts/file_manager.js(8 hunks)app/assets/stylesheets/file_manager.scss(1 hunks)app/controllers/file_manager_controller.rb(5 hunks)app/views/file_manager/index.html.erb(2 hunks)app/views/file_manager/tar_file_view.html.erb(1 hunks)app/views/file_manager/tar_preview.html.erb(1 hunks)config/routes.rb(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/assets/stylesheets/file_manager.scss
- app/assets/javascripts/file_manager.js
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/file_manager/index.html.erb
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: najclark
Repo: autolab/Autolab PR: 2011
File: app/controllers/courses_controller.rb:1316-1320
Timestamp: 2024-06-10T19:38:42.782Z
Learning: The user confirmed the concern about potential security risks when setting `flash[:error]` and `flash[:html_safe]` with user-supplied input, specifically mentioning `course_name` derived from a tar file's contents. It's important to ensure proper sanitization of user-supplied input to prevent XSS vulnerabilities, especially when `flash[:html_safe]` is used, which indicates the content should not be escaped.
Applied to files:
app/controllers/file_manager_controller.rb
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/file_manager_controller.rb
📚 Learning: 2024-11-13T17:02:14.676Z
Learnt from: jhs-panda
Repo: autolab/Autolab PR: 2214
File: app/controllers/schedulers_controller.rb:0-0
Timestamp: 2024-11-13T17:02:14.676Z
Learning: In the `validate_compile_action_file` method in `app/controllers/schedulers_controller.rb`, adding a check to ensure that `action_path` starts with `Rails.root.to_s` is unnecessary.
Applied to files:
app/controllers/file_manager_controller.rb
📚 Learning: 2024-09-19T06:35:55.985Z
Learnt from: KesterTan
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:32-36
Timestamp: 2024-09-19T06:35:55.985Z
Learning: In the `AutogradersController#edit` method, passing the file paths to the view is necessary to send the files in `download_file`.
Applied to files:
app/controllers/file_manager_controller.rb
🧬 Code graph analysis (1)
app/controllers/file_manager_controller.rb (2)
app/models/assessment.rb (3)
path(505-507)folder_path(93-95)load_dir_to_tar(467-490)app/models/course.rb (2)
directory_path(52-54)generate_tar(340-378)
🪛 ast-grep (0.39.7)
app/controllers/file_manager_controller.rb
[warning] 187-187: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: assessment.load_dir_to_tar(absolute_path.to_s, "", tar_assessment, filter, "")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Brakeman (7.1.0)
app/controllers/file_manager_controller.rb
[weak] 198-198: Parameter value used in file name
Type: File Access
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/file_access/
(File Access)
🪛 GitHub Check: CodeQL
app/controllers/file_manager_controller.rb
[failure] 453-453: Uncontrolled data used in path expression
This path depends on a user-provided value.
[failure] 507-507: Uncontrolled data used in path expression
This path depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (9)
config/routes.rb (1)
75-76: LGTM! Route addition follows Rails conventions.The new route for tar file viewing is properly configured with a named route helper that's used in the related views.
app/views/file_manager/tar_preview.html.erb (2)
40-66: LGTM! Robust handling of tar entry metadata.The template properly handles both epoch timestamps and Time objects for modification times, and safely renders tar entry metadata with appropriate escaping.
12-14: The edge case concern is not substantiated by the code structure.After analyzing the controller logic:
@pathcannot be empty in thetar_previewview because the code at line 30–31 checks(path == "" && is_instructor_of_any_course)first and renders the directory view instead. Thetar_previewblock is only reached when handling actual files, so@pathwill always have a value.Additionally, the codebase already uses
File.dirname(path)without special handling in similar contexts (line 43), indicating this pattern is an established convention. TheFile.dirname(".")case—if a tar file exists at the root level—would follow the same behavior as existing code.No actionable issues found.
app/views/file_manager/tar_file_view.html.erb (2)
34-36: LGTM! Safe file content rendering.The file content is properly escaped by ERB and displayed within appropriate semantic HTML elements for code display.
29-32: This review comment is incorrect and can be dismissed.The code at line 281 already uses
File.dirname(tar_path)in a redirect within the same method, and this pattern is working correctly. Additionally, the tar_preview.html.erb template uses an identicalFile.dirname(@path)pattern for navigation, confirming this is an established, proven approach in the codebase. Path validation occurs at line 271 withcheck_path_exist(tar_path)before@tar_pathis ever set, ensuring only valid paths reach the view.File.dirname()navigating to the parent directory is the intended behavior, not a navigation break.Likely an incorrect or invalid review comment.
app/controllers/file_manager_controller.rb (4)
35-44: LGTM! Well-structured tar preview logic.The tar preview flow properly validates the file type, handles errors gracefully, and ensures authorization before displaying archive contents.
141-166: LGTM! Robust directory-to-tar implementation.The method properly handles files, directories, and symlinks, preserves permissions, and correctly recurses through subdirectories.
182-195: LGTM! Assessment tar generation is correct.The assessment tar creation properly uses the Assessment model's methods for serialization. Note: The static analysis warning about a "hardcoded passphrase" on line 187 is a false positive—the empty string is a path parameter, not a security credential.
350-354: LGTM! Proper permissions display with error handling.The permissions field correctly formats file modes as octal strings and gracefully handles errors with a fallback value.
| def view_tar_file | ||
| tar_path = params[:tar_path] | ||
| file_path = params[:file_path] | ||
|
|
||
| absolute_tar_path = check_path_exist(tar_path) | ||
|
|
||
| unless check_instructor(absolute_tar_path) | ||
| flash[:error] = "You are not authorized to view this path" | ||
| redirect_to root_path | ||
| return | ||
| end | ||
|
|
||
| unless is_tar_file?(absolute_tar_path) | ||
| flash[:error] = "Not a valid tar file" | ||
| redirect_to file_manager_index_path(path: File.dirname(tar_path)) | ||
| return | ||
| end | ||
|
|
||
| begin | ||
| file_content = extract_single_file_from_tar(absolute_tar_path, file_path) | ||
|
|
||
| if file_content.nil? | ||
| flash[:error] = "File not found in archive" | ||
| redirect_to file_manager_index_path(path: "#{tar_path}?preview=true") | ||
| return | ||
| end | ||
|
|
||
| is_binary = file_content.bytes.take(8192).any? { |byte| | ||
| byte.zero? || (byte < 32 && ![9, 10, 13].include?(byte)) | ||
| } | ||
|
|
||
| if is_binary || file_content.bytesize > 1_000_000 | ||
| send_data file_content, | ||
| filename: File.basename(file_path), | ||
| disposition: 'attachment' | ||
| else | ||
| @tar_path = tar_path | ||
| @file_path = file_path | ||
| @file = file_content.force_encoding('UTF-8') | ||
| @tar_file_name = File.basename(absolute_tar_path) | ||
| @file_name = file_path | ||
| render :tar_file_view, formats: :html | ||
| end | ||
| rescue StandardError => e | ||
| flash[:error] = "Unable to extract file: #{e.message}" | ||
| redirect_to file_manager_index_path(path: "#{tar_path}?preview=true") | ||
| end | ||
| end |
There was a problem hiding this comment.
Path traversal check needed before extraction.
The file_path parameter from user input is passed directly to extract_single_file_from_tar without validation. Unlike list_tar_contents (which skips entries with .. or starting with /), the extraction method lacks these safety checks, potentially allowing extraction of files outside the intended archive structure.
Add path traversal validation before extraction:
begin
+ # Validate file_path doesn't contain path traversal attempts
+ if file_path.include?('..') || file_path.start_with?('/')
+ flash[:error] = "Invalid file path"
+ redirect_to file_manager_index_path(path: File.dirname(tar_path))
+ return
+ end
+
file_content = extract_single_file_from_tar(absolute_tar_path, file_path)🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 267 to 314, validate
and sanitize the incoming file_path before calling extract_single_file_from_tar
to prevent path traversal: reject or handle any file_path that contains ".."
segments, begins with "/", or is otherwise absolute (also reject patterns like
"%2e%2e" if you decode input), normalize the path (e.g. remove leading slashes
and collapse "../" sequences) and if invalid set flash[:error] and redirect
exactly like the other checks; only pass the sanitized, relative member path
into extract_single_file_from_tar and keep current behavior for valid files.
| def extract_single_file_from_tar(tar_path, file_path) | ||
| if File.extname(tar_path).downcase.in?(['.gz', '.tgz']) || | ||
| tar_path.to_s.downcase.end_with?('.tar.gz') | ||
| require 'zlib' | ||
| Zlib::GzipReader.open(tar_path) do |gz| | ||
| tar_reader = Gem::Package::TarReader.new(gz) | ||
| tar_reader.rewind | ||
| tar_reader.each do |entry| | ||
| if entry.full_name == file_path && entry.file? | ||
| return entry.read | ||
| end | ||
| end | ||
| end | ||
| else | ||
| File.open(tar_path, 'rb') do |file| | ||
| tar_reader = Gem::Package::TarReader.new(file) | ||
| tar_reader.rewind | ||
| tar_reader.each do |entry| | ||
| if entry.full_name == file_path && entry.file? | ||
| return entry.read | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| nil | ||
| end |
There was a problem hiding this comment.
Critical: Add path traversal protection to extraction method.
Unlike list_tar_contents, this method lacks validation for malicious paths. Add the same safety checks to prevent extracting files with path traversal sequences.
Add path traversal checks before extraction:
def extract_single_file_from_tar(tar_path, file_path)
+ # Prevent path traversal attacks
+ if file_path.include?('..') || file_path.start_with?('/')
+ raise ArgumentError, "Invalid file path: contains path traversal"
+ end
+
if File.extname(tar_path).downcase.in?(['.gz', '.tgz']) ||
tar_path.to_s.downcase.end_with?('.tar.gz')
require 'zlib'
Zlib::GzipReader.open(tar_path) do |gz|
tar_reader = Gem::Package::TarReader.new(gz)
tar_reader.rewind
tar_reader.each do |entry|
+ # Additional check on tar entry itself
+ next if entry.full_name.include?('..') || entry.full_name.start_with?('/')
if entry.full_name == file_path && entry.file?
return entry.read
end
end
end
else
File.open(tar_path, 'rb') do |file|
tar_reader = Gem::Package::TarReader.new(file)
tar_reader.rewind
tar_reader.each do |entry|
+ # Additional check on tar entry itself
+ next if entry.full_name.include?('..') || entry.full_name.start_with?('/')
if entry.full_name == file_path && entry.file?
return entry.read
end
end
end
end
nil
end🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 507-507: Uncontrolled data used in path expression
This path depends on a user-provided value.
🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 493 to 519, the
extract_single_file_from_tar method is missing path traversal protection; add
the same safety checks used in list_tar_contents to validate each entry before
reading it: skip any entry whose full_name starts with a '/' or contains '..'
segments, normalize the joined path (File.expand_path(File.join(base_dir,
entry.full_name))) and ensure it begins with the intended base extraction
directory, and only then allow reading/returning the entry; apply these checks
in both the Gzip and non-Gzip branches and return nil or skip the entry when
validation fails.
| <% content_for :javascripts do %> | ||
| <%= javascript_include_tag "highlight.pack" %> | ||
| <script> | ||
| document.addEventListener('DOMContentLoaded', function() { | ||
| document.querySelectorAll('pre code').forEach((block) => { | ||
| hljs.highlightBlock(block); | ||
| }); | ||
| }); | ||
| </script> | ||
| <% end %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use modern highlight.js API.
hljs.highlightBlock() is deprecated in highlight.js v10+. Use hljs.highlightElement() instead for compatibility with current versions.
Apply this diff:
<script>
document.addEventListener('DOMContentLoaded', function() {
document.querySelectorAll('pre code').forEach((block) => {
- hljs.highlightBlock(block);
+ hljs.highlightElement(block);
});
});
</script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <% content_for :javascripts do %> | |
| <%= javascript_include_tag "highlight.pack" %> | |
| <script> | |
| document.addEventListener('DOMContentLoaded', function() { | |
| document.querySelectorAll('pre code').forEach((block) => { | |
| hljs.highlightBlock(block); | |
| }); | |
| }); | |
| </script> | |
| <% end %> | |
| <% content_for :javascripts do %> | |
| <%= javascript_include_tag "highlight.pack" %> | |
| <script> | |
| document.addEventListener('DOMContentLoaded', function() { | |
| document.querySelectorAll('pre code').forEach((block) => { | |
| hljs.highlightElement(block); | |
| }); | |
| }); | |
| </script> | |
| <% end %> |
🤖 Prompt for AI Agents
In app/views/file_manager/tar_file_view.html.erb around lines 7 to 16, the code
uses the deprecated hljs.highlightBlock(block); call; update the DOM-ready code
to call hljs.highlightElement(block) instead (keeping the querySelectorAll('pre
code') loop and event listener intact) so it uses the modern highlight.js API
and remains compatible with v10+.
Description
Previously, courses, labs and assessments that were downloaded as tar files ended up being corrupted. There was also no quick and easy way to view permission bits and change permissions of files on the system.
This PR allows
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingIf unsure, feel free to submit first and we'll help you along.