FIX: Disable webhooks on 410 and 404 HTTP responses (#7392)

FIX: Disable webhooks on 410 and 404 HTTP responses (#7392)

app/jobs/regular/emit_web_hook_event.rb

@@ -41,27 +41,21 @@ module Jobs
 
     def send_webhook!
       uri = URI(web_hook.payload_url.strip)
-
+      conn = Excon.new(uri.to_s, ssl_verify_peer: web_hook.verify_certificate, retry_limit: 0)
-      conn = Excon.new(
-        uri.to_s,
-        ssl_verify_peer: web_hook.verify_certificate,
-        retry_limit: 0
-      )
 
       web_hook_body = build_webhook_body
       web_hook_event = create_webhook_event(web_hook_body)
       web_hook_headers = build_webhook_headers(uri, web_hook_body, web_hook_event)
-
+      web_hook_response = nil
-      response = nil
 
       begin
         now = Time.zone.now
-        response = conn.post(headers: web_hook_headers, body: web_hook_body)
+        web_hook_response = conn.post(headers: web_hook_headers, body: web_hook_body)
         web_hook_event.update!(
           headers: MultiJson.dump(web_hook_headers),
-          status: response.status,
+          status: web_hook_response.status,
-          response_headers: MultiJson.dump(response.headers),
+          response_headers: MultiJson.dump(web_hook_response.headers),
-          response_body: response.body,
+          response_body: web_hook_response.body,
           duration: ((Time.zone.now - now) * 1000).to_i
         )
       rescue => e
@@ -74,7 +68,23 @@ module Jobs
       end
 
       publish_webhook_event(web_hook_event)
-      retry_web_hook if response&.status != 200
+      process_webhook_response(web_hook_response)
+    end
+
+    def process_webhook_response(web_hook_response)
+      return if web_hook_response&.status.blank?
+
+      case web_hook_response.status
+      when 200..299
+        return
+      when 404, 410
+        if @retry_count >= MAX_RETRY_COUNT
+          web_hook.update_attribute(:active, false)
+          StaffActionLogger.new(Discourse.system_user).log_web_hook_deactivate(web_hook, web_hook_response.status)
+        end
+      else
+        retry_web_hook
+      end
     end
 
     def retry_web_hook

app/models/user_history.rb

@@ -91,7 +91,8 @@ class UserHistory < ActiveRecord::Base
       web_hook_destroy: 72,
       embeddable_host_create: 73,
       embeddable_host_update: 74,
-      embeddable_host_destroy: 75
+      embeddable_host_destroy: 75,
+      web_hook_deactivate: 76
     )
   end
 
@@ -159,6 +160,7 @@ class UserHistory < ActiveRecord::Base
       :web_hook_create,
       :web_hook_update,
       :web_hook_destroy,
+      :web_hook_deactivate,
       :embeddable_host_create,
       :embeddable_host_update,
       :embeddable_host_destroy

app/services/staff_action_logger.rb

@@ -599,6 +599,19 @@ class StaffActionLogger
     ))
   end
 
+  def log_web_hook_deactivate(web_hook, response_http_status, opts = {})
+    context = [
+      "webhook_id: #{web_hook.id}",
+      "webhook_response_status: #{response_http_status}"
+    ]
+
+    UserHistory.create!(params.merge(
+      action: UserHistory.actions[:web_hook_deactivate],
+      context: context,
+      details: I18n.t('staff_action_logs.webhook_deactivation_reason', status: response_http_status)
+    ))
+  end
+
   def log_embeddable_host(embeddable_host, action, opts = {})
     old_values, new_values = get_changes(opts[:changes])
 

config/locales/client.en.yml

@@ -3744,6 +3744,7 @@ en:
             web_hook_create: "webhook create"
             web_hook_update: "webhook update"
             web_hook_destroy: "webhook destroy"
+            web_hook_deactivate: "webhook deactivate"
             embeddable_host_create: "embeddable host create"
             embeddable_host_update: "embeddable host update"
             embeddable_host_destroy: "embeddable host destroy"

config/locales/server.en.yml

@@ -4400,6 +4400,7 @@ en:
     unknown: "unknown"
     user_merged: "%{username} was merged into this account"
     user_delete_self: "Deleted by self from %{url}"
