Merge branch 'fix/quoted-printable-unicode-in-mails' into 'master'

fix: quoted-printable unicode and newlines in mails

Closes #197386

See merge request gitlab-org/gitlab!24153

Gemfile

@@ -488,3 +488,8 @@ gem 'liquid', '~> 4.0'
 gem 'lru_redux'
 
 gem 'erubi', '~> 1.9.0'
+
+# Locked as long as quoted-printable encoding issues are not resolved
+# Monkey-patched in `config/initializers/mail_encoding_patch.rb`
+# See https://gitlab.com/gitlab-org/gitlab/issues/197386
+gem 'mail', '= 2.7.1'

Gemfile.lock

@@ -1283,6 +1283,7 @@ DEPENDENCIES
   lograge (~> 0.5)
   loofah (~> 2.2)
   lru_redux
+  mail (= 2.7.1)
   mail_room (~> 0.10.0)
   marginalia (~> 1.8.0)
   memory_profiler (~> 0.9)

changelogs/unreleased/fix-quoted-printable-unicode-in-mails.yml

+---
+title: Fix quoted-printable encoding for unicode and newlines in mails
+merge_request: 24153
+author: Diego Louzán
+type: fixed

config/initializers/mail_encoding_patch.rb

+# Monkey patch mail 2.7.1 to fix quoted-printable issues with newlines
+# The issues upstream invalidate SMIME signatures under some conditions
+# This was working properly in 2.6.6
+#
+# See https://gitlab.com/gitlab-org/gitlab/issues/197386
+# See https://github.com/mikel/mail/issues/1190
+
+module Mail
+  module Encodings
+    # PATCH
+    # This reverts https://github.com/mikel/mail/pull/1113, which solves some
+    # encoding issues with binary attachments encoded in quoted-printable, but
+    # unfortunately breaks re-encoding of messages
+    class QuotedPrintable < SevenBit
+      def self.decode(str)
+        ::Mail::Utilities.to_lf str.gsub(/(?:=0D=0A|=0D|=0A)\r\n/, "\r\n").unpack1("M*")
+      end
+
+      def self.encode(str)
+        ::Mail::Utilities.to_crlf([::Mail::Utilities.to_lf(str)].pack("M"))
+      end
+    end
+  end
+
+  class Body
+    def encoded(transfer_encoding = nil, charset = nil)
+      # PATCH
+      # Use provided parameter charset (from parent Message) if not nil,
+      # otherwise use own self.charset
+      # Required because the Message potentially has on its headers the charset
+      # that needs to be used (e.g. 'Content-Type: text/plain; charset=UTF-8')
+      charset = self.charset if charset.nil?
+
+      if multipart?
+        self.sort_parts!
+        encoded_parts = parts.map { |p| p.encoded }
+        ([preamble] + encoded_parts).join(crlf_boundary) + end_boundary + epilogue.to_s
+      else
+        dec = Mail::Encodings.get_encoding(encoding)
+        enc = if Utilities.blank?(transfer_encoding)
+                dec
+              else
+                negotiate_best_encoding(transfer_encoding)
+              end
+
+        if dec.nil?
+          # Cannot decode, so skip normalization
+          raw_source
+        else
+          # Decode then encode to normalize and allow transforming
+          # from base64 to Q-P and vice versa
+          decoded = dec.decode(raw_source)
+
+          if defined?(Encoding) && charset && charset != "US-ASCII"
+            # PATCH
+            # We need to force the encoding: in the case of quoted-printable
+            # this will throw an exception otherwise, because `decoded` will have
+            # an encoding of BINARY (or its equivalent ASCII-8BIT),
+            # coming from QuotedPrintable#decode, and inside it from String#unpack1
+            decoded = decoded.force_encoding(charset)
+            decoded.force_encoding('BINARY') unless Encoding.find(charset).ascii_compatible?
+          end
+
+          enc.encode(decoded)
+        end
+      end
+    end
+  end
+
+  class Message
+    def encoded
+      ready_to_send!
+      buffer = header.encoded
+      buffer << "\r\n"
+      # PATCH
+      # Pass the Message charset down to the contained Body, the headers
+      # potentially contain the charset needed to be applied
+      buffer << body.encoded(content_transfer_encoding, charset)
+      buffer
+    end
+  end
+end

