diff --git a/lib/hooks/app/auth/auth.rb b/lib/hooks/app/auth/auth.rb index 5d1e14ad..7a552dee 100644 --- a/lib/hooks/app/auth/auth.rb +++ b/lib/hooks/app/auth/auth.rb @@ -20,11 +20,12 @@ 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) + auth_type = auth_config&.dig(:type) + unless auth_type&.is_a?(String) && !auth_type.strip.empty? error!("authentication configuration missing or invalid", 500) end - auth_plugin_type = auth_config[:type].downcase + auth_plugin_type = auth_type.downcase auth_class = nil diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb index 3ce384f8..180da285 100644 --- a/lib/hooks/app/helpers.rb +++ b/lib/hooks/app/helpers.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "securerandom" +require_relative "../security" module Hooks module App @@ -121,13 +122,7 @@ def valid_handler_class_name?(class_name) 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) + return false if Hooks::Security::DANGEROUS_CLASSES.include?(class_name) true end diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb index f11a0352..2a4a4a70 100644 --- a/lib/hooks/core/config_validator.rb +++ b/lib/hooks/core/config_validator.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "dry-schema" +require_relative "../security" module Hooks module Core @@ -119,13 +120,7 @@ def self.valid_handler_name?(handler_name) 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) + return false if Hooks::Security::DANGEROUS_CLASSES.include?(handler_name) true end diff --git a/lib/hooks/security.rb b/lib/hooks/security.rb new file mode 100644 index 00000000..274138ba --- /dev/null +++ b/lib/hooks/security.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Hooks + module Security + # List of dangerous class names that should not be loaded as handlers + # for security reasons. These classes provide system access that could + # be exploited if loaded dynamically. + # + # @return [Array] Array of dangerous class names + 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 + ].freeze + end +end 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 18857e3e..7a0b9150 100644 --- a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb +++ b/spec/unit/lib/hooks/app/auth/auth_security_spec.rb @@ -3,6 +3,7 @@ require_relative "../../../../spec_helper" describe Hooks::App::Auth do + let(:log) { instance_double(Logger).as_null_object } let(:test_class) do Class.new do include Hooks::App::Auth @@ -17,6 +18,10 @@ def error!(message, code) let(:payload) { '{"test": "data"}' } let(:headers) { { "Content-Type" => "application/json" } } + before(:each) do + Hooks::Log.instance = log + end + describe "#validate_auth!" do context "when testing security vulnerabilities" do context "with missing auth configuration" do @@ -63,39 +68,77 @@ def error!(message, code) end it "rejects request with empty string type" do - # TODO + 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 missing secret configuration" do it "rejects request with missing secret_env_key" do - # TODO + endpoint_config = { auth: { type: "hmac" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error(StandardError, /authentication failed/) end it "rejects request with nil secret_env_key" do - # TODO + endpoint_config = { auth: { type: "hmac", secret_env_key: nil } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error(StandardError, /authentication failed/) end it "rejects request with empty secret_env_key" do - # TODO + endpoint_config = { auth: { type: "hmac", secret_env_key: "" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error(StandardError, /authentication failed/) end it "rejects request with whitespace-only secret_env_key" do - # TODO + endpoint_config = { auth: { type: "hmac", secret_env_key: " " } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error(StandardError, /authentication failed/) end it "rejects request with non-string secret_env_key" do - # TODO + endpoint_config = { auth: { type: "hmac", secret_env_key: 123 } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error(StandardError, /authentication failed/) end end context "with missing environment variable" do it "uses generic error message for missing secrets" do - # TODO + ENV.delete("NONEXISTENT_SECRET") + 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 failed/) end it "does not leak the environment variable name in error" do - # TODO + ENV.delete("SECRET_WEBHOOK_KEY") + endpoint_config = { auth: { type: "hmac", secret_env_key: "SECRET_WEBHOOK_KEY" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config) + end.to raise_error do |error| + # Ensure error message is generic and doesn't leak the environment variable name + expect(error.message).not_to include("SECRET_WEBHOOK_KEY") + expect(error.message).to match(/authentication failed/) + end end end diff --git a/spec/unit/lib/hooks/app/helpers_security_spec.rb b/spec/unit/lib/hooks/app/helpers_security_spec.rb index 15f1cb1f..3d35bee2 100644 --- a/spec/unit/lib/hooks/app/helpers_security_spec.rb +++ b/spec/unit/lib/hooks/app/helpers_security_spec.rb @@ -37,9 +37,7 @@ def env describe "#load_handler security" do context "with malicious handler class names" do it "rejects system class names" do - dangerous_classes = %w[File Dir Kernel Object Class Module Proc Method] - - dangerous_classes.each do |class_name| + Hooks::Security::DANGEROUS_CLASSES.each do |class_name| expect do instance.load_handler(class_name, handler_dir) end.to raise_error(StandardError, /invalid handler class name/) @@ -48,6 +46,8 @@ def env it "rejects network-related class names" do network_classes = %w[IO Socket TCPSocket UDPSocket BasicSocket] + # Verify these are all in our dangerous classes list + network_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) } network_classes.each do |class_name| expect do @@ -58,6 +58,8 @@ def env it "rejects process and system class names" do system_classes = %w[Process Thread Fiber Mutex ConditionVariable] + # Verify these are all in our dangerous classes list + system_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) } system_classes.each do |class_name| expect do @@ -68,6 +70,8 @@ def env it "rejects serialization class names" do serialization_classes = %w[Marshal YAML JSON Pathname] + # Verify these are all in our dangerous classes list + serialization_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) } serialization_classes.each do |class_name| expect do diff --git a/spec/unit/lib/hooks/core/config_validator_security_spec.rb b/spec/unit/lib/hooks/core/config_validator_security_spec.rb index e94c1ff1..e49bd743 100644 --- a/spec/unit/lib/hooks/core/config_validator_security_spec.rb +++ b/spec/unit/lib/hooks/core/config_validator_security_spec.rb @@ -25,24 +25,9 @@ end it "rejects dangerous system class names" do - dangerous_configs = [ - { path: "/webhook", handler: "File" }, - { path: "/webhook", handler: "Dir" }, - { path: "/webhook", handler: "Kernel" }, - { path: "/webhook", handler: "Object" }, - { path: "/webhook", handler: "Class" }, - { path: "/webhook", handler: "Module" }, - { path: "/webhook", handler: "Proc" }, - { path: "/webhook", handler: "Method" }, - { path: "/webhook", handler: "IO" }, - { path: "/webhook", handler: "Socket" }, - { path: "/webhook", handler: "TCPSocket" }, - { path: "/webhook", handler: "Process" }, - { path: "/webhook", handler: "Thread" }, - { path: "/webhook", handler: "Marshal" }, - { path: "/webhook", handler: "YAML" }, - { path: "/webhook", handler: "JSON" } - ] + dangerous_configs = Hooks::Security::DANGEROUS_CLASSES.map do |class_name| + { path: "/webhook", handler: class_name } + end dangerous_configs.each do |config| expect do