Merge pull request #12 from github/copilot/fix-11

GrantBirki · web-flow · commit 35efbda4cb1e · 2025-06-10T23:23:14.000-07:00
Audit and improve security related tests: eliminate code duplication and implement missing tests
diff --git a/lib/hooks/app/auth/auth.rb b/lib/hooks/app/auth/auth.rb
@@ -20,11 +20,12 @@ def validate_auth!(payload, headers, endpoint_config)
         auth_config = endpoint_config[:auth]
 
         # Security: Ensure auth type is present and valid
-        unless auth_config&.dig(:type)&.is_a?(String)
+        auth_type = auth_config&.dig(:type)
+        unless auth_type&.is_a?(String) && !auth_type.strip.empty?
           error!("authentication configuration missing or invalid", 500)
         end
 
-        auth_plugin_type = auth_config[:type].downcase
+        auth_plugin_type = auth_type.downcase
 
         auth_class = nil
 
diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "securerandom"
+require_relative "../security"
 
 module Hooks
   module App
@@ -121,13 +122,7 @@ def valid_handler_class_name?(class_name)
         return false unless class_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/)
 
         # Must not be a system/built-in class name
-        dangerous_classes = %w[
-          File Dir Kernel Object Class Module Proc Method
-          IO Socket TCPSocket UDPSocket BasicSocket
-          Process Thread Fiber Mutex ConditionVariable
-          Marshal YAML JSON Pathname
-        ]
-        return false if dangerous_classes.include?(class_name)
+        return false if Hooks::Security::DANGEROUS_CLASSES.include?(class_name)
 
         true
       end
diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "dry-schema"
+require_relative "../security"
 
 module Hooks
   module Core
@@ -119,13 +120,7 @@ def self.valid_handler_name?(handler_name)
         return false unless handler_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/)
 
         # Must not be a system/built-in class name
-        dangerous_classes = %w[
-          File Dir Kernel Object Class Module Proc Method
-          IO Socket TCPSocket UDPSocket BasicSocket
-          Process Thread Fiber Mutex ConditionVariable
-          Marshal YAML JSON Pathname
-        ]
-        return false if dangerous_classes.include?(handler_name)
+        return false if Hooks::Security::DANGEROUS_CLASSES.include?(handler_name)
 
         true
       end
diff --git a/lib/hooks/security.rb b/lib/hooks/security.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module Hooks
+  module Security
+    # List of dangerous class names that should not be loaded as handlers
+    # for security reasons. These classes provide system access that could
+    # be exploited if loaded dynamically.
+    #
+    # @return [Array<String>] Array of dangerous class names
+    DANGEROUS_CLASSES = %w[
+      File Dir Kernel Object Class Module Proc Method
+      IO Socket TCPSocket UDPSocket BasicSocket
+      Process Thread Fiber Mutex ConditionVariable
+      Marshal YAML JSON Pathname
+    ].freeze
+  end
+end
diff --git a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb b/spec/unit/lib/hooks/app/auth/auth_security_spec.rb
@@ -3,6 +3,7 @@
 require_relative "../../../../spec_helper"
 
 describe Hooks::App::Auth do
+  let(:log) { instance_double(Logger).as_null_object }
   let(:test_class) do
     Class.new do
       include Hooks::App::Auth
@@ -17,6 +18,10 @@ def error!(message, code)
   let(:payload) { '{"test": "data"}' }
   let(:headers) { { "Content-Type" => "application/json" } }
 
+  before(:each) do
+    Hooks::Log.instance = log
+  end
+
   describe "#validate_auth!" do
     context "when testing security vulnerabilities" do
       context "with missing auth configuration" do
@@ -63,39 +68,77 @@ def error!(message, code)
         end
 
         it "rejects request with empty string type" do
