From 01ef1d08fc0a87be606bc4b0dbbd2a73defffc02 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 21 Feb 2022 11:26:39 +1000
Subject: [PATCH] FIX: Conform EmailLog#bounce_error_code to RFC (#16010)

This commit makes sure that the email log's bounce_error_code
conforms to the SMTP error code RFC on save, so that
it is always in the format X.X.X or XXX without any
additional string details. Also included is a migration
to fix this issue for past records.
---
 app/models/email_log.rb                       | 10 ++++++++++
 ...0220220234155_conform_bounce_error_code.rb | 19 +++++++++++++++++++
 spec/models/email_log_spec.rb                 | 17 +++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100644 db/post_migrate/20220220234155_conform_bounce_error_code.rb

diff --git a/app/models/email_log.rb b/app/models/email_log.rb
index 153cb0b142b..4182134767a 100644
--- a/app/models/email_log.rb
+++ b/app/models/email_log.rb
@@ -14,6 +14,9 @@ class EmailLog < ActiveRecord::Base
     signup_after_approval
   }
 
+  # cf. https://www.iana.org/assignments/smtp-enhanced-status-codes/smtp-enhanced-status-codes.xhtml
+  SMTP_ERROR_CODE_REGEXP = Regexp.new(/\d\.\d\.\d|\d\d\d/).freeze
+
   belongs_to :user
   belongs_to :post
   belongs_to :smtp_group, class_name: 'Group'
@@ -34,6 +37,13 @@ class EmailLog < ActiveRecord::Base
     SQL
   end
 
+  before_save do
+    if self.bounce_error_code.present?
+      match = SMTP_ERROR_CODE_REGEXP.match(self.bounce_error_code)
+      self.bounce_error_code = match.present? ? match[0] : nil
+    end
+  end
+
   after_create do
     # Update last_emailed_at if the user_id is present and email was sent
     User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present?
diff --git a/db/post_migrate/20220220234155_conform_bounce_error_code.rb b/db/post_migrate/20220220234155_conform_bounce_error_code.rb
new file mode 100644
index 00000000000..349073070ea
--- /dev/null
+++ b/db/post_migrate/20220220234155_conform_bounce_error_code.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+#
+class ConformBounceErrorCode < ActiveRecord::Migration[6.1]
+  def up
+    DB.exec(<<~SQL, regexp: '\d\.\d\.\d|\d\d\d')
+      UPDATE email_logs
+      SET bounce_error_code = (
+        SELECT array_to_string(
+          regexp_matches(bounce_error_code, :regexp),
+          ''
+        )
+      ) WHERE bounce_error_code IS NOT NULL;
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb
index 60064f4b86d..be6befc16a2 100644
--- a/spec/models/email_log_spec.rb
+++ b/spec/models/email_log_spec.rb
@@ -161,4 +161,21 @@ describe EmailLog do
       expect(EmailLog.addressed_to_user(user).count).to eq(0)
     end
   end
+
+  describe "bounce_error_code fix before update" do
+    fab!(:email_log) { Fabricate(:email_log) }
+
+    it "makes sure the bounce_error_code is in the format X.X.X or XXX" do
+      email_log.update!(bounce_error_code: "5.1.1")
+      expect(email_log.reload.bounce_error_code).to eq("5.1.1")
+      email_log.update!(bounce_error_code: "5.0.0 (permanent failure)")
+      expect(email_log.reload.bounce_error_code).to eq("5.0.0")
+      email_log.update!(bounce_error_code: "422")
+      expect(email_log.reload.bounce_error_code).to eq("422")
+      email_log.update!(bounce_error_code: "5.2")
+      expect(email_log.reload.bounce_error_code).to eq(nil)
+      email_log.update!(bounce_error_code: "blah")
+      expect(email_log.reload.bounce_error_code).to eq(nil)
+    end
+  end
 end