Disallow creating or updating scheduler if invalid #2214
Conversation
…or updating current scheduler.
…s if visual run under action does not return an error.
📝 Walkthrough📝 WalkthroughWalkthroughThe changes made to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
app/controllers/schedulers_controller.rb (2)
29-30: Correct error message formatting to avoid unintended newlinesThe error message string spans multiple lines without proper concatenation, which can include unintended newline characters in the displayed message. To ensure the error message is formatted correctly, combine the string into a single line or use string concatenation.
Apply this diff to fix the formatting:
- flash[:error] = "Scheduler create failed. Action file does not exist or is - not readable at #{action_path}." + flash[:error] = "Scheduler create failed. Action file does not exist or is " \ + "not readable at #{action_path}."
109-110: Correct error message formatting to avoid unintended newlinesSimilar to the issue in the
createmethod, the error message here spans multiple lines without proper concatenation. To ensure the message is displayed correctly, combine the string into a single line or use string concatenation.Apply this diff to fix the formatting:
- flash[:error] = "Scheduler update failed. Action file does not exist or is - not readable at #{action_path}." + flash[:error] = "Scheduler update failed. Action file does not exist or is " \ + "not readable at #{action_path}."
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/controllers/schedulers_controller.rb (3 hunks)
Additional comments not posted (2)
app/controllers/schedulers_controller.rb (2)
35-44: Robust error handling added to ensure scheduler validityThe addition of error handling in the
createmethod ensures that if thetest_run_visual_schedulerfails, the created scheduler is destroyed, and an appropriate error message is displayed. This enhances the reliability of the scheduler creation process.
106-113: Refactor repeated file existence checks into a private methodAs mentioned earlier, the file existence and readability check here is duplicated from the
createmethod. Extracting this logic into a private method will reduce duplication and improve code maintainability.
jhs-panda
requested review from
a team and
20wildmanj
and removed request for
a team
| @@ -23,9 +23,25 @@ def new; end | |||
| action_auth_level :create, :instructor | |||
| def create | |||
| @scheduler = @course.scheduler.new(scheduler_params) | |||
| # Check if the action file exists and is readable | |||
| action_path = Rails.root.join(scheduler_params[:action]).to_path | |||
There was a problem hiding this comment.
Can you also check that the action is within the assessment directory? See Archive.in_dir? which for example is used in handout.rb.
There was a problem hiding this comment.
Hi! Do you mean the course directory? From what I can tell schedulers are for courses, not specific assessments.
| action_path = Rails.root.join(scheduler_params[:action]).to_path | ||
| # Check if the action file exists, is readable, and compiles | ||
| if validate_compile_action_file(action_path) | ||
| if @scheduler.save | ||
| # Ensure visual run is successful | ||
| if run_visual_scheduler(@scheduler) | ||
| flash[:success] = "Scheduler created and executed successfully!" | ||
| redirect_to(course_schedulers_path(@course)) and return | ||
| else | ||
| @scheduler.destroy | ||
| flash[:error] = "Scheduler creation failed during execution." | ||
| end | ||
| else | ||
| flash[:error] = "Scheduler create failed. Please check all fields." | ||
| end | ||
| end | ||
| redirect_to(new_course_scheduler_path(@course)) |
There was a problem hiding this comment.
Security and data integrity concerns in scheduler creation
Several critical issues need to be addressed:
- The code doesn't validate if the action file is within the assessment directory, which could lead to security vulnerabilities.
- The creation process should be wrapped in a transaction to maintain data consistency.
- Using
Rails.root.joinwith user input could lead to path traversal attacks.
Consider implementing these fixes:
def create
@scheduler = @course.scheduler.new(scheduler_params)
+
+ ActiveRecord::Base.transaction do
+ # Validate file is within assessment directory
+ action_path = Rails.root.join(scheduler_params[:action]).to_path
+ unless Archive.in_dir?(action_path, @course.assessment.directory)
+ raise "Action file must be within the assessment directory"
+ end
+
if validate_compile_action_file(action_path)
if @scheduler.save
if run_visual_scheduler(@scheduler)
flash[:success] = "Scheduler created and executed successfully!"
redirect_to(course_schedulers_path(@course)) and return
else
raise ActiveRecord::Rollback
end
end
end
+ raise ActiveRecord::Rollback
+ end
flash[:error] ||= "Scheduler creation failed"
redirect_to(new_course_scheduler_path(@course))
endCommittable suggestion skipped: line range outside the PR's diff.
| @scheduler = Scheduler.find(params[:scheduler_id]) | ||
| @log = execute_action(@scheduler) | ||
| render partial: "visual_test" |
There was a problem hiding this comment.
Add error handling to visual_run action
The action should handle potential exceptions from execute_action and provide appropriate feedback.
def visual_run
@scheduler = Scheduler.find(params[:scheduler_id])
+ begin
@log = execute_action(@scheduler)
+ flash[:success] = "Visual test completed successfully"
+ rescue StandardError => e
+ @log = "Error executing scheduler: #{e.message}"
+ flash[:error] = "Visual test failed"
+ end
render partial: "visual_test"
end📝 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.
| @scheduler = Scheduler.find(params[:scheduler_id]) | |
| @log = execute_action(@scheduler) | |
| render partial: "visual_test" | |
| @scheduler = Scheduler.find(params[:scheduler_id]) | |
| begin | |
| @log = execute_action(@scheduler) | |
| flash[:success] = "Visual test completed successfully" | |
| rescue StandardError => e | |
| @log = "Error executing scheduler: #{e.message}" | |
| flash[:error] = "Visual test failed" | |
| end | |
| render partial: "visual_test" |
| def validate_compile_action_file(action_path) | ||
| # Check if the action file exists and is readable | ||
| unless File.exist?(action_path) && File.readable?(action_path) | ||
| flash[:error] = "Scheduler update failed. Action file does not exist or is | ||
| not readable at #{action_path}." | ||
| return false | ||
| end | ||
|
|
||
| # compile action file to check for syntax errors | ||
| begin | ||
| RubyVM::InstructionSequence.compile(File.read(action_path)) | ||
| rescue SyntaxError => e | ||
| flash[:error] = "Syntax error in action file: #{e.message}" | ||
| return false | ||
| end | ||
|
|
||
| true | ||
| end |
There was a problem hiding this comment.
Enhance action file validation
The validation method should be more robust and secure.
Consider these improvements:
def validate_compile_action_file(action_path)
+ # Validate absolute path to prevent directory traversal
+ unless action_path.start_with?(Rails.root.to_s)
+ flash[:error] = "Invalid action path"
+ return false
+ end
# Check if the action file exists and is readable
unless File.exist?(action_path) && File.readable?(action_path)
flash[:error] = "Scheduler update failed. Action file does not exist or is not readable"
return false
end
# compile action file to check for syntax errors
begin
RubyVM::InstructionSequence.compile(File.read(action_path))
rescue SyntaxError => e
flash[:error] = "Syntax error in action file: #{e.message}"
return false
+ rescue StandardError => e
+ flash[:error] = "Error validating action file: #{e.message}"
+ return false
end
true
end📝 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.
| def validate_compile_action_file(action_path) | |
| # Check if the action file exists and is readable | |
| unless File.exist?(action_path) && File.readable?(action_path) | |
| flash[:error] = "Scheduler update failed. Action file does not exist or is | |
| not readable at #{action_path}." | |
| return false | |
| end | |
| # compile action file to check for syntax errors | |
| begin | |
| RubyVM::InstructionSequence.compile(File.read(action_path)) | |
| rescue SyntaxError => e | |
| flash[:error] = "Syntax error in action file: #{e.message}" | |
| return false | |
| end | |
| true | |
| end | |
| def validate_compile_action_file(action_path) | |
| # Validate absolute path to prevent directory traversal | |
| unless action_path.start_with?(Rails.root.to_s) | |
| flash[:error] = "Invalid action path" | |
| return false | |
| end | |
| # Check if the action file exists and is readable | |
| unless File.exist?(action_path) && File.readable?(action_path) | |
| flash[:error] = "Scheduler update failed. Action file does not exist or is not readable" | |
| return false | |
| end | |
| # compile action file to check for syntax errors | |
| begin | |
| RubyVM::InstructionSequence.compile(File.read(action_path)) | |
| rescue SyntaxError => e | |
| flash[:error] = "Syntax error in action file: #{e.message}" | |
| return false | |
| rescue StandardError => e | |
| flash[:error] = "Error validating action file: #{e.message}" | |
| return false | |
| end | |
| true | |
| end |
| def run_visual_scheduler(scheduler) | ||
| log = execute_action(scheduler) | ||
| # Ensure visual run is successful or return error | ||
| if log.include?("Error") | ||
| flash[:error] = "Scheduler execution failed." | ||
| false | ||
| else | ||
| flash[:success] = "Scheduler executed successfully!" | ||
| true | ||
| end | ||
| end |
There was a problem hiding this comment.
Improve error detection in run_visual_scheduler
The current error detection using string matching is unreliable.
def run_visual_scheduler(scheduler)
log = execute_action(scheduler)
- # Ensure visual run is successful or return error
- if log.include?("Error")
+ # Check execution status from execute_action
+ if log[:status] == :error
- flash[:error] = "Scheduler execution failed."
+ flash[:error] = "Scheduler execution failed: #{log[:message]}"
false
else
flash[:success] = "Scheduler executed successfully!"
true
end
endCommittable suggestion skipped: line range outside the PR's diff.
| action_path = Rails.root.join(scheduler_params[:action]).to_path | ||
| # Check if the action file exists, is readable, and compiles | ||
| if validate_compile_action_file(action_path) | ||
| previous_state = @scheduler.attributes | ||
| if @scheduler.update(scheduler_params) | ||
| # Ensure visual run is successful | ||
| if run_visual_scheduler(@scheduler) | ||
| flash[:success] = "Scheduler updated and executed successfully!" | ||
| redirect_to(course_schedulers_path(@course)) and return | ||
| else | ||
| @scheduler.update(previous_state) # If error, revert to previous state. | ||
| flash[:error] = "Scheduler update failed during execution. Reverted to previous state." | ||
| end | ||
| else | ||
| flash[:error] = "Scheduler update failed! Please check your fields." | ||
| end | ||
| end | ||
| redirect_to(edit_course_scheduler_path(@course, @scheduler)) |
There was a problem hiding this comment.
Improve update action security and reliability
The current implementation has similar security issues as the create action and uses manual state management which could be problematic.
Consider this improved implementation:
def update
@scheduler = Scheduler.find_by(id: params[:id])
+ ActiveRecord::Base.transaction do
+ # Validate file is within assessment directory
+ action_path = Rails.root.join(scheduler_params[:action]).to_path
+ unless Archive.in_dir?(action_path, @course.assessment.directory)
+ raise "Action file must be within the assessment directory"
+ end
+
if validate_compile_action_file(action_path)
- previous_state = @scheduler.attributes
if @scheduler.update(scheduler_params)
if run_visual_scheduler(@scheduler)
flash[:success] = "Scheduler updated and executed successfully!"
redirect_to(course_schedulers_path(@course)) and return
else
- @scheduler.update(previous_state)
+ raise ActiveRecord::Rollback
end
end
end
+ raise ActiveRecord::Rollback
+ end
flash[:error] ||= "Scheduler update failed"
redirect_to(edit_course_scheduler_path(@course, @scheduler))
endCommittable suggestion skipped: line range outside the PR's diff.
| def execute_action(scheduler) | ||
| action_path = Rails.root.join(scheduler.action).to_path | ||
| # https://stackoverflow.com/a/1076445 | ||
| read, write = IO.pipe | ||
| log = "Executing #{action_path}\n" | ||
| begin | ||
| pid = fork do | ||
| read.close | ||
| mod_name = action_path | ||
| fork_log = "" | ||
| begin | ||
| require mod_name | ||
| output = Updater.update(scheduler.course) | ||
| if output.respond_to?(:to_str) | ||
| fork_log << "----- Script Output -----\n" | ||
| fork_log << output | ||
| fork_log << "\n----- End Script Output -----" | ||
| end | ||
| rescue ScriptError, StandardError => e | ||
| fork_log << "----- Script Error Output -----\n" | ||
| fork_log << "Error in '#{scheduler.course.name}' updater: #{e.message}\n" | ||
| fork_log << e.backtrace.join("\n\t") | ||
| fork_log << "\n---- End Script Error Output -----" | ||
| end | ||
| write.print fork_log | ||
| end | ||
|
|
||
| write.close | ||
| result = read.read | ||
| Process.wait2(pid) | ||
| log << result | ||
| rescue StandardError => e | ||
| log << "----- Error Output -----\n" | ||
| log << "Error during execution: #{e.message}\n" | ||
| log << e.backtrace.join("\n\t") | ||
| log << "\n---- End Error Output -----" | ||
| end | ||
| log << "\nCompleted running action." | ||
| log | ||
| end |
There was a problem hiding this comment.
Critical security and reliability improvements needed in execute_action
Several critical issues need to be addressed in the execution logic:
- Unsafe
requireof user input - No timeout on forked process
- Potential file descriptor leaks
- No proper process cleanup
Consider this improved implementation:
def execute_action(scheduler)
action_path = Rails.root.join(scheduler.action).to_path
read, write = IO.pipe
log = "Executing #{action_path}\n"
+ result = { status: :success, message: "" }
begin
+ Timeout.timeout(30) do # Add timeout
pid = fork do
read.close
- mod_name = action_path
fork_log = ""
begin
- require mod_name
+ # Load file content without require
+ eval(File.read(action_path), binding, action_path, 1)
output = Updater.update(scheduler.course)
if output.respond_to?(:to_str)
fork_log << "----- Script Output -----\n"
fork_log << output
fork_log << "\n----- End Script Output -----"
end
rescue ScriptError, StandardError => e
fork_log << "----- Script Error Output -----\n"
fork_log << "Error in '#{scheduler.course.name}' updater: #{e.message}\n"
fork_log << e.backtrace.join("\n\t")
fork_log << "\n---- End Script Error Output -----"
end
write.print fork_log
+ exit!(0)
end
write.close
result = read.read
- Process.wait2(pid)
+ _, status = Process.wait2(pid)
+ unless status.success?
+ result[:status] = :error
+ result[:message] = "Process exited with status #{status.exitstatus}"
+ end
log << result
end
+ rescue Timeout::Error
+ Process.kill('TERM', pid) rescue nil
+ result[:status] = :error
+ result[:message] = "Execution timed out after 30 seconds"
rescue StandardError => e
+ result[:status] = :error
+ result[:message] = e.message
log << "----- Error Output -----\n"
log << "Error during execution: #{e.message}\n"
log << e.backtrace.join("\n\t")
log << "\n---- End Error Output -----"
ensure
read.close rescue nil
write.close rescue nil
end
log << "\nCompleted running action."
- log
+ result.merge(log: log)
end📝 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.
| def execute_action(scheduler) | |
| action_path = Rails.root.join(scheduler.action).to_path | |
| # https://stackoverflow.com/a/1076445 | |
| read, write = IO.pipe | |
| log = "Executing #{action_path}\n" | |
| begin | |
| pid = fork do | |
| read.close | |
| mod_name = action_path | |
| fork_log = "" | |
| begin | |
| require mod_name | |
| output = Updater.update(scheduler.course) | |
| if output.respond_to?(:to_str) | |
| fork_log << "----- Script Output -----\n" | |
| fork_log << output | |
| fork_log << "\n----- End Script Output -----" | |
| end | |
| rescue ScriptError, StandardError => e | |
| fork_log << "----- Script Error Output -----\n" | |
| fork_log << "Error in '#{scheduler.course.name}' updater: #{e.message}\n" | |
| fork_log << e.backtrace.join("\n\t") | |
| fork_log << "\n---- End Script Error Output -----" | |
| end | |
| write.print fork_log | |
| end | |
| write.close | |
| result = read.read | |
| Process.wait2(pid) | |
| log << result | |
| rescue StandardError => e | |
| log << "----- Error Output -----\n" | |
| log << "Error during execution: #{e.message}\n" | |
| log << e.backtrace.join("\n\t") | |
| log << "\n---- End Error Output -----" | |
| end | |
| log << "\nCompleted running action." | |
| log | |
| end | |
| def execute_action(scheduler) | |
| action_path = Rails.root.join(scheduler.action).to_path | |
| read, write = IO.pipe | |
| log = "Executing #{action_path}\n" | |
| result = { status: :success, message: "" } | |
| begin | |
| Timeout.timeout(30) do # Add timeout | |
| pid = fork do | |
| read.close | |
| fork_log = "" | |
| begin | |
| # Load file content without require | |
| eval(File.read(action_path), binding, action_path, 1) | |
| output = Updater.update(scheduler.course) | |
| if output.respond_to?(:to_str) | |
| fork_log << "----- Script Output -----\n" | |
| fork_log << output | |
| fork_log << "\n----- End Script Output -----" | |
| end | |
| rescue ScriptError, StandardError => e | |
| fork_log << "----- Script Error Output -----\n" | |
| fork_log << "Error in '#{scheduler.course.name}' updater: #{e.message}\n" | |
| fork_log << e.backtrace.join("\n\t") | |
| fork_log << "\n---- End Script Error Output -----" | |
| end | |
| write.print fork_log | |
| exit!(0) | |
| end | |
| write.close | |
| result = read.read | |
| _, status = Process.wait2(pid) | |
| unless status.success? | |
| result[:status] = :error | |
| result[:message] = "Process exited with status #{status.exitstatus}" | |
| end | |
| log << result | |
| end | |
| rescue Timeout::Error | |
| Process.kill('TERM', pid) rescue nil | |
| result[:status] = :error | |
| result[:message] = "Execution timed out after 30 seconds" | |
| rescue StandardError => e | |
| result[:status] = :error | |
| result[:message] = e.message | |
| log << "----- Error Output -----\n" | |
| log << "Error during execution: #{e.message}\n" | |
| log << e.backtrace.join("\n\t") | |
| log << "\n---- End Error Output -----" | |
| ensure | |
| read.close rescue nil | |
| write.close rescue nil | |
| end | |
| log << "\nCompleted running action." | |
| result.merge(log: log) | |
| end |
Description
When creating or updating a scheduler, tests visual run to ensure action can be loaded and works.
If visual run test works, allow scheduler creation/update.
If visual run test doesn't work on creation, delete created scheduler.
If visual run test doesn't work on update, reinstate previous scheduler prior to update attempt.
Motivation and Context
Resolves #2187
How Has This Been Tested?
Before:
Allows creation as long as inputs to Scheduler Creation filled. However, when running scheduler, gets many errors.
After:
Notes error if action does not exist or is not readable:
Notes error if action exists and is readable but cannot execute properly:
If update is not successful, notes error and reverts to previous state:
For successful actions that load properly, work when run:
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required