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}
| end | ||
| else | ||
| # Handle regular tar files - ensure proper resource management | ||
| File.open(absolute_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 16 days ago
To remediate the flaw, we need to validate and sanitize any user input that contributes to file path expressions, in particular, params[:path]. This includes:
- For all key entry points (such as in
index, and potentially elsewhere), filter, validate, and sanitizeparams[:path]values before passing them along the flow that leads to path computation. - At minimum, enforce a whitelist-based approach: allow only subdirectory names that match a strict, safe pattern (e.g., alphanumeric,
-and_, not starting with., no separators/, no whitespace, and of reasonable length). - Rejection or normalization of invalid path components/filenames should occur before passing the path anywhere that would construct a file path.
- Additional sanitization may also be required for extraction directories derived from filenames, which may inherit risky characters.
In app/controllers/file_manager_controller.rb, we will:
- Add a
sanitize_path_parammethod to validate and sanitize all components ofparams[:path]. - Use this method at the beginning of the
indexaction (and any other action that usesparams[:path]for filesystem access). - Optionally, strengthen
safe_expand_pathwith a stricter check, or make sure it never receives an unsanitized path. - Sanitize the base name of the extracted directory (where the tarfile is unpacked) before using it in path expressions.
This requires defining a utility method in the controller for path sanitization and using it appropriately.
| @@ -5,12 +5,15 @@ | ||
|
|
||
| class FileManagerController < ApplicationController | ||
| BASE_DIRECTORY = Rails.root.join('courses') | ||
| # Allow only safe directory/file names: no slashes, no starting dot, certain characters only | ||
| SAFE_PATH_COMPONENT = /\A[\w\-]+\z/ | ||
| skip_before_action :set_course | ||
| skip_before_action :authorize_user_for_course | ||
| skip_before_action :update_persistent_announcements | ||
|
|
||
| def index | ||
| path = params[:path].nil? ? "" : params[:path] | ||
| raw_path = params[:path].nil? ? "" : params[:path] | ||
| path = sanitize_path_param(raw_path) | ||
| new_url = "#{path}/" | ||
| parts = new_url.split('/') | ||
| if parts.empty? | ||
| @@ -47,7 +45,9 @@ | ||
| base_name = File.basename(absolute_path, '.*') | ||
| # Remove additional extensions for files like .tar.gz | ||
| base_name = base_name.gsub(/\.tar$/, '') | ||
| extract_dir = File.join(parent_dir, base_name) | ||
| # Sanitize directory name to prevent traversal and injection | ||
| safe_extract_base = base_name.match(SAFE_PATH_COMPONENT) ? base_name : "extracted" | ||
| extract_dir = File.join(parent_dir, safe_extract_base) | ||
|
|
||
| # Ensure extract directory doesn't already exist to avoid conflicts | ||
| counter = 1 | ||
| @@ -380,6 +380,16 @@ | ||
|
|
||
| private | ||
|
|
||
| # Sanitize a path parameter: allow only a chain of safe path components separated by '/' | ||
| def sanitize_path_param(path_param) | ||
| return "" if path_param.blank? | ||
| # Split on '/', filter out unsafe components | ||
| components = path_param.split("/") | ||
| safe_components = components.select { |c| c.match(SAFE_PATH_COMPONENT) } | ||
| safe_path = safe_components.join("/") | ||
| safe_path | ||
| end | ||
|
|
||
| def populate_directory(current_directory, current_url) | ||
| directory = Dir.entries(current_directory) | ||
| new_url = current_url == '/' ? '' : current_url |
| FileUtils.mkdir_p(parent_dir) unless parent_dir == extract_dir | ||
|
|
||
| # Properly read and write file data to prevent corruption | ||
| File.open(target_path, 'wb') do |f| |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix the issue, we need to ensure that files extracted from the tar archive (or any supported archive) cannot escape the intended target extraction directory (extract_dir). This is best done by normalizing the final extraction path for every archive entry using Pathname.new(...).expand_path and checking that it is a subpath of extract_dir. Any entry that would be written outside extract_dir should be skipped. This whitelisting approach, together with existing filters (.. exclusion, no leading /), will robustly block attempts to perform directory traversal through archive contents.
Specifically, in the extract_tar_entries method, replace the calculation of target_path and the subsequent extraction logic by:
- Normalize
target_pathto its absolute expanded form. - Ensure
target_pathis strictly a child (or descendant) ofextract_dir(using either string comparison or, preferably,Pathnamesemantics). - Skip extraction if this check fails.
- Insert code comments explaining the logic.
Only changes in app/controllers/file_manager_controller.rb in the body of extract_tar_entries (lines 478–565) are needed.
| @@ -484,16 +484,23 @@ | ||
|
|
||
| # Handle entries that might be at the root level or in subdirectories | ||
| target_path = File.join(extract_dir, entry.full_name) | ||
| normalized_target_path = Pathname.new(target_path).expand_path | ||
| # Ensure extraction stays inside extract_dir | ||
| extract_dir_expanded = Pathname.new(extract_dir).expand_path | ||
| unless normalized_target_path.to_s.start_with?(extract_dir_expanded.to_s + File::SEPARATOR) | ||
| # Skip potentially malicious paths that escape extract_dir | ||
| next | ||
| end | ||
|
|
||
| if entry.directory? | ||
| FileUtils.mkdir_p(target_path) | ||
| FileUtils.mkdir_p(normalized_target_path) | ||
| elsif entry.file? | ||
| # Ensure parent directory exists (especially important for files at root level) | ||
| parent_dir = File.dirname(target_path) | ||
| FileUtils.mkdir_p(parent_dir) unless parent_dir == extract_dir | ||
| parent_dir = File.dirname(normalized_target_path) | ||
| FileUtils.mkdir_p(parent_dir) unless parent_dir == extract_dir_expanded.to_s | ||
|
|
||
| # Properly read and write file data to prevent corruption | ||
| File.open(target_path, 'wb') do |f| | ||
| File.open(normalized_target_path, 'wb') do |f| | ||
| # Read in chunks to handle large files properly and detect corruption | ||
| bytes_written = 0 | ||
| expected_size = entry.header.size |
| begin | ||
| if File.exist?(file_path) && File.readable?(file_path) && File.size(file_path) > 0 | ||
| # Read first few bytes to check for executable signatures | ||
| first_bytes = File.read(file_path, [File.size(file_path), 16].min) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix the issue, sanitize and validate the user-supplied path parameter before using it in any file system expression. The best approach is two-fold:
- Validate the
params[:path]value right after it's read. Enforce a whitelist: only allow path components matching a safe pattern, forbid directory separators (/,\), forbid more than one.in a component, and reject traversal attempts (such as..). - Call this validation logic before passing to
safe_expand_path, so all internal code can assume paths are safe. - Add a private method in the controller (e.g.,
sanitize_path_componentorvalidate_path!) and call it from the controller's entry points (indexmethod, at the point whereparams[:path]is received).
This requires:
- Adding a method like
validate_path!which raises an error if path is invalid. - Inserting a call to
validate_path!before using the parameter, e.g. at lines13or29.
No external libraries beyond Rails are required: Regex and Ruby string methods suffice.
| @@ -11,6 +11,7 @@ | ||
|
|
||
| def index | ||
| path = params[:path].nil? ? "" : params[:path] | ||
| validate_path!(path) | ||
| new_url = "#{path}/" | ||
| parts = new_url.split('/') | ||
| if parts.empty? | ||
| @@ -380,6 +381,22 @@ | ||
|
|
||
| private | ||
|
|
||
| # Validate user-supplied path against whitelist pattern | ||
| # Only allows simple path components separated by '/', forbids traversal and separators | ||
| def validate_path!(raw_path) | ||
| return if raw_path.blank? | ||
| # Split the path into components and validate each one | ||
| components = raw_path.split("/") | ||
| safe_component = /\A[\w\-.]+\z/ | ||
| components.each do |comp| | ||
| next if comp.blank? | ||
| # Forbid ".", "..", empty, any component containing "/" or "\", or more than one "." | ||
| if comp == "." || comp == ".." || comp.include?("\\") || comp.count(".") > 1 || !(comp =~ safe_component) | ||
| raise ActionController::RoutingError, "Unsafe path component: #{comp.inspect}" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def populate_directory(current_directory, current_url) | ||
| directory = Dir.entries(current_directory) | ||
| new_url = current_url == '/' ? '' : current_url |
| else | ||
| # Validate regular tar file | ||
| begin | ||
| 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 16 days ago
To effectively resolve this issue, user input must be sanitized and strictly validated before it is used in constructing file paths. This means explicitly denying paths that contain directory traversal sequences (e.g., ..), slashes, backslashes, or multiple "." (dots) in inappropriate locations, and optionally matching against a set of whitelist patterns that are permitted filenames. The most appropriate location to perform this validation is immediately after retrieving the params[:path] value and before performing any operations that could lead to file access. Add a method called sanitize_path_param(path_param) that performs robust checks—denying any path which contains forbidden patterns or fails the whitelist.
These changes should be made:
- Add a method
sanitize_path_param(path_param)in theFileManagerControllerclass. - Use this method in the
indexaction after retrievingparams[:path], raising or redirecting in case of invalid input.
You only need to add or modify code in app/controllers/file_manager_controller.rb. To implement these changes:
- Add the new method to the private section.
- Before any usage of
params[:path], apply the sanitizer and respond appropriately if there is an error. - The sanitizer should reject any path containing forbidden patterns, multiple dots, slashes or backslashes, and only permit known safe patterns.
No external dependencies are required for this fix, since Ruby’s built-in string manipulation capabilities suffice.
| @@ -10,7 +10,14 @@ | ||
| skip_before_action :update_persistent_announcements | ||
|
|
||
| def index | ||
| path = params[:path].nil? ? "" : params[:path] | ||
| path_param = params[:path].nil? ? "" : params[:path] | ||
| # Sanitize user input for path before proceeding | ||
| unless sanitize_path_param(path_param) | ||
| flash[:error] = "Invalid path parameter" | ||
| redirect_to root_path | ||
| return | ||
| end | ||
| path = path_param | ||
| new_url = "#{path}/" | ||
| parts = new_url.split('/') | ||
| if parts.empty? | ||
| @@ -380,6 +387,25 @@ | ||
|
|
||
| private | ||
|
|
||
| # Reject path if it contains traversal sequences, slashes, backslashes, more than one dot in a row, | ||
| # or anything other than permitted filename pattern. Allow empty path (root). | ||
| def sanitize_path_param(path_param) | ||
| # Allow root | ||
| return true if path_param == "" | ||
|
|
||
| # Only allow segments with alphanumerics, underscores, hyphens, and single period (not at start/end) | ||
| # Disallow slashes, backslashes, "..", "./", ".\", consecutive dots | ||
| return false if path_param.include?("/") || path_param.include?("\\") | ||
| return false if path_param.include?("..") | ||
| return false if path_param.include?("./") || path_param.include?(".\\") | ||
| return false if path_param.match?(/\.{2,}/) | ||
| # Whitelist: only allow these patterns (alphanumeric, underscore, hyphen, one dot, and optional subfolder segments) | ||
| safe_pattern = /\A[\w\-\.]+\z/ | ||
| return false unless path_param.match?(safe_pattern) | ||
|
|
||
| true | ||
| end | ||
|
|
||
| def populate_directory(current_directory, current_url) | ||
| directory = Dir.entries(current_directory) | ||
| new_url = current_url == '/' ? '' : current_url |
📝 WalkthroughWalkthroughAdds permissions editing (frontend UI, JS handler, backend chmod route/action), updates listing to show permissions, and introduces tar handling: detect archives in the UI, support tar download generation and secure extraction on request. Includes SCSS for permissions controls, a standalone submission import script, and a spec asserting the new Permissions column. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as FileManager UI
participant JS as file_manager.js
participant FM as FileManagerController
participant FS as Filesystem
User->>UI: Edit octal in Permissions input
User->>JS: Click Apply / press Enter
JS->>FM: PATCH /file_manager/:path/chmod {permissions}
FM->>FM: Validate octal + authorize
FM->>FS: chmod(path, mode)
FS-->>FM: result / error
alt success
FM-->>JS: 200 OK
JS->>UI: Reload page
else error
FM-->>JS: 4xx/5xx with message
JS->>UI: Show inline error
end
sequenceDiagram
autonumber
actor User
participant UI as FileManager UI
participant FM as FileManagerController
participant FS as Filesystem
participant Tar as Tar/Gzip libs
Note over UI,FM: Tar file detected in listing
User->>UI: Click Extract (confirm)
UI->>FM: GET/POST with extract=true (tar target)
FM->>FM: Validate tar file (size/integrity)
FM->>FS: Create extract dir
FM->>Tar: Open and iterate entries
Tar-->>FM: Entries stream
FM->>FS: Write files/symlinks safely
alt invalid/executable/unsafe
FM-->>UI: Error + cleanup
else success
FM-->>UI: Success response
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 (dir/course/assessment)
UI->>FM: GET download
alt file
FM->>FS: Read file
FM-->>UI: Send file
else directory/course/assessment
FM->>Tar: Stream tar creation
FM->>FS: Recursively add entries
Tar-->>FM: tar stream
FM-->>UI: Send tar
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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.
| 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.
Executable tar files should be extracted, not prevented.
Lines 38-42 prevent extraction of tar files that are themselves executables. However, this check conflates the tar file being executable with the tar contents potentially containing executables. A tar file with executable permissions could still contain legitimate course materials that should be extractable.
Remove this overly restrictive check or clarify the intent:
-# 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
-endIf the concern is preventing execution of malicious tar files, the check should be on the extraction process (which already validates tar format) rather than file permissions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/file_manager_controller.rb around lines 38 to 42, remove the
overly restrictive guard that blocks extraction when the tar file itself has
executable permissions; instead allow extraction to proceed and rely on the
existing tar-format validation/inspection during the extraction process (or add
content-based checks at extraction time if needed). Delete or replace the
is_likely_executable? check so a tar file’s file-mode doesn’t prevent
extraction, and ensure any security checks are performed on the tar contents
during extraction rather than on the archive’s executable bit.
| 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.
| <input type="text" | ||
| id="permissions-<%= entry[:relative].gsub(/[^a-zA-Z0-9]/, '_') %>" | ||
| value="<%= entry[:permissions] %>" | ||
| class="permissions-input" | ||
| data-file-path="<%= entry[:relative] %>" | ||
| maxlength="4" | ||
| pattern="[0-7]{3,4}" | ||
| autocomplete="off" | ||
| title="Enter 3-4 digit octal permissions (e.g., 755, 644)"> |
There was a problem hiding this comment.
Input ID generation could collide on special characters.
Line 90 generates IDs by replacing non-alphanumeric characters with underscores. Paths like foo/bar and foo_bar would both generate foo_bar, causing ID collisions.
Use a more robust ID generation approach:
-id="permissions-<%= entry[:relative].gsub(/[^a-zA-Z0-9]/, '_') %>"
+id="permissions-<%= Digest::MD5.hexdigest(entry[:relative]) %>"Or use a counter/index-based approach:
<% @directory.each_with_index do |entry, idx| %>
...
id="permissions-<%= idx %>"
...
onclick="changePermissions('<%= entry[:relative] %>', document.getElementById('permissions-<%= idx %>').value)"🤖 Prompt for AI Agents
In app/views/file_manager/index.html.erb around lines 89 to 97, the input ID is
generated by gsubbing non-alphanumeric chars which can produce collisions (e.g.,
"foo/bar" and "foo_bar"); replace this fragile ID generation with a stable
unique identifier per row such as using the each_with_index loop index or a
short hash of the file path: update the view to use id="permissions-<%= idx %>"
(or id="permissions-<%= Digest::MD5.hexdigest(entry[:relative]) %>") and update
any JS calls/onclicks that reference the old ID to use the new index/hash-based
ID so IDs are guaranteed unique and collision-free.
| 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
1 participant
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
497987865-1c75129c-c828-48a4-a23f-3fd5e439e996.mov
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.