Split recipient strings without breaking a quoted display name

lib/courrier/email/providers/base.rb

@@ -41,14 +41,21 @@ def headers
         def default_headers = {}
 
         def address_list(value, as: :email)
-          list = value&.to_s&.split(",")&.map(&:strip)&.reject(&:empty?)
-          return unless list && !list.empty?
+          list = split_addresses(value).map(&:strip).reject(&:empty?)
+          return if list.empty?
 
           list.map { |address| address_element(address, as) }
         end
 
         def address_line(value) = address_list(value, as: :plain)&.join(", ")
 
+        # Split a recipient string on the commas that separate addresses, leaving
+        # a comma inside a quoted display name (`"Doe, Jane" <jane@example.com>`,
+        # which is what `Courrier::Email::Address.with_name` produces) untouched.
+        def split_addresses(value)
+          value.to_s.scan(/(?:"(?:\\.|[^"\\])*"|[^,])+/)
+        end
+
         def address_element(address, as)
           case as
           when :email then {"email" => address}

lib/courrier/email/providers/lettermint.rb

@@ -12,10 +12,10 @@ def body
           {
             "route" => @provider_options.route,
             "from" => @options.from,
-            "to" => @options.to.to_s.split(",").map(&:strip),
-            "cc" => @options.cc&.split(",")&.map(&:strip),
-            "bcc" => @options.bcc&.split(",")&.map(&:strip),
-            "reply_to" => @options.reply_to&.split(",")&.map(&:strip),
+            "to" => address_list(@options.to, as: :plain),
+            "cc" => address_list(@options.cc, as: :plain),
+            "bcc" => address_list(@options.bcc, as: :plain),
+            "reply_to" => address_list(@options.reply_to, as: :plain),
             "subject" => @options.subject,
             "html" => @options.html,
             "text" => @options.text

lib/courrier/email/providers/ses.rb

@@ -28,12 +28,12 @@ def body
           {
             "FromEmailAddress" => @options.from,
             "Destination" => {
-              "ToAddresses" => Array(@options.to),
-              "CcAddresses" => @options.cc ? Array(@options.cc) : nil,
-              "BccAddresses" => @options.bcc ? Array(@options.bcc) : nil
+              "ToAddresses" => address_list(@options.to, as: :plain),
+              "CcAddresses" => address_list(@options.cc, as: :plain),
+              "BccAddresses" => address_list(@options.bcc, as: :plain)
             }.compact,
 
-            "ReplyToAddresses" => @options.reply_to ? Array(@options.reply_to) : nil,
+            "ReplyToAddresses" => address_list(@options.reply_to, as: :plain),
             "Content" => {
               "Simple" => {
                 "Subject" => {"Data" => @options.subject},

lib/courrier/email/providers/smtp2go.rb

@@ -9,9 +9,9 @@ class Smtp2go < Base
         def body
           {
             "sender" => @options.from,
-            "to" => @options.to.to_s.split(",").map(&:strip),
-            "cc" => @options.cc&.split(",")&.map(&:strip),
-            "bcc" => @options.bcc&.split(",")&.map(&:strip),
+            "to" => address_list(@options.to, as: :plain),
+            "cc" => address_list(@options.cc, as: :plain),
+            "bcc" => address_list(@options.bcc, as: :plain),
             "subject" => @options.subject,
             "html_body" => @options.html,
             "text_body" => @options.text

test/courrier/email/providers/brevo_test.rb

@@ -30,6 +30,15 @@ def test_formats_transactional_email
       )
     end
 
+    def test_keeps_a_comma_inside_a_quoted_display_name
+      recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "#{recipient}, bob@example.com")
+
+      body = Brevo.new(api_key: "test_key", options: email.options).body
+
+      assert_equal [{"email" => recipient}, {"email" => "bob@example.com"}], body["to"]
+    end
+
     def test_authenticates_with_api_key
       assert_equal({"api-key" => "test_key"}, @provider.send(:default_headers))
     end

test/courrier/email/providers/lettermint_test.rb

@@ -0,0 +1,79 @@
+require "test_helper"
+
+module Courrier::Email::Providers
+  class LettermintTest < Minitest::Test
+    def setup
+      email = TestEmail.new(
+        from: "devs@railsdesigner.com",
+        to: "first@example.com, second@example.com",
+        reply_to: "support@railsdesigner.com",
+        cc: "copy@example.com",
+        bcc: "archive@example.com"
+      )
+
+      @provider = provider_for(email)
+    end
+
+    def test_formats_transactional_email
+      assert_equal(
+        {
+          "route" => "test_route",
+          "from" => "devs@railsdesigner.com",
+          "to" => ["first@example.com", "second@example.com"],
+          "cc" => ["copy@example.com"],
+          "bcc" => ["archive@example.com"],
+          "reply_to" => ["support@railsdesigner.com"],
+          "subject" => "Test Subject",
+          "html" => "<p>Test HTML Body</p>",
+          "text" => "Test Body"
+        },
+        @provider.body
+      )
+    end
+
+    def test_keeps_a_comma_inside_a_quoted_display_name
+      recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "#{recipient}, bob@example.com")
+
+      assert_equal [recipient, "bob@example.com"], provider_for(email).body["to"]
+    end
+
+    def test_omits_empty_address_fields
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "first@example.com", cc: "", bcc: "   ", reply_to: ",")
+
+      body = provider_for(email).body
+
+      refute_includes body.keys, "cc"
+      refute_includes body.keys, "bcc"
+      refute_includes body.keys, "reply_to"
+    end
+
+    def test_authenticates_with_api_key
+      assert_equal({"x-lettermint-token" => "test_key"}, @provider.send(:default_headers))
+    end
+
+    def test_is_available_through_provider_registry
+      mock_provider = Minitest::Mock.new
+      mock_provider.expect(:deliver, nil)
+
+      Lettermint.stub :new, mock_provider do
+        Courrier::Email::Provider.new(
+          provider: "lettermint",
+          api_key: "test_key",
+          options: @provider.instance_variable_get(:@options)
+        ).deliver
+      end
+
+      mock_provider.verify
+    end
+
+    private
+
+    def provider_for(email)
+      provider_options = Courrier::Configuration::ProviderConfig.new
+      provider_options.route = "test_route"
+
+      Lettermint.new(api_key: "test_key", options: email.options, provider_options: provider_options)
+    end
+  end
+end

test/courrier/email/providers/mailgun_test.rb

@@ -43,6 +43,15 @@ def test_omits_empty_address_fields
       refute_includes body.keys, "bcc"
     end
 
+    def test_keeps_a_comma_inside_a_quoted_display_name
+      # `address_line` splits then rejoins with ", ", so a name whose comma is not
+      # already followed by exactly one space is the case that gets rewritten.
+      recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe,Jane")
+      body = provider_for(TestEmail.new(from: "devs@railsdesigner.com", to: "first@example.com", cc: recipient)).body
+
+      assert_equal recipient, body["cc"]
+    end
+
     def test_builds_endpoint_url_from_domain
       assert_equal "https://api.mailgun.net/v3/railsdesigner.com/messages", @provider.send(:endpoint_url)
     end

test/courrier/email/providers/ses_test.rb

@@ -0,0 +1,79 @@
+require "test_helper"
+
+module Courrier::Email::Providers
+  class SesTest < Minitest::Test
+    def setup
+      email = TestEmail.new(
+        from: "devs@railsdesigner.com",
+        to: "first@example.com, second@example.com",
+        reply_to: "support@railsdesigner.com",
+        cc: "copy@example.com",
+        bcc: "archive@example.com"
+      )
+
+      @provider = Ses.new(api_key: "test_key", options: email.options)
+    end
+
+    def test_formats_transactional_email
+      assert_equal(
+        {
+          "FromEmailAddress" => "devs@railsdesigner.com",
+          "Destination" => {
+            "ToAddresses" => ["first@example.com", "second@example.com"],
+            "CcAddresses" => ["copy@example.com"],
+            "BccAddresses" => ["archive@example.com"]
+          },
+          "ReplyToAddresses" => ["support@railsdesigner.com"],
+          "Content" => {
+            "Simple" => {
+              "Subject" => {"Data" => "Test Subject"},
+              "Body" => {
+                "Text" => {"Data" => "Test Body"},
+                "Html" => {"Data" => "<p>Test HTML Body</p>"}
+              }
+            }
+          }
+        },
+        @provider.body
+      )
+    end
+
+    def test_splits_a_comma_separated_recipient_string
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "a@example.com, b@example.com, c@example.com")
+
+      assert_equal ["a@example.com", "b@example.com", "c@example.com"], Ses.new(api_key: "k", options: email.options).body["Destination"]["ToAddresses"]
+    end
+
+    def test_keeps_a_comma_inside_a_quoted_display_name
+      recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "#{recipient}, bob@example.com")
+
+      assert_equal [recipient, "bob@example.com"], Ses.new(api_key: "k", options: email.options).body["Destination"]["ToAddresses"]
+    end
+
+    def test_omits_empty_address_fields
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "first@example.com", cc: "", bcc: "   ", reply_to: ",")
+
+      body = Ses.new(api_key: "k", options: email.options).body
+
+      refute_includes body["Destination"].keys, "CcAddresses"
+      refute_includes body["Destination"].keys, "BccAddresses"
+      refute_includes body.keys, "ReplyToAddresses"
+    end
+
+    def test_is_available_through_provider_registry
+      mock_provider = Minitest::Mock.new
+      mock_provider.expect(:deliver, nil)
+
+      Ses.stub :new, mock_provider do
+        Courrier::Email::Provider.new(
+          provider: "ses",
+          api_key: "test_key",
+          options: @provider.instance_variable_get(:@options)
+        ).deliver
+      end
+
+      mock_provider.verify
+    end
+  end
+end