lib/gitlab/email/hook/smime_signature_interceptor.rb

@@ -11,6 +11,7 @@ module Gitlab
               cert: certificate.cert,
               key: certificate.key,
               data: message.encoded)
+
             signed_email = Mail.new(signed_message)
 
             overwrite_body(message, signed_email)

spec/initializers/mail_encoding_patch_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+require 'mail'
+require_relative '../../config/initializers/mail_encoding_patch.rb'
+
+describe 'Mail quoted-printable transfer encoding patch and Unicode characters' do
+  shared_examples 'email encoding' do |email|
+    it 'enclosing in a new object does not change the encoded original' do
+      new_email = Mail.new(email)
+
+      expect(new_email.subject).to eq(email.subject)
+      expect(new_email.from).to eq(email.from)
+      expect(new_email.to).to eq(email.to)
+      expect(new_email.content_type).to eq(email.content_type)
+      expect(new_email.content_transfer_encoding).to eq(email.content_transfer_encoding)
+
+      expect(new_email.encoded).to eq(email.encoded)
+    end
+  end
+
+  context 'with a text email' do
+    context 'with a body that encodes to exactly 74 characters (final newline)' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/plain; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "-123456789-123456789-123456789-123456789-123456789-123456789-123456789-1\n"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+
+    context 'with a body that encodes to exactly 74 characters (no final newline)' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/plain; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "-123456789-123456789-123456789-123456789-123456789-123456789-123456789-12"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+
+    context 'with a body that encodes to exactly 75 characters' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/plain; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "-123456789-123456789-123456789-123456789-123456789-123456789-123456789-12\n"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+  end
+
+  context 'with an html email' do
+    context 'with a body that encodes to exactly 74 characters (final newline)' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/html; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "<p>-123456789-123456789-123456789-123456789-123456789-123456789-1234</p>\n"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+
+    context 'with a body that encodes to exactly 74 characters (no final newline)' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/html; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "<p>-123456789-123456789-123456789-123456789-123456789-123456789-12345</p>"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+
+    context 'with a body that encodes to exactly 75 characters' do
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+        content_type 'text/html; charset=UTF-8'
+        content_transfer_encoding 'quoted-printable'
+        body "<p>-123456789-123456789-123456789-123456789-123456789-123456789-12345</p>\n"
+      end
+
+      it_behaves_like 'email encoding', email
+    end
+  end
+
+  context 'a multipart email' do
+    email = Mail.new do
+      to 'jane.doe@example.com'
+      from 'John Dóe <john.doe@example.com>'
+      subject 'Encoding tést'
+    end
+
+    text_part = Mail::Part.new do
+      content_type 'text/plain; charset=UTF-8'
+      content_transfer_encoding 'quoted-printable'
+      body "\r\n\r\n@john.doe, now known as John Dóe has accepted your invitation to join the Administrator / htmltest project.\r\n\r\nhttp://169.254.169.254:3000/root/htmltest\r\n\r\n-- \r\nYou're receiving this email because of your account on 169.254.169.254.\r\n\r\n\r\n\r\n"
+    end
+
+    html_part = Mail::Part.new do
+      content_type 'text/html; charset=UTF-8'
+      content_transfer_encoding 'quoted-printable'
+      body "\r\n\r\n@john.doe, now known as John Dóe has accepted your invitation to join the Administrator / htmltest project.\r\n\r\nhttp://169.254.169.254:3000/root/htmltest\r\n\r\n-- \r\nYou're receiving this email because of your account on 169.254.169.254.\r\n\r\n\r\n\r\n"
+    end
+
+    email.text_part = text_part
+    email.html_part = html_part
+
+    it_behaves_like 'email encoding', email
+  end
+
+  context 'with non UTF-8 charset' do
+    email = Mail.new do
+      to 'jane.doe@example.com'
+      from 'John Dóe <john.doe@example.com>'
+      subject 'Encoding tést'
+      content_type 'text/plain; charset=windows-1251'
+      content_transfer_encoding 'quoted-printable'
+      body "This line is very long and will be put in multiple quoted-printable lines. Some Russian character: Д\n\n\n".encode('windows-1251')
+    end
+
+    it_behaves_like 'email encoding', email
+
+    it 'can be decoded back' do
+      expect(Mail.new(email).body.decoded.dup.force_encoding('windows-1251').encode('utf-8')).to include('Some Russian character: Д')
+    end
+  end
+
+  context 'with binary content' do
+    context 'can be encoded with \'base64\' content-transfer-encoding' do
+      image = File.binread('spec/fixtures/rails_sample.jpg')
+
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+      end
+
+      part = Mail::Part.new
+      part.body = [image].pack('m')
+      part.content_type = 'image/jpg'
+      part.content_transfer_encoding = 'base64'
+
+      email.parts << part
+
+      it_behaves_like 'email encoding', email
+
+      it 'binary contents are not modified' do
+        expect(email.parts.first.decoded).to eq(image)
+
+        # Enclosing in a new Mail object does not corrupt encoded data
+        expect(Mail.new(email).parts.first.decoded).to eq(image)
+      end
+    end
+
+    context 'encoding fails with \'quoted-printable\' content-transfer-encoding' do
+      image = File.binread('spec/fixtures/rails_sample.jpg')
+
+      email = Mail.new do
+        to 'jane.doe@example.com'
+        from 'John Dóe <john.doe@example.com>'
+        subject 'Encoding tést'
+      end
+
+      part = Mail::Part.new
+      part.body = [image].pack('M*')
+      part.content_type = 'image/jpg'
+      part.content_transfer_encoding = 'quoted-printable'
+
+      email.parts << part
+
+      # The Mail patch in `config/initializers/mail_encoding_patch.rb` fixes
+      # encoding of non-binary content. The failure below is expected since we
+      # reverted some upstream changes in order to properly support SMIME signatures
+      # See https://gitlab.com/gitlab-org/gitlab/issues/197386
+      it 'content cannot be decoded back' do
+        # Headers are ok
+        expect(email.subject).to eq(email.subject)
+        expect(email.from).to eq(email.from)
+        expect(email.to).to eq(email.to)
+        expect(email.content_type).to eq(email.content_type)
+        expect(email.content_transfer_encoding).to eq(email.content_transfer_encoding)
+
+        # Content cannot be recovered
+        expect(email.parts.first.decoded).not_to eq(image)
+      end
+    end
+  end
+end

