Add security validations for headers and payloads in auth plugins

GrantBirki · GrantBirki · commit bf47eadd65c8 · 2025-06-16T22:24:53.000-07:00
- Introduced MAX_HEADER_VALUE_LENGTH and MAX_PAYLOAD_SIZE constants for validation.
- Implemented header and payload size validation methods in Base class.
- Updated HMAC and SharedSecret validators to utilize new validation methods.
- Added tests for oversized headers and payloads to ensure compliance with limits.
diff --git a/lib/hooks/plugins/auth/base.rb b/lib/hooks/plugins/auth/base.rb
@@ -14,6 +14,10 @@ module Auth
       class Base
         extend Hooks::Core::ComponentAccess
 
+        # Security constants shared across auth validators
+        MAX_HEADER_VALUE_LENGTH = ENV.fetch("HOOKS_MAX_HEADER_VALUE_LENGTH", 1024).to_i # Prevent DoS attacks via large header values
+        MAX_PAYLOAD_SIZE = ENV.fetch("HOOKS_MAX_PAYLOAD_SIZE", 10 * 1024 * 1024).to_i # 10MB limit for payload validation
+
         # Validate request
         #
         # @param payload [String] Raw request body
@@ -67,6 +71,61 @@ def self.find_header_value(headers, header_name)
           end
           nil
         end
+
+        # Validate headers object for security issues
+        #
+        # @param headers [Object] Headers to validate
+        # @return [Boolean] true if headers are valid
+        def self.valid_headers?(headers)
+          unless headers.respond_to?(:each)
+            log.warn("Auth validation failed: Invalid headers object")
+            return false
+          end
+          true
+        end
+
+        # Validate payload size for security issues
+        #
+        # @param payload [String] Payload to validate
+        # @return [Boolean] true if payload is valid
+        def self.valid_payload_size?(payload)
+          return true if payload.nil?
+
+          if payload.bytesize > MAX_PAYLOAD_SIZE
+            log.warn("Auth validation failed: Payload size exceeds maximum limit of #{MAX_PAYLOAD_SIZE} bytes")
+            return false
+          end
+          true
+        end
+
+        # Validate header value for security issues
+        #
+        # @param header_value [String] Header value to validate
+        # @param header_name [String] Header name for logging
+        # @return [Boolean] true if header value is valid
+        def self.valid_header_value?(header_value, header_name)
+          return false if header_value.nil? || header_value.empty?
+
+          # Check length to prevent DoS
+          if header_value.length > MAX_HEADER_VALUE_LENGTH
+            log.warn("Auth validation failed: #{header_name} exceeds maximum length")
+            return false
+          end
+
+          # Check for whitespace tampering
+          if header_value != header_value.strip
+            log.warn("Auth validation failed: #{header_name} contains leading/trailing whitespace")
+            return false
+          end
+
+          # Check for control characters
+          if header_value.match?(/[\u0000-\u001f\u007f-\u009f]/)
+            log.warn("Auth validation failed: #{header_name} contains control characters")
+            return false
+          end
+
+          true
+        end
       end
     end
   end
diff --git a/lib/hooks/plugins/auth/hmac.rb b/lib/hooks/plugins/auth/hmac.rb
@@ -48,7 +48,6 @@ module Auth
       class HMAC < Base
         # Security constants
         MAX_SIGNATURE_LENGTH = ENV.fetch("HOOKS_MAX_SIGNATURE_LENGTH", 1024).to_i # Prevent DoS attacks via large signatures
-        MAX_PAYLOAD_SIZE = ENV.fetch("MAX_PAYLOAD_SIZE", 10 * 1024 * 1024).to_i # 10MB limit for payload validation
 
         # Default configuration values for HMAC validation
         #
@@ -114,43 +113,22 @@ def self.valid?(payload:, headers:, config:)
 
           validator_config = build_config(config)
 
-          # Security: Check raw headers BEFORE normalization to detect tampering
-          unless headers.respond_to?(:each)
-            log.warn("Auth::HMAC validation failed: Invalid headers object")
-            return false
-          end
+          # Security: Check raw headers and payload BEFORE processing
+          return false unless valid_headers?(headers)
+          return false unless valid_payload_size?(payload)
 
           signature_header = validator_config[:header]
 
           # Find the signature header with case-insensitive matching
           raw_signature = find_header_value(headers, signature_header)
 
-          if payload && payload.bytesize > MAX_PAYLOAD_SIZE
-            log.warn("Auth::HMAC validation failed: Payload size exceeds maximum limit of #{MAX_PAYLOAD_SIZE} bytes")
-            return false
-          end
-
           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
