Merge remote-tracking branch 'origin/fix/cw-7469-twilio-media-credential-scope' into fix/cw-7469-twilio-media-safefetch

# Conflicts:
#	app/services/twilio/incoming_message_service.rb
#	spec/services/twilio/incoming_message_service_spec.rb

app/services/twilio/incoming_message_service.rb

@@ -1,8 +1,10 @@
-class Twilio::IncomingMessageService
+class Twilio::IncomingMessageService # rubocop:disable Metrics/ClassLength
   include ::FileTypeHelper
   include ::Twilio::WhatsappIdentifierHelper
   include ::Twilio::ReferralParamsHelper
 
+  TWILIO_DOMAIN = 'twilio.com'.freeze
+
   pattr_initialize [:params!]
 
   def perform
@@ -150,18 +152,50 @@ class Twilio::IncomingMessageService
   end
 
   def attach_single_file(media_url)
-    attachment_file = download_attachment_file(media_url)
-    return if attachment_file.blank?
+    fetch_media(media_url) do |io, filename, content_type|
+      @message.attachments.new(
+        account_id: @message.account_id,
+        file_type: file_type(content_type),
+        file: { io: io, filename: filename, content_type: content_type }
+      )
+    end
+  end
 
-    @message.attachments.new(
-      account_id: @message.account_id,
-      file_type: file_type(attachment_file.content_type),
-      file: {
-        io: attachment_file,
-        filename: attachment_file.original_filename,
-        content_type: attachment_file.content_type
-      }
-    )
+  # Twilio-hosted media is fetched directly with credentials (following the
+  # provider's CDN redirect). Any other URL from the payload is fetched through
+  # SafeFetch so it cannot reach internal addresses.
+  def fetch_media(media_url)
+    if twilio_hosted?(media_url)
+      file = download_attachment_file(media_url)
+      yield file, file.original_filename, file.content_type if file.present?
+    else
+      SafeFetch.fetch(media_url, validate_content_type: false) do |result|
+        yield persisted_copy(result.tempfile), result.original_filename, result.content_type
+      end
+    end
+    # Twilio originally skipped failed media and saved the message; keep that by
+    # catching download/transport failures (SafeFetch wraps some, not connection
+    # resets or closed streams) rather than aborting the whole message.
+  rescue SafeFetch::Error, SystemCallError, IOError => e
+    Rails.logger.info "Error downloading Twilio media: #{e.class}: Skipping"
+  end
+
+  # Only the provider's exact endpoint (https, default port, no userinfo) is fetched
+  # with credentials; anything else from the payload goes through SafeFetch instead.
+  def twilio_hosted?(media_url)
+    uri = URI.parse(media_url)
+    canonical_https?(uri) && twilio_host?(uri.host)
+  rescue URI::InvalidURIError
+    false
+  end
+
+  # SafeFetch closes its tempfile when the block returns, but the attachment is
+  # uploaded later on save!, so stream the contents into a tempfile that survives.
+  def persisted_copy(source)
+    tempfile = Tempfile.new('twilio-media', binmode: true)
+    IO.copy_stream(source, tempfile)
+    tempfile.rewind
+    tempfile
   end
 
   def download_attachment_file(media_url)
@@ -171,15 +205,20 @@ class Twilio::IncomingMessageService
   end
 
   def download_with_auth(media_url)
-    auth_credentials = if twilio_channel.api_key_sid.present?
-                         # When using api_key_sid, the auth token should be the api_secret_key
-                         [twilio_channel.api_key_sid, twilio_channel.auth_token]
-                       else
-                         # When using account_sid, the auth token is the account's auth token
-                         [twilio_channel.account_sid, twilio_channel.auth_token]
-                       end
+    # auth_token is the password for both api-key and account-sid auth
+    credentials = [twilio_channel.api_key_sid.presence || twilio_channel.account_sid, twilio_channel.auth_token]
+    Down.download(media_url, http_basic_authentication: credentials)
+  end
 
-    Down.download(media_url, http_basic_authentication: auth_credentials)
+  # Credentials go only to the provider's exact endpoint form: https, default port,
+  # no embedded userinfo.
+  def canonical_https?(uri)
+    uri.scheme == 'https' && uri.userinfo.nil? && uri.port == URI::HTTPS.default_port
+  end
+
+  def twilio_host?(host)
+    host = host&.downcase
+    host == TWILIO_DOMAIN || host&.end_with?(".#{TWILIO_DOMAIN}") || false
   end
 
   def handle_download_attachment_error(error, media_url)

spec/services/twilio/incoming_message_service_spec.rb

@@ -10,6 +10,16 @@ describe Twilio::IncomingMessageService do
   let(:contact_inbox) { create(:contact_inbox, source_id: '+12345', contact: contact, inbox: twilio_channel.inbox) }
   let!(:conversation) { create(:conversation, contact: contact, inbox: twilio_channel.inbox, contact_inbox: contact_inbox) }
 