-          # TODO
+          endpoint_config = { auth: { type: "" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication configuration missing or invalid/)
         end
       end
 
       context "with missing secret configuration" do
         it "rejects request with missing secret_env_key" do
-          # TODO
+          endpoint_config = { auth: { type: "hmac" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
 
         it "rejects request with nil secret_env_key" do
-          # TODO
+          endpoint_config = { auth: { type: "hmac", secret_env_key: nil } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
 
         it "rejects request with empty secret_env_key" do
-          # TODO
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
 
         it "rejects request with whitespace-only secret_env_key" do
-          # TODO
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "   " } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
 
         it "rejects request with non-string secret_env_key" do
-          # TODO
+          endpoint_config = { auth: { type: "hmac", secret_env_key: 123 } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
       end
 
       context "with missing environment variable" do
         it "uses generic error message for missing secrets" do
-          # TODO
+          ENV.delete("NONEXISTENT_SECRET")
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "NONEXISTENT_SECRET" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error(StandardError, /authentication failed/)
         end
 
         it "does not leak the environment variable name in error" do
-          # TODO
+          ENV.delete("SECRET_WEBHOOK_KEY")
+          endpoint_config = { auth: { type: "hmac", secret_env_key: "SECRET_WEBHOOK_KEY" } }
+
+          expect do
+            instance.validate_auth!(payload, headers, endpoint_config)
+          end.to raise_error do |error|
+            # Ensure error message is generic and doesn't leak the environment variable name
+            expect(error.message).not_to include("SECRET_WEBHOOK_KEY")
+            expect(error.message).to match(/authentication failed/)
+          end
         end
       end
 
diff --git a/spec/unit/lib/hooks/app/helpers_security_spec.rb b/spec/unit/lib/hooks/app/helpers_security_spec.rb
@@ -37,9 +37,7 @@ def env
   describe "#load_handler security" do
     context "with malicious handler class names" do
       it "rejects system class names" do
-        dangerous_classes = %w[File Dir Kernel Object Class Module Proc Method]
-
-        dangerous_classes.each do |class_name|
+        Hooks::Security::DANGEROUS_CLASSES.each do |class_name|
           expect do
             instance.load_handler(class_name, handler_dir)
           end.to raise_error(StandardError, /invalid handler class name/)
@@ -48,6 +46,8 @@ def env
 
       it "rejects network-related class names" do
         network_classes = %w[IO Socket TCPSocket UDPSocket BasicSocket]
+        # Verify these are all in our dangerous classes list
+        network_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) }
 
         network_classes.each do |class_name|
           expect do
@@ -58,6 +58,8 @@ def env
 
       it "rejects process and system class names" do
         system_classes = %w[Process Thread Fiber Mutex ConditionVariable]
+        # Verify these are all in our dangerous classes list
+        system_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) }
 
         system_classes.each do |class_name|
           expect do
@@ -68,6 +70,8 @@ def env
 
       it "rejects serialization class names" do
         serialization_classes = %w[Marshal YAML JSON Pathname]
+        # Verify these are all in our dangerous classes list
+        serialization_classes.each { |cls| expect(Hooks::Security::DANGEROUS_CLASSES).to include(cls) }
 
         serialization_classes.each do |class_name|
           expect 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
@@ -25,24 +25,9 @@
         end
 
         it "rejects dangerous system class names" do
-          dangerous_configs = [
-            { path: "/webhook", handler: "File" },
-            { path: "/webhook", handler: "Dir" },
-            { path: "/webhook", handler: "Kernel" },
-            { path: "/webhook", handler: "Object" },
-            { path: "/webhook", handler: "Class" },
-            { path: "/webhook", handler: "Module" },
-            { path: "/webhook", handler: "Proc" },
-            { path: "/webhook", handler: "Method" },
-            { path: "/webhook", handler: "IO" },
-            { path: "/webhook", handler: "Socket" },
-            { path: "/webhook", handler: "TCPSocket" },
-            { path: "/webhook", handler: "Process" },
-            { path: "/webhook", handler: "Thread" },
-            { path: "/webhook", handler: "Marshal" },
-            { path: "/webhook", handler: "YAML" },
-            { path: "/webhook", handler: "JSON" }
-          ]
+          dangerous_configs = Hooks::Security::DANGEROUS_CLASSES.map do |class_name|
+            { path: "/webhook", handler: class_name }
+          end
 
           dangerous_configs.each do |config|
             expect do
