Merge pull request #10 from github/copilot/fix-9

Security audit: Fix critical authentication bypass and class injection vulnerabilities

Author: GrantBirki

lib/hooks/app/auth/auth.rb

@@ -18,16 +18,14 @@ module Auth
       # @note This method will halt execution with an error if authentication fails.
       def validate_auth!(payload, headers, endpoint_config)
         auth_config = endpoint_config[:auth]
-        auth_plugin_type = auth_config[:type].downcase
-        secret_env_key = auth_config[:secret_env_key]
-
-        return unless secret_env_key
 
-        secret = ENV[secret_env_key]
-        unless secret
-          error!("secret '#{secret_env_key}' not found in environment", 500)
+        # Security: Ensure auth type is present and valid
+        unless auth_config&.dig(:type)&.is_a?(String)
+          error!("authentication configuration missing or invalid", 500)
         end
 
+        auth_plugin_type = auth_config[:type].downcase
+
         auth_class = nil
 
         case auth_plugin_type
@@ -42,7 +40,6 @@ def validate_auth!(payload, headers, endpoint_config)
         unless auth_class.valid?(
           payload:,
           headers:,
-          secret:,
           config: endpoint_config
         )
           error!("authentication failed", 401)

lib/hooks/app/helpers.rb

@@ -68,20 +68,68 @@ def parse_payload(raw_body, headers, symbolize: true)
       # @raise [LoadError] If the handler file or class cannot be found
       # @raise [StandardError] Halts with error if handler cannot be loaded
       def load_handler(handler_class_name, handler_dir)
+        # Security: Validate handler class name to prevent arbitrary class loading
+        unless valid_handler_class_name?(handler_class_name)
+          error!("invalid handler class name: #{handler_class_name}", 400)
+        end
+
         # Convert class name to file name (e.g., Team1Handler -> team1_handler.rb)
         # E.g.2: GithubHandler -> github_handler.rb
         # E.g.3: GitHubHandler -> git_hub_handler.rb
         file_name = handler_class_name.gsub(/([A-Z])/, '_\1').downcase.sub(/^_/, "") + ".rb"
         file_path = File.join(handler_dir, file_name)
 
+        # Security: Ensure the file path doesn't escape the handler directory
+        normalized_handler_dir = Pathname.new(File.expand_path(handler_dir))
+        normalized_file_path = Pathname.new(File.expand_path(file_path))
+        unless normalized_file_path.descend.any? { |path| path == normalized_handler_dir }
+          error!("handler path outside of handler directory", 400)
+        end
+
         if File.exist?(file_path)
           require file_path
-          Object.const_get(handler_class_name).new
+          handler_class = Object.const_get(handler_class_name)
+
+          # Security: Ensure the loaded class inherits from the expected base class
+          unless handler_class < Hooks::Handlers::Base
+            error!("handler class must inherit from Hooks::Handlers::Base", 400)
+          end
+
+          handler_class.new
         else
           raise LoadError, "Handler #{handler_class_name} not found at #{file_path}"
         end
       rescue => e
-        error!("failed to load handler #{handler_class_name}: #{e.message}", 500)
+        error!("failed to load handler: #{e.message}", 500)
+      end
+
+      private
+
+      # Validate that a handler class name is safe to load
+      #
+      # @param class_name [String] The class name to validate
+      # @return [Boolean] true if the class name is safe, false otherwise
+      def valid_handler_class_name?(class_name)
+        # Must be a string
+        return false unless class_name.is_a?(String)
+
+        # Must not be empty or only whitespace
+        return false if class_name.strip.empty?
+
+        # Must match a safe pattern: alphanumeric + underscore, starting with uppercase
+        # Examples: MyHandler, GitHubHandler, Team1Handler
+        return false unless class_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/)
+
+        # Must not be a system/built-in class name
+        dangerous_classes = %w[
+          File Dir Kernel Object Class Module Proc Method
+          IO Socket TCPSocket UDPSocket BasicSocket
+          Process Thread Fiber Mutex ConditionVariable
+          Marshal YAML JSON Pathname
+        ]
+        return false if dangerous_classes.include?(class_name)
+
+        true
       end
 
       # Determine HTTP error code from exception

lib/hooks/core/config_validator.rb

@@ -60,7 +60,7 @@ def self.validate_global_config(config)
         result.to_h
       end
 
-      # Validate endpoint configuration
+      # Validate endpoint configuration with additional security checks
       #
       # @param config [Hash] Endpoint configuration to validate
       # @return [Hash] Validated configuration
@@ -72,7 +72,15 @@ def self.validate_endpoint_config(config)
           raise ValidationError, "Invalid endpoint configuration: #{result.errors.to_h}"
         end
 
-        result.to_h
+        validated_config = result.to_h
+
+        # Security: Additional validation for handler name
+        handler_name = validated_config[:handler]
+        unless valid_handler_name?(handler_name)
+          raise ValidationError, "Invalid handler name: #{handler_name}"
+        end
+
+        validated_config
       end
 
       # Validate array of endpoint configurations
