Update ConfigValidator and PluginLoader for snake_case handlers

Copilot · GrantBirki · Copilot · commit 25588574db35 · 2025-06-17T06:16:59.000Z
Co-authored-by: GrantBirki &lt;23362539+GrantBirki@users.noreply.github.com&gt;
diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb
@@ -74,7 +74,7 @@ def parse_payload(raw_body, headers, symbolize: false)
 
       # Load handler class
       #
-      # @param handler_class_name [String] The name of the handler class to load
+      # @param handler_class_name [String] The name of the handler in snake_case (e.g., "github_handler")
       # @return [Object] An instance of the loaded handler class
       # @raise [StandardError] If handler cannot be found
       def load_handler(handler_class_name)
diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb
@@ -125,11 +125,12 @@ def self.valid_handler_name?(handler_name)
         # Must not be empty or only whitespace
         return false if handler_name.strip.empty?
 
-        # Must match a safe pattern: alphanumeric + underscore, starting with uppercase
-        return false unless handler_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/)
+        # Must match a safe pattern: alphanumeric + underscore, starting with lowercase
+        return false unless handler_name.match?(/\A[a-z][a-z0-9_]*\z/)
 
-        # Must not be a system/built-in class name
-        return false if Hooks::Security::DANGEROUS_CLASSES.include?(handler_name)
+        # Convert to PascalCase for security check (since DANGEROUS_CLASSES uses PascalCase)
+        pascal_case_name = handler_name.split("_").map(&:capitalize).join("")
+        return false if Hooks::Security::DANGEROUS_CLASSES.include?(pascal_case_name)
 
         true
       end
diff --git a/lib/hooks/core/plugin_loader.rb b/lib/hooks/core/plugin_loader.rb
@@ -61,11 +61,13 @@ def get_auth_plugin(plugin_name)
 
         # Get handler plugin class by name
         #
-        # @param handler_name [String] Name of the handler (e.g., "DefaultHandler", "Team1Handler")
+        # @param handler_name [String] Name of the handler in snake_case (e.g., "github_handler", "team_1_handler")
         # @return [Class] The handler plugin class
         # @raise [StandardError] if handler not found
         def get_handler_plugin(handler_name)
-          plugin_class = @handler_plugins[handler_name]
+          # Convert snake_case to PascalCase for registry lookup
+          pascal_case_name = handler_name.split("_").map(&:capitalize).join("")
+          plugin_class = @handler_plugins[pascal_case_name]
 
           unless plugin_class
             raise StandardError, "Handler plugin '#{handler_name}' not found. Available handlers: #{@handler_plugins.keys.join(', ')}"
diff --git a/spec/integration/global_lifecycle_hooks_spec.rb b/spec/integration/global_lifecycle_hooks_spec.rb
@@ -103,7 +103,7 @@ def call(payload:, headers:, env:, config:)
     # Create an endpoint configuration
     endpoint_config_content = <<~YAML
       path: /integration-test
-      handler: IntegrationTestHandler
+      handler: integration_test_handler
     YAML
     File.write(File.join(temp_endpoints_dir, "integration_test.yml"), endpoint_config_content)
   end
diff --git a/spec/integration/hooks_integration_spec.rb b/spec/integration/hooks_integration_spec.rb
@@ -33,7 +33,7 @@ def app
     FileUtils.mkdir_p("./spec/integration/tmp/endpoints")
     File.write("./spec/integration/tmp/endpoints/test.yaml", {
       path: "/test",
-      handler: "TestHandler",
+      handler: "test_handler",
       opts: { test_mode: true }
     }.to_yaml)
 
diff --git a/spec/unit/app/rack_env_builder_spec.rb b/spec/unit/app/rack_env_builder_spec.rb
@@ -16,7 +16,7 @@
   let(:request_context) do
     {
       request_id: "req-123",
-      handler: "TestHandler"
+      handler: "test_handler"
     }
   end
   let(:endpoint_config) do
