Security fixes: timing attacks, plugin loading, config validation

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

Author: Copilot

lib/hooks/app/auth/auth.rb

@@ -24,9 +24,8 @@ def validate_auth!(payload, headers, endpoint_config, global_config = {}, reques
         auth_config = endpoint_config[:auth]
         request_id = request_context&.dig(:request_id)
 
-        # Ensure auth type is present and valid
-        auth_type = auth_config&.dig(:type)
-        unless auth_type&.is_a?(String) && !auth_type.strip.empty?
+        # Validate auth configuration structure
+        unless auth_config.is_a?(Hash) && !auth_config.empty?
           log.error("authentication configuration missing or invalid - request_id: #{request_id}")
           error!({
             error: "authentication_configuration_error",
@@ -35,6 +34,28 @@ def validate_auth!(payload, headers, endpoint_config, global_config = {}, reques
           }, 500)
         end
 
+        # Ensure auth type is present and valid
+        auth_type = auth_config[:type]
+        unless auth_type.is_a?(String) && !auth_type.strip.empty?
+          log.error("authentication type missing or invalid - request_id: #{request_id}")
+          error!({
+            error: "authentication_configuration_error",
+            message: "authentication configuration missing or invalid",
+            request_id:
+          }, 500)
+        end
+
+        # Sanitize auth type to prevent potential exploits
+        auth_type = auth_type.strip.downcase
+        unless auth_type.match?(/\A[a-z0-9_]+\z/)
+          log.error("authentication type contains invalid characters - request_id: #{request_id}")
+          error!({
+            error: "authentication_configuration_error",
+            message: "authentication configuration contains invalid characters",
+            request_id:
+          }, 400)
+        end
+
         # Get auth plugin from loaded plugins registry (boot-time loaded only)
         begin
           auth_class = Core::PluginLoader.get_auth_plugin(auth_type)

lib/hooks/app/helpers.rb

@@ -122,9 +122,19 @@ def ip_filtering!(headers, endpoint_config, global_config, request_context, env)
       def safe_json_parse(json_string)
         # Security limits for JSON parsing
         max_nesting = ENV.fetch("JSON_MAX_NESTING", "20").to_i
+        max_size = ENV.fetch("JSON_MAX_SIZE", "10485760").to_i # 10MB default
+
+        # Validate security limits
+        if max_nesting <= 0 || max_nesting > 100
+          raise ArgumentError, "Invalid JSON_MAX_NESTING value: must be between 1 and 100"
+        end
+
+        if max_size <= 0 || max_size > 100 * 1024 * 1024 # 100MB max
+          raise ArgumentError, "Invalid JSON_MAX_SIZE value: must be between 1 and 104857600 bytes"
+        end
 
         # Additional size check before parsing
-        if json_string.length > ENV.fetch("JSON_MAX_SIZE", "10485760").to_i # 10MB default
+        if json_string.length > max_size
           raise ArgumentError, "JSON payload too large for parsing"
         end
 

lib/hooks/core/config_loader.rb

@@ -107,7 +107,8 @@ def self.load_config_file(file_path)
         when ".json"
           JSON.parse(content)
         when ".yml", ".yaml"
-          YAML.safe_load(content, permitted_classes: [Symbol])
+          # Security: Use safe_load without permitting classes for maximum security
+          YAML.safe_load(content, permitted_classes: [], permitted_symbols: [], aliases: false)
         else
           nil
         end

lib/hooks/core/plugin_loader.rb

@@ -204,8 +204,13 @@ def load_custom_auth_plugin(file_path, auth_plugin_dir)
           # Load the file
           require file_path
 
-          # Get the class and validate it
-          auth_plugin_class = Object.const_get("Hooks::Plugins::Auth::#{class_name}")
+          # Get the class and validate it - use scoped const_get to prevent potential exploits
+          auth_plugin_class = begin
+            Hooks::Plugins::Auth.const_get(class_name, false) # false = don't inherit from ancestors
+          rescue NameError
+            raise StandardError, "Auth plugin class not found in Hooks::Plugins::Auth namespace: #{class_name}"
+          end
+
           unless auth_plugin_class < Hooks::Plugins::Auth::Base
             raise StandardError, "Auth plugin class must inherit from Hooks::Plugins::Auth::Base: #{class_name}"
           end
@@ -239,8 +244,14 @@ def load_custom_handler_plugin(file_path, handler_plugin_dir)
           # Load the file
           require file_path
 
-          # Get the class and validate it
-          handler_class = Object.const_get(class_name)
+          # Get the class and validate it - use safe constant lookup
+          handler_class = begin
+            # Check if the constant exists in the global namespace for handlers
+            Object.const_get(class_name, false) # false = don't inherit from ancestors
+          rescue NameError
+            raise StandardError, "Handler class not found: #{class_name}"
+          end
+
           unless handler_class < Hooks::Plugins::Handlers::Base
             raise StandardError, "Handler class must inherit from Hooks::Plugins::Handlers::Base: #{class_name}"
           end
@@ -274,8 +285,12 @@ def load_custom_lifecycle_plugin(file_path, lifecycle_plugin_dir)
           # Load the file
           require file_path
 
-          # Get the class and validate it
-          lifecycle_class = Object.const_get(class_name)
+          # Get the class and validate it - use safe constant lookup
+          lifecycle_class = begin
+            Object.const_get(class_name, false) # false = don't inherit from ancestors
+          rescue NameError
+            raise StandardError, "Lifecycle plugin class not found: #{class_name}"
+          end
           unless lifecycle_class < Hooks::Plugins::Lifecycle
             raise StandardError, "Lifecycle plugin class must inherit from Hooks::Plugins::Lifecycle: #{class_name}"
           end
@@ -309,8 +324,12 @@ def load_custom_instrument_plugin(file_path, instruments_plugin_dir)
           # Load the file
           require file_path
 
-          # Get the class and validate it
-          instrument_class = Object.const_get(class_name)
+          # Get the class and validate it - use safe constant lookup
+          instrument_class = begin
+            Object.const_get(class_name, false) # false = don't inherit from ancestors
+          rescue NameError
+            raise StandardError, "Instrument plugin class not found: #{class_name}"
+          end
 
           # Determine instrument type based on inheritance
           if instrument_class < Hooks::Plugins::Instruments::StatsBase

lib/hooks/plugins/auth/shared_secret.rb

@@ -81,10 +81,10 @@ def self.valid?(payload:, headers:, config:)
             return false
           end
 
-          stripped_secret = raw_secret.strip
-
           # Use secure comparison to prevent timing attacks
-          result = Rack::Utils.secure_compare(secret, stripped_secret)
+          # Note: Since valid_header_value? already ensures no leading/trailing whitespace,
+          # we can safely use the raw_secret directly to prevent any potential timing attacks
+          result = Rack::Utils.secure_compare(secret, raw_secret)
           if result
             log.debug("Auth::SharedSecret validation successful for header '#{secret_header}'")
           else

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

@@ -79,6 +79,55 @@ def error!(message, code)
             instance.validate_auth!(payload, headers, endpoint_config)
           end.to raise_error(StandardError, /authentication configuration missing or invalid/)
         end
+
+        it "rejects request with whitespace-only type" do
+          endpoint_config = { auth: { type: "   " } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
+        end
+      end
+
+      context "with invalid auth type characters" do
+        it "rejects request with auth type containing invalid characters" do
+          endpoint_config = { auth: { type: "hmac-test!" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration contains invalid characters/)
+        end
+
+        it "rejects request with auth type containing spaces" do
+          endpoint_config = { auth: { type: "hmac test" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration contains invalid characters/)
+        end
+
+        it "accepts valid auth type with uppercase (converts to lowercase)" do
+          endpoint_config = { auth: { type: "HMAC", secret_env_key: "TEST_SECRET" } }
+          ENV["TEST_SECRET"] = "test-secret"
+
+          # Should convert HMAC to hmac and then fail authentication (not configuration error)
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
+
+          ENV.delete("TEST_SECRET")
+        end
+
+        it "accepts valid auth type with underscores and numbers" do
+          endpoint_config = { auth: { type: "hmac_v2_test", secret_env_key: "TEST_SECRET" } }
+          ENV["TEST_SECRET"] = "test-secret"
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /unsupported auth type 'hmac_v2_test'/)
+
+          ENV.delete("TEST_SECRET")
+        end
       end
 
       context "with missing secret configuration" do

spec/unit/lib/hooks/app/helpers_spec.rb

@@ -324,6 +324,45 @@ def error!(message, code)
         helper.send(:safe_json_parse, large_json)
       }.to raise_error(ArgumentError, "JSON payload too large for parsing")
     end
+
+    it "raises ArgumentError when JSON_MAX_NESTING is invalid (too low)" do
+      stub_const("ENV", ENV.to_h.merge("JSON_MAX_NESTING" => "0"))
+
+      expect {
+        helper.send(:safe_json_parse, '{"test": "data"}')
+      }.to raise_error(ArgumentError, "Invalid JSON_MAX_NESTING value: must be between 1 and 100")
+    end
+
+    it "raises ArgumentError when JSON_MAX_NESTING is invalid (too high)" do
+      stub_const("ENV", ENV.to_h.merge("JSON_MAX_NESTING" => "101"))
+
+      expect {
+        helper.send(:safe_json_parse, '{"test": "data"}')
+      }.to raise_error(ArgumentError, "Invalid JSON_MAX_NESTING value: must be between 1 and 100")
+    end
+
+    it "raises ArgumentError when JSON_MAX_SIZE is invalid (too low)" do
+      stub_const("ENV", ENV.to_h.merge("JSON_MAX_SIZE" => "0"))
+
+      expect {
+        helper.send(:safe_json_parse, '{"test": "data"}')
+      }.to raise_error(ArgumentError, "Invalid JSON_MAX_SIZE value: must be between 1 and 104857600 bytes")
+    end
+
+    it "raises ArgumentError when JSON_MAX_SIZE is invalid (too high)" do
+      stub_const("ENV", ENV.to_h.merge("JSON_MAX_SIZE" => "104857601"))
+
+      expect {
+        helper.send(:safe_json_parse, '{"test": "data"}')
+      }.to raise_error(ArgumentError, "Invalid JSON_MAX_SIZE value: must be between 1 and 104857600 bytes")
+    end
+
+    it "parses valid JSON with valid limits" do
+      stub_const("ENV", ENV.to_h.merge("JSON_MAX_NESTING" => "5", "JSON_MAX_SIZE" => "100"))
+
+      result = helper.send(:safe_json_parse, '{"test": "data"}')
+      expect(result).to eq({ "test" => "data" })
+    end
   end
 
   describe "#determine_error_code" do

spec/unit/lib/hooks/core/plugin_loader_spec.rb

@@ -238,6 +238,28 @@ def self.valid?(payload:, headers:, config:)
           described_class.send(:load_custom_auth_plugin, wrong_file, temp_auth_dir)
         }.to raise_error(StandardError, /Auth plugin class must inherit from Hooks::Plugins::Auth::Base/)
       end
+
+      it "raises error when auth plugin class is not found after loading" do
+        temp_auth_dir = File.join(temp_dir, "auth_missing_class")
+        FileUtils.mkdir_p(temp_auth_dir)
+
+        # Create plugin file that doesn't define the expected class
+        missing_file = File.join(temp_auth_dir, "missing_auth.rb")
+        File.write(missing_file, <<~RUBY)
+          # This file doesn't define MissingAuth class
+          module Hooks
+            module Plugins
+              module Auth
+                # Nothing here
+              end
+            end
+          end
+        RUBY
+
+        expect {
+          described_class.send(:load_custom_auth_plugin, missing_file, temp_auth_dir)
+        }.to raise_error(StandardError, /Auth plugin class not found in Hooks::Plugins::Auth namespace: MissingAuth/)
+      end
     end
 
     describe "handler plugin loading failures" do
@@ -298,6 +320,23 @@ def call(payload:, headers:, env:, config:)
           described_class.send(:load_custom_handler_plugin, wrong_file, temp_handler_dir)
         }.to raise_error(StandardError, /Handler class must inherit from Hooks::Plugins::Handlers::Base/)
       end
+
+      it "raises error when handler plugin class is not found after loading" do
+        temp_handler_dir = File.join(temp_dir, "handler_missing_class")
+        FileUtils.mkdir_p(temp_handler_dir)
+
+        # Create plugin file that doesn't define the expected class
+        missing_file = File.join(temp_handler_dir, "missing_handler.rb")
+        File.write(missing_file, <<~RUBY)
+          # This file doesn't define MissingHandler class
+          class SomeOtherClass
+          end
+        RUBY
+
+        expect {
+          described_class.send(:load_custom_handler_plugin, missing_file, temp_handler_dir)
+        }.to raise_error(StandardError, /Handler class not found: MissingHandler/)
+      end
     end
 
     describe "lifecycle plugin loading failures" do
@@ -358,6 +397,23 @@ def on_request(env)
           described_class.send(:load_custom_lifecycle_plugin, wrong_file, temp_lifecycle_dir)
         }.to raise_error(StandardError, /Lifecycle plugin class must inherit from Hooks::Plugins::Lifecycle/)
       end
