From 3e3538d6031ea555602a15b33d3c950093033a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 8 Jun 2016 22:38:38 +0200 Subject: [PATCH] loosen security a bit on mailgun's webhook --- app/controllers/webhooks_controller.rb | 59 +++++--------------- spec/controllers/webhooks_controller_spec.rb | 9 ++- 2 files changed, 18 insertions(+), 50 deletions(-) diff --git a/app/controllers/webhooks_controller.rb b/app/controllers/webhooks_controller.rb index 09acadc541f..f85d485da7f 100644 --- a/app/controllers/webhooks_controller.rb +++ b/app/controllers/webhooks_controller.rb @@ -13,41 +13,42 @@ class WebhooksController < ActionController::Base # prevent replay attack key = "mailgun_token_#{token}" return mailgun_failure unless $redis.setnx(key, 1) - $redis.expire(key, 8.hours) + $redis.expire(key, 10.minutes) # ensure timestamp isn't too far from current time timestamp = params.delete("timestamp") - return mailgun_failure if (Time.at(timestamp.to_i) - Time.now).abs > 1.hour.to_i + return mailgun_failure if (Time.at(timestamp.to_i) - Time.now).abs > 24.hours.to_i # check the signature return mailgun_failure unless mailgun_verify(timestamp, token, params["signature"]) - handled = false event = params.delete("event") + message_id = params.delete("message-id") # only handle soft bounces, because hard bounces are also handled # by the "dropped" event and we don't want to increase bounce score twice # for the same message if event == "bounced".freeze && params["error"]["4."] - handled = mailgun_process(params, Email::Receiver::SOFT_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::SOFT_BOUNCE_SCORE) elsif event == "dropped".freeze - handled = mailgun_process(params, Email::Receiver::HARD_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::HARD_BOUNCE_SCORE) end - handled ? mailgun_success : mailgun_failure + mailgun_success end def sendgrid events = params["_json"] || [params] events.each do |event| + message_id = (event["smtp-id"] || "").tr("<>", "") if event["event"] == "bounce".freeze if event["status"]["4."] - sendgrid_process(event, Email::Receiver::SOFT_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::SOFT_BOUNCE_SCORE) else - sendgrid_process(event, Email::Receiver::HARD_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::HARD_BOUNCE_SCORE) end elsif event["event"] == "dropped".freeze - sendgrid_process(event, Email::Receiver::HARD_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::HARD_BOUNCE_SCORE) end end @@ -57,11 +58,12 @@ class WebhooksController < ActionController::Base def mailjet events = params["_json"] || [params] events.each do |event| + message_id = event["CustomID"] if event["event"] == "bounce".freeze if event["hard_bounce"] - mailjet_process(event, Email::Receiver::HARD_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::HARD_BOUNCE_SCORE) else - mailjet_process(event, Email::Receiver::SOFT_BOUNCE_SCORE) + process_bounce(message_id, Email::Receiver::SOFT_BOUNCE_SCORE) end end end @@ -85,40 +87,7 @@ class WebhooksController < ActionController::Base signature == OpenSSL::HMAC.hexdigest(digest, SiteSetting.mailgun_api_key, data) end - def mailgun_process(params, bounce_score) - return false if params["message-headers"].blank? - - return_path_header = params["message-headers"].first { |h| h[0] == "Return-Path".freeze } - return false if return_path_header.blank? - - return_path = return_path_header[1] - return false if return_path.blank? - - bounce_key = return_path[/\+verp-(\h{32})@/, 1] - return false if bounce_key.blank? - - email_log = EmailLog.find_by(bounce_key: bounce_key) - return false if email_log.nil? - - email_log.update_columns(bounced: true) - Email::Receiver.update_bounce_score(email_log.user.email, bounce_score) - - true - end - - def sendgrid_process(event, bounce_score) - message_id = event["smtp-id"] - return if message_id.blank? - - email_log = EmailLog.find_by(message_id: message_id.tr("<>", "")) - return if email_log.nil? - - email_log.update_columns(bounced: true) - Email::Receiver.update_bounce_score(email_log.user.email, bounce_score) - end - - def mailjet_process(event, bounce_score) - message_id = event["CustomID"] + def process_bounce(message_id, bounce_score) return if message_id.blank? email_log = EmailLog.find_by(message_id: message_id) diff --git a/spec/controllers/webhooks_controller_spec.rb b/spec/controllers/webhooks_controller_spec.rb index 2b0eb57e840..5ff1de6f87e 100644 --- a/spec/controllers/webhooks_controller_spec.rb +++ b/spec/controllers/webhooks_controller_spec.rb @@ -9,18 +9,17 @@ describe WebhooksController do it "works" do SiteSetting.mailgun_api_key = "key-8221462f0c915af3f6f2e2df7aa5a493" - token = "705a8ccd2ce932be8e98c221fe701c1b4a0afcb8bbd57726de" + message_id = "12345@il.com" user = Fabricate(:user, email: email) - email_log = Fabricate(:email_log, user: user, bounce_key: SecureRandom.hex) - return_path = "foo+verp-#{email_log.bounce_key}@bar.com" + email_log = Fabricate(:email_log, user: user, message_id: message_id) WebhooksController.any_instance.expects(:mailgun_verify).returns(true) - post :mailgun, "token" => token, + post :mailgun, "token" => "705a8ccd2ce932be8e98c221fe701c1b4a0afcb8bbd57726de", "timestamp" => Time.now.to_i, "event" => "dropped", - "message-headers" => [["Return-Path", return_path]] + "message-id" => message_id expect(response).to be_success