Add more error handling, revamp regex string to better reflect valid …

…ruby module names, add better sanitization for display name -> name conversion, fix docs to reflect actually valid assessment names.

app/controllers/assessments_controller.rb

@@ -111,7 +111,7 @@ def install_assessment
                                          filename)) || (filename == "..") || (filename == ".")
 
       # assessment names must be only lowercase letters and digits
-      if filename !~ /^[A-Za-z_][A-Za-z][A-Za-z0-9_-]*$/
+      if filename !~ Assessment::VALID_NAME_REGEX
         # add line break if adding to existing error message
         flash.now[:error] = flash.now[:error] ? "#{flash.now[:error]} <br>" : ""
         flash.now[:error] += "An error occurred while trying to display an existing assessment " \
@@ -263,6 +263,7 @@ def importAssessment
       @assessment.load_yaml # this will save the assessment
     rescue StandardError => e
       flash[:error] = "Error loading yaml: #{e}"
+      Rails.logger.debug e.backtrace
       destroy_no_redirect
       # delete files explicitly b/c the paths don't match ONLY if
       # import was from tarball
@@ -296,12 +297,25 @@ def create
       # We just want to prevent file traversal attacks here, and stop names that break routing
       # first regex - try to sanitize input, allow special characters in display name but not name
       # if the sanitized doesn't match the required identifier structure, then we reject
-      sanitized_display_name = @assessment.display_name.gsub(/[!@#$%^&*(),.?":{}|<>\s]/, "")
-      Rails.logger.debug(sanitized_display_name)
-      if sanitized_display_name !~ /^[A-Za-z_][A-Za-z][A-Za-z0-9_-]*$/
-        # TODO: update with docs link
+      begin
+        # Attempt name generation, try to match to a substring that is valid within the display name
+        match = @assessment.display_name.match(Assessment::VALID_NAME_SANITIZER_REGEX)
+        unless match.nil?
+          sanitized_display_name = match.captures[0]
+        end
+
+        if sanitized_display_name !~ Assessment::VALID_NAME_REGEX
+          flash[:error] =
+            "Assessment name is blank or contains disallowed characters. Find more information on "\
+            "valid assessment names "\
+            '<a href="https://docs.autolabproject.com/lab/#assessment-naming-rules">here</a>'
+          flash[:html_safe] = true
+          redirect_to(action: :install_assessment)
+          return
+        end
+      rescue StandardError
         flash[:error] =
-          "Assessment name is blank or contains disallowed characters. Find more information on"\
+          "Error creating name from display name. Find more information on "\
           "valid assessment names "\
           '<a href="https://docs.autolabproject.com/lab/#assessment-naming-rules">here</a>'
         flash[:html_safe] = true
@@ -1067,7 +1081,7 @@ def valid_asmt_tar(tar_extract)
     # it is possible that the assessment path does not match the
     # the expected assessment path when the Ruby config file
     # has a different name then the pathname
-    if !asmt_name.nil? && asmt_name !~ /^[A-Za-z_][A-Za-z][A-Za-z0-9_-]*$/
+    if !asmt_name.nil? && asmt_name !~ Assessment::VALID_NAME_REGEX
       flash[:error] = "Errors found in tarball: Assessment name #{asmt_name} is invalid.
                        Find more information on valid assessment names "\
           '<a href="https://docs.autolabproject.com/lab/#assessment-naming-rules">here</a> <br>'

app/helpers/assessment_autograde_core.rb

@@ -510,7 +510,9 @@ def extend_config_module(assessment, submission, cud)
         require assessment.config_file_path
       end
     rescue TypeError => e
-      raise AutogradeError, "could not find the assessment config file"
+      raise AutogradeError, "could not find the assessment config file: #{e}"
+    rescue LoadError => e
+      raise AutogradeError, "could not load the assessment config file: #{e}"
     end
 
 

app/models/assessment.rb

@@ -46,7 +46,8 @@ class Assessment < ApplicationRecord
   # Constants
   ORDERING = "due_at ASC, name ASC".freeze
   RELEASED = "start_at < ?".freeze
-
+  VALID_NAME_REGEX = /^[A-Za-z][A-Za-z0-9_-]*$/.freeze
+  VALID_NAME_SANITIZER_REGEX = /^[^A-Za-z]*([^A-Za-z]*[A-Za-z0-9_-]+)/.freeze
   # Scopes
   scope :ordered, -> { order(ORDERING) }
   scope :released, ->(as_of = Time.current) { where(RELEASED, as_of) }
@@ -232,7 +233,7 @@ def load_config_file
       # rename module name if it doesn't match new unique naming scheme
       if config_source !~ /\b#{unique_config_module_name}\b/
         match = config_source.match(/module\s+(\w+)/)
-        config_source = config_source.gsub(match[1], unique_config_module_name)
+        config_source = config_source.sub(match[0], "module #{unique_config_module_name}")
       end
       File.open(unique_source_config_file_path, "w"){ |f| f.write(config_source) }
       File.rename(source_config_file_path, source_config_file_backup_path)
@@ -250,7 +251,7 @@ def load_config_file
     # uniquely rename module (so that it's unique among all assessment modules loaded in Autolab)
     if config_source !~ /\b#{unique_config_module_name}\b/
       match = config_source.match(/module\s+(\w+)/)
-      config_source = config_source.gsub(match[1], unique_config_module_name)
+      config_source = config_source.sub(match[0], "module #{unique_config_module_name}")
     end
 
     # backup old *unique* configs

docs/lab.md

@@ -430,9 +430,10 @@ but hyphens are also allowed. The assessment name must also be unique within a c
 - `autolabAsmt`
 - `Autolab-Asmt`
 - `Autolab_Asmt10`
-- `_4utol4b-_4smt`
+- `AuTol4b-_4smt`
 
 #### Invalid Assessment Names
+- `_autolab`
 - `4utolab`
 - `autolabAsmt!!`
 - `-autolabAsmt`