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) <noreply@anthropic.com> Co-authored-by: Vishnu Narayanan <iamwishnu@gmail.com>
This commit is contained in:
committed by
GitHub
parent
0592cccca9
commit
722e68eecb
@@ -1,4 +1,6 @@
|
|||||||
class Email::BaseBuilder
|
class Email::BaseBuilder
|
||||||
|
include EmailAddressParseable
|
||||||
|
|
||||||
pattr_initialize [:inbox!]
|
pattr_initialize [:inbox!]
|
||||||
|
|
||||||
private
|
private
|
||||||
@@ -47,8 +49,4 @@ class Email::BaseBuilder
|
|||||||
# can save it in the format "Name <email@domain.com>"
|
# can save it in the format "Name <email@domain.com>"
|
||||||
parse_email(account.support_email)
|
parse_email(account.support_email)
|
||||||
end
|
end
|
||||||
|
|
||||||
def parse_email(email_string)
|
|
||||||
Mail::Address.new(email_string).address
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ class ConversationReplyMailer < ApplicationMailer
|
|||||||
|
|
||||||
include ConversationReplyMailerHelper
|
include ConversationReplyMailerHelper
|
||||||
include ReferencesHeaderBuilder
|
include ReferencesHeaderBuilder
|
||||||
|
include EmailAddressParseable
|
||||||
default from: ENV.fetch('MAILER_SENDER_EMAIL', 'Chatwoot <accounts@chatwoot.com>')
|
default from: ENV.fetch('MAILER_SENDER_EMAIL', 'Chatwoot <accounts@chatwoot.com>')
|
||||||
layout :choose_layout
|
layout :choose_layout
|
||||||
|
|
||||||
@@ -139,10 +140,6 @@ class ConversationReplyMailer < ApplicationMailer
|
|||||||
sender_name(@channel.email)
|
sender_name(@channel.email)
|
||||||
end
|
end
|
||||||
|
|
||||||
def parse_email(email_string)
|
|
||||||
Mail::Address.new(email_string).address
|
|
||||||
end
|
|
||||||
|
|
||||||
def inbox_from_email_address
|
def inbox_from_email_address
|
||||||
return @inbox.email_address if @inbox.email_address
|
return @inbox.email_address if @inbox.email_address
|
||||||
|
|
||||||
|
|||||||
@@ -30,50 +30,7 @@ class Account < ApplicationRecord
|
|||||||
include CacheKeys
|
include CacheKeys
|
||||||
include CaptainFeaturable
|
include CaptainFeaturable
|
||||||
include AccountEmailRateLimitable
|
include AccountEmailRateLimitable
|
||||||
|
include AccountSettingsSchema
|
||||||
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
|
|
||||||
|
|
||||||
DEFAULT_QUERY_SETTING = {
|
DEFAULT_QUERY_SETTING = {
|
||||||
flag_query_mode: :bit_operator,
|
flag_query_mode: :bit_operator,
|
||||||
@@ -86,6 +43,7 @@ class Account < ApplicationRecord
|
|||||||
schema: SETTINGS_PARAMS_SCHEMA,
|
schema: SETTINGS_PARAMS_SCHEMA,
|
||||||
attribute_resolver: ->(record) { record.settings }
|
attribute_resolver: ->(record) { record.settings }
|
||||||
validate :validate_reporting_timezone
|
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
|
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'))
|
errors.add(:reporting_timezone, I18n.t('errors.account.reporting_timezone.invalid'))
|
||||||
end
|
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
|
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 camp_dpid_seq_#{id}")
|
||||||
ActiveRecord::Base.connection.exec_query("drop sequence IF EXISTS conv_dpid_seq_#{id}")
|
ActiveRecord::Base.connection.exec_query("drop sequence IF EXISTS conv_dpid_seq_#{id}")
|
||||||
|
|||||||
47
app/models/concerns/account_settings_schema.rb
Normal file
47
app/models/concerns/account_settings_schema.rb
Normal file
@@ -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
|
||||||
15
app/models/concerns/email_address_parseable.rb
Normal file
15
app/models/concerns/email_address_parseable.rb
Normal file
@@ -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
|
||||||
@@ -51,6 +51,8 @@ en:
|
|||||||
account:
|
account:
|
||||||
reporting_timezone:
|
reporting_timezone:
|
||||||
invalid: is not a valid timezone
|
invalid: is not a valid timezone
|
||||||
|
support_email:
|
||||||
|
invalid: is not a valid email address
|
||||||
validations:
|
validations:
|
||||||
presence: must not be blank
|
presence: must not be blank
|
||||||
webhook:
|
webhook:
|
||||||
|
|||||||
@@ -256,6 +256,29 @@ RSpec.describe Account do
|
|||||||
end
|
end
|
||||||
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 <support@example.com>'
|
||||||
|
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
|
context 'when reporting_timezone is set' do
|
||||||
it 'allows valid timezone names' do
|
it 'allows valid timezone names' do
|
||||||
account.reporting_timezone = 'America/New_York'
|
account.reporting_timezone = 'America/New_York'
|
||||||
|
|||||||
Reference in New Issue
Block a user