Make errors on mail sending failures more readable

[Suhopoljac]

Used Zammad Version

6.3.1

Environment

• Installation method: any
• Operating system (if you're unsure: cat /etc/os-release ): any
• Database + version: any
• Elasticsearch version: any
• Browser + version: MS Edge, v. 126.0.2592.87 (official build) (64-bit)

Actual behaviour

Due to the change in the mailbox configuration, the mails cannot be sent.

The users receive a general error message which, without having access to the production.log, does not say much about the cause of the error:

"Unable to send email to 'XXX@XXX': can't modify frozen String: "execution expired"",

Expected behaviour

If a configuration change is made (either by the provider or the administrator of the instance), the error message should be easier to understand and lead the troubleshooting to the right place:

"Unable to send email to 'XXX@XXX': Unable to connect to mail server: "execution expired""

Steps to reproduce the behaviour

1. Configure a firewall so that sending is only possible via certain ports, e.g. 465.
2. A change occurs and now the above port cannot be used for sending because sending is now only allowed with 587, which causes the said error.
3. The user receives the error message but has no idea what it means and how to fix it.

Support Ticket

Ticket#10157869,10159164,10158296,10157290,10156282,10150110,10148197,10148331,10116296,10112196,10101394,10116282

I'm sure this is a bug and no feature request or a general question.

yes

[dominikklein]

@Suhopoljac But is this not more like an enhancement?

[MrGeneration]

Technically it is, right.

[martini]

In my opinion it's more a bug. Because can't modify frozen String has nothing to do with the actual error/problem (and comes from the ruby scope). In this case the user did not understand what the problem is, for this the use was not able to solve it by him self.

[martini]

More technical info here:

1. Actual bug: Ruby error message can't modify frozen String The can't modify frozen String comes IMO from here, we try to modify the error messages form an exception, which seems to be a frozen string: https://github.com/zammad/zammad/blob/develop/app/jobs/ticket_article_communicate_email_job.rb#L125

At least we should "copy" the string to proceed without this can't modify frozen String message.

PS: Maybe a simple .to_s works already like message.to_s.encode!('UTF-8', 'UTF-8', invalid: :replace, replace: '?')

2. Improve error message (optional):

To make sure where the error message is displayed (with example error message):

How to reproduce/force the error message on rails c:

begin
  instance = Channel::Driver::Smtp.new
  instance.deliver(
    {
      host:                 '10.1.1.1',
      port:                 25,
      enable_starttls_auto: true, # optional
      openssl_verify_mode:  'none', # optional
      user:                 'someuser',
      password:             'somepass',
      authentication:       nil, # nil, autodetection - to use certain schema use 'plain', 'login', 'xoauth2' or 'cram_md5'
    },
    {
      from:    '[email protected]',
      to:      '[email protected]',
      subject: 'some subject',
      body:    'some body',
    },
    false, # true|false - specify if notification headers (like X-Loop or Precedence) are added or not
  )
recude => e
  puts e.message
end

Approach to provide better / meaning full error messages:

diff --git a/app/models/channel/driver/smtp.rb b/app/models/channel/driver/smtp.rb
index f4e6e49479..b4bb2a745a 100644
--- a/app/models/channel/driver/smtp.rb
+++ b/app/models/channel/driver/smtp.rb
@@ -94,7 +99,13 @@ class Channel::Driver::Smtp
 
     Certificate::ApplySSLCertificates.ensure_fresh_ssl_context if options[:ssl] || options[:enable_starttls_auto]
 
-    mail.delivery_method :smtp, smtp_params
-    mail.deliver
+    begin
+      mail.delivery_method :smtp, smtp_params
+      mail.deliver
+    rescue Net::OpenTimeout => e
+      raise e.class, "The network connection to SMTP server '#{options[:host]}:#{options[:port]}' timeout (#{e.message})"
+    rescue Errno::ECONNREFUSED => e
+      raise e.class, "The network connection to SMTP server '#{options[:host]}:#{options[:port]}' could not be established. The connection was rejected. (#{e.message})"
+    rescue Net::SMTPAuthenticationError => e
+      raise e.class, "The authentication to SMTP server '#{options[:host]}:#{options[:port]}' was not possible (#{e.message})"
+    rescue => e
+      raise e.class, "Unable to send email via SMTP server '#{options[:host]}:#{options[:port]}' (#{e.message})"
+    end
   end

Just a few examples which did come in my mind - compare the messages to Zammad GUI users by your self the Old/New variant:

Old: execution expired New: The network connection to SMTP server '10.1.1.1:25' timeout (execution expired)

Old: 502 5.5.2 Error: command not recognized New: The authentication to SMTP server 'mx2.example.com:25' was not possible (502 5.5.2 Error: command not recognized)

Old: 535 5.7.8 Error: authentication failed: (reason unavailable) New: The authentication to SMTP server 'mx2.example.com:587' was not possible (535 5.7.8 Error: authentication failed)

Old: Connection refused - connect(2) for "mx2.example.com" port 123 New: The network connection to SMTP server 'mx2.example.com:123' could not be established. The connection was rejected. (Connection refused - connect(2) for "mx2.example.com" port 123)

Old: 530 5.7.0 Must issue a STARTTLS command first New: The authentication to SMTP server 'mx2.example.com:587' was not possible (530 5.7.0 Must issue a STARTTLS command first)

Old: 554 5.7.1 <example.com[10.1.1.1]>: Client host rejected: Access denied New: Unable to send email via SMTP server 'mx2.example:587' (554 5.7.1 <client.example.com[10.1.1.1]>: Client host rejected: Access denied)

[dominikklein]

Sure this frozen string is not optimal, and could be improved.

However showing specific error messages is an enhancement and is currently a problem in many other places.

[MrGeneration]

However showing specific error messages is an enhancement and is currently a problem in many other places.

True, but also a requested improvement on e.g. notification related stuff since a while by now. It's a quality of life change that would also help me in monitoring scenarios (and every other admin imho) that has to look into log files due to unprecise messages.

[martini]

However showing specific error messages is an enhancement and is currently a problem in many other places.

I agree. Technically: When I took a quick look at the source code, I was just about there. My heart would have bled if I hadn't written down my 5 minutes of research here to make users (and support) live in this context easier (so it's IMO a good starting point if somebody starts here in the SMTP backend).

[fliebe92]

~~@martini how to reproduce this issue? The provided code snippet for rails c works just fine.~~

~~Output: Net::OpenTimeout: execution expired~~

Reproduction is possible with the following snippet:

begin
  instance = Channel::Driver::Smtp.new
  instance.deliver(
    {
      host:                 '10.1.1.1',
      port:                 25,
      enable_starttls_auto: true,
      openssl_verify_mode:  'none',
      user:                 'someuser',
      password:             'somepass',
      authentication:       nil,
    },
    {
      from:    '[email protected]',
      to:      '[email protected]',
      subject: 'some subject',
      body:    'some body',
    },
    false,
  )
rescue => e
  puts e.message.encode!('UTF-8', 'UTF-8', invalid: :replace, replace: '?')
end