Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disallow creating or updating scheduler if invalid #2214

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Disallow creating or updating scheduler if invalid #2214

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5990374
Add checks for existing and valid Action when creating new scheduler …
jhs-panda Sep 20, 2024
dcb71c2
Fix formatting.
jhs-panda Sep 20, 2024
288fc88
Only allow for creating new schedulers and updating current scheduler…
jhs-panda Sep 22, 2024
352afba
Correct logic of returning error.
jhs-panda Sep 22, 2024
25d78cf
Addressed comments in review.
jhs-panda Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 107 additions & 49 deletions app/controllers/schedulers_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,23 @@ def new; end
action_auth_level :create, :instructor
def create
@scheduler = @course.scheduler.new(scheduler_params)
if @scheduler.save
flash[:success] = "Scheduler created!"
redirect_to(course_schedulers_path(@course))
else
flash[:error] = "Scheduler create failed. Please check all fields."
redirect_to(new_course_scheduler_path(@course))
action_path = Rails.root.join(scheduler_params[:action]).to_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also check that the action is within the assessment directory? See Archive.in_dir? which for example is used in handout.rb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Do you mean the course directory? From what I can tell schedulers are for courses, not specific assessments.

# 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))
Comment on lines +26 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security and data integrity concerns in scheduler creation

Several critical issues need to be addressed:

  1. The code doesn't validate if the action file is within the assessment directory, which could lead to security vulnerabilities.
  2. The creation process should be wrapped in a transaction to maintain data consistency.
  3. Using Rails.root.join with 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))
end

Committable suggestion skipped: line range outside the PR's diff.

end

action_auth_level :edit, :instructor
Expand All @@ -44,56 +54,32 @@ def run

action_auth_level :visual_run, :instructor
def visual_run
action = Scheduler.find(params[:scheduler_id])
# https://stackoverflow.com/a/1076445
read, write = IO.pipe
@log = "Executing #{Rails.root.join(action.action)}\n"
begin
pid = fork do
read.close
mod_name = Rails.root.join(action.action).to_path
fork_log = ""
begin
require mod_name
output = Updater.update(action.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 '#{@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 in '#{@course.name}' updater: #{e.message}\n"
@log << e.backtrace.join("\n\t")
@log << "\n---- End Error Output -----"
end
@log << "\nCompleted running action."
@scheduler = Scheduler.find(params[:scheduler_id])
@log = execute_action(@scheduler)
render partial: "visual_test"
Comment on lines +57 to 59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
@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"

end

action_auth_level :update, :instructor
def update
@scheduler = Scheduler.find_by(id: params[:id])
if @scheduler&.update(scheduler_params)
flash[:success] = "Scheduler updated."
redirect_to(course_schedulers_path(@course))
else
flash[:error] = "Scheduler update failed! Please check your fields."
redirect_to(edit_course_scheduler_path(@course, @scheduler))
action_path = Rails.root.join(scheduler_params[:action]).to_path
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved
# 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))
Comment on lines +65 to +82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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))
end

Committable suggestion skipped: line range outside the PR's diff.

end

action_auth_level :destroy, :instructor
Expand All @@ -119,4 +105,76 @@ def set_manage_scheduler_breadcrumb

@breadcrumbs << (view_context.link_to "Manage Schedulers", course_schedulers_path(@course))
end

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
Comment on lines +109 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
Comment on lines +128 to +138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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
end

Committable 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
Comment on lines +140 to +179
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical security and reliability improvements needed in execute_action

Several critical issues need to be addressed in the execution logic:

  1. Unsafe require of user input
  2. No timeout on forked process
  3. Potential file descriptor leaks
  4. 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.

Suggested change
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

end