+    webhook_deactivation_reason: "Your webhook has been automatically deactivated. We received multiple '%{status}' HTTP status failure responses."
 
   reviewables:
     missing_version: "You must supply a version parameter"

spec/jobs/emit_web_hook_event_spec.rb

@@ -40,54 +40,88 @@ describe Jobs::EmitWebHookEvent do
   context 'when the web hook is failed' do
     before do
       SiteSetting.retry_web_hook_events = true
-
-      stub_request(:post, post_hook.payload_url)
-        .to_return(body: 'Invalid Access', status: 403)
     end
 
-    it 'retry if site setting is enabled' do
+    context 'when the webhook has failed for 404 or 410' do
-      expect do
+      before do
-        subject.execute(
+        stub_request(:post, post_hook.payload_url).to_return(body: 'Invalid Access', status: response_status)
-          web_hook_id: post_hook.id,
+      end
-          event_type: described_class::PING_EVENT
-        )
-      end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(1)
-    end
 
-    it 'does not retry for more than maximum allowed times' do
+      let(:response_status) { 410 }
-      expect do
+
+      it 'disables the webhook' do
+        expect do
+          subject.execute(
+            web_hook_id: post_hook.id,
+            event_type: described_class::PING_EVENT,
+            retry_count: described_class::MAX_RETRY_COUNT
+          )
+        end.to change { post_hook.reload.active }.to(false)
+      end
+
+      it 'logs webhook deactivation reason' do
         subject.execute(
           web_hook_id: post_hook.id,
           event_type: described_class::PING_EVENT,
           retry_count: described_class::MAX_RETRY_COUNT
         )
-      end.to_not change { Jobs::EmitWebHookEvent.jobs.size }
+        user_history = UserHistory.find_by(action: UserHistory.actions[:web_hook_deactivate], acting_user: Discourse.system_user)
+        expect(user_history).to be_present
+        expect(user_history.context).to eq([
+          "webhook_id: #{post_hook.id}",
+          "webhook_response_status: #{response_status}"
+        ].to_s)
+      end
     end
 
-    it 'does not retry if site setting is disabled' do
+    context 'when the webhook has failed' do
-      SiteSetting.retry_web_hook_events = false
+      before do
+        stub_request(:post, post_hook.payload_url).to_return(body: 'Invalid Access', status: 403)
+      end
 
-      expect do
+      it 'retry if site setting is enabled' do
+        expect do
+          subject.execute(
+            web_hook_id: post_hook.id,
+            event_type: described_class::PING_EVENT
+          )
+        end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(1)
+      end
+
+      it 'does not retry for more than maximum allowed times' do
+        expect do
+          subject.execute(
+            web_hook_id: post_hook.id,
+            event_type: described_class::PING_EVENT,
+            retry_count: described_class::MAX_RETRY_COUNT
+          )
+        end.to_not change { Jobs::EmitWebHookEvent.jobs.size }
+      end
+
+      it 'does not retry if site setting is disabled' do
+        SiteSetting.retry_web_hook_events = false
+
+        expect do
+          subject.execute(
+            web_hook_id: post_hook.id,
+            event_type: described_class::PING_EVENT
+          )
+        end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(0)
+      end
+
+      it 'properly logs error on rescue' do
+        stub_request(:post, post_hook.payload_url).to_raise("connection error")
         subject.execute(
           web_hook_id: post_hook.id,
           event_type: described_class::PING_EVENT
         )
-      end.to change { Jobs::EmitWebHookEvent.jobs.size }.by(0)
+
+        event = WebHookEvent.last
+        expect(event.payload).to eq(MultiJson.dump(ping: 'OK'))
+        expect(event.status).to eq(-1)
+        expect(MultiJson.load(event.response_headers)['error']).to eq('connection error')
+      end
     end
-
-    it 'properly logs error on rescue' do
-      stub_request(:post, post_hook.payload_url).to_raise("connection error")
-      subject.execute(
-        web_hook_id: post_hook.id,
-        event_type: described_class::PING_EVENT
-      )
-
-      event = WebHookEvent.last
-      expect(event.payload).to eq(MultiJson.dump(ping: 'OK'))
-      expect(event.status).to eq(-1)
-      expect(MultiJson.load(event.response_headers)['error']).to eq('connection error')
-    end
-
   end
 
   it 'does not raise an error for a ping event without payload' do