From 5fb81adbe8994d5dd0299d15b3864dccea928798 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Tue, 17 Jun 2025 21:46:45 -0700 Subject: [PATCH 1/5] remove redundant `.strip` --- lib/hooks/plugins/auth/shared_secret.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/hooks/plugins/auth/shared_secret.rb b/lib/hooks/plugins/auth/shared_secret.rb index 2de0e1a..c55f12e 100644 --- a/lib/hooks/plugins/auth/shared_secret.rb +++ b/lib/hooks/plugins/auth/shared_secret.rb @@ -81,10 +81,8 @@ 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) + result = Rack::Utils.secure_compare(secret, raw_secret) if result log.debug("Auth::SharedSecret validation successful for header '#{secret_header}'") else From 214e99af75c6b6fa3c546cb0ae4c175bc37e44c4 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Tue, 17 Jun 2025 21:47:15 -0700 Subject: [PATCH 2/5] refactor: rename variable for clarity in shared secret validation --- lib/hooks/plugins/auth/shared_secret.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/hooks/plugins/auth/shared_secret.rb b/lib/hooks/plugins/auth/shared_secret.rb index c55f12e..457329e 100644 --- a/lib/hooks/plugins/auth/shared_secret.rb +++ b/lib/hooks/plugins/auth/shared_secret.rb @@ -68,21 +68,21 @@ def self.valid?(payload:, headers:, config:) secret_header = validator_config[:header] # Find the secret header with case-insensitive matching - raw_secret = find_header_value(headers, secret_header) + provided_secret = find_header_value(headers, secret_header) - if raw_secret.nil? || raw_secret.empty? + if provided_secret.nil? || provided_secret.empty? log.warn("Auth::SharedSecret validation failed: Missing or empty secret header '#{secret_header}'") return false end # Validate secret format using shared validation - unless valid_header_value?(raw_secret, "Secret") + unless valid_header_value?(provided_secret, "Secret") log.warn("Auth::SharedSecret validation failed: Invalid secret format") return false end # Use secure comparison to prevent timing attacks - result = Rack::Utils.secure_compare(secret, raw_secret) + result = Rack::Utils.secure_compare(secret, provided_secret) if result log.debug("Auth::SharedSecret validation successful for header '#{secret_header}'") else From e75000465f0bf27953f7ce827901c25663797725 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Tue, 17 Jun 2025 22:00:03 -0700 Subject: [PATCH 3/5] test: add error handling tests for missing plugin classes and don't inherit from ancestors when loading custom plugins --- lib/hooks/core/plugin_loader.rb | 31 ++++++-- .../unit/lib/hooks/core/plugin_loader_spec.rb | 73 +++++++++++++++++++ 2 files changed, 97 insertions(+), 7 deletions(-) diff --git a/lib/hooks/core/plugin_loader.rb b/lib/hooks/core/plugin_loader.rb index 7c137b8..13d4981 100644 --- a/lib/hooks/core/plugin_loader.rb +++ b/lib/hooks/core/plugin_loader.rb @@ -205,7 +205,11 @@ def load_custom_auth_plugin(file_path, auth_plugin_dir) require file_path # Get the class and validate it - auth_plugin_class = Object.const_get("Hooks::Plugins::Auth::#{class_name}") + 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 +243,13 @@ 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 +283,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 +322,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/spec/unit/lib/hooks/core/plugin_loader_spec.rb b/spec/unit/lib/hooks/core/plugin_loader_spec.rb index 4cb6f51..0245753 100644 --- a/spec/unit/lib/hooks/core/plugin_loader_spec.rb +++ b/spec/unit/lib/hooks/core/plugin_loader_spec.rb @@ -240,6 +240,28 @@ def self.valid?(payload:, headers:, config:) end 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 + describe "handler plugin loading failures" do it "raises error when handler plugin file fails to load" do temp_handler_dir = File.join(temp_dir, "handler_failures") @@ -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 classAdd commentMore actions + 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 From 2d214ad8f246d81788343ec5b99b91c5e6a13d73 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Tue, 17 Jun 2025 22:02:45 -0700 Subject: [PATCH 4/5] bump --- Gemfile.lock | 2 +- lib/hooks/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index f4b7cde..5461111 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - hooks-ruby (0.3.1) + hooks-ruby (0.3.2) dry-schema (~> 1.14, >= 1.14.1) grape (~> 2.3) puma (~> 6.6) diff --git a/lib/hooks/version.rb b/lib/hooks/version.rb index a50c58a..7dba295 100644 --- a/lib/hooks/version.rb +++ b/lib/hooks/version.rb @@ -4,5 +4,5 @@ module Hooks # Current version of the Hooks webhook framework # @return [String] The version string following semantic versioning - VERSION = "0.3.1".freeze + VERSION = "0.3.2".freeze end From 22eb0c66e2ae93257f33369e5a57818a82a2c210 Mon Sep 17 00:00:00 2001 From: Grant Birkinbine Date: Tue, 17 Jun 2025 22:03:40 -0700 Subject: [PATCH 5/5] Update spec/unit/lib/hooks/core/plugin_loader_spec.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- spec/unit/lib/hooks/core/plugin_loader_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/lib/hooks/core/plugin_loader_spec.rb b/spec/unit/lib/hooks/core/plugin_loader_spec.rb index 0245753..df212d0 100644 --- a/spec/unit/lib/hooks/core/plugin_loader_spec.rb +++ b/spec/unit/lib/hooks/core/plugin_loader_spec.rb @@ -479,7 +479,7 @@ def record(metric_name, value, tags = {}) 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 classAdd commentMore actions + # 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