test/courrier/email/providers/smtp2go_test.rb

@@ -0,0 +1,66 @@
+require "test_helper"
+
+module Courrier::Email::Providers
+  class Smtp2goTest < Minitest::Test
+    def setup
+      email = TestEmail.new(
+        from: "devs@railsdesigner.com",
+        to: "first@example.com, second@example.com",
+        cc: "copy@example.com",
+        bcc: "archive@example.com"
+      )
+
+      @provider = Smtp2go.new(api_key: "test_key", options: email.options)
+    end
+
+    def test_formats_transactional_email
+      assert_equal(
+        {
+          "sender" => "devs@railsdesigner.com",
+          "to" => ["first@example.com", "second@example.com"],
+          "cc" => ["copy@example.com"],
+          "bcc" => ["archive@example.com"],
+          "subject" => "Test Subject",
+          "html_body" => "<p>Test HTML Body</p>",
+          "text_body" => "Test Body"
+        },
+        @provider.body
+      )
+    end
+
+    def test_keeps_a_comma_inside_a_quoted_display_name
+      recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "#{recipient}, bob@example.com")
+
+      assert_equal [recipient, "bob@example.com"], Smtp2go.new(api_key: "test_key", options: email.options).body["to"]
+    end
+
+    def test_omits_empty_address_fields
+      email = TestEmail.new(from: "devs@railsdesigner.com", to: "first@example.com", cc: "", bcc: "   ")
+
+      body = Smtp2go.new(api_key: "test_key", options: email.options).body
+
+      refute_includes body.keys, "cc"
+      refute_includes body.keys, "bcc"
+    end
+
+    def test_authenticates_with_api_key
+      assert_equal({"X-Smtp2go-Api-Key" => "test_key"}, @provider.send(:default_headers))
+    end
+
+    def test_is_available_through_provider_registry
+      mock_provider = Minitest::Mock.new
+      mock_provider.expect(:deliver, nil)
+
+      Smtp2go.stub :new, mock_provider do
+        Courrier::Email::Provider.new(
+          provider: "smtp2go",
+          api_key: "test_key",
+          options: @provider.instance_variable_get(:@options)
+        ).deliver
+      end
+
+      mock_provider.verify
+    end
+  end
+end

