Drop signatures in email replies

Since signature validation is not performed in incoming emails, it's
cleaner to just remove them from the mail attachments before uploading
them. This removes noisy email signature objects included as
attachments in comments.

changelogs/unreleased/feat-drop-signatures-email-replies.yml

+---
+title: Drop signatures in email replies
+merge_request: 25389
+author: Diego Louzán
+type: changed

lib/gitlab/email/attachment_uploader.rb

@@ -12,7 +12,7 @@ module Gitlab
       def execute(upload_parent:, uploader_class:)
         attachments = []
 
-        message.attachments.each do |attachment|
+        filter_signature_attachments(message).each do |attachment|
           tmp = Tempfile.new("gitlab-email-attachment")
           begin
             File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
@@ -32,6 +32,22 @@ module Gitlab
 
         attachments
       end
+
+      private
+
+      # If this is a signed message (e.g. S/MIME or PGP), remove the signature
+      # from the uploaded attachments
+      def filter_signature_attachments(message)
+        attachments = message.attachments
+
+        if message.content_type&.starts_with?('multipart/signed')
+          signature_protocol = message.content_type_parameters[:protocol]
+
+          attachments.delete_if { |attachment| attachment.content_type.starts_with?(signature_protocol) } if signature_protocol.present?
+        end
+
+        attachments
+      end
     end
   end
 end

spec/fixtures/emails/.gitattributes

+# Do not mangle line endings or signature will be invalid
+valid_reply_signed_smime.eml eol=crlf
\ No newline at end of file

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -16,5 +16,20 @@ describe Gitlab::Email::AttachmentUploader do
       expect(link[:alt]).to eq("bricks")
       expect(link[:url]).to include("bricks.png")
     end
+
+    context 'with a signed message' do
+      let(:message_raw) { fixture_file("emails/valid_reply_signed_smime.eml") }
+
+      it 'uploads all attachments except the signature' do
+        links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
+
+        expect(links).not_to include(a_hash_including(alt: 'smime.p7s'))
+
+        image_link = links.first
+        expect(image_link).not_to be_nil
+        expect(image_link[:alt]).to eq('gitlab_logo')
+        expect(image_link[:url]).to include('gitlab_logo.png')
+      end
+    end
   end
 end

spec/lib/gitlab/email/handler_spec.rb

@@ -3,17 +3,23 @@
 require 'spec_helper'
 
 describe Gitlab::Email::Handler do
+  let(:email) { Mail.new { body 'email' } }
+
   describe '.for' do
     it 'picks issue handler if there is no merge request prefix' do
-      expect(described_class.for('email', 'project+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateIssueHandler)
+      expect(described_class.for(email, 'project+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateIssueHandler)
     end
 
     it 'picks merge request handler if there is merge request key' do
-      expect(described_class.for('email', 'project+merge-request+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateMergeRequestHandler)
+      expect(described_class.for(email, 'project+merge-request+key')).to be_an_instance_of(Gitlab::Email::Handler::CreateMergeRequestHandler)
     end
 
     it 'returns nil if no handler is found' do
-      expect(described_class.for('email', '')).to be_nil
+      expect(described_class.for(email, '')).to be_nil
+    end
+
+    it 'returns nil if provided email is nil' do
+      expect(described_class.for(nil, '')).to be_nil
     end
   end
 
@@ -25,7 +31,7 @@ describe Gitlab::Email::Handler do
 
     it 'picks each handler at least once' do
       matched_handlers = addresses.map do |address|
-        described_class.for('email', address).class
+        described_class.for(email, address).class
       end
 
       expect(matched_handlers.uniq).to match_array(ce_handlers)
@@ -34,7 +40,7 @@ describe Gitlab::Email::Handler do
     it 'can pick exactly one handler for each address' do
       addresses.each do |address|
         matched_handlers = ce_handlers.select do |handler|
-          handler.new('email', address).can_handle?
+          handler.new(email, address).can_handle?
         end
 
         expect(matched_handlers.count).to eq(1), "#{address} matches #{matched_handlers.count} handlers: #{matched_handlers}"