spec/lib/gitlab/email/hook/smime_signature_interceptor_spec.rb

@@ -20,8 +20,14 @@ describe Gitlab::Email::Hook::SmimeSignatureInterceptor do
     Gitlab::Email::Smime::Certificate.new(@cert[:key], @cert[:cert])
   end
 
+  let(:mail_body) { "signed hello with Unicode €áø and\r\n newlines\r\n" }
+
   let(:mail) do
-    ActionMailer::Base.mail(to: 'test@example.com', from: 'info@example.com', body: 'signed hello')
+    ActionMailer::Base.mail(to: 'test@example.com',
+                            from: 'info@example.com',
+                            content_transfer_encoding: 'quoted-printable',
+                            content_type: 'text/plain; charset=UTF-8',
+                            body: mail_body)
   end
 
   before do
@@ -46,9 +52,16 @@ describe Gitlab::Email::Hook::SmimeSignatureInterceptor do
       ca_cert: root_certificate.cert,
       signed_data: mail.encoded)
 
+    # re-verify signature from a new Mail object content
+    # See https://gitlab.com/gitlab-org/gitlab/issues/197386
+    Gitlab::Email::Smime::Signer.verify_signature(
+      cert: certificate.cert,
+      ca_cert: root_certificate.cert,
+      signed_data: Mail.new(mail).encoded)
+
     # envelope in a Mail object and obtain the body
     decoded_mail = Mail.new(p7enc.data)
 
-    expect(decoded_mail.body.encoded).to eq('signed hello')
+    expect(decoded_mail.body.decoded.dup.force_encoding(decoded_mail.charset)).to eq(mail_body)
   end
 end