Merge branch 'fix/normalize-signature-protocol-attachments' into 'master'

Normalize signature mime types when filtering attachments in emails

See merge request gitlab-org/gitlab!28865

changelogs/unreleased/fix-normalize-signature-protocol-attachments.yml

+---
+title: Normalize signature mime types when filtering attachments in emails
+merge_request: 28865
+author: Diego Louzán
+type: fixed

lib/gitlab/email/attachment_uploader.rb

@@ -39,15 +39,22 @@ module Gitlab
       # from the uploaded attachments
       def filter_signature_attachments(message)
         attachments = message.attachments
+        content_type = normalize_mime(message.content_type)
+        protocol = normalize_mime(message.content_type_parameters[:protocol])
 
-        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?
+        if content_type == 'multipart/signed' && protocol
+          attachments.delete_if { |attachment| protocol == normalize_mime(attachment.content_type) }
         end
 
         attachments
       end
+
+      # normalizes mime-type ignoring case and removing extra data
+      # also removes potential "x-" prefix from subtype, since some MUAs mix them
+      # e.g. "application/x-pkcs7-signature" with "application/pkcs7-signature"
+      def normalize_mime(content_type)
+        MIME::Type.simplified(content_type, remove_x_prefix: true)
+      end
     end
   end
 end

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -31,5 +31,20 @@ describe Gitlab::Email::AttachmentUploader do
         expect(image_link[:url]).to include('gitlab_logo.png')
       end
     end
+
+    context 'with a signed message with mixed protocol prefix' do
+      let(:message_raw) { fixture_file("emails/valid_reply_signed_smime_mixed_protocol_prefix.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