-          if raw_signature != raw_signature.strip
-            log.warn("Auth::HMAC validation failed: Signature contains leading/trailing whitespace")
-            return false
-          end
-
-          if raw_signature.length > MAX_SIGNATURE_LENGTH
-            log.warn("Auth::HMAC validation failed: Signature length exceeds maximum limit of #{MAX_SIGNATURE_LENGTH} characters")
-            return false
-          end
-
-          # Security: Reject signatures containing null bytes or other control characters
-          if raw_signature.match?(/[\u0000-\u001f\u007f-\u009f]/)
-            log.warn("Auth::HMAC validation failed: Signature contains control characters")
-            return false
-          end
+          # Validate signature format using shared validation but with HMAC-specific length limit
+          return false unless validate_signature_format(raw_signature)
 
           # Now we can safely normalize headers for the rest of the validation
           normalized_headers = normalize_headers(headers)
@@ -210,6 +188,22 @@ def self.valid?(payload:, headers:, config:)
 
         private
 
+        # Validate signature format for HMAC (uses HMAC-specific length limit)
+        #
+        # @param signature [String] Raw signature to validate
+        # @return [Boolean] true if signature is valid
+        # @api private
+        def self.validate_signature_format(signature)
+          # Check signature length with HMAC-specific limit
+          if signature.length > MAX_SIGNATURE_LENGTH
+            log.warn("Auth::HMAC validation failed: Signature length exceeds maximum limit of #{MAX_SIGNATURE_LENGTH} characters")
+            return false
+          end
+
+          # Use shared validation for other checks
+          valid_header_value?(signature, "Signature")
+        end
+
         # Build final configuration by merging defaults with provided config
         #
         # Combines default configuration values with user-provided settings,
diff --git a/lib/hooks/plugins/auth/shared_secret.rb b/lib/hooks/plugins/auth/shared_secret.rb
@@ -61,11 +61,9 @@ def self.valid?(payload:, headers:, config:)
 
           validator_config = build_config(config)
 
-          # Security: Check raw headers BEFORE normalization to detect tampering
-          unless headers.respond_to?(:each)
-            log.warn("Auth::SharedSecret validation failed: Invalid headers object")
-            return false
-          end
+          # Security: Check raw headers and payload BEFORE processing
+          return false unless valid_headers?(headers)
+          return false unless valid_payload_size?(payload)
 
           secret_header = validator_config[:header]
 
@@ -77,19 +75,13 @@ def self.valid?(payload:, headers:, config:)
             return false
           end
 
-          stripped_secret = raw_secret.strip
-
-          # Security: Reject secrets with leading/trailing whitespace
-          if raw_secret != stripped_secret
-            log.warn("Auth::SharedSecret validation failed: Secret contains leading/trailing whitespace")
+          # Validate secret format using shared validation
+          unless valid_header_value?(raw_secret, "Secret")
+            log.warn("Auth::SharedSecret validation failed: Invalid secret format")
             return false
           end
 
-          # Security: Reject secrets containing null bytes or other control characters
-          if raw_secret.match?(/[\u0000-\u001f\u007f-\u009f]/)
-            log.warn("Auth::SharedSecret validation failed: Secret contains control characters")
-            return false
-          end
+          stripped_secret = raw_secret.strip
 
           # Use secure comparison to prevent timing attacks
           result = Rack::Utils.secure_compare(secret, stripped_secret)
@@ -106,12 +98,6 @@ def self.valid?(payload:, headers:, config:)
 
         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/shared_secret_spec.rb b/spec/unit/lib/hooks/plugins/auth/shared_secret_spec.rb
@@ -26,6 +26,7 @@ def valid_with(args = {})
   end
 
   before do
+    allow(ENV).to receive(:[]).and_call_original
     allow(ENV).to receive(:[]).with("SUPER_WEBHOOK_SECRET").and_return(secret)
   end
 
@@ -99,6 +100,22 @@ def valid_with(args = {})
         nil_headers = { default_header => nil }
         expect(valid_with(headers: nil_headers)).to be false
       end
+
+      it "returns false for secrets exceeding maximum length limit" do
+        # Create secret larger than MAX_HEADER_VALUE_LENGTH (1024 + 1 characters)
+        oversized_secret = "a" * (1024 + 1)
+        oversized_headers = { default_header => oversized_secret }
+        expect(log).to receive(:warn).with(/exceeds maximum length/)
+        expect(valid_with(headers: oversized_headers)).to be false
+      end
+
+      it "returns false for payloads exceeding maximum size limit" do
+        # Create payload larger than MAX_PAYLOAD_SIZE (10MB + 1 byte)
+        oversized_payload = "a" * (10 * 1024 * 1024 + 1)
+        headers = { default_header => secret }
+        expect(log).to receive(:warn).with(/Payload size exceeds maximum limit/)
+        expect(valid_with(payload: oversized_payload, headers: headers)).to be false
+      end
     end
 
     context "with custom header configuration" do