test/courrier/providers/base_test.rb

@@ -68,4 +68,41 @@ def test_headers_returns_only_default_headers_when_no_custom_headers
 
     assert_equal({}, headers)
   end
+
+  def test_address_list_splits_a_plain_comma_separated_list
+    assert_equal(
+      [{"email" => "first@example.com"}, {"email" => "second@example.com"}],
+      @provider.send(:address_list, "first@example.com, second@example.com")
+    )
+  end
+
+  def test_address_list_keeps_a_comma_inside_a_quoted_display_name
+    recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+
+    assert_equal [{"email" => recipient}], @provider.send(:address_list, recipient)
+    assert_equal(
+      [{"email" => recipient}, {"email" => "bob@example.com"}],
+      @provider.send(:address_list, "#{recipient}, bob@example.com")
+    )
+  end
+
+  def test_address_list_tolerates_an_escaped_quote_in_the_display_name
+    recipient = Courrier::Email::Address.with_name("j@example.com", 'John "JD" Doe')
+
+    assert_equal [recipient], @provider.send(:address_list, recipient, as: :plain)
+  end
+
+  def test_address_list_returns_nil_for_blank_input
+    assert_nil @provider.send(:address_list, nil)
+    assert_nil @provider.send(:address_list, "")
+    assert_nil @provider.send(:address_list, "   ")
+    assert_nil @provider.send(:address_list, ",")
+  end
+
+  def test_address_line_joins_without_breaking_a_quoted_name
+    recipient = Courrier::Email::Address.with_name("jane@example.com", "Doe, Jane")
+    line = "#{recipient}, bob@example.com"
+
+    assert_equal line, @provider.send(:address_line, line)
+  end
 end