From 9a20000a8feeeae2e1a2bff4397071ecf31ba08a Mon Sep 17 00:00:00 2001
From: Marc Saleiko <msaleiko@gitlab.com>
Date: Wed, 30 Aug 2023 16:51:49 +0000
Subject: [PATCH] Prevents Mail::SMTP monkey patch from overriding defaults
When changing the delivery method settings
of a Mail::Message the monkey patch constructor
would override the defaults of Mail::SMTP with the
configured settings. Now we ensure we use fresh
defaults for every new delivery method settings.
Changelog: fixed
---
config/initializers/mail_starttls_patch.rb | 2 +-
spec/initializers/mail_starttls_patch_spec.rb | 31 +++++++++++++++++++
spec/mailers/emails/service_desk_spec.rb | 19 ++++++++++++
3 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/config/initializers/mail_starttls_patch.rb b/config/initializers/mail_starttls_patch.rb
index c4fe102edfb72..51d454238cabe 100644
--- a/config/initializers/mail_starttls_patch.rb
+++ b/config/initializers/mail_starttls_patch.rb
@@ -9,7 +9,7 @@
module Mail
class SMTP
def initialize(values)
- self.settings = DEFAULTS
+ self.settings = DEFAULTS.dup
settings[:enable_starttls_auto] = nil
settings.merge!(values)
end
diff --git a/spec/initializers/mail_starttls_patch_spec.rb b/spec/initializers/mail_starttls_patch_spec.rb
index 99c8edddd125d..0ceeb78a5b82b 100644
--- a/spec/initializers/mail_starttls_patch_spec.rb
+++ b/spec/initializers/mail_starttls_patch_spec.rb
@@ -17,6 +17,37 @@
end
end
+ # As long as this monkey patch exists and overrides the constructor
+ # we should test that the defaults of Mail::SMTP are not overriden.
+ #
+ # @see issue https://gitlab.com/gitlab-org/gitlab/-/issues/423268
+ # @see incident https://gitlab.com/gitlab-com/gl-infra/production/-/issues/16223
+ it 'does not override default constants values' do
+ expected_settings = Mail::SMTP.new({}).settings.dup
+
+ Mail.new.delivery_method(Mail::SMTP, { user_name: 'user@example.com' })
+
+ expect(Mail::SMTP.new({}).settings).to eq(expected_settings)
+ end
+
+ describe 'enable_starttls_auto setting' do
+ let(:settings) { {} }
+
+ subject(:smtp) { Mail::SMTP.new(settings) }
+
+ it 'uses default for enable_starttls_auto' do
+ expect(smtp.settings).to include(enable_starttls_auto: nil)
+ end
+
+ context 'when set to false' do
+ let(:settings) { { enable_starttls_auto: false } }
+
+ it 'overrides default and sets value' do
+ expect(smtp.settings).to include(enable_starttls_auto: false)
+ end
+ end
+ end
+
# Taken from https://github.com/mikel/mail/pull/1536#issue-1490438378
where(:ssl, :tls, :enable_starttls, :enable_starttls_auto, :smtp_tls, :smtp_starttls_mode) do
true | nil | nil | nil | true | false
diff --git a/spec/mailers/emails/service_desk_spec.rb b/spec/mailers/emails/service_desk_spec.rb
index e7775022e9a55..ae676fcf8b193 100644
--- a/spec/mailers/emails/service_desk_spec.rb
+++ b/spec/mailers/emails/service_desk_spec.rb
@@ -438,6 +438,18 @@
end
describe '.service_desk_custom_email_verification_email' do
+ # Use strict definition here because Mail::SMTP.new({}).settings
+ # might have been changed before.
+ let(:expected_delivery_method_defaults) do
+ {
+ address: 'localhost',
+ domain: 'localhost.localdomain',
+ port: 25,
+ password: nil,
+ user_name: nil
+ }
+ end
+
subject { Notify.service_desk_custom_email_verification_email(service_desk_setting) }
it_behaves_like 'a custom email verification process email'
@@ -448,6 +460,13 @@
it 'forcibly uses SMTP delivery method and has correct settings' do
expect_service_desk_custom_email_delivery_options(service_desk_setting)
+
+ # defaults are unchanged after email overrode settings
+ expect(Mail::SMTP.new({}).settings).to include(expected_delivery_method_defaults)
+
+ # other mailers are unchanged after email overrode settings
+ other_mail = Notify.test_email(email, 'Test subject', 'Test body')
+ expect(other_mail.delivery_method).to be_a(Mail::TestMailer)
end
it 'uses verification email address as recipient' do
--
GitLab