@@ -93,6 +101,34 @@ def self.validate_endpoints(endpoints)
 
         validated_endpoints
       end
+
+      private
+
+      # Validate that a handler name is safe
+      #
+      # @param handler_name [String] The handler name to validate
+      # @return [Boolean] true if the handler name is safe, false otherwise
+      def self.valid_handler_name?(handler_name)
+        # Must be a string
+        return false unless handler_name.is_a?(String)
+
+        # Must not be empty or only whitespace
+        return false if handler_name.strip.empty?
+
+        # Must match a safe pattern: alphanumeric + underscore, starting with uppercase
+        return false unless handler_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/)
+
+        # Must not be a system/built-in class name
+        dangerous_classes = %w[
+          File Dir Kernel Object Class Module Proc Method
+          IO Socket TCPSocket UDPSocket BasicSocket
+          Process Thread Fiber Mutex ConditionVariable
+          Marshal YAML JSON Pathname
+        ]
+        return false if dangerous_classes.include?(handler_name)
+
+        true
+      end
     end
   end
 end

lib/hooks/plugins/auth/base.rb

@@ -14,11 +14,10 @@ class Base
         #
         # @param payload [String] Raw request body
         # @param headers [Hash<String, String>] HTTP headers
-        # @param secret [String] Secret key for validation
         # @param config [Hash] Endpoint configuration
         # @return [Boolean] true if request is valid
         # @raise [NotImplementedError] if not implemented by subclass
-        def self.valid?(payload:, headers:, secret:, config:)
+        def self.valid?(payload:, headers:, config:)
           raise NotImplementedError, "Validator must implement .valid? class method"
         end
 
@@ -33,6 +32,30 @@ def self.valid?(payload:, headers:, secret:, config:)
         def self.log
           Hooks::Log.instance
         end
+
+        # Retrieve the secret from the environment variable based on the key set in the configuration
+        #
+        # Note: This method is intended to be used by subclasses
+        # It is a helper method and may not work with all authentication types
+        #
+        # @param config [Hash] Configuration hash containing :auth key
+        # @param secret_env_key [Symbol] The key to look up in the config for the environment variable name
+        # @return [String] The secret
+        # @raise [StandardError] if secret_env_key is missing or empty
+        def self.fetch_secret(config, secret_env_key_name: :secret_env_key)
+          secret_env_key = config.dig(:auth, secret_env_key_name)
+          if secret_env_key.nil? || !secret_env_key.is_a?(String) || secret_env_key.strip.empty?
+            raise StandardError, "authentication configuration incomplete: missing secret_env_key"
+          end
+
+          secret = ENV[secret_env_key]
+
+          if secret.nil? || !secret.is_a?(String) || secret.strip.empty?
+            raise StandardError, "authentication configuration incomplete: missing secret value bound to #{secret_env_key_name}"
+          end
+
+          return secret.strip
+        end
       end
     end
   end

lib/hooks/plugins/auth/hmac.rb

@@ -64,7 +64,6 @@ class HMAC < Base
         #
         # @param payload [String] Raw request body to validate
         # @param headers [Hash<String, String>] HTTP headers from the request
-        # @param secret [String] Secret key for HMAC computation
         # @param config [Hash] Endpoint configuration containing validator settings
         # @option config [Hash] :auth Validator-specific configuration
         # @option config [String] :header ('X-Signature') Header containing the signature
@@ -82,11 +81,11 @@ class HMAC < Base
         #   HMAC.valid?(
         #     payload: request_body,
         #     headers: request.headers,
-        #     secret: ENV['WEBHOOK_SECRET'],
         #     config: { auth: { header: 'X-Signature' } }
         #   )
-        def self.valid?(payload:, headers:, secret:, config:)
-          return false if secret.nil? || secret.empty?
+        def self.valid?(payload:, headers:, config:)
+          # fetch the required secret from environment variable as specified in the config
+          secret = fetch_secret(config)
 
           validator_config = build_config(config)
 

lib/hooks/plugins/auth/shared_secret.rb

@@ -43,7 +43,6 @@ class SharedSecret < Base
         #
         # @param payload [String] Raw request body (unused but required by interface)
         # @param headers [Hash<String, String>] HTTP headers from the request
-        # @param secret [String] Expected secret value for comparison
         # @param config [Hash] Endpoint configuration containing validator settings
         # @option config [Hash] :auth Validator-specific configuration
         # @option config [String] :header ('Authorization') Header containing the secret
@@ -55,11 +54,10 @@ class SharedSecret < Base
         #   SharedSecret.valid?(
         #     payload: request_body,
         #     headers: request.headers,
-        #     secret: ENV['WEBHOOK_SECRET'],
         #     config: { auth: { header: 'Authorization' } }
         #   )
-        def self.valid?(payload:, headers:, secret:, config:)
-          return false if secret.nil? || secret.empty?
+        def self.valid?(payload:, headers:, config:)
+          secret = fetch_secret(config)
 
           validator_config = build_config(config)