+
+      it "raises error when lifecycle plugin class is not found after loading" do
+        temp_lifecycle_dir = File.join(temp_dir, "lifecycle_missing_class")
+        FileUtils.mkdir_p(temp_lifecycle_dir)
+
+        # Create plugin file that doesn't define the expected class
+        missing_file = File.join(temp_lifecycle_dir, "missing_lifecycle.rb")
+        File.write(missing_file, <<~RUBY)
+          # This file doesn't define MissingLifecycle class
+          class SomeOtherClass
+          end
+        RUBY
+
+        expect {
+          described_class.send(:load_custom_lifecycle_plugin, missing_file, temp_lifecycle_dir)
+        }.to raise_error(StandardError, /Lifecycle plugin class not found: MissingLifecycle/)
+      end
     end
 
     describe "instrument plugin loading failures" do
@@ -418,6 +474,23 @@ def record(metric_name, value, tags = {})
           described_class.send(:load_custom_instrument_plugin, wrong_file, temp_instrument_dir)
         }.to raise_error(StandardError, /Instrument plugin class must inherit from StatsBase or FailbotBase/)
       end
+
+      it "raises error when instrument plugin class is not found after loading" do
+        temp_instrument_dir = File.join(temp_dir, "instrument_missing_class")
+        FileUtils.mkdir_p(temp_instrument_dir)
+
+        # Create plugin file that doesn't define the expected class
+        missing_file = File.join(temp_instrument_dir, "missing_instrument.rb")
+        File.write(missing_file, <<~RUBY)
+          # This file doesn't define MissingInstrument class
+          class SomeOtherClass
+          end
+        RUBY
+
+        expect {
+          described_class.send(:load_custom_instrument_plugin, missing_file, temp_instrument_dir)
+        }.to raise_error(StandardError, /Instrument plugin class not found: MissingInstrument/)
+      end
     end
   end
 end

spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb

@@ -250,6 +250,19 @@ def valid_with(args = {})
         expect(valid_with(headers:)).to be true
       end
 
+      it "prevents timing attacks via whitespace manipulation" do
+        # Ensure that if somehow whitespace gets through validation,
+        # we still don't expose timing information by comparing directly
+        headers_with_raw_secret = { default_header => secret }
+
+        # Mock valid_header_value? to return true (simulating bypass)
+        allow(described_class).to receive(:valid_header_value?).and_return(true)
+
+        # Verify secure_compare is called with the raw secret, not stripped
+        expect(Rack::Utils).to receive(:secure_compare).with(secret, secret).and_return(true)
+        expect(valid_with(headers: headers_with_raw_secret)).to be true
+      end
+
       it "handles exceptions gracefully" do
         # Mock an exception during validation
         allow(Rack::Utils).to receive(:secure_compare).and_raise(StandardError.new("test error"))