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