+  # Non-Twilio media is fetched through SafeFetch (SsrfFilter), which resolves the
+  # host and rejects private addresses. Stub resolution to public IPs for test hosts.
+  before do
+    allow(Resolv).to receive(:getaddresses).and_call_original
+    allow(Resolv).to receive(:getaddresses).with('chatwoot-assets.local').and_return(['93.184.216.34'])
+    allow(Resolv).to receive(:getaddresses).with('other.example').and_return(['93.184.216.34'])
+    allow(Resolv).to receive(:getaddresses).with('api.twilio.com').and_return(['93.184.216.34'])
+    allow(Resolv).to receive(:getaddresses).with('internal.example').and_return(['169.254.169.254'])
+  end
+
   describe '#perform' do
     it 'creates a new message in existing conversation' do
       params = {
@@ -198,14 +208,7 @@ describe Twilio::IncomingMessageService do
     end
 
     context 'when there is an error downloading the attachment' do
-      before do
-        stub_request(:get, 'https://chatwoot-assets.local/sample.png')
-          .to_raise(Down::Error.new('Download error'))
-
-        stub_request(:get, 'https://chatwoot-assets.local/sample.png')
-          .to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
-      end
-
+      let(:twilio_media_url) { 'https://api.twilio.com/2010-04-01/Accounts/ACxxx/Media/MExx' }
       let(:params_with_attachment_error) do
         {
           SmsSid: 'SMxx',
@@ -215,10 +218,19 @@ describe Twilio::IncomingMessageService do
           Body: 'testing3',
           NumMedia: '1',
           MediaContentType0: 'image/jpeg',
-          MediaUrl0: 'https://chatwoot-assets.local/sample.png'
+          MediaUrl0: twilio_media_url
         }
       end
 
+      before do
+        stub_request(:get, twilio_media_url)
+          .with(basic_auth: ['ACxxx', twilio_channel.auth_token])
+          .to_raise(Down::Error.new('Download error'))
+
+        stub_request(:get, twilio_media_url)
+          .to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
+      end
+
       it 'retries downloading the attachment without a token after an error' do
         expect do
           described_class.new(params: params_with_attachment_error).perform
@@ -230,6 +242,73 @@ describe Twilio::IncomingMessageService do
       end
     end
 
+    context 'when downloading media attachments' do
+      let(:twilio_media_url) { 'https://api.twilio.com/2010-04-01/Accounts/ACxxx/Messages/MMxx/Media/MExx' }
+
+      let(:media_params) do
+        {
+          SmsSid: 'SMxx',
+          From: '+12345',
+          AccountSid: 'ACxxx',
+          MessagingServiceSid: twilio_channel.messaging_service_sid,
+          Body: 'media',
+          NumMedia: '1',
+          MediaContentType0: 'image/png'
+        }
+      end
+
+      it 'attaches Twilio credentials for media on a Twilio host' do
+        stub_request(:get, twilio_media_url).to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
+
+        described_class.new(params: media_params.merge(MediaUrl0: twilio_media_url)).perform
+
+        expect(a_request(:get, twilio_media_url).with(basic_auth: ['ACxxx', twilio_channel.auth_token])).to have_been_made
+      end
+
+      it 'downloads media from non-Twilio hosts without credentials' do
+        other_url = 'http://other.example/file.png'
+        stub_request(:get, other_url).to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
+
+        described_class.new(params: media_params.merge(MediaUrl0: other_url)).perform
+
+        expect(a_request(:get, other_url).with(basic_auth: ['ACxxx', twilio_channel.auth_token])).not_to have_been_made
+        expect(conversation.reload.messages.last.attachments.count).to eq(1)
+      end
+
+      it 'does not fetch media that resolves to an internal address' do
+        described_class.new(params: media_params.merge(MediaUrl0: 'http://internal.example/file.png')).perform
+
+        expect(conversation.reload.messages.last.attachments.count).to eq(0)
+      end
+
+      it 'does not attach credentials to a plaintext Twilio url' do
+        http_url = 'http://api.twilio.com/2010-04-01/Accounts/ACxxx/Messages/MMxx/Media/MExx'
+        stub_request(:get, http_url).to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
+
+        described_class.new(params: media_params.merge(MediaUrl0: http_url)).perform
+
+        expect(a_request(:get, http_url).with(basic_auth: ['ACxxx', twilio_channel.auth_token])).not_to have_been_made
+      end
+
+      it 'saves the message when a non-Twilio media download fails at the transport level' do
+        stub_request(:get, 'http://other.example/file.png').to_raise(Errno::ECONNREFUSED)
+
+        described_class.new(params: media_params.merge(MediaUrl0: 'http://other.example/file.png')).perform
+
+        expect(conversation.reload.messages.last).to be_present
+        expect(conversation.reload.messages.last.attachments.count).to eq(0)
+      end
+
+      it 'does not attach credentials to a Twilio url on a non-default port' do
+        url = 'https://api.twilio.com:8443/2010-04-01/Accounts/ACxxx/Messages/MMxx/Media/MExx'
+        stub_request(:get, url).to_return(status: 200, body: 'image data', headers: { 'Content-Type' => 'image/png' })
+
+        described_class.new(params: media_params.merge(MediaUrl0: url)).perform
+
+        expect(a_request(:get, url).with(basic_auth: ['ACxxx', twilio_channel.auth_token])).not_to have_been_made
+      end
+    end
+
     context 'when a message with multiple attachments is received' do
       before do
         stub_request(:get, 'https://chatwoot-assets.local/sample.png')