From 722e68eecb6b9085e97ddc5770893478c4887f4b Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Mon, 13 Apr 2026 19:06:06 +0700 Subject: [PATCH] fix: validate support_email format and handle parse errors in mailer (#13958) ## Description ConversationReplyMailer#parse_email calls Mail::Address.new(email_string).address without error handling. When an account's support_email contains a non-email string (e.g., "Smith Smith"), the mail gem raises Mail::Field::IncompleteParseError, crashing conversation transcript emails. This has caused 1,056 errors on Sentry (EXTERNAL-CHATINC-JX) since Feb 25, all from a single account that has a name stored in the support_email field instead of a valid email address. Closes https://linear.app/chatwoot/issue/CW-6687/mailfieldincompleteparseerror-mailaddresslist-can-not-parse-orsmith ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: Vishnu Narayanan --- app/builders/email/base_builder.rb | 6 +- app/mailers/conversation_reply_mailer.rb | 5 +- app/models/account.rb | 56 ++++--------------- .../concerns/account_settings_schema.rb | 47 ++++++++++++++++ .../concerns/email_address_parseable.rb | 15 +++++ config/locales/en.yml | 2 + spec/models/account_spec.rb | 23 ++++++++ 7 files changed, 102 insertions(+), 52 deletions(-) create mode 100644 app/models/concerns/account_settings_schema.rb create mode 100644 app/models/concerns/email_address_parseable.rb diff --git a/app/builders/email/base_builder.rb b/app/builders/email/base_builder.rb index 731b1b0f5..6f79d6018 100644 --- a/app/builders/email/base_builder.rb +++ b/app/builders/email/base_builder.rb @@ -1,4 +1,6 @@ class Email::BaseBuilder + include EmailAddressParseable + pattr_initialize [:inbox!] private @@ -47,8 +49,4 @@ class Email::BaseBuilder # can save it in the format "Name " parse_email(account.support_email) end - - def parse_email(email_string) - Mail::Address.new(email_string).address - end end diff --git a/app/mailers/conversation_reply_mailer.rb b/app/mailers/conversation_reply_mailer.rb index 220531221..d9e6ec8e0 100644 --- a/app/mailers/conversation_reply_mailer.rb +++ b/app/mailers/conversation_reply_mailer.rb @@ -5,6 +5,7 @@ class ConversationReplyMailer < ApplicationMailer include ConversationReplyMailerHelper include ReferencesHeaderBuilder + include EmailAddressParseable default from: ENV.fetch('MAILER_SENDER_EMAIL', 'Chatwoot ') layout :choose_layout @@ -139,10 +140,6 @@ class ConversationReplyMailer < ApplicationMailer sender_name(@channel.email) end - def parse_email(email_string) - Mail::Address.new(email_string).address - end - def inbox_from_email_address return @inbox.email_address if @inbox.email_address diff --git a/app/models/account.rb b/app/models/account.rb index 06f47636e..181081aad 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -30,50 +30,7 @@ class Account < ApplicationRecord include CacheKeys include CaptainFeaturable include AccountEmailRateLimitable - - SETTINGS_PARAMS_SCHEMA = { - 'type': 'object', - 'properties': - { - 'auto_resolve_after': { 'type': %w[integer null], 'minimum': 10, 'maximum': 1_439_856 }, - 'auto_resolve_message': { 'type': %w[string null] }, - 'auto_resolve_ignore_waiting': { 'type': %w[boolean null] }, - 'audio_transcriptions': { 'type': %w[boolean null] }, - 'auto_resolve_label': { 'type': %w[string null] }, - 'keep_pending_on_bot_failure': { 'type': %w[boolean null] }, - 'captain_auto_resolve_mode': { 'type': %w[string null], 'enum': ['evaluated', 'legacy', 'disabled', nil] }, - 'conversation_required_attributes': { - 'type': %w[array null], - 'items': { 'type': 'string' } - }, - 'captain_models': { - 'type': %w[object null], - 'properties': { - 'editor': { 'type': %w[string null] }, - 'assistant': { 'type': %w[string null] }, - 'copilot': { 'type': %w[string null] }, - 'label_suggestion': { 'type': %w[string null] }, - 'audio_transcription': { 'type': %w[string null] }, - 'help_center_search': { 'type': %w[string null] } - }, - 'additionalProperties': false - }, - 'captain_features': { - 'type': %w[object null], - 'properties': { - 'editor': { 'type': %w[boolean null] }, - 'assistant': { 'type': %w[boolean null] }, - 'copilot': { 'type': %w[boolean null] }, - 'label_suggestion': { 'type': %w[boolean null] }, - 'audio_transcription': { 'type': %w[boolean null] }, - 'help_center_search': { 'type': %w[boolean null] } - }, - 'additionalProperties': false - } - }, - 'required': [], - 'additionalProperties': true - }.to_json.freeze + include AccountSettingsSchema DEFAULT_QUERY_SETTING = { flag_query_mode: :bit_operator, @@ -86,6 +43,7 @@ class Account < ApplicationRecord schema: SETTINGS_PARAMS_SCHEMA, attribute_resolver: ->(record) { record.settings } validate :validate_reporting_timezone + validate :validate_support_email_format, if: :will_save_change_to_support_email? store_accessor :settings, :auto_resolve_after, :auto_resolve_message, :auto_resolve_ignore_waiting @@ -223,6 +181,16 @@ class Account < ApplicationRecord errors.add(:reporting_timezone, I18n.t('errors.account.reporting_timezone.invalid')) end + def validate_support_email_format + value = attributes['support_email'] + return if value.blank? + + parsed = Mail::Address.new(value).address + errors.add(:support_email, I18n.t('errors.account.support_email.invalid')) if parsed.blank? + rescue Mail::Field::ParseError, Mail::Field::IncompleteParseError + errors.add(:support_email, I18n.t('errors.account.support_email.invalid')) + end + def remove_account_sequences ActiveRecord::Base.connection.exec_query("drop sequence IF EXISTS camp_dpid_seq_#{id}") ActiveRecord::Base.connection.exec_query("drop sequence IF EXISTS conv_dpid_seq_#{id}") diff --git a/app/models/concerns/account_settings_schema.rb b/app/models/concerns/account_settings_schema.rb new file mode 100644 index 000000000..52e1c2811 --- /dev/null +++ b/app/models/concerns/account_settings_schema.rb @@ -0,0 +1,47 @@ +module AccountSettingsSchema + extend ActiveSupport::Concern + + SETTINGS_PARAMS_SCHEMA = { + 'type': 'object', + 'properties': + { + 'auto_resolve_after': { 'type': %w[integer null], 'minimum': 10, 'maximum': 1_439_856 }, + 'auto_resolve_message': { 'type': %w[string null] }, + 'auto_resolve_ignore_waiting': { 'type': %w[boolean null] }, + 'audio_transcriptions': { 'type': %w[boolean null] }, + 'auto_resolve_label': { 'type': %w[string null] }, + 'keep_pending_on_bot_failure': { 'type': %w[boolean null] }, + 'captain_auto_resolve_mode': { 'type': %w[string null], 'enum': ['evaluated', 'legacy', 'disabled', nil] }, + 'conversation_required_attributes': { + 'type': %w[array null], + 'items': { 'type': 'string' } + }, + 'captain_models': { + 'type': %w[object null], + 'properties': { + 'editor': { 'type': %w[string null] }, + 'assistant': { 'type': %w[string null] }, + 'copilot': { 'type': %w[string null] }, + 'label_suggestion': { 'type': %w[string null] }, + 'audio_transcription': { 'type': %w[string null] }, + 'help_center_search': { 'type': %w[string null] } + }, + 'additionalProperties': false + }, + 'captain_features': { + 'type': %w[object null], + 'properties': { + 'editor': { 'type': %w[boolean null] }, + 'assistant': { 'type': %w[boolean null] }, + 'copilot': { 'type': %w[boolean null] }, + 'label_suggestion': { 'type': %w[boolean null] }, + 'audio_transcription': { 'type': %w[boolean null] }, + 'help_center_search': { 'type': %w[boolean null] } + }, + 'additionalProperties': false + } + }, + 'required': [], + 'additionalProperties': true + }.to_json.freeze +end diff --git a/app/models/concerns/email_address_parseable.rb b/app/models/concerns/email_address_parseable.rb new file mode 100644 index 000000000..7cca4a577 --- /dev/null +++ b/app/models/concerns/email_address_parseable.rb @@ -0,0 +1,15 @@ +module EmailAddressParseable + extend ActiveSupport::Concern + + private + + def parse_email(email_string) + Mail::Address.new(email_string).address.presence || default_sender_email_address + rescue Mail::Field::ParseError, Mail::Field::IncompleteParseError + default_sender_email_address + end + + def default_sender_email_address + Mail::Address.new(ENV.fetch('MAILER_SENDER_EMAIL', 'accounts@chatwoot.com')).address + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 1cb3c4d12..057f41b81 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -51,6 +51,8 @@ en: account: reporting_timezone: invalid: is not a valid timezone + support_email: + invalid: is not a valid email address validations: presence: must not be blank webhook: diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index e010a3123..76dbbcba2 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -256,6 +256,29 @@ RSpec.describe Account do end end + context 'when support_email is set' do + it 'allows a plain email address' do + account.support_email = 'support@example.com' + expect(account).to be_valid + end + + it 'allows display-name format' do + account.support_email = 'Support Team ' + expect(account).to be_valid + end + + it 'allows blank values' do + account.support_email = '' + expect(account).to be_valid + end + + it 'rejects malformed strings with no email part' do + account.support_email = 'Smith Smith' + expect(account).not_to be_valid + expect(account.errors[:support_email]).to include(I18n.t('errors.account.support_email.invalid')) + end + end + context 'when reporting_timezone is set' do it 'allows valid timezone names' do account.reporting_timezone = 'America/New_York'