fix(email): strip null bytes from inbound mailbox messages (#14546)
Inbound emails containing null bytes could fail while Chatwoot persisted the incoming message, causing IMAP sync to drop and repeatedly retry the same malformed email. This strips null bytes at the inbound mailbox message persistence boundary so the rest of the email can be saved normally. Fixes https://linear.app/chatwoot/issue/CW-7165/argumenterror-string-contains-null-byte-argumenterror Sentry: https://chatwoot-p3.sentry.io/issues/7405946944/ ## Test - [x] added specs - [x] manually tested the logic on prod for the inbox(email) which was throwing the error ## What changed - Sanitizes null bytes only while building attributes for inbound mailbox message persistence. - Uses the sanitized source ID for both duplicate lookup and message creation. - Adds regression coverage for the mailbox message creation path. --------- Co-authored-by: Sony Mathew <2040199+sony-mathew@users.noreply.github.com> Co-authored-by: Sony Mathew <sony@chatwoot.com>
This commit is contained in:
co-authored by
Sony Mathew
Sony Mathew
parent
d8656edc61
commit
d1c482cb64
@@ -65,11 +65,10 @@ class Imap::ImapMailbox
|
||||
end
|
||||
|
||||
def in_reply_to
|
||||
@processed_mail.in_reply_to
|
||||
sanitize_mailbox_value(@processed_mail.in_reply_to)
|
||||
end
|
||||
|
||||
def find_conversation_by_references
|
||||
references = Array.wrap(@inbound_mail.references)
|
||||
references.each do |message_id|
|
||||
match = FALLBACK_CONVERSATION_PATTERN.match(message_id)
|
||||
|
||||
@@ -80,8 +79,6 @@ class Imap::ImapMailbox
|
||||
def find_message_by_references
|
||||
message_to_return = nil
|
||||
|
||||
references = Array.wrap(@inbound_mail.references)
|
||||
|
||||
references.each do |message_id|
|
||||
message = @inbox.messages.find_by(source_id: message_id)
|
||||
message_to_return = message if message.present?
|
||||
@@ -100,7 +97,7 @@ class Imap::ImapMailbox
|
||||
source: 'email',
|
||||
in_reply_to: in_reply_to,
|
||||
auto_reply: @processed_mail.auto_reply?,
|
||||
mail_subject: @processed_mail.subject,
|
||||
mail_subject: sanitize_mailbox_value(@processed_mail.subject),
|
||||
initiated_at: {
|
||||
timestamp: Time.now.utc
|
||||
}
|
||||
@@ -110,7 +107,7 @@ class Imap::ImapMailbox
|
||||
end
|
||||
|
||||
def find_or_create_contact
|
||||
@contact = @inbox.contacts.from_email(@processed_mail.original_sender)
|
||||
@contact = @inbox.contacts.from_email(original_sender_email)
|
||||
if @contact.present?
|
||||
@contact_inbox = ContactInbox.find_by(inbox: @inbox, contact: @contact)
|
||||
else
|
||||
@@ -119,6 +116,14 @@ class Imap::ImapMailbox
|
||||
end
|
||||
|
||||
def identify_contact_name
|
||||
processed_mail.sender_name || processed_mail.from.first.split('@').first
|
||||
sanitize_mailbox_value(processed_mail.sender_name || processed_mail.from.first.split('@').first)
|
||||
end
|
||||
|
||||
def original_sender_email
|
||||
sanitize_mailbox_value(@processed_mail.original_sender)
|
||||
end
|
||||
|
||||
def references
|
||||
sanitize_mailbox_value(Array.wrap(@inbound_mail.references))
|
||||
end
|
||||
end
|
||||
|
||||
@@ -1,27 +1,16 @@
|
||||
module MailboxHelper
|
||||
include MailboxInlineAttachmentHelper
|
||||
include MailboxSanitizer
|
||||
include ::FileTypeHelper
|
||||
|
||||
private
|
||||
|
||||
def create_message
|
||||
Rails.logger.info "[MailboxHelper] Creating message #{processed_mail.message_id}"
|
||||
return if @conversation.messages.find_by(source_id: processed_mail.message_id).present?
|
||||
source_id = sanitize_mailbox_value(processed_mail.message_id)
|
||||
return if @conversation.messages.find_by(source_id: source_id).present?
|
||||
|
||||
@message = @conversation.messages.create!(
|
||||
account_id: @conversation.account_id,
|
||||
sender: @conversation.contact,
|
||||
content: mail_content&.truncate(150_000),
|
||||
inbox_id: @conversation.inbox_id,
|
||||
message_type: 'incoming',
|
||||
content_type: 'incoming_email',
|
||||
source_id: processed_mail.message_id,
|
||||
content_attributes: {
|
||||
email: processed_mail.serialized_data,
|
||||
cc_email: processed_mail.cc,
|
||||
bcc_email: processed_mail.bcc
|
||||
}
|
||||
)
|
||||
@message = @conversation.messages.create!(sanitized_message_attributes(source_id))
|
||||
end
|
||||
|
||||
def add_attachments_to_message
|
||||
@@ -101,13 +90,16 @@ module MailboxHelper
|
||||
end
|
||||
|
||||
def create_contact
|
||||
sender_email = sanitize_mailbox_value(processed_mail.original_sender)
|
||||
message_id = sanitize_mailbox_value(processed_mail.message_id)
|
||||
|
||||
@contact_inbox = ::ContactInboxWithContactBuilder.new(
|
||||
source_id: processed_mail.original_sender,
|
||||
source_id: sender_email,
|
||||
inbox: @inbox,
|
||||
contact_attributes: {
|
||||
name: identify_contact_name,
|
||||
email: processed_mail.original_sender,
|
||||
additional_attributes: { source_id: "email:#{processed_mail.message_id}" }
|
||||
name: sanitize_mailbox_value(identify_contact_name),
|
||||
email: sender_email,
|
||||
additional_attributes: { source_id: "email:#{message_id}" }
|
||||
}
|
||||
).perform
|
||||
|
||||
|
||||
@@ -0,0 +1,34 @@
|
||||
module MailboxSanitizer
|
||||
NULL_BYTE = "\u0000".freeze
|
||||
|
||||
private
|
||||
|
||||
def sanitized_message_attributes(source_id)
|
||||
{
|
||||
account_id: @conversation.account_id,
|
||||
sender: @conversation.contact,
|
||||
content: sanitize_mailbox_value(mail_content)&.truncate(150_000),
|
||||
inbox_id: @conversation.inbox_id,
|
||||
message_type: 'incoming',
|
||||
content_type: 'incoming_email',
|
||||
source_id: source_id,
|
||||
content_attributes: sanitized_content_attributes
|
||||
}
|
||||
end
|
||||
|
||||
def sanitized_content_attributes
|
||||
sanitize_mailbox_value(
|
||||
email: processed_mail.serialized_data,
|
||||
cc_email: processed_mail.cc,
|
||||
bcc_email: processed_mail.bcc
|
||||
)
|
||||
end
|
||||
|
||||
def sanitize_mailbox_value(value)
|
||||
return value.delete(NULL_BYTE) if value.is_a?(String)
|
||||
return value.map { |item| sanitize_mailbox_value(item) } if value.is_a?(Array)
|
||||
return value.transform_values { |item| sanitize_mailbox_value(item) } if value.is_a?(Hash)
|
||||
|
||||
value
|
||||
end
|
||||
end
|
||||
@@ -1,4 +1,6 @@
|
||||
class Mailbox::ConversationFinderStrategies::BaseStrategy
|
||||
include MailboxSanitizer
|
||||
|
||||
attr_reader :mail
|
||||
|
||||
def initialize(mail)
|
||||
|
||||
@@ -14,7 +14,7 @@ class Mailbox::ConversationFinderStrategies::InReplyToStrategy < Mailbox::Conver
|
||||
def find
|
||||
return nil if mail.in_reply_to.blank?
|
||||
|
||||
in_reply_to_addresses = Array.wrap(mail.in_reply_to)
|
||||
in_reply_to_addresses = sanitize_mailbox_value(Array.wrap(mail.in_reply_to))
|
||||
|
||||
in_reply_to_addresses.each do |in_reply_to|
|
||||
# Try extracting UUID from patterns
|
||||
|
||||
@@ -45,11 +45,11 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
|
||||
end
|
||||
|
||||
def original_sender_email
|
||||
@processed_mail.original_sender&.downcase
|
||||
sanitize_mailbox_value(@processed_mail.original_sender)&.downcase
|
||||
end
|
||||
|
||||
def identify_contact_name
|
||||
@processed_mail.sender_name || @processed_mail.from.first.split('@').first
|
||||
sanitize_mailbox_value(@processed_mail.sender_name || @processed_mail.from.first.split('@').first)
|
||||
end
|
||||
|
||||
def build_conversation
|
||||
@@ -63,7 +63,7 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
|
||||
in_reply_to: in_reply_to,
|
||||
source: 'email',
|
||||
auto_reply: @processed_mail.auto_reply?,
|
||||
mail_subject: @processed_mail.subject,
|
||||
mail_subject: sanitize_mailbox_value(@processed_mail.subject),
|
||||
initiated_at: {
|
||||
timestamp: Time.now.utc
|
||||
}
|
||||
@@ -72,7 +72,7 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
|
||||
end
|
||||
|
||||
def in_reply_to
|
||||
mail['In-Reply-To'].try(:value)
|
||||
sanitize_mailbox_value(mail['In-Reply-To'].try(:value))
|
||||
end
|
||||
|
||||
def find_conversation_by_in_reply_to
|
||||
|
||||
@@ -21,7 +21,7 @@ class Mailbox::ConversationFinderStrategies::ReferencesStrategy < Mailbox::Conve
|
||||
return nil if mail.references.blank?
|
||||
return nil unless @channel # No valid channel found
|
||||
|
||||
references = Array.wrap(mail.references)
|
||||
references = sanitize_mailbox_value(Array.wrap(mail.references))
|
||||
|
||||
references.each do |reference|
|
||||
conversation = find_conversation_from_reference(reference)
|
||||
|
||||
@@ -99,6 +99,33 @@ RSpec.describe Imap::ImapMailbox do
|
||||
end
|
||||
end
|
||||
|
||||
context 'when a new email contains null bytes' do
|
||||
let(:inbound_mail) do
|
||||
Mail.new.tap do |mail|
|
||||
mail.from = 'email@gmail.com'
|
||||
mail.to = 'imap@gmail.com'
|
||||
mail.subject = "Hello\u0000"
|
||||
mail.message_id = "message\u0000@example.com"
|
||||
mail['In-Reply-To'] = "source\u0000@example.com"
|
||||
mail.references = ["reference\u0000@example.com"]
|
||||
mail.content_type = 'text/plain'
|
||||
mail.body = "Body\u0000 text"
|
||||
end
|
||||
end
|
||||
|
||||
it 'creates sanitized conversation and message records' do
|
||||
expect { class_instance.process(inbound_mail, channel) }.to change(Conversation, :count).by(1)
|
||||
|
||||
message = conversation.messages.last
|
||||
|
||||
expect(conversation.additional_attributes['in_reply_to']).to eq('source@example.com')
|
||||
expect(conversation.additional_attributes['mail_subject']).to eq('Hello')
|
||||
expect(message.source_id).to eq('message@example.com')
|
||||
expect(message.content).to eq('Body text')
|
||||
expect(message.content_attributes.to_json).not_to include('\u0000')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when a new email with invalid from' do
|
||||
let(:inbound_mail) { create_inbound_email_from_mail(from: 'invalidemail', to: 'imap@gmail.com', subject: 'Hello!') }
|
||||
|
||||
|
||||
@@ -47,6 +47,31 @@ RSpec.describe MailboxHelper do
|
||||
helper_instance.send(:create_message)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when message data contains null bytes' do
|
||||
let(:mail) do
|
||||
mail = Mail.new
|
||||
mail.from = 'Sender <sender@example.com>'
|
||||
mail.to = 'Inbox <inbox@example.com>'
|
||||
mail.subject = "Hello\u0000"
|
||||
mail.message_id = "message\u0000@example.com"
|
||||
mail.content_type = 'text/plain'
|
||||
mail.body = "Body\u0000 text"
|
||||
mail
|
||||
end
|
||||
|
||||
it 'creates the message with sanitized values' do
|
||||
helper_instance = mailbox_helper_obj.new(conversation, processed_mail)
|
||||
|
||||
expect { helper_instance.send(:create_message) }.to change(conversation.messages, :count).by(1)
|
||||
|
||||
message = conversation.messages.last
|
||||
expect(message.source_id).to eq('message@example.com')
|
||||
expect(message.content).to eq('Body text')
|
||||
expect(message.content_attributes.dig('email', 'message_id')).to eq('message@example.com')
|
||||
expect(message.content_attributes.to_json).not_to include('\u0000')
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '#embed_plain_text_email_with_inline_image' do
|
||||
|
||||
@@ -67,6 +67,40 @@ RSpec.describe ReplyMailbox do
|
||||
end
|
||||
end
|
||||
|
||||
context 'when new conversation email contains null bytes' do
|
||||
let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) }
|
||||
let(:null_byte_mail) { create_inbound_email_from_mail(from: 'sender@example.com', to: email_channel.email, subject: 'Hello') }
|
||||
let(:mail_with_null_bytes) do
|
||||
Mail.new.tap do |mail|
|
||||
mail.from = 'sender@example.com'
|
||||
mail.to = email_channel.email
|
||||
mail.subject = "Hello\u0000"
|
||||
mail.message_id = "message\u0000@example.com"
|
||||
mail['In-Reply-To'] = "source\u0000@example.com"
|
||||
mail.references = ["reference\u0000@example.com"]
|
||||
mail.content_type = 'text/plain'
|
||||
mail.body = "Body\u0000 text"
|
||||
end
|
||||
end
|
||||
|
||||
before do
|
||||
allow(null_byte_mail).to receive(:mail).and_return(mail_with_null_bytes)
|
||||
end
|
||||
|
||||
it 'creates sanitized conversation and message records' do
|
||||
expect { described_class.receive null_byte_mail }.to change(Conversation, :count).by(1)
|
||||
|
||||
conversation = Conversation.last
|
||||
message = conversation.messages.last
|
||||
|
||||
expect(conversation.additional_attributes['in_reply_to']).to eq('source@example.com')
|
||||
expect(conversation.additional_attributes['mail_subject']).to eq('Hello')
|
||||
expect(message.source_id).to eq('message@example.com')
|
||||
expect(message.content).to eq('Body text')
|
||||
expect(message.content_attributes.to_json).not_to include('\u0000')
|
||||
end
|
||||
end
|
||||
|
||||
context 'with inline attachments' do
|
||||
let(:mail_with_inline_images) { create_inbound_email_from_fixture('mail_with_inline_images.eml') }
|
||||
let(:described_subject) { described_class.receive mail_with_inline_images }
|
||||
|
||||
Reference in New Issue
Block a user