@@ -86,7 +86,7 @@
 
     it "includes Hooks-specific environment variables" do
       expect(result["hooks.request_id"]).to eq("req-123")
-      expect(result["hooks.handler"]).to eq("TestHandler")
+      expect(result["hooks.handler"]).to eq("test_handler")
       expect(result["hooks.endpoint_config"]).to eq(endpoint_config)
       expect(result["hooks.start_time"]).to eq("2025-06-16T10:30:45Z")
       expect(result["hooks.full_path"]).to eq("/api/v1/test")
@@ -280,7 +280,7 @@
         described_class.new(
           mock_request,
           { "Accept" => "application/vnd.api+json" },
-          { request_id: "mock-123", handler: "MockHandler" },
+          { request_id: "mock-123", handler: "mock_handler" },
           { path: "/resource", method: "patch" },
           Time.parse("2025-06-16T15:45:30Z"),
           "/api/v2/resource"
diff --git a/spec/unit/lib/hooks/app/helpers_security_spec.rb b/spec/unit/lib/hooks/app/helpers_security_spec.rb
@@ -59,7 +59,7 @@ def env
       end
 
       it "successfully loads DefaultHandler" do
-        handler = instance.load_handler("DefaultHandler")
+        handler = instance.load_handler("default_handler")
         expect(handler).to be_an_instance_of(DefaultHandler)
       end
     end
diff --git a/spec/unit/lib/hooks/app/helpers_spec.rb b/spec/unit/lib/hooks/app/helpers_spec.rb
@@ -307,7 +307,7 @@ def error!(message, code)
       end
 
       it "successfully loads DefaultHandler" do
-        handler = helper.load_handler("DefaultHandler")
+        handler = helper.load_handler("default_handler")
         expect(handler).to be_an_instance_of(DefaultHandler)
       end
     end
diff --git a/spec/unit/lib/hooks/core/config_loader_spec.rb b/spec/unit/lib/hooks/core/config_loader_spec.rb
@@ -281,14 +281,14 @@
       let(:endpoint1_config) do
         {
           "path" => "/webhook/test1",
-          "handler" => "TestHandler1",
+          "handler" => "test_handler_1",
           "method" => "POST"
         }
       end
       let(:endpoint2_config) do
         {
           "path" => "/webhook/test2",
-          "handler" => "TestHandler2",
+          "handler" => "test_handler_2",
           "method" => "PUT"
         }
       end
@@ -304,12 +304,12 @@
         expect(endpoints).to have_attributes(size: 2)
         expect(endpoints).to include(
           path: "/webhook/test1",
-          handler: "TestHandler1",
+          handler: "test_handler_1",
           method: "POST"
         )
         expect(endpoints).to include(
           path: "/webhook/test2",
-          handler: "TestHandler2",
+          handler: "test_handler_2",
           method: "PUT"
         )
       end
@@ -320,7 +320,7 @@
       let(:endpoint_config) do
         {
           "path" => "/webhook/json",
-          "handler" => "JsonHandler",
+          "handler" => "json_handler",
           "method" => "POST"
         }
       end
@@ -335,7 +335,7 @@
         expect(endpoints).to have_attributes(size: 1)
         expect(endpoints.first).to eq(
           path: "/webhook/json",
-          handler: "JsonHandler",
+          handler: "json_handler",
           method: "POST"
         )
       end
@@ -348,7 +348,7 @@
       let(:valid_config) do
         {
           "path" => "/webhook/valid",
-          "handler" => "ValidHandler"
+          "handler" => "valid_handler"
         }
       end
 
@@ -364,7 +364,7 @@
         expect(endpoints).to have_attributes(size: 1)
         expect(endpoints.first).to eq(
           path: "/webhook/valid",
-          handler: "ValidHandler"
+          handler: "valid_handler"
         )
       end
       it "allows environment variable setup" do
diff --git a/spec/unit/lib/hooks/core/config_validator_security_spec.rb b/spec/unit/lib/hooks/core/config_validator_security_spec.rb
@@ -8,13 +8,13 @@
       context "with secure handler names" do
         it "accepts valid handler names" do
           valid_configs = [
-            { path: "/webhook", handler: "MyHandler" },
-            { path: "/webhook", handler: "GitHubHandler" },
-            { path: "/webhook", handler: "Team1Handler" },
-            { path: "/webhook", handler: "WebhookHandler" },
-            { path: "/webhook", handler: "CustomWebhookHandler" },
-            { path: "/webhook", handler: "Handler123" },
-            { path: "/webhook", handler: "My_Handler" }
+            { path: "/webhook", handler: "my_handler" },
+            { path: "/webhook", handler: "github_handler" },
+            { path: "/webhook", handler: "team_1_handler" },
+            { path: "/webhook", handler: "webhook_handler" },
+            { path: "/webhook", handler: "custom_webhook_handler" },
+            { path: "/webhook", handler: "handler_123" },
+            { path: "/webhook", handler: "my_handler" }
           ]
 
           valid_configs.each do |config|
@@ -26,7 +26,9 @@
 
         it "rejects dangerous system class names" do
           dangerous_configs = Hooks::Security::DANGEROUS_CLASSES.map do |class_name|
-            { path: "/webhook", handler: class_name }
+            # Convert PascalCase to snake_case for config
+            snake_case_name = class_name.gsub(/([A-Z])/, '_\1').downcase.sub(/^_/, '')
+            { path: "/webhook", handler: snake_case_name }
           end
 
           dangerous_configs.each do |config|
@@ -38,14 +40,16 @@
 
         it "rejects handler names with invalid format" do
           invalid_configs = [
-            { path: "/webhook", handler: "handler" },           # lowercase start
-            { path: "/webhook", handler: "123Handler" },        # number start
-            { path: "/webhook", handler: "_Handler" },          # underscore start
-            { path: "/webhook", handler: "Handler$Test" },      # special characters
-            { path: "/webhook", handler: "Handler.Test" },      # dots
-            { path: "/webhook", handler: "Handler/Test" },      # slashes
-            { path: "/webhook", handler: "Handler Test" },      # spaces
-            { path: "/webhook", handler: "Handler\nTest" }      # newlines
+            { path: "/webhook", handler: "Handler" },           # uppercase start
+            { path: "/webhook", handler: "123handler" },        # number start
+            { path: "/webhook", handler: "_handler" },          # underscore start
+            { path: "/webhook", handler: "handler$test" },      # special characters
+            { path: "/webhook", handler: "handler.test" },      # dots
+            { path: "/webhook", handler: "handler/test" },      # slashes
+            { path: "/webhook", handler: "handler test" },      # spaces
+            { path: "/webhook", handler: "handler\ntest" },     # newlines
+            { path: "/webhook", handler: "handlerTest" },       # camelCase
+            { path: "/webhook", handler: "HandlerTest" }        # PascalCase
           ]
 
           invalid_configs.each do |config|
@@ -88,9 +92,9 @@
       context "with endpoint arrays" do
         it "validates all endpoints in an array and reports the problematic one" do
           endpoints = [
-            { path: "/webhook1", handler: "ValidHandler" },
-            { path: "/webhook2", handler: "File" },  # This should fail
-            { path: "/webhook3", handler: "AnotherValidHandler" }
+            { path: "/webhook1", handler: "valid_handler" },
+            { path: "/webhook2", handler: "File" },  # This should fail (PascalCase)
+            { path: "/webhook3", handler: "another_valid_handler" }
           ]
 
           expect do
diff --git a/spec/unit/lib/hooks/core/config_validator_spec.rb b/spec/unit/lib/hooks/core/config_validator_spec.rb
diff --git a/spec/unit/lib/hooks/handlers/base_spec.rb b/spec/unit/lib/hooks/handlers/base_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ def parse_payload(raw_body, headers, symbolize: false)`
`74`	`74`
`75`	`75`	`# Load handler class`
`76`	`76`	`#`
`77`		`- # @param handler_class_name [String] The name of the handler class to load`
	`77`	`+ # @param handler_class_name [String] The name of the handler in snake_case (e.g., "github_handler")`
`78`	`78`	`# @return [Object] An instance of the loaded handler class`
`79`	`79`	`# @raise [StandardError] If handler cannot be found`
`80`	`80`	`def load_handler(handler_class_name)`
Original file line number	Diff line number	Diff line change
`@@ -281,14 +281,14 @@`
`281`	`281`	`let(:endpoint1_config) do`
`282`	`282`	`{`
`283`	`283`	`"path" => "/webhook/test1",`
`284`		`- "handler" => "TestHandler1",`
	`284`	`+ "handler" => "test_handler_1",`
`285`	`285`	`"method" => "POST"`
`286`	`286`	`}`
`287`	`287`	`end`
`288`	`288`	`let(:endpoint2_config) do`
`289`	`289`	`{`
`290`	`290`	`"path" => "/webhook/test2",`
`291`		`- "handler" => "TestHandler2",`
	`291`	`+ "handler" => "test_handler_2",`
`292`	`292`	`"method" => "PUT"`
`293`	`293`	`}`
`294`	`294`	`end`
`@@ -304,12 +304,12 @@`
`304`	`304`	`expect(endpoints).to have_attributes(size: 2)`
`305`	`305`	`expect(endpoints).to include(`
`306`	`306`	`path: "/webhook/test1",`
`307`		`- handler: "TestHandler1",`
	`307`	`+ handler: "test_handler_1",`
`308`	`308`	`method: "POST"`
`309`	`309`	`)`
`310`	`310`	`expect(endpoints).to include(`
`311`	`311`	`path: "/webhook/test2",`
`312`		`- handler: "TestHandler2",`
	`312`	`+ handler: "test_handler_2",`
`313`	`313`	`method: "PUT"`
`314`	`314`	`)`
`315`	`315`	`end`
`@@ -320,7 +320,7 @@`
`320`	`320`	`let(:endpoint_config) do`
`321`	`321`	`{`
`322`	`322`	`"path" => "/webhook/json",`
`323`		`- "handler" => "JsonHandler",`
	`323`	`+ "handler" => "json_handler",`
`324`	`324`	`"method" => "POST"`
`325`	`325`	`}`
`326`	`326`	`end`
`@@ -335,7 +335,7 @@`
`335`	`335`	`expect(endpoints).to have_attributes(size: 1)`
`336`	`336`	`expect(endpoints.first).to eq(`
`337`	`337`	`path: "/webhook/json",`
`338`		`- handler: "JsonHandler",`
	`338`	`+ handler: "json_handler",`
`339`	`339`	`method: "POST"`
`340`	`340`	`)`
`341`	`341`	`end`
`@@ -348,7 +348,7 @@`
`348`	`348`	`let(:valid_config) do`
`349`	`349`	`{`
`350`	`350`	`"path" => "/webhook/valid",`
`351`		`- "handler" => "ValidHandler"`
	`351`	`+ "handler" => "valid_handler"`
`352`	`352`	`}`
`353`	`353`	`end`
`354`	`354`
`@@ -364,7 +364,7 @@`
`364`	`364`	`expect(endpoints).to have_attributes(size: 1)`
`365`	`365`	`expect(endpoints.first).to eq(`
`366`	`366`	`path: "/webhook/valid",`
`367`		`- handler: "ValidHandler"`
	`367`	`+ handler: "valid_handler"`
`368`	`368`	`)`
`369`	`369`	`end`
`370`	`370`	`it "allows environment variable setup" do`