diff --git a/lib/hooks/app/api.rb b/lib/hooks/app/api.rb index 39b8936d..6570b70f 100644 --- a/lib/hooks/app/api.rb +++ b/lib/hooks/app/api.rb @@ -74,6 +74,8 @@ def validate_request(payload, headers, endpoint_config) case validator_type when "hmac" validator_class = Plugins::RequestValidator::HMAC + when "shared_secret" + validator_class = Plugins::RequestValidator::SharedSecret else error!("Custom validators not implemented in POC", 500) end diff --git a/lib/hooks/plugins/request_validator/base.rb b/lib/hooks/plugins/request_validator/base.rb index e3df7341..46df3209 100644 --- a/lib/hooks/plugins/request_validator/base.rb +++ b/lib/hooks/plugins/request_validator/base.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "rack/utils" + module Hooks module Plugins module RequestValidator diff --git a/lib/hooks/plugins/request_validator/hmac.rb b/lib/hooks/plugins/request_validator/hmac.rb index c5ca2ee2..2e0db37a 100644 --- a/lib/hooks/plugins/request_validator/hmac.rb +++ b/lib/hooks/plugins/request_validator/hmac.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "openssl" -require "rack/utils" require "time" require_relative "base" diff --git a/lib/hooks/plugins/request_validator/shared_secret.rb b/lib/hooks/plugins/request_validator/shared_secret.rb new file mode 100644 index 00000000..ee2bb699 --- /dev/null +++ b/lib/hooks/plugins/request_validator/shared_secret.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require_relative "base" + +module Hooks + module Plugins + module RequestValidator + # Generic shared secret validator for webhooks + # + # This validator provides simple shared secret authentication for webhook requests. + # It compares a secret value sent in a configurable HTTP header against the expected + # secret value. This is a common (though less secure than HMAC) authentication pattern + # used by various webhook providers. + # + # @example Basic configuration + # request_validator: + # type: shared_secret + # secret_env_key: WEBHOOK_SECRET + # header: Authorization + # + # @example Custom header configuration + # request_validator: + # type: shared_secret + # secret_env_key: SOME_OTHER_WEBHOOK_SECRET + # header: X-API-Key + # + # @note This validator performs direct string comparison of the shared secret. + # While simpler than HMAC, it provides less security since the secret is + # transmitted directly in the request header. + class SharedSecret < Base + # Default configuration values for shared secret validation + # + # @return [Hash] Default configuration settings + DEFAULT_CONFIG = { + header: "Authorization" + }.freeze + + # Validate shared secret from webhook requests + # + # Performs secure comparison of the shared secret value from the configured + # header against the expected secret. Uses secure comparison to prevent + # timing attacks. + # + # @param payload [String] Raw request body (unused but required by interface) + # @param headers [Hash] HTTP headers from the request + # @param secret [String] Expected secret value for comparison + # @param config [Hash] Endpoint configuration containing validator settings + # @option config [Hash] :request_validator Validator-specific configuration + # @option config [String] :header ('Authorization') Header containing the secret + # @return [Boolean] true if secret is valid, false otherwise + # @raise [StandardError] Rescued internally, returns false on any error + # @note This method is designed to be safe and will never raise exceptions + # @note Uses Rack::Utils.secure_compare to prevent timing attacks + # @example Basic validation + # SharedSecret.valid?( + # payload: request_body, + # headers: request.headers, + # secret: ENV['WEBHOOK_SECRET'], + # config: { request_validator: { header: 'Authorization' } } + # ) + def self.valid?(payload:, headers:, secret:, config:) + return false if secret.nil? || secret.empty? + + validator_config = build_config(config) + + # Security: Check raw headers BEFORE normalization to detect tampering + return false unless headers.respond_to?(:each) + + 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 + + return false if raw_secret.nil? || raw_secret.empty? + + stripped_secret = raw_secret.strip + + # Security: Reject secrets with leading/trailing whitespace + return false if raw_secret != stripped_secret + + # Security: Reject secrets containing null bytes or other control characters + return false if raw_secret.match?(/[\u0000-\u001f\u007f-\u009f]/) + + # Use secure comparison to prevent timing attacks + Rack::Utils.secure_compare(secret, stripped_secret) + rescue StandardError => _e + # Log error in production - for now just return false + false + end + + private + + # Build final configuration by merging defaults with provided config + # + # Combines default configuration values with user-provided settings, + # ensuring all required configuration keys are present with sensible defaults. + # + # @param config [Hash] Raw endpoint configuration + # @return [Hash] Merged configuration with defaults applied + # @note Missing configuration values are filled with DEFAULT_CONFIG values + # @api private + def self.build_config(config) + validator_config = config.dig(:request_validator) || {} + + DEFAULT_CONFIG.merge({ + header: validator_config[:header] || DEFAULT_CONFIG[:header] + }) + end + end + end + end +end diff --git a/spec/acceptance/acceptance_tests.rb b/spec/acceptance/acceptance_tests.rb index 8e7bde4b..c2c3f185 100644 --- a/spec/acceptance/acceptance_tests.rb +++ b/spec/acceptance/acceptance_tests.rb @@ -2,6 +2,7 @@ FAKE_HMAC_SECRET = "octoawesome-secret" FAKE_ALT_HMAC_SECRET = "octoawesome-2-secret" +FAKE_SHARED_SECRET = "octoawesome-shared-secret" require "rspec" require "net/http" @@ -126,5 +127,26 @@ expect(response.body).to include("request validation failed") end end + + describe "okta" do + it "receives a POST request but contains an invalid shared secret" do + payload = { event: "user.login", user: { id: "12345" } } + headers = { "Content-Type" => "application/json", "Authorization" => "badvalue" } + response = http.post("/webhooks/okta", payload.to_json, headers) + + expect(response).to be_a(Net::HTTPUnauthorized) + expect(response.body).to include("request validation failed") + end + + it "successfully processes a valid POST request with shared secret" do + payload = { event: "user.login", user: { id: "12345" } } + headers = { "Content-Type" => "application/json", "Authorization" => FAKE_SHARED_SECRET } + response = http.post("/webhooks/okta", payload.to_json, headers) + + expect(response).to be_a(Net::HTTPSuccess) + body = JSON.parse(response.body) + expect(body["status"]).to eq("success") + end + end end end diff --git a/spec/acceptance/config/endpoints/okta.yaml b/spec/acceptance/config/endpoints/okta.yaml new file mode 100644 index 00000000..5252d1d8 --- /dev/null +++ b/spec/acceptance/config/endpoints/okta.yaml @@ -0,0 +1,7 @@ +path: /okta +handler: OktaHandler + +request_validator: + type: shared_secret + secret_env_key: SHARED_SECRET # the name of the environment variable containing the shared secret + header: Authorization diff --git a/spec/acceptance/docker-compose.yml b/spec/acceptance/docker-compose.yml index 74da9e09..ebe85eed 100644 --- a/spec/acceptance/docker-compose.yml +++ b/spec/acceptance/docker-compose.yml @@ -10,6 +10,7 @@ services: LOG_LEVEL: DEBUG GITHUB_WEBHOOK_SECRET: "octoawesome-secret" ALT_WEBHOOK_SECRET: "octoawesome-too-secret" + SHARED_SECRET: "octoawesome-shared-secret" command: ["script/server"] healthcheck: test: ["CMD", "curl", "-f", "http://0.0.0.0:8080/health"] diff --git a/spec/acceptance/handlers/okta_handler.rb b/spec/acceptance/handlers/okta_handler.rb new file mode 100644 index 00000000..014f0260 --- /dev/null +++ b/spec/acceptance/handlers/okta_handler.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class OktaHandler < Hooks::Handlers::Base + def call(payload:, headers:, config:) + return { + status: "success" + } + end +end diff --git a/spec/unit/lib/hooks/plugins/request_validator/shared_secret_spec.rb b/spec/unit/lib/hooks/plugins/request_validator/shared_secret_spec.rb new file mode 100644 index 00000000..81a05eba --- /dev/null +++ b/spec/unit/lib/hooks/plugins/request_validator/shared_secret_spec.rb @@ -0,0 +1,326 @@ +# frozen_string_literal: true + +require_relative "../../../../spec_helper" + +describe Hooks::Plugins::RequestValidator::SharedSecret do + let(:secret) { "my-super-secret-key-12345" } + let(:payload) { '{"foo":"bar"}' } + let(:default_header) { "Authorization" } + let(:default_config) do + { + request_validator: { + header: default_header + } + } + end + + def valid_with(args = {}) + args = { config: default_config }.merge(args) + described_class.valid?(payload:, secret:, **args) + end + + describe ".valid?" do + context "with default Authorization header" do + let(:headers) { { default_header => secret } } + + it "returns true for a valid shared secret" do + expect(valid_with(headers:)).to be true + end + + it "returns false for an invalid shared secret" do + bad_headers = { default_header => "wrong-secret" } + expect(valid_with(headers: bad_headers)).to be false + end + + it "returns false if secret header is missing" do + expect(valid_with(headers: {})).to be false + end + + it "returns false if secret is nil or empty" do + expect(valid_with(headers:, secret: nil)).to be false + expect(valid_with(headers:, secret: "")).to be false + end + + it "normalizes header names to lowercase" do + upcase_headers = { default_header.upcase => secret } + expect(valid_with(headers: upcase_headers)).to be true + + downcase_headers = { default_header.downcase => secret } + expect(valid_with(headers: downcase_headers)).to be true + + mixed_case_headers = { "aUtHoRiZaTiOn" => secret } + expect(valid_with(headers: mixed_case_headers)).to be true + end + + it "rejects secrets with leading/trailing whitespace for security" do + # Security check: reject raw values with whitespace to prevent certain attacks + padded_secret = " #{secret} " + padded_headers = { default_header => padded_secret } + + # The validator should reject this because the raw value has whitespace + expect(valid_with(headers: padded_headers)).to be false + + # Also test various whitespace patterns + expect(valid_with(headers: { default_header => " #{secret}" })).to be false + expect(valid_with(headers: { default_header => "#{secret} " })).to be false + expect(valid_with(headers: { default_header => "\t#{secret}\t" })).to be false + end + + it "rejects secrets with control characters" do + bad_headers = { default_header => "secret\x00with\x01null" } + expect(valid_with(headers: bad_headers)).to be false + + bad_headers = { default_header => "secret\nwith\nnewline" } + expect(valid_with(headers: bad_headers)).to be false + + bad_headers = { default_header => "secret\twith\ttab" } + expect(valid_with(headers: bad_headers)).to be false + end + + it "handles empty header values" do + empty_headers = { default_header => "" } + expect(valid_with(headers: empty_headers)).to be false + + nil_headers = { default_header => nil } + expect(valid_with(headers: nil_headers)).to be false + end + end + + context "with custom header configuration" do + let(:custom_header) { "X-API-Key" } + let(:custom_config) do + { + request_validator: { + header: custom_header + } + } + end + let(:headers) { { custom_header => secret } } + + it "returns true for valid secret in custom header" do + expect(valid_with(headers:, config: custom_config)).to be true + end + + it "returns false when secret is in wrong header" do + wrong_headers = { "Authorization" => secret } + expect(valid_with(headers: wrong_headers, config: custom_config)).to be false + end + + it "supports case-insensitive custom header matching" do + upcase_headers = { custom_header.upcase => secret } + expect(valid_with(headers: upcase_headers, config: custom_config)).to be true + + downcase_headers = { custom_header.downcase => secret } + expect(valid_with(headers: downcase_headers, config: custom_config)).to be true + end + end + + context "with no configuration" do + let(:no_config) { {} } + let(:headers) { { "Authorization" => secret } } + + it "uses default Authorization header when no config provided" do + expect(valid_with(headers:, config: no_config)).to be true + end + + it "returns false when secret is in non-default header and no config" do + custom_headers = { "X-API-Key" => secret } + expect(valid_with(headers: custom_headers, config: no_config)).to be false + end + end + + context "with invalid configurations" do + it "handles nil config gracefully" do + headers = { "Authorization" => secret } + expect(valid_with(headers:, config: nil)).to be false + end + + it "handles config without request_validator section" do + headers = { "Authorization" => secret } + config = { other_setting: "value" } + expect(valid_with(headers:, config:)).to be true + end + end + + context "with edge case inputs" do + let(:headers) { { default_header => secret } } + + it "handles different header types" do + # String keys (most common) + string_headers = { "Authorization" => secret } + expect(valid_with(headers: string_headers)).to be true + + # Symbol keys + symbol_headers = { Authorization: secret } + expect(valid_with(headers: symbol_headers)).to be true + + # Mixed keys + mixed_headers = { "authorization" => secret } + expect(valid_with(headers: mixed_headers)).to be true + end + + it "handles non-hash headers" do + expect(valid_with(headers: nil)).to be false + expect(valid_with(headers: "not a hash")).to be false + expect(valid_with(headers: [])).to be false + expect(valid_with(headers: 123)).to be false + end + + it "handles different secret types" do + headers = { default_header => "123" } + + # String secret + expect(valid_with(headers:, secret: "123")).to be true + + # Numeric secret (converted to string) + expect(valid_with(headers:, secret: 123)).to be false # Different types + + # Symbol secret + expect(valid_with(headers:, secret: :symbol)).to be false # Different types + end + + it "is case-sensitive for secret values" do + headers = { default_header => "MySecret" } + expect(valid_with(headers:, secret: "MySecret")).to be true + expect(valid_with(headers:, secret: "mysecret")).to be false + expect(valid_with(headers:, secret: "MYSECRET")).to be false + end + + it "handles very long secrets" do + long_secret = "a" * 1000 + headers = { default_header => long_secret } + expect(valid_with(headers:, secret: long_secret)).to be true + expect(valid_with(headers:, secret: long_secret + "x")).to be false + end + + it "handles special characters in secrets" do + special_secret = "secret!@#$%^&*()_+-=[]{}|;':\",./<>?" + headers = { default_header => special_secret } + expect(valid_with(headers:, secret: special_secret)).to be true + end + + it "handles unicode characters in secrets" do + unicode_secret = "sëcrét-ûñíçødé-🔑" + headers = { default_header => unicode_secret } + expect(valid_with(headers:, secret: unicode_secret)).to be true + end + end + + context "security considerations" do + let(:headers) { { default_header => secret } } + + it "uses secure comparison to prevent timing attacks" do + # Test that we're using Rack::Utils.secure_compare + expect(Rack::Utils).to receive(:secure_compare).with(secret, secret).and_return(true) + expect(valid_with(headers:)).to be true + end + + it "handles exceptions gracefully" do + # Mock an exception during validation + allow(Rack::Utils).to receive(:secure_compare).and_raise(StandardError.new("test error")) + expect(valid_with(headers:)).to be false + end + + it "rejects headers with suspicious patterns" do + # Test various suspicious header values + suspicious_values = [ + "secret\x00injection", + "secret\u0001control", + "secret\x7fdelete" + ] + + suspicious_values.each do |suspicious_value| + bad_headers = { default_header => suspicious_value } + expect(valid_with(headers: bad_headers)).to be false + end + end + end + end + + describe "DEFAULT_CONFIG" do + it "has expected default values" do + expect(described_class::DEFAULT_CONFIG).to include( + header: "Authorization" + ) + end + + it "is frozen" do + expect(described_class::DEFAULT_CONFIG).to be_frozen + end + end + + describe "inheritance" do + it "extends Base class" do + expect(described_class.ancestors).to include(Hooks::Plugins::RequestValidator::Base) + end + + it "implements required valid? method" do + expect(described_class).to respond_to(:valid?) + end + + it "valid? method has expected signature" do + method = described_class.method(:valid?) + expect(method.parameters).to include([:keyreq, :payload]) + expect(method.parameters).to include([:keyreq, :headers]) + expect(method.parameters).to include([:keyreq, :secret]) + expect(method.parameters).to include([:keyreq, :config]) + end + end + + describe "real-world scenarios" do + context "Okta-style webhook validation" do + let(:okta_secret) { "my-okta-webhook-secret-key" } + let(:okta_config) do + { + request_validator: { + header: "Authorization" + } + } + end + + it "validates Okta-style webhook with Authorization header" do + headers = { "Authorization" => okta_secret } + result = described_class.valid?( + payload: '{"eventType":"user.lifecycle.activate","eventId":"abc123"}', + headers:, + secret: okta_secret, + config: okta_config + ) + expect(result).to be true + end + + it "rejects invalid Okta-style webhook" do + headers = { "Authorization" => "wrong-secret" } + result = described_class.valid?( + payload: '{"eventType":"user.lifecycle.activate","eventId":"abc123"}', + headers:, + secret: okta_secret, + config: okta_config + ) + expect(result).to be false + end + end + + context "Generic API key validation" do + let(:api_key) { "sk_live_12345abcdef" } + let(:api_config) do + { + request_validator: { + header: "X-API-Key" + } + } + end + + it "validates API key in custom header" do + headers = { "X-API-Key" => api_key } + result = described_class.valid?( + payload: '{"data":"value"}', + headers:, + secret: api_key, + config: api_config + ) + expect(result).to be true + end + end + end +end