diff --git a/lib/hooks/plugins/auth/base.rb b/lib/hooks/plugins/auth/base.rb index fc41e35..86324b9 100644 --- a/lib/hooks/plugins/auth/base.rb +++ b/lib/hooks/plugins/auth/base.rb @@ -43,11 +43,30 @@ def self.fetch_secret(config, secret_env_key_name: :secret_env_key) secret = ENV[secret_env_key] if secret.nil? || !secret.is_a?(String) || secret.strip.empty? - raise StandardError, "authentication configuration incomplete: missing secret value bound to #{secret_env_key_name}" + raise StandardError, "authentication configuration incomplete: missing secret value for environment variable" end return secret.strip end + + # Find a header value by name with case-insensitive matching + # + # @param headers [Hash] HTTP headers from the request + # @param header_name [String] Name of the header to find + # @return [String, nil] The header value if found, nil otherwise + # @note This method performs case-insensitive header matching + def self.find_header_value(headers, header_name) + return nil unless headers.respond_to?(:each) + return nil if header_name.nil? || header_name.strip.empty? + + target_header = header_name.downcase + headers.each do |key, value| + if key.to_s.downcase == target_header + return value.to_s + end + end + nil + end end end end diff --git a/lib/hooks/plugins/auth/hmac.rb b/lib/hooks/plugins/auth/hmac.rb index cae23b9..be937ae 100644 --- a/lib/hooks/plugins/auth/hmac.rb +++ b/lib/hooks/plugins/auth/hmac.rb @@ -92,26 +92,32 @@ def self.valid?(payload:, headers:, config:) validator_config = build_config(config) # Security: Check raw headers BEFORE normalization to detect tampering - return false unless headers.respond_to?(:each) + unless headers.respond_to?(:each) + log.warn("Auth::HMAC validation failed: Invalid headers object") + return false + end signature_header = validator_config[:header] - # Find the signature header with case-insensitive matching but preserve original value - raw_signature = nil - headers.each do |key, value| - if key.to_s.downcase == signature_header.downcase - raw_signature = value.to_s - break - end - end + # Find the signature header with case-insensitive matching + raw_signature = find_header_value(headers, signature_header) - return false if raw_signature.nil? || raw_signature.empty? + if raw_signature.nil? || raw_signature.empty? + log.warn("Auth::HMAC validation failed: Missing or empty signature header '#{signature_header}'") + return false + end # Security: Reject signatures with leading/trailing whitespace - return false if raw_signature != raw_signature.strip + if raw_signature != raw_signature.strip + log.warn("Auth::HMAC validation failed: Signature contains leading/trailing whitespace") + return false + end # Security: Reject signatures containing null bytes or other control characters - return false if raw_signature.match?(/[\u0000-\u001f\u007f-\u009f]/) + if raw_signature.match?(/[\u0000-\u001f\u007f-\u009f]/) + log.warn("Auth::HMAC validation failed: Signature contains control characters") + return false + end # Now we can safely normalize headers for the rest of the validation normalized_headers = normalize_headers(headers) @@ -134,7 +140,13 @@ def self.valid?(payload:, headers:, config:) ) # Use secure comparison to prevent timing attacks - Rack::Utils.secure_compare(computed_signature, provided_signature) + result = Rack::Utils.secure_compare(computed_signature, provided_signature) + if result + log.debug("Auth::HMAC validation successful for header '#{signature_header}'") + else + log.warn("Auth::HMAC validation failed: Signature mismatch") + end + result rescue StandardError => e log.error("Auth::HMAC validation failed: #{e.message}") false diff --git a/lib/hooks/plugins/auth/shared_secret.rb b/lib/hooks/plugins/auth/shared_secret.rb index cd7d5a7..50550bd 100644 --- a/lib/hooks/plugins/auth/shared_secret.rb +++ b/lib/hooks/plugins/auth/shared_secret.rb @@ -62,37 +62,56 @@ def self.valid?(payload:, headers:, config:) validator_config = build_config(config) # Security: Check raw headers BEFORE normalization to detect tampering - return false unless headers.respond_to?(:each) + unless headers.respond_to?(:each) + log.warn("Auth::SharedSecret validation failed: Invalid headers object") + return false + end secret_header = validator_config[:header] - # Find the secret header with case-insensitive matching but preserve original value - raw_secret = nil - headers.each do |key, value| - if key.to_s.downcase == secret_header.downcase - raw_secret = value.to_s - break - end - end + # Find the secret header with case-insensitive matching + raw_secret = find_header_value(headers, secret_header) - return false if raw_secret.nil? || raw_secret.empty? + if raw_secret.nil? || raw_secret.empty? + log.warn("Auth::SharedSecret validation failed: Missing or empty secret header '#{secret_header}'") + return false + end stripped_secret = raw_secret.strip # Security: Reject secrets with leading/trailing whitespace - return false if raw_secret != stripped_secret + if raw_secret != stripped_secret + log.warn("Auth::SharedSecret validation failed: Secret contains leading/trailing whitespace") + return false + end # Security: Reject secrets containing null bytes or other control characters - return false if raw_secret.match?(/[\u0000-\u001f\u007f-\u009f]/) + if raw_secret.match?(/[\u0000-\u001f\u007f-\u009f]/) + log.warn("Auth::SharedSecret validation failed: Secret contains control characters") + return false + end # Use secure comparison to prevent timing attacks - Rack::Utils.secure_compare(secret, stripped_secret) - rescue StandardError => _e + result = Rack::Utils.secure_compare(secret, stripped_secret) + if result + log.debug("Auth::SharedSecret validation successful for header '#{secret_header}'") + else + log.warn("Auth::SharedSecret validation failed: Signature mismatch") + end + result + rescue StandardError => e + log.error("Auth::SharedSecret validation failed: #{e.message}") false end private + # Short logger accessor for auth module + # @return [Hooks::Log] Logger instance + def self.log + Hooks::Log.instance + end + # Build final configuration by merging defaults with provided config # # Combines default configuration values with user-provided settings, diff --git a/spec/unit/lib/hooks/plugins/auth/base_spec.rb b/spec/unit/lib/hooks/plugins/auth/base_spec.rb index df241ef..1d4779d 100644 --- a/spec/unit/lib/hooks/plugins/auth/base_spec.rb +++ b/spec/unit/lib/hooks/plugins/auth/base_spec.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative "../../../../spec_helper" + describe Hooks::Plugins::Auth::Base do describe ".valid?" do let(:payload) { '{"test": "data"}' } @@ -206,7 +208,7 @@ def self.valid?(payload:, headers:, config:) describe "documentation compliance" do it "has the expected public interface" do - expect(described_class.methods).to include(:valid?, :log, :stats, :failbot, :fetch_secret) + expect(described_class.methods).to include(:valid?, :log, :stats, :failbot, :fetch_secret, :find_header_value) end it "valid? method accepts the documented parameters" do @@ -217,6 +219,46 @@ def self.valid?(payload:, headers:, config:) end end + describe ".find_header_value" do + it "finds header value with case-insensitive matching" do + headers = { "Content-Type" => "application/json", "X-Test" => "value" } + + expect(described_class.find_header_value(headers, "content-type")).to eq("application/json") + expect(described_class.find_header_value(headers, "CONTENT-TYPE")).to eq("application/json") + expect(described_class.find_header_value(headers, "x-test")).to eq("value") + expect(described_class.find_header_value(headers, "X-TEST")).to eq("value") + end + + it "returns nil for missing headers" do + headers = { "Content-Type" => "application/json" } + + expect(described_class.find_header_value(headers, "Missing-Header")).to be_nil + expect(described_class.find_header_value(headers, "")).to be_nil + expect(described_class.find_header_value(headers, " ")).to be_nil + expect(described_class.find_header_value(headers, nil)).to be_nil + end + + it "handles invalid headers object" do + expect(described_class.find_header_value(nil, "Content-Type")).to be_nil + expect(described_class.find_header_value("not a hash", "Content-Type")).to be_nil + expect(described_class.find_header_value(123, "Content-Type")).to be_nil + end + + it "converts non-string values to strings" do + headers = { "X-Count" => 42, "X-Boolean" => true } + + expect(described_class.find_header_value(headers, "X-Count")).to eq("42") + expect(described_class.find_header_value(headers, "x-boolean")).to eq("true") + end + + it "handles headers with symbol keys" do + headers = { :content_type => "application/json", "X-Test" => "value" } + + expect(described_class.find_header_value(headers, "content_type")).to eq("application/json") + expect(described_class.find_header_value(headers, "x-test")).to eq("value") + end + end + describe "global component access" do describe ".log" do it "provides access to global log" do diff --git a/spec/unit/lib/hooks/plugins/auth/hmac_spec.rb b/spec/unit/lib/hooks/plugins/auth/hmac_spec.rb index aab7dff..e8b37f1 100644 --- a/spec/unit/lib/hooks/plugins/auth/hmac_spec.rb +++ b/spec/unit/lib/hooks/plugins/auth/hmac_spec.rb @@ -282,6 +282,13 @@ def create_timestamped_signature(timestamp, version = "v0") long_headers = { default_header => long_signature } expect(valid_with(payload: long_payload, headers: long_headers)).to be true end + + it "returns false and logs for signature containing non-null control characters" do + control_char = "\x01" + headers_with_control = { default_header => signature + control_char } + expect(log).to receive(:warn).with(/control characters/) + expect(valid_with(headers: headers_with_control)).to be false + end end context "format mismatch attacks" do @@ -650,4 +657,43 @@ def test_iso_timestamp(iso_timestamp, should_be_valid) expect(described_class.send(:valid_timestamp?, headers, config)).to be true end end + + describe "debug and warning logging" do + let(:secret) { "test-secret-123" } + let(:config) do + { + auth: { + header: "X-Signature", + secret_env_key: "TEST_SECRET" + } + } + end + + before do + allow(ENV).to receive(:[]).with("TEST_SECRET").and_return(secret) + allow(Hooks::Log.instance).to receive(:debug) + allow(Hooks::Log.instance).to receive(:warn) + end + + it "logs debug message on successful validation" do + payload = '{"data": "test"}' + signature = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), secret, payload) + headers = { "X-Signature" => "sha256=#{signature}" } + + result = described_class.valid?(payload: payload, headers: headers, config: config) + + expect(result).to be true + expect(Hooks::Log.instance).to have_received(:debug).with("Auth::HMAC validation successful for header 'X-Signature'") + end + + it "logs warning message on signature mismatch" do + payload = '{"data": "test"}' + headers = { "X-Signature" => "sha256=wrong_signature" } + + result = described_class.valid?(payload: payload, headers: headers, config: config) + + expect(result).to be false + expect(Hooks::Log.instance).to have_received(:warn).with("Auth::HMAC validation failed: Signature mismatch") + 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 6a895c3..25fc286 100644 --- a/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb +++ b/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb @@ -345,6 +345,65 @@ def valid_with(args = {}) ) expect(result).to be true end + + it "logs debug message on successful validation" do + headers = { "X-API-Key" => api_key } + allow(Hooks::Log.instance).to receive(:debug) + + described_class.valid?( + payload: '{"data":"value"}', + headers:, + config: api_config + ) + + expect(Hooks::Log.instance).to have_received(:debug).with("Auth::SharedSecret validation successful for header 'X-API-Key'") + end + end + + context "Debug and warning logging coverage" do + let(:test_secret) { "test-secret-123" } + let(:test_config) do + { + auth: { + header: "Authorization", + secret_env_key: "TEST_SECRET" + } + } + end + + before do + allow(ENV).to receive(:[]).with("TEST_SECRET").and_return(test_secret) + allow(Hooks::Log.instance).to receive(:debug) + allow(Hooks::Log.instance).to receive(:warn) + allow(Hooks::Log.instance).to receive(:error) + end + + it "logs warning when signature mismatch occurs" do + headers = { "Authorization" => "wrong-secret" } + + result = described_class.valid?( + payload: '{"data":"value"}', + headers:, + config: test_config + ) + + expect(result).to be false + expect(Hooks::Log.instance).to have_received(:warn).with("Auth::SharedSecret validation failed: Signature mismatch") + end + + it "logs error and returns false on exception" do + # Force an exception by mocking fetch_secret to raise + allow(described_class).to receive(:fetch_secret).and_raise(StandardError, "Test error") + + result = described_class.valid?( + payload: '{"data":"value"}', + headers: { "Authorization" => "test" }, + config: test_config + ) + + expect(result).to be false + expect(Hooks::Log.instance).to have_received(:error).with("Auth::SharedSecret validation failed: Test error") + end end end end