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 6 commits into
base: master
Choose a base branch
from
Open

Disallow creating or updating scheduler if invalid #2214

Changes from 5 commits
Commits
Show all changes
6 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
5f48195
Added additional error checking suggested by CodeRabbit.
jhs-panda Nov 13, 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))
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved
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

Add path validation to prevent directory traversal

The current implementation using Rails.root.join with user input could be vulnerable to path traversal attacks. Consider adding validation to ensure the action file is within the allowed course directory.

 def create
   @scheduler = @course.scheduler.new(scheduler_params)
+  course_dir = Rails.root.join('courses', @course.name).to_path
   action_path = Rails.root.join(scheduler_params[:action]).to_path
+  
+  unless action_path.start_with?(course_dir)
+    flash[:error] = "Action file must be within the course directory"
+    redirect_to(new_course_scheduler_path(@course)) and return
+  end

   # Check if the action file exists, is readable, and compiles
   if validate_compile_action_file(action_path)

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"
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved
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.

🛠️ Refactor suggestion

Use database transaction for atomic updates

The current implementation manually manages state rollback. Using a database transaction would ensure atomic updates and proper rollback of all changes.

 def update
   @scheduler = Scheduler.find_by(id: params[:id])
-  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
+  ActiveRecord::Base.transaction do
+    action_path = Rails.root.join(scheduler_params[:action]).to_path
+    
+    if validate_compile_action_file(action_path)
+      if @scheduler.update(scheduler_params)
+        unless run_visual_scheduler(@scheduler)
+          raise ActiveRecord::Rollback, "Visual run failed"
+        end
+        flash[:success] = "Scheduler updated and executed successfully!"
+        redirect_to(course_schedulers_path(@course)) and return
+      end
     end
-  end
+    raise ActiveRecord::Rollback
+  end
+  flash[:error] ||= "Scheduler update failed"
   redirect_to(edit_course_scheduler_path(@course, @scheduler))
 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
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))
def update
@scheduler = Scheduler.find_by(id: params[:id])
ActiveRecord::Base.transaction do
action_path = Rails.root.join(scheduler_params[:action]).to_path
if validate_compile_action_file(action_path)
if @scheduler.update(scheduler_params)
unless run_visual_scheduler(@scheduler)
raise ActiveRecord::Rollback, "Visual run failed"
end
flash[:success] = "Scheduler updated and executed successfully!"
redirect_to(course_schedulers_path(@course)) and return
end
end
raise ActiveRecord::Rollback
end
flash[:error] ||= "Scheduler update failed"
redirect_to(edit_course_scheduler_path(@course, @scheduler))
end

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
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved

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
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved

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
jhs-panda marked this conversation as resolved.
Show resolved Hide resolved
end