Merge branch 'feat/drop-signatures-email-replies' into 'master'

Drop signatures in email replies

See merge request gitlab-org/gitlab!25389

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}"