From 6928f14a762e5ae40b74d5901fb7685c46ac4ee2 Mon Sep 17 00:00:00 2001 From: Gabor Barany <23645175+gbarany@users.noreply.github.com> Date: Tue, 21 Apr 2026 10:44:25 +0100 Subject: [PATCH] fix: Validate Twilio webhook signatures (X-Twilio-Signature) (#13638) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #13619 ## Summary - Add `TwilioSignatureVerifyConcern` that validates the `X-Twilio-Signature` header using `Twilio::Security::RequestValidator` (already bundled via `twilio-ruby` gem) - Include the concern in `Twilio::CallbackController` and `Twilio::DeliveryStatusController` — both endpoints were previously accepting requests from any source with no authentication - Channels using API key authentication (`api_key_sid` present) skip validation with a warning log, since Twilio signs with the account auth token which isn't stored for those channels ## How it works 1. `before_action` looks up the `Channel::TwilioSms` from request params (`MessagingServiceSid` or `AccountSid` + phone number) 2. Validates the HMAC-SHA1 signature using the channel's auth token 3. Returns `403 Forbidden` if signature is invalid, missing, or channel not found 4. Handles reverse proxy URL reconstruction via `X-Forwarded-Proto` header Follows the same pattern used by `Webhooks::ShopifyController` and `Webhooks::TiktokController`. ## Test plan - [x] Valid signature → 204 No Content, job enqueued - [x] Invalid signature → 403 Forbidden, job not enqueued - [x] Missing signature header → 403 Forbidden - [x] Channel not found → 403 Forbidden - [x] API key channel → skips validation, job enqueued (with warning log) - [x] MessagingServiceSid lookup → validates and enqueues - [x] All existing Twilio service/job specs pass (99 examples, 0 failures) --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: Muhsin Keloth Co-authored-by: Sojan Jose --- .../twilio_signature_verify_concern.rb | 88 ++++++++++ app/controllers/twilio/callback_controller.rb | 2 + .../twilio/delivery_status_controller.rb | 6 + .../twilio/callbacks_controller_spec.rb | 151 +++++++++++++++++- .../twilio/delivery_status_controller_spec.rb | 125 ++++++++++++++- 5 files changed, 356 insertions(+), 16 deletions(-) create mode 100644 app/controllers/concerns/twilio_signature_verify_concern.rb diff --git a/app/controllers/concerns/twilio_signature_verify_concern.rb b/app/controllers/concerns/twilio_signature_verify_concern.rb new file mode 100644 index 000000000..b7a4754a4 --- /dev/null +++ b/app/controllers/concerns/twilio_signature_verify_concern.rb @@ -0,0 +1,88 @@ +module TwilioSignatureVerifyConcern + extend ActiveSupport::Concern + + included do + before_action :verify_twilio_signature! + end + + private + + def verify_twilio_signature! + channel = find_twilio_channel + return log_and_reject_missing_channel if channel.blank? + return if channel.api_key_sid.present? && log_api_key_skip(channel) + + head :forbidden unless valid_signature?(channel) + end + + def log_and_reject_missing_channel + Rails.logger.warn( + '[TWILIO] Channel not found for webhook ' \ + "account_sid=#{params[:AccountSid]} messaging_service_sid=#{params[:MessagingServiceSid]} " \ + "to=#{params[:To]} from=#{params[:From]}" + ) + head :forbidden + end + + def log_api_key_skip(channel) + Rails.logger.warn( + '[TWILIO] Signature validation skipped: channel uses API key authentication. ' \ + "account_sid=#{params[:AccountSid]} channel_id=#{channel.id}" + ) + end + + def valid_signature?(channel) + signature = request.headers['X-Twilio-Signature'] + if signature.blank? + Rails.logger.warn("[TWILIO] Missing X-Twilio-Signature header account_sid=#{params[:AccountSid]}") + return false + end + + validator = Twilio::Security::RequestValidator.new(channel.auth_token) + request_url = reconstruct_url + return true if validator.validate(request_url, request.request_parameters, signature) + + Rails.logger.warn( + '[TWILIO] Signature validation failed ' \ + "account_sid=#{params[:AccountSid]} channel_id=#{channel.id} url=#{request_url} ip=#{request.remote_ip}" + ) + false + end + + def find_twilio_channel + if params[:MessagingServiceSid].present? + channel = ::Channel::TwilioSms.find_by(messaging_service_sid: params[:MessagingServiceSid]) + return channel if channel.present? && (params[:AccountSid].blank? || channel.account_sid == params[:AccountSid]) + + return nil + end + return if params[:AccountSid].blank? + + find_channel_by_phone_number + end + + def find_channel_by_phone_number + channel_lookup_phone_numbers.each do |phone| + channel = ::Channel::TwilioSms.find_by(account_sid: params[:AccountSid], phone_number: phone) + return channel if channel + end + nil + end + + def channel_lookup_phone_numbers + [params[:To], params[:From]].compact_blank + end + + def reconstruct_url + url = request.original_url + url = url.sub('http://', 'https://') if url.start_with?('http://') && https_request? + url + end + + def https_request? + return true if request.ssl? + + forwarded_proto = request.headers['X-Forwarded-Proto'].to_s.split(',').map(&:strip).find(&:present?) + forwarded_proto&.casecmp?('https') + end +end diff --git a/app/controllers/twilio/callback_controller.rb b/app/controllers/twilio/callback_controller.rb index d607ba151..9b42cd034 100644 --- a/app/controllers/twilio/callback_controller.rb +++ b/app/controllers/twilio/callback_controller.rb @@ -1,4 +1,6 @@ class Twilio::CallbackController < ApplicationController + include TwilioSignatureVerifyConcern + def create Webhooks::TwilioEventsJob.perform_later(permitted_params.to_unsafe_hash) diff --git a/app/controllers/twilio/delivery_status_controller.rb b/app/controllers/twilio/delivery_status_controller.rb index 1c756a1c2..8e846a737 100644 --- a/app/controllers/twilio/delivery_status_controller.rb +++ b/app/controllers/twilio/delivery_status_controller.rb @@ -1,4 +1,6 @@ class Twilio::DeliveryStatusController < ApplicationController + include TwilioSignatureVerifyConcern + def create Webhooks::TwilioDeliveryStatusJob.perform_later(permitted_params.to_unsafe_hash) @@ -18,4 +20,8 @@ class Twilio::DeliveryStatusController < ApplicationController :ErrorMessage ) end + + def channel_lookup_phone_numbers + [params[:From]].compact_blank + end end diff --git a/spec/controllers/twilio/callbacks_controller_spec.rb b/spec/controllers/twilio/callbacks_controller_spec.rb index d16acf229..09b558096 100644 --- a/spec/controllers/twilio/callbacks_controller_spec.rb +++ b/spec/controllers/twilio/callbacks_controller_spec.rb @@ -4,25 +4,160 @@ RSpec.describe 'Twilio::CallbacksController', type: :request do include Rails.application.routes.url_helpers describe 'POST /twilio/callback' do + let(:account) { create(:account) } + let(:twilio_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } let(:params) do { 'From' => '+1234567890', - 'To' => '+0987654321', + 'To' => twilio_channel.phone_number, 'Body' => 'Test message', 'AccountSid' => 'AC123', 'SmsSid' => 'SM123' } end - it 'enqueues the Twilio events job' do - expect do - post twilio_callback_index_url, params: params - end.to have_enqueued_job(Webhooks::TwilioEventsJob).with(params) + def post_with_signature(url, params:) + validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) + signature = validator.build_signature_for(url, params) + post url, params: params, headers: { 'X-Twilio-Signature' => signature } end - it 'returns no content status' do - post twilio_callback_index_url, params: params - expect(response).to have_http_status(:no_content) + context 'with valid signature' do + it 'enqueues the Twilio events job' do + url = twilio_callback_index_url + expect do + post_with_signature(url, params: params) + end.to have_enqueued_job(Webhooks::TwilioEventsJob) + end + + it 'returns no content status' do + url = twilio_callback_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:no_content) + end + end + + context 'with invalid signature' do + it 'returns forbidden status' do + post twilio_callback_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } + expect(response).to have_http_status(:forbidden) + end + + it 'does not enqueue the job' do + expect do + post twilio_callback_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } + end.not_to have_enqueued_job(Webhooks::TwilioEventsJob) + end + end + + context 'with missing signature header' do + it 'returns forbidden status' do + post twilio_callback_index_url, params: params + expect(response).to have_http_status(:forbidden) + end + end + + context 'when channel is not found' do + it 'returns forbidden status' do + post twilio_callback_index_url, params: params.merge('AccountSid' => 'UNKNOWN', 'To' => '+0000000000') + expect(response).to have_http_status(:forbidden) + end + end + + context 'when channel uses API key authentication' do + let(:twilio_channel) do + create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') + end + + it 'skips signature validation and enqueues the job' do + expect do + post twilio_callback_index_url, params: params + end.to have_enqueued_job(Webhooks::TwilioEventsJob) + end + end + + context 'when behind a reverse proxy with X-Forwarded-Proto' do + it 'validates signature against the HTTPS URL' do + http_url = twilio_callback_index_url + https_url = http_url.sub('http://', 'https://') + validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) + signature = validator.build_signature_for(https_url, params) + post http_url, params: params, headers: { + 'X-Twilio-Signature' => signature, + 'X-Forwarded-Proto' => 'https' + } + expect(response).to have_http_status(:no_content) + end + + it 'validates signature when forwarded proto is a comma-separated chain' do + http_url = twilio_callback_index_url + https_url = http_url.sub('http://', 'https://') + validator = Twilio::Security::RequestValidator.new(twilio_channel.auth_token) + signature = validator.build_signature_for(https_url, params) + post http_url, params: params, headers: { + 'X-Twilio-Signature' => signature, + 'X-Forwarded-Proto' => 'https,http' + } + expect(response).to have_http_status(:no_content) + end + end + + context 'with MessagingServiceSid lookup' do + let(:twilio_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC123') } + let(:params) do + { + 'From' => '+1234567890', + 'Body' => 'Test message', + 'AccountSid' => 'AC123', + 'SmsSid' => 'SM123', + 'MessagingServiceSid' => twilio_channel.messaging_service_sid + } + end + + it 'validates and enqueues the job' do + url = twilio_callback_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:no_content) + end + end + + context 'when MessagingServiceSid is present but does not match a channel' do + let(:params) do + { + 'From' => '+1234567890', + 'To' => twilio_channel.phone_number, + 'Body' => 'Test message', + 'AccountSid' => 'AC123', + 'SmsSid' => 'SM123', + 'MessagingServiceSid' => 'MG_UNKNOWN' + } + end + + it 'returns forbidden without falling back to phone number lookup' do + url = twilio_callback_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:forbidden) + end + end + + context 'when MessagingServiceSid matches a channel but AccountSid does not' do + let(:other_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC_OTHER') } + let(:params) do + { + 'From' => '+1234567890', + 'To' => twilio_channel.phone_number, + 'Body' => 'Test message', + 'AccountSid' => 'AC123', + 'SmsSid' => 'SM123', + 'MessagingServiceSid' => other_channel.messaging_service_sid + } + end + + it 'returns forbidden without falling back to phone number lookup' do + url = twilio_callback_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:forbidden) + end end end end diff --git a/spec/controllers/twilio/delivery_status_controller_spec.rb b/spec/controllers/twilio/delivery_status_controller_spec.rb index fc21f8f94..dc99fdf23 100644 --- a/spec/controllers/twilio/delivery_status_controller_spec.rb +++ b/spec/controllers/twilio/delivery_status_controller_spec.rb @@ -4,23 +4,132 @@ RSpec.describe 'Twilio::DeliveryStatusController', type: :request do include Rails.application.routes.url_helpers describe 'POST /twilio/delivery_status' do + let(:account) { create(:account) } + let(:twilio_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } let(:params) do { 'MessageSid' => 'SM123', 'MessageStatus' => 'delivered', - 'AccountSid' => 'AC123' + 'AccountSid' => 'AC123', + 'From' => twilio_channel.phone_number } end - it 'enqueues the Twilio delivery status job' do - expect do - post twilio_delivery_status_index_url, params: params - end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob).with(params) + def post_with_signature(url, params:, channel: twilio_channel) + validator = Twilio::Security::RequestValidator.new(channel.auth_token) + signature = validator.build_signature_for(url, params) + post url, params: params, headers: { 'X-Twilio-Signature' => signature } end - it 'returns no content status' do - post twilio_delivery_status_index_url, params: params - expect(response).to have_http_status(:no_content) + context 'with valid signature' do + it 'enqueues the delivery status job' do + url = twilio_delivery_status_index_url + expect do + post_with_signature(url, params: params) + end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) + end + + it 'returns no content status' do + url = twilio_delivery_status_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:no_content) + end + end + + context 'with invalid signature' do + it 'returns forbidden status' do + post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } + expect(response).to have_http_status(:forbidden) + end + + it 'does not enqueue the job' do + expect do + post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } + end.not_to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) + end + end + + context 'with missing signature header' do + it 'returns forbidden status' do + post twilio_delivery_status_index_url, params: params + expect(response).to have_http_status(:forbidden) + end + end + + context 'when channel uses API key authentication' do + let(:twilio_channel) do + create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') + end + + it 'skips signature validation and enqueues the job' do + expect do + post twilio_delivery_status_index_url, params: params + end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) + end + end + + context 'with MessagingServiceSid lookup' do + let(:twilio_channel) { create(:channel_twilio_sms, account: account, account_sid: 'AC123') } + let(:params) do + { + 'MessageSid' => 'SM123', + 'MessageStatus' => 'delivered', + 'AccountSid' => 'AC123', + 'MessagingServiceSid' => twilio_channel.messaging_service_sid + } + end + + it 'validates and enqueues the job' do + url = twilio_delivery_status_index_url + post_with_signature(url, params: params) + expect(response).to have_http_status(:no_content) + end + end + + context 'when To does not map to a channel but From does' do + let(:params) do + { + 'MessageSid' => 'SM123', + 'MessageStatus' => 'delivered', + 'AccountSid' => 'AC123', + 'To' => '+19999999999', + 'From' => twilio_channel.phone_number + } + end + + it 'falls back to From lookup and enqueues the delivery status job' do + url = twilio_delivery_status_index_url + + expect do + post_with_signature(url, params: params) + end.to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) + + expect(response).to have_http_status(:no_content) + end + end + + context 'when To maps to an API-key channel but From maps to a different channel' do + let!(:api_key_channel) do + create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123', api_key_sid: 'SK123') + end + let!(:from_channel) { create(:channel_twilio_sms, :with_phone_number, account: account, account_sid: 'AC123') } + let(:params) do + { + 'MessageSid' => 'SM123', + 'MessageStatus' => 'delivered', + 'AccountSid' => 'AC123', + 'To' => api_key_channel.phone_number, + 'From' => from_channel.phone_number + } + end + + it 'rejects invalid signatures instead of skipping verification' do + expect do + post twilio_delivery_status_index_url, params: params, headers: { 'X-Twilio-Signature' => 'invalid' } + end.not_to have_enqueued_job(Webhooks::TwilioDeliveryStatusJob) + + expect(response).to have_http_status(:forbidden) + end end end end