Merge pull request #22 from github/copilot/fix-21

Production readiness and performance audit: Security and optimization improvements

Author: GrantBirki

lib/hooks.rb

@@ -2,21 +2,28 @@
 
 require_relative "hooks/version"
 require_relative "hooks/core/builder"
-
-# Load all core components
-Dir[File.join(__dir__, "hooks/core/**/*.rb")].sort.each do |file|
-  require file
-end
-
-# Load all plugins (auth plugins, handler plugins, lifecycle hooks, etc.)
-Dir[File.join(__dir__, "hooks/plugins/**/*.rb")].sort.each do |file|
-  require file
-end
-
-# Load all utils
-Dir[File.join(__dir__, "hooks/utils/**/*.rb")].sort.each do |file|
-  require file
-end
+require_relative "hooks/core/config_loader"
+require_relative "hooks/core/config_validator"
+require_relative "hooks/core/logger_factory"
+require_relative "hooks/core/plugin_loader"
+require_relative "hooks/core/global_components"
+require_relative "hooks/core/log"
+require_relative "hooks/core/failbot"
+require_relative "hooks/core/stats"
+require_relative "hooks/plugins/auth/base"
+require_relative "hooks/plugins/auth/hmac"
+require_relative "hooks/plugins/auth/shared_secret"
+require_relative "hooks/plugins/handlers/base"
+require_relative "hooks/plugins/handlers/default"
+require_relative "hooks/plugins/lifecycle"
+require_relative "hooks/plugins/instruments/stats_base"
+require_relative "hooks/plugins/instruments/failbot_base"
+require_relative "hooks/plugins/instruments/stats"
+require_relative "hooks/plugins/instruments/failbot"
+require_relative "hooks/utils/normalize"
+require_relative "hooks/utils/retry"
+require_relative "hooks/security"
+require_relative "hooks/version"
 
 # Main module for the Hooks webhook server framework
 module Hooks

lib/hooks/app/helpers.rb

@@ -21,13 +21,15 @@ def uuid
       # @return [void]
       # @note Timeout enforcement should be handled at the server level (e.g., Puma)
       def enforce_request_limits(config)
-        # Check content length (handle different header formats and sources)
-        content_length = headers["Content-Length"] || headers["CONTENT_LENGTH"] ||
-                        headers["content-length"] || headers["HTTP_CONTENT_LENGTH"] ||
-                        env["CONTENT_LENGTH"] || env["HTTP_CONTENT_LENGTH"]
+        # Optimized content length check - check most common sources first
+        content_length = request.content_length if respond_to?(:request) && request.respond_to?(:content_length)
 
-        # Also try to get from request object directly
-        content_length ||= request.content_length if respond_to?(:request) && request.respond_to?(:content_length)
+        content_length ||= headers["Content-Length"] ||
+                          headers["CONTENT_LENGTH"] ||
+                          headers["content-length"] ||
+                          headers["HTTP_CONTENT_LENGTH"] ||
+                          env["CONTENT_LENGTH"] ||
+                          env["HTTP_CONTENT_LENGTH"]
 
         content_length = content_length&.to_i
 
@@ -45,16 +47,21 @@ def enforce_request_limits(config)
       # @param symbolize [Boolean] Whether to symbolize keys in parsed JSON (default: true)
       # @return [Hash, String] Parsed JSON as Hash (optionally symbolized), or raw body if not JSON
       def parse_payload(raw_body, headers, symbolize: true)
+        # Optimized content type check - check most common header first
         content_type = headers["Content-Type"] || headers["CONTENT_TYPE"] || headers["content-type"] || headers["HTTP_CONTENT_TYPE"]
 
         # Try to parse as JSON if content type suggests it or if it looks like JSON
         if content_type&.include?("application/json") || (raw_body.strip.start_with?("{", "[") rescue false)
           begin
-            parsed_payload = JSON.parse(raw_body)
+            # Security: Limit JSON parsing depth and complexity to prevent JSON bombs
+            parsed_payload = safe_json_parse(raw_body)
             parsed_payload = parsed_payload.transform_keys(&:to_sym) if symbolize && parsed_payload.is_a?(Hash)
             return parsed_payload
-          rescue JSON::ParserError
-            # If JSON parsing fails, return raw body
+          rescue JSON::ParserError, ArgumentError => e
+            # If JSON parsing fails or security limits exceeded, return raw body
+            if e.message.include?("nesting") || e.message.include?("depth")
+              log.warn("JSON parsing limit exceeded: #{e.message}")
+            end
           end
         end
 
@@ -79,6 +86,29 @@ def load_handler(handler_class_name)
 
       private
 
+      # Safely parse JSON
+      #
+      # @param json_string [String] The JSON string to parse
+      # @return [Hash, Array] Parsed JSON object
+      # @raise [JSON::ParserError] If JSON is invalid
+      # @raise [ArgumentError] If security limits are exceeded
+      def safe_json_parse(json_string)
+        # Security limits for JSON parsing
+        max_nesting = ENV.fetch("JSON_MAX_NESTING", "20").to_i
+
+        # Additional size check before parsing
+        if json_string.length > ENV.fetch("JSON_MAX_SIZE", "10485760").to_i # 10MB default
+          raise ArgumentError, "JSON payload too large for parsing"
+        end
+
+        JSON.parse(json_string, {
+          max_nesting: max_nesting,
+          create_additions: false,  # Security: Disable object creation from JSON
+          object_class: Hash,       # Use plain Hash instead of custom classes
+          array_class: Array        # Use plain Array instead of custom classes
+        })
+      end
+
       # Determine HTTP error code from exception
       #
       # @param exception [Exception] The exception to map to an HTTP status code

spec/unit/lib/hooks/app/helpers_spec.rb

@@ -204,6 +204,49 @@ def error!(message, code)
       end
     end
 
+    context "with JSON security limits" do
+      it "handles deeply nested JSON within limits" do
+        headers = { "Content-Type" => "application/json" }
+        # Create a nested JSON structure within reasonable limits
+        nested_json = '{"level1": {"level2": {"level3": {"value": "test"}}}}'
+
+        result = helper.parse_payload(nested_json, headers)
+
+        expect(result).to eq({ level1: { "level2" => { "level3" => { "value" => "test" } } } })
+      end
+
+      it "returns raw body when JSON exceeds size limits" do
+        headers = { "Content-Type" => "application/json" }
+
+        # Mock the safe_json_parse method to test the size limit behavior
+        allow(helper).to receive(:safe_json_parse).and_raise(ArgumentError, "JSON payload too large for parsing")
+
+        # Create a JSON string
+        json_data = '{"data": "test"}'
+
+        result = helper.parse_payload(json_data, headers)
+
+        # Should return raw body when size limit exceeded
+        expect(result).to eq(json_data)
+      end
+
+      it "logs debug message when JSON security limits are exceeded" do
+        headers = { "Content-Type" => "application/json" }
+
+        # Mock logger to capture debug messages
+        logger = instance_double("Logger")
+        allow(helper).to receive(:log).and_return(logger)
+        expect(logger).to receive(:warn).with(/JSON parsing limit exceeded/)
+
+        # Mock the safe_json_parse method to simulate nesting limit exceeded
+        allow(helper).to receive(:safe_json_parse).and_raise(ArgumentError, "nesting exceeded")
+
+        json_data = '{"data": "test"}'
+        result = helper.parse_payload(json_data, headers)
+        expect(result).to eq(json_data)
+      end
+    end
+
     context "with non-JSON content" do
       it "returns raw body for plain text" do
         headers = { "Content-Type" => "text/plain" }