feat(brevo): make sure webhooks come from brevo (#2563)

* make sure webhook come from brevo

* minor improvement

* refacto brevo tests

* fix linter

* prevent flaky

app/controllers/brevo/ip_whitelist_concern.rb

@@ -0,0 +1,30 @@
+require "ipaddr"
+
+module Brevo::IpWhitelistConcern
+  extend ActiveSupport::Concern
+
+  # IP list comes from
+  # https://help.brevo.com/hc/en-us/articles/15127404548498-Brevo-IP-ranges-List-of-publicly-exposed-services#h_01HENC062K8KJKJE7BJNYMPM77
+  IP_WHITELIST_RANGE = "1.179.112.0/20".freeze
+
+  included do
+    before_action :ensure_ip_comes_from_brevo_ips
+  end
+
+  private
+
+  def ensure_ip_comes_from_brevo_ips
+    # In case Brevo decides to use some other IP range without notice
+    # we need a quick way to skip this check
+    return if ENV["DISABLE_BREVO_IP_WHITELIST"].present?
+
+    return if IPAddr.new(IP_WHITELIST_RANGE).include?(request.remote_ip)
+
+    Sentry.capture_message("Brevo Webhook received with following non whitelisted IP", {
+                             extra: {
+                               ip: request.remote_ip
+                             }
+                           })
+    head :forbidden
+  end
+end

app/controllers/brevo/mail_webhooks_controller.rb

@@ -1,5 +1,6 @@
 module Brevo
   class MailWebhooksController < ApplicationController
+    include Brevo::IpWhitelistConcern
     skip_before_action :authenticate_agent!, :verify_authenticity_token
 
     PERMITTED_PARAMS = %i[

app/controllers/brevo/sms_webhooks_controller.rb

@@ -1,5 +1,6 @@
 module Brevo
   class SmsWebhooksController < ApplicationController
+    include Brevo::IpWhitelistConcern
     skip_before_action :authenticate_agent!, :verify_authenticity_token
 
     PERMITTED_PARAMS = %i[

app/services/inbound_webhooks/brevo/assign_delivery_status_and_date_base.rb

@@ -10,7 +10,6 @@ def call
         return if old_update?
         return if @record.delivered?
 
-        alert_sentry_if_webhook_mismatch
         set_last_brevo_webhook_received_at
         return unless delivery_status.in?(record_class.delivery_statuses.keys)
 
@@ -29,10 +28,6 @@ def record_class
         @record_class ||= @record.class
       end
 
-      def alert_sentry_if_webhook_mismatch
-        raise NoMethodError
-      end
-
       def delivery_status
         raise NoMethodError
       end

app/services/inbound_webhooks/brevo/assign_mail_delivery_status_and_date.rb

@@ -6,13 +6,6 @@ class AssignMailDeliveryStatusAndDate < AssignDeliveryStatusAndDateBase
       def delivery_status
         @delivery_status ||= @webhook_params[:event]
       end
-
-      def alert_sentry_if_webhook_mismatch
-        return if @record.email.casecmp?(@webhook_params[:email])
-
-        Sentry.capture_message("#{record_class} email and webhook email does not match",
-                               extra: { record: @record, webhook: @webhook_params })
-      end
     end
   end
 end

app/services/inbound_webhooks/brevo/assign_sms_delivery_status_and_date.rb

@@ -6,13 +6,6 @@ class AssignSmsDeliveryStatusAndDate < AssignDeliveryStatusAndDateBase
       def delivery_status
         @delivery_status ||= @webhook_params[:msg_status]
       end
-
-      def alert_sentry_if_webhook_mismatch
-        return if @record.user.phone_number == PhoneNumberHelper.format_phone_number(@webhook_params[:to])
-
-        Sentry.capture_message("#{record_class} mobile phone and webhook mobile phone does not match",
-                               extra: { record: @record, webhook: @webhook_params })
-      end
     end
   end
 end

spec/controllers/brevo/mail_webhooks_controller_spec.rb

@@ -1,4 +1,6 @@
 describe Brevo::MailWebhooksController do
+  include_context "with ip whitelist"
+
   describe "#create" do
     before do
       allow(InboundWebhooks::Brevo::ProcessMailDeliveryStatusJob).to receive(:perform_later)
@@ -13,6 +15,12 @@
       }
     end
 
+    context "when called with non-matching IP" do
+      let(:params) { valid_mail_params }
+
+      include_examples "returns 403 for non-whitelisted IP", "18.12.12.12"
+    end
+
     context "when X-Mailin-custom header is present and environment matches" do
       it "enqueues the job for processing mail delivery status" do
         expect(InboundWebhooks::Brevo::ProcessMailDeliveryStatusJob).to receive(:perform_later)

spec/controllers/brevo/sms_webhooks_controller_spec.rb

@@ -1,4 +1,6 @@
 describe Brevo::SmsWebhooksController do
+  include_context "with ip whitelist"
+
   describe "#create" do
     before do
       allow(InboundWebhooks::Brevo::ProcessSmsDeliveryStatusJob).to receive(:perform_later)
@@ -8,6 +10,12 @@
       { to: "1234567890", msg_status: "delivered", date: "2023-06-07T12:34:56Z", record_identifier: "invitation_1" }
     end
 
+    context "when called with non-matching IP" do
+      let(:params) { valid_sms_params }
+
+      include_examples "returns 403 for non-whitelisted IP", "18.12.12.12"
+    end
+
     it "enqueues the job for processing SMS delivery status" do
       expect(InboundWebhooks::Brevo::ProcessSmsDeliveryStatusJob).to receive(:perform_later)
         .with({ to: "1234567890", msg_status: "delivered", date: "2023-06-07T12:34:56Z" }, "invitation_1")

spec/rails_helper.rb

@@ -45,6 +45,7 @@
   config.include MotifCategoriesHelper
   config.include NirHelper
   config.include SortingHelper
+  config.include BrevoIpHelper
   config.include ConfirmModalHelper
   config.include StubHelper
   config.include UploadHelper

spec/requests/brevo_mail_webhooks_spec.rb

@@ -1,4 +1,6 @@
 RSpec.describe "BrevoMailWebhooks" do
+  include_context "with ip whitelist"
+
   let(:webhook_params) do
     {
       email: "test@example.com",
@@ -29,28 +31,6 @@
       expect(invitation.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))
     end
 
-    context "when emails doesnt match" do
-      let(:mismatched_webhook_params) do
-        {
-          email: "email_changed@example.com",
-          event: "delivered",
-          date: "2023-06-07T12:34:56Z",
-          :"X-Mailin-custom" => "{\"record_identifier\": \"#{invitation.record_identifier}\"}"
-        }
-      end
-
-      it "update the invitation but captures an error" do
-        post "/brevo/mail_webhooks", params: mismatched_webhook_params, as: :json
-        expect(response).to be_successful
-
-        invitation.reload
-        expect(invitation.delivery_status).to eq("delivered")
-        expect(invitation.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))
-        expect(Sentry).to have_received(:capture_message).with("Invitation email and webhook email does not match",
-                                                               any_args)
-      end
-    end
-
     context "with an event not in enum" do
       let(:unprocessed_event_webhook_params) do
         {

spec/requests/brevo_sms_webhooks_spec.rb

@@ -1,4 +1,6 @@
 RSpec.describe "BrevoSmsWebhooks" do
+  include_context "with ip whitelist"
+
   let(:webhook_params) do
     {
       to: "0601010101",
@@ -27,28 +29,6 @@
       expect(invitation.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))
     end
 
-    context "when phones doesnt match" do
-      let(:mismatched_webhook_params) do
-        {
-          to: "0987654321",
-          msg_status: "delivered",
-          date: "2023-06-07T12:34:56Z"
-        }
-      end
-
-      it "update the invitation but captures an error" do
-        post "/brevo/sms_webhooks/#{invitation.record_identifier}", params: mismatched_webhook_params, as: :json
-        expect(response).to be_successful
-
-        invitation.reload
-        expect(invitation.delivery_status).to eq("delivered")
-        expect(invitation.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))
-        expect(Sentry).to have_received(:capture_message).with(
-          "Invitation mobile phone and webhook mobile phone does not match", any_args
-        )
-      end
-    end
-
     context "when event is not in enum" do
       let(:unprocessed_event_webhook_params) do
         {

spec/services/inbound_webhooks/brevo/assign_mail_delivery_status_and_date_spec.rb

@@ -22,25 +22,8 @@
         end
       end
 
-      context "when emails does not match" do
-        let(:webhook_params) do
-          { email: "email_changed@example.com", event: "delivered", date: "2023-06-07T12:34:56Z" }
-        end
-
-        it "update the #{record_type} but capture an error" do
-          expect(Sentry).to receive(:capture_message).with(
-            "#{record_type.capitalize} email and webhook email does not match",
-            any_args
-          )
-          subject
-          expect(record.delivery_status).to eq("delivered")
-          expect(record.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))
-        end
-      end
-
       it "updates the #{record_type} with the correct delivery status and date" do
         subject
-        expect(Sentry).not_to receive(:capture_message).with(any_args)
         record.reload
         expect(record.delivery_status).to eq("delivered")
         expect(record.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))

