Make validation async (#13)

Most mistakes in webhook configuration are usually related to the
configuration of the secrets for signatures. While it is good in some
situations to reject the incorrectly signed webhooks immediately, in way
more situations it will actually be the misconfigured signature that'll
be at fault. A better approach is to allow the webhooks to be respooled
for processing when the signature configuration has been rectified.

- Use `valid?` in the background job instead of the controller. Most
common configuration issue is an incorrectly specified signing secret,
or an incorrectly implemented input validation. When these happen, it is
better to allow the webhook to be reprocessed
- Use instance methods in handlers instead of class methods, as they are
shorter to define. Assume a handler module supports `.new` - with a
module using singleton methods it may return `self` from `new`.
- In the config, allow the handlers specified as strings. Module
resolution in Rails happens after the config gets loaded, because the
config may alter the Zeitwerk load paths. To allow the config to get
loaded and to allow handlers to be autoloaded using Zeitwerk, the
handler modules have to be resolved lazily. This also permits the
handlers to be reloadable, like any module under Rails' autoloading
control.
- Simplify the Rails app used in tests to be small and keep it in a
single file
- If a handler is willing to expose errors to the caller, let Rails
rescue the error and display an error page or do whatever else is
configured for Rails globally.
- Store request headers with the received webhook to allow for async
validation

Closes #12 #11

---------

Co-authored-by: Stanislav Katkov <[email protected]>

.github/workflows/main.yml

@@ -23,5 +23,5 @@ jobs:
         with:
           ruby-version: ${{ matrix.ruby }}
           bundler-cache: true
-      - name: Run tests
-        run: bundle exec rails test
+      - name: Tests and lint
+        run: bundle exec rake

.gitignore

@@ -15,3 +15,5 @@
 /test/dummy/tmp/
 
 .DS_Store
+
+development.sqlite3*

CHANGELOG.md

@@ -3,31 +3,31 @@ All notable changes to this project will be documented in this file.
 
 This format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## 0.3.1
 
-### Changed
+## Unreleased
+
+- Use `valid?` in the background job instead of the controller. Most common configuration issue is an incorrectly specified signing secret, or an incorrectly implemented input validation. When these happen, it is better to allow the webhook to be reprocessed
+- Use instance methods in handlers instead of class methods, as they are shorter to define. Assume a handler module supports `.new` - with a module using singleton methods it may return `self` from `new`.
+- In the config, allow the handlers specified as strings. Module resolution in Rails happens after the config gets loaded, because the config may alter the Zeitwerk load paths. To allow the config to get loaded and to allow handlers to be autoloaded using Zeitwerk, the handler modules have to be resolved lazily. This also permits the handlers to be reloadable, like any module under Rails' autoloading control.
+- Simplify the Rails app used in tests to be small and keep it in a single file
+- If a handler is willing to expose errors to the caller, let Rails rescue the error and display an error page or do whatever else is configured for Rails globally.
+- Store request headers with the received webhook to allow for async validation
+
+## 0.3.1
 
 - BaseHandler#expose_errors_to_sender? default to true now.
 
 ## 0.3.0
 
-### Changed
 - state_machine_enum library was moved in it's own library/gem.
-
-### Fixed
 - Provide handled: true attribute for Rails.error.report method, because it is required in Rails 7.0.
 
 ## 0.2.0
 
-### Changed
-
 - Handler methods are now defined as instance methods for simplicity.
 - Define service_id in initializer with active_handlers, instead of handler class.
 - Use ruby 3.0 as a base for standard/rubocop, format all code according to it.
-
-### Added
-
-- Introduce Rails common error reporter ( https://guides.rubyonrails.org/error_reporting.html )
+- Use Rails common error reporter ( https://guides.rubyonrails.org/error_reporting.html )
 
 ## 0.1.0
 

Gemfile.lock

@@ -128,6 +128,8 @@ GEM
     nio4r (2.7.3)
     nokogiri (1.16.5-arm64-darwin)
       racc (~> 1.4)
+    nokogiri (1.16.5-x86_64-darwin)
+      racc (~> 1.4)
     nokogiri (1.16.5-x86_64-linux)
       racc (~> 1.4)
     parallel (1.24.0)
@@ -208,6 +210,7 @@ GEM
       activesupport (>= 5.2)
       sprockets (>= 3.0.0)
     sqlite3 (1.7.3-arm64-darwin)
+    sqlite3 (1.7.3-x86_64-darwin)
     sqlite3 (1.7.3-x86_64-linux)
     standard (1.36.0)
       language_server-protocol (~> 3.17.0.2)
@@ -238,6 +241,7 @@ GEM
 
 PLATFORMS
   arm64-darwin
+  x86_64-darwin-20
   x86_64-linux
 
 DEPENDENCIES

README.md

@@ -22,11 +22,13 @@ If bundler is not being used to manage dependencies, install the gem by executin
 
 Generate migrations and initializer file.
 
-`munster:install`
+`bin/rails g munster:install`
 
 Mount munster engine in your routes.
 
-`mount Munster::Engine, at: "/webhooks"`
+```ruby
+mount Munster::Engine, at: "/webhooks"
+```
 
 ## Requirements
 
@@ -40,7 +42,8 @@ This project depends on two dependencies:
 This gem uses [Rails common error reporter](https://guides.rubyonrails.org/error_reporting.html) to report any possible error to services like Honeybadger, Appsignal, Sentry and etc. Most of those services already support this common interface, if not - it's not that hard to add this support on your own.
 
 It's possible to provide additional context for every error. e.g.
-```
+
+```ruby
 Munster.configure do |config|
   config.error_context = { appsignal: { namespace: "webhooks" } }
 end

Rakefile

@@ -1,12 +1,7 @@
 # frozen_string_literal: true
 
 require "bundler/setup"
-
-APP_RAKEFILE = File.expand_path("test/dummy/Rakefile", __dir__)
-load "rails/tasks/engine.rake"
-
-load "rails/tasks/statistics.rake"
-
+require "rake/testtask"
 require "bundler/gem_tasks"
 require "standard/rake"
 
@@ -15,4 +10,17 @@ task :format do
   `bundle exec magic_frozen_string_literal .`
 end
 
-task default: %i[standard]
+Rake::TestTask.new(:test) do |t|
+  t.libs << "test"
+  t.libs << "lib"
+
+  file_name = ARGV[1]
+
+  t.test_files = if file_name
+    [file_name]
+  else
+    FileList["test/**/*_test.rb"]
+  end
+end
+
+task default: [:test, :standard]

example/config/initializers/munster.rb

@@ -3,5 +3,7 @@
 require_relative "../../app/webhooks/webhook_test_handler"
 
 Munster.configure do |config|
-  config.active_handlers = [WebhookTestHandler]
+  config.active_handlers = {
+    "test-handler" => "WebhookTestHandler"
+  }
 end

lib/munster.rb

@@ -18,7 +18,7 @@ def self.configure
 class Munster::Configuration
   include ActiveSupport::Configurable
 
-  config_accessor(:processing_job_class) { Munster::ProcessingJob }
-  config_accessor(:active_handlers) { [] }
-  config_accessor(:error_context) { {} }
+  config_accessor(:processing_job_class, default: Munster::ProcessingJob)
+  config_accessor(:active_handlers, default: {})
+  config_accessor(:error_context, default: {})
 end

lib/munster/base_handler.rb

@@ -4,40 +4,58 @@
 
 module Munster
   class BaseHandler
-    # Gets called from the background job
-    def self.process(...)
-      new.process(...)
-    end
-
-    # Reimplement this method, it's being used in WebhooksController to store incoming webhook.
-    # Also que for processing in the end.
+    # `handle` accepts the ActionDispatch HTTP request and saves the webhook for later processing. It then
+    # enqueues an ActiveJob which will perform the processing using `process`.
+    #
+    # @param action_dispatch_request[ActionDispatch::Request] the request from the controller
     # @return [void]
     def handle(action_dispatch_request)
-      binary_body_str = action_dispatch_request.body.read.force_encoding(Encoding::BINARY)
-      attrs = {
-        body: binary_body_str,
-        handler_module_name: self.class.name,
-        handler_event_id: extract_event_id_from_request(action_dispatch_request)
-      }
-      webhook = Munster::ReceivedWebhook.create!(**attrs)
+      handler_module_name = is_a?(Munster::BaseHandler) ? self.class.name : to_s
+      handler_event_id = extract_event_id_from_request(action_dispatch_request)
+
+      webhook = Munster::ReceivedWebhook.new(request: action_dispatch_request, handler_event_id: handler_event_id, handler_module_name: handler_module_name)
+      webhook.save!
 
       Munster.configuration.processing_job_class.perform_later(webhook)
-    rescue ActiveRecord::RecordNotUnique # Deduplicated
-      nil
+    rescue ActiveRecord::RecordNotUnique # Webhook deduplicated
+      Rails.logger.info { "#{inspect} Webhook #{handler_event_id} is a duplicate delivery and will not be stored." }
     end
 
-    # This method will be used to process webhook by async worker.
+    # This is the heart of your webhook processing. Override this method and define your processing inside of it.
+    # The `received_webhook` will provide access to the `ReceivedWebhook` model, which contains the received
+    # body of the webhook request, but also the full (as-full-as-possible) clone of the original ActionDispatch::Request
+    # that you can use.
+    #
+    # @param received_webhook[Munster::ReceivedWebhook]
+    # @return [void]
     def process(received_webhook)
     end
 
-    # This method verifies that request actually comes from provider:
-    # signature validation, HTTP authentication, IP whitelisting and the like
+    # This method verifies that request is not malformed and actually comes from the webhook sender:
+    # signature validation, HTTP authentication, IP whitelisting and the like. There is a difference depending
+    # on whether you validate sync (in the receiving controller) or async (in the processing job):
+    # Validation is async - it takes place in the background job that gets enqueued to process the webhook.
+    # The `action_dispatch_request` will be reconstructed from the `ReceivedWebhook` data. Background validation
+    # is used because the most common misconfiguration that may occur is usually forgetting or misidentifying the
+    # signature for signed webhooks. If such a misconfiguration has taken place, the background validation
+    # (instead of rejecting the webhook at input) permits you to still process the webhook once the secrets
+    # have been configured correctly.
+    #
+    # If this method returns `false`, the webhook will be marked as `failed_validation` in the database. If this
+    # method returns `true`, the `process` method of the handler is going to be called.
+    #
+    # @see Munster::ReceivedWebhook#request
+    # @param action_dispatch_request[ActionDispatch::Request] the reconstructed request from the controller
+    # @return [Boolean]
     def valid?(action_dispatch_request)
       true
     end
 
-    # Default implementation just generates UUID, but if the webhook sender sends us
-    # an event ID we use it for deduplication.
+    # Default implementation just generates a random UUID, but if the webhook sender sends us
+    # an event ID we use it for deduplication. A duplicate webhook is not going to be
+    # stored in the database if it is already present there.
+    #
+    # @return [String]
     def extract_event_id_from_request(action_dispatch_request)
       SecureRandom.uuid
     end
@@ -47,6 +65,8 @@ def extract_event_id_from_request(action_dispatch_request)
     # data and do not disable your endpoint forcibly. We allow this to be configured
     # on a per-handler basis - a better webhooks sender will be able to make out
     # some sense of the errors.
+    #
+    # @return [Boolean]
     def expose_errors_to_sender?
       true
     end
@@ -55,6 +75,8 @@ def expose_errors_to_sender?
     # to deactivate a particular handler via feature flags for example, or use other
     # logic to determine whether the handler may be used to create new received webhooks
     # in the system. This is primarily needed for load shedding.
+    #
+    # @return [Boolean]
     def active?
       true
     end

lib/munster/controllers/receive_webhooks_controller.rb

@@ -2,55 +2,54 @@
 
 module Munster
   class ReceiveWebhooksController < ActionController::API
-    class HandlerRefused < StandardError
+    class HandlerInactive < StandardError
     end
 
-    class HandlerInactive < StandardError
+    class UnknownHandler < StandardError
     end
 
     def create
-      handler = lookup_handler(params[:service_id]).new
-
+      Rails.error.set_context(**Munster.configuration.error_context)
+      handler = lookup_handler(service_id)
       raise HandlerInactive unless handler.active?
-      raise HandlerRefused unless handler.valid?(request)
-
       handler.handle(request)
-      head :ok
-    rescue KeyError # handler was not found, so we return generic 404 error.
-      render_error("Required parameters were not present in the request", :not_found)
+      render(json: {ok: true, error: nil})
+    rescue UnknownHandler => e
+      Rails.error.report(e, handled: true, severity: :error)
+      render_error_with_status("No handler found for #{service_id.inspect}", status: :not_found)
+    rescue HandlerInactive => e
+      Rails.error.report(e, handled: true, severity: :error)
+      render_error_with_status("Webhook handler #{service_id.inspect} is inactive", status: :service_unavailable)
     rescue => e
-      Rails.error.set_context(**Munster.configuration.error_context)
-      # Rails 7.1 only requires `error` attribute for .report method, but Rails 7.0 requires `handled:` attribute additionally.
-      # We're setting `handled:` and `severity:` attributes to maintain compatibility with all versions of > rails 7.
+      raise e unless handler
+      raise e if handler.expose_errors_to_sender?
       Rails.error.report(e, handled: true, severity: :error)
-
-      if handler&.expose_errors_to_sender?
-        error_for_sender_from_exception(e)
-      else
-        head :ok
-      end
+      render_error_with_status("Internal error (#{e})")
     end
 
-    def error_for_sender_from_exception(e)
-      case e
-      when HandlerRefused
-        render_error("Webhook handler did not validate the request (signature or authentication may be invalid)", :forbidden)
-      when HandlerInactive
-        render_error("Webhook handler is inactive", :service_unavailable)
-      when JSON::ParserError
-        render_error("Request body is not a valid JSON", :bad_request)
-      else
-        render_error("Internal error", :internal_server_error)
-      end
+    def service_id
+      params.require(:service_id)
     end
 
-    def render_error(message_str, status_sym)
-      json = {error: message_str}.to_json
-      render(json: json, status: status_sym)
+    def render_error_with_status(message_str, status: :ok)
+      json = {ok: false, error: message_str}.to_json
+      render(json: json, status: status)
     end
 
     def lookup_handler(service_id_str)
-      Munster.configuration.active_handlers.with_indifferent_access.fetch(service_id_str)
+      active_handlers = Munster.configuration.active_handlers.with_indifferent_access
+      # The config can specify a mapping of:
+      # {"service-1" => MyHandler }
+      # or
+      # {"service-2" => "MyOtherHandler"}
+      # We need to support both, because `MyHandler` is not loaded yet when Rails initializers run.
+      # Zeitwerk takes over after the initializers. So we can't really use a module in the init cycle just yet.
+      # We can, however, use the module name - and resolve it lazily, later.
+      handler_class_or_class_name = active_handlers.fetch(service_id_str)
+      handler_class = handler_class_or_class_name.respond_to?(:constantize) ? handler_class_or_class_name.constantize : handler_class_or_class_name
+      handler_class.new
+    rescue KeyError
+      raise UnknownHandler
     end
   end
 end