Merge pull request #46 from github/copilot/fix-45

Security audit: Fix information disclosure, enhance logging, and optimize auth plugins

Author: GrantBirki

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

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

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,

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

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

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