spec/services/inbound_webhooks/brevo/assign_sms_delivery_status_and_date_spec.rb

@@ -26,9 +26,6 @@
         let(:webhook_params) { { to: "0987654321", msg_status: "delivered", date: "2023-06-07T12:34:56Z" } }
 
         it "update the #{record_type} but capture an error" do
-          expect(Sentry).to receive(:capture_message).with(
-            "#{record_type.capitalize} mobile phone and webhook mobile phone does not match", any_args
-          )
           subject
           expect(record.delivery_status).to eq("delivered")
           expect(record.last_brevo_webhook_received_at).to eq(Time.zone.parse("2023-06-07T12:34:56Z"))

spec/support/brevo_ip_helper.rb

@@ -0,0 +1,58 @@
+module BrevoIpHelper
+  def stub_brevo_webhook_ip(remote_ip = "1.179.112.1")
+    [
+      Brevo::MailWebhooksController,
+      Brevo::SmsWebhooksController
+    ].each do |controller|
+      allow_any_instance_of(controller).to receive(:request).and_wrap_original do |original_request|
+        original_request.call.tap do |request|
+          allow(request).to receive(:remote_ip).and_return(remote_ip)
+        end
+      end
+    end
+  end
+end
+
+RSpec.shared_context "with ip whitelist", shared_context: :metadata do |ip|
+  let(:remote_ip) { ip || "1.179.112.1" }
+
+  before do
+    stub_brevo_webhook_ip(remote_ip)
+  end
+end
+
+RSpec.shared_examples "returns 403 for non-whitelisted IP" do |ip|
+  include_context "with ip whitelist", ip
+
+  context "when called with non-matching IP" do
+    it "returns 403" do
+      expect(Sentry).to(
+        receive(:capture_message).with("Brevo Webhook received with following non whitelisted IP", {
+                                         extra: { ip: ip }
+                                       })
+      )
+      post :create, params:, as: :json
+      expect(response).to have_http_status(:forbidden)
+    end
+  end
+
+  context "when whitelisting is disabled" do
+    before do
+      ENV["DISABLE_BREVO_IP_WHITELIST"] = "true"
+    end
+
+    after do
+      ENV["DISABLE_BREVO_IP_WHITELIST"] = nil
+    end
+
+    it "skips whitelisting" do
+      expect(Sentry).not_to(
+        receive(:capture_message).with("Brevo Webhook received with following non whitelisted IP", {
+                                         extra: { ip: ip }
+                                       })
+      )
+      post :create, params:, as: :json
+      expect(response).not_to have_http_status(:forbidden)
+    end
+  end
+end