diff --git a/.bundle/config b/.bundle/config index 0146a1ce..f9263841 100644 --- a/.bundle/config +++ b/.bundle/config @@ -1,7 +1,8 @@ --- BUNDLE_BIN: "bin" -BUNDLE_PATH: "vendor/gems" +BUNDLE_PATH: "/home/runner/work/hooks/hooks/vendor/bundle" BUNDLE_CACHE_PATH: "vendor/cache" BUNDLE_CACHE_ALL: "true" BUNDLE_SPECIFIC_PLATFORM: "true" BUNDLE_NO_INSTALL: "true" +BUNDLE_DEPLOYMENT: "true" diff --git a/lib/hooks/app/auth/auth.rb b/lib/hooks/app/auth/auth.rb index 93e9addf..1f0b4a84 100644 --- a/lib/hooks/app/auth/auth.rb +++ b/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) diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb index 7c3b89a7..5713b6eb 100644 --- a/lib/hooks/app/helpers.rb +++ b/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 diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index 69fd677a..06bb5a57 100644 --- a/lib/hooks/core/config_loader.rb +++ b/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 diff --git a/lib/hooks/core/plugin_loader.rb b/lib/hooks/core/plugin_loader.rb index 7c137b81..3bdc6532 100644 --- a/lib/hooks/core/plugin_loader.rb +++ b/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 diff --git a/lib/hooks/plugins/auth/shared_secret.rb b/lib/hooks/plugins/auth/shared_secret.rb index 2de0e1ac..3233d2ea 100644 --- a/lib/hooks/plugins/auth/shared_secret.rb +++ b/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 diff --git a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb b/spec/unit/lib/hooks/app/auth/auth_security_spec.rb index b94d8b74..33ab83dd 100644 --- a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb +++ b/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 diff --git a/spec/unit/lib/hooks/app/helpers_spec.rb b/spec/unit/lib/hooks/app/helpers_spec.rb index 21cb7cd6..5609c8e5 100644 --- a/spec/unit/lib/hooks/app/helpers_spec.rb +++ b/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 diff --git a/spec/unit/lib/hooks/core/plugin_loader_spec.rb b/spec/unit/lib/hooks/core/plugin_loader_spec.rb index 4cb6f511..6810aa64 100644 --- a/spec/unit/lib/hooks/core/plugin_loader_spec.rb +++ b/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 diff --git a/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb b/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb index 64373e13..47d6ecb3 100644 --- a/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb +++ b/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"))