Security fixes: prevent auth bypass, handler injection, and add comprehensive security tests

Co-authored-by: GrantBirki <[email protected]>

Author: Copilot

lib/hooks/app/auth/auth.rb

@@ -18,14 +18,23 @@ 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]
+
+        # 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
         secret_env_key = auth_config[:secret_env_key]
 
-        return unless secret_env_key
+        # Security: Require secret_env_key when auth is configured
+        unless secret_env_key&.is_a?(String) && !secret_env_key.strip.empty?
+          error!("authentication configuration incomplete", 500)
+        end
 
         secret = ENV[secret_env_key]
         unless secret
-          error!("secret '#{secret_env_key}' not found in environment", 500)
+          error!("authentication secret not configured", 500)
         end
 
         auth_class = nil

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 = File.expand_path(handler_dir)
+        normalized_file_path = File.expand_path(file_path)
+        unless normalized_file_path.start_with?(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

spec/unit/lib/hooks/app/auth/auth_security_spec.rb

@@ -0,0 +1,216 @@
+# frozen_string_literal: true
+
+require_relative "../../../../spec_helper"
+
+describe Hooks::App::Auth do
+  let(:test_class) do
+    Class.new do
+      include Hooks::App::Auth
+
+      def error!(message, code)
+        raise StandardError, "#{message} (#{code})"
+      end
+    end
+  end
+
+  let(:instance) { test_class.new }
+  let(:payload) { '{"test": "data"}' }
+  let(:headers) { { "Content-Type" => "application/json" } }
+
+  describe "#validate_auth!" do
+    context "when testing security vulnerabilities" do
+      context "with missing auth configuration" do
+        it "rejects request with no auth config" do
+          endpoint_config = {}
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+
+        it "rejects request with nil auth config" do
+          endpoint_config = { auth: nil }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+
+        it "rejects request with empty auth config" do
+          endpoint_config = { auth: {} }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+      end
+
+      context "with missing auth type" do
+        it "rejects request with nil type" do
+          endpoint_config = { auth: { type: nil } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+
+        it "rejects request with non-string type" do
+          endpoint_config = { auth: { type: 123 } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+
+        it "rejects request with empty string type" do
+          endpoint_config = { auth: { type: "" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+      end
+
+      context "with missing secret configuration" do
+        it "rejects request with missing secret_env_key" do
+          endpoint_config = { auth: { type: "hmac" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+
+        it "rejects request with nil secret_env_key" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: nil } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+
+        it "rejects request with empty secret_env_key" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+
+        it "rejects request with whitespace-only secret_env_key" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "   " } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+
+        it "rejects request with non-string secret_env_key" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: 123 } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration incomplete/)
+        end
+      end
+
+      context "with missing environment variable" do
+        it "uses generic error message for missing secrets" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "NONEXISTENT_SECRET" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication secret not configured/)
+        end
+
+        it "does not leak the environment variable name in error" do
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "SUPER_SECRET_WEBHOOK_KEY" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error do |error|
+            expect(error.message).not_to include("SUPER_SECRET_WEBHOOK_KEY")
+            expect(error.message).to include("authentication secret not configured")
+          end
+        end
+      end
+
+      context "with valid configuration but invalid secrets" do
+        before do
+          ENV["TEST_WEBHOOK_SECRET"] = "test-secret"
+        end
+
+        after do
+          ENV.delete("TEST_WEBHOOK_SECRET")
+        end
+
+        it "properly validates HMAC authentication" do
+          endpoint_config = {
+            auth: {
+              type: "hmac",
+              secret_env_key: "TEST_WEBHOOK_SECRET",
+              header: "X-Signature"
+            }
+          }
+          invalid_headers = headers.merge("X-Signature" => "invalid-signature")
+
+          expect do
+            instance.validate_auth!(payload, invalid_headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
+        end
+
+        it "properly validates shared_secret authentication" do
+          endpoint_config = {
+            auth: {
+              type: "shared_secret",
+              secret_env_key: "TEST_WEBHOOK_SECRET",
+              header: "Authorization"
+            }
+          }
+          invalid_headers = headers.merge("Authorization" => "wrong-secret")
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
+        end
+      end
+
+      context "with edge case auth types" do
+        before do
+          ENV["TEST_WEBHOOK_SECRET"] = "test-secret"
+        end
+
+        after do
+          ENV.delete("TEST_WEBHOOK_SECRET")
+        end
+
+        it "handles case-insensitive auth types" do
+          endpoint_config = {
+            auth: {
+              type: "HMAC",
+              secret_env_key: "TEST_WEBHOOK_SECRET",
+              header: "X-Signature"
+            }
+          }
+          invalid_headers = headers.merge("X-Signature" => "invalid")
+
+          expect do
+            instance.validate_auth!(payload, invalid_headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
+        end
+
+        it "rejects unknown auth types" do
+          endpoint_config = {
+            auth: {
+              type: "unknown_type",
+              secret_env_key: "TEST_WEBHOOK_SECRET"
+            }
+          }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /Custom validators not implemented/)
+        end
+      end
+    end
+  end
+end