fix(mailbox): handle malformed sender address headers (#13486)
## How to reproduce When an inbound email has malformed sender headers (for example `From: McDonald <info@example.com` without a closing `>`), mailbox processing can raise `Mail::Field::IncompleteParseError` while resolving sender data in `MailPresenter`. ## What changed This PR hardens sender parsing in `MailPresenter` with a small, readable implementation: - Added/used a safe parser (`parse_mail_address`) that rescues `Mail::Field::ParseError` and `Mail::Field::IncompleteParseError`. - `sender_name` now uses the same safe parser path. - `original_sender` now resolves candidates in order via a compact `filter_map` flow: - `Reply-To` - `X-Original-Sender` - `From` - All three candidates are parsed as email addresses before use (including `X-Original-Sender`), and invalid values are ignored. - `notification_email_from_chatwoot?` now compares sender addresses case-insensitively (`casecmp?`) to avoid case-only mismatches. ## Test coverage Added focused presenter specs for: - malformed `From` header returns nil sender values and does not classify as notification sender - malformed `Reply-To` falls back to valid `From` - valid `X-Original-Sender` is used when present - invalid `X-Original-Sender` falls back to valid `From` - mixed-case sender address still matches configured `MAILER_SENDER_EMAIL` ## How this was tested Ran: - `bundle exec rspec spec/presenters/mail_presenter_spec.rb` - `bundle exec rubocop app/presenters/mail_presenter.rb spec/presenters/mail_presenter_spec.rb` Sentry issue: [CHATWOOT-B9Y](https://chatwoot-p3.sentry.io/issues/7005483640/)
This commit is contained in:
@@ -130,11 +130,15 @@ class MailPresenter < SimpleDelegator
|
||||
end
|
||||
|
||||
def sender_name
|
||||
Mail::Address.new((@mail[:reply_to] || @mail[:from]).value).name
|
||||
parse_mail_address((@mail[:reply_to] || @mail[:from]).value)&.name
|
||||
end
|
||||
|
||||
def original_sender
|
||||
from_email_address(@mail[:reply_to].try(:value)) || @mail['X-Original-Sender'].try(:value) || from_email_address(from.first)
|
||||
[
|
||||
@mail[:reply_to]&.value,
|
||||
@mail['X-Original-Sender']&.value,
|
||||
@mail[:from]&.value
|
||||
].filter_map { |email| parse_mail_address(email)&.address }.first
|
||||
end
|
||||
|
||||
def headers_data
|
||||
@@ -147,10 +151,6 @@ class MailPresenter < SimpleDelegator
|
||||
headers.presence
|
||||
end
|
||||
|
||||
def from_email_address(email)
|
||||
Mail::Address.new(email).address
|
||||
end
|
||||
|
||||
def email_forwarded_for
|
||||
@mail['X-Forwarded-For'].try(:value)
|
||||
end
|
||||
@@ -175,11 +175,20 @@ class MailPresenter < SimpleDelegator
|
||||
|
||||
def notification_email_from_chatwoot?
|
||||
# notification emails are send via mailer sender email address. so it should match
|
||||
original_sender == Mail::Address.new(ENV.fetch('MAILER_SENDER_EMAIL', 'Chatwoot <accounts@chatwoot.com>')).address
|
||||
configured_sender = Mail::Address.new(ENV.fetch('MAILER_SENDER_EMAIL', 'Chatwoot <accounts@chatwoot.com>')).address
|
||||
original_sender.to_s.casecmp?(configured_sender)
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def parse_mail_address(email)
|
||||
return if email.blank?
|
||||
|
||||
Mail::Address.new(email)
|
||||
rescue Mail::Field::ParseError, Mail::Field::IncompleteParseError
|
||||
nil
|
||||
end
|
||||
|
||||
def auto_submitted?
|
||||
@mail['Auto-Submitted'].present? && @mail['Auto-Submitted'].value != 'no'
|
||||
end
|
||||
|
||||
@@ -178,5 +178,85 @@ RSpec.describe MailPresenter do
|
||||
expect(decorated_mail.serialized_data[:auto_reply]).to be_falsey
|
||||
end
|
||||
end
|
||||
|
||||
describe 'malformed sender headers' do
|
||||
let(:mail_with_malformed_from) do
|
||||
Mail.new do
|
||||
header['From'] = 'Kevin McDonald <info@example.com'
|
||||
to 'Inbox <inbox@example.com>'
|
||||
subject :header
|
||||
body 'Hi'
|
||||
end
|
||||
end
|
||||
|
||||
let(:mail_with_malformed_reply_to) do
|
||||
Mail.new do
|
||||
from 'Sender <sender@example.com>'
|
||||
to 'Inbox <inbox@example.com>'
|
||||
subject :header
|
||||
body 'Hi'
|
||||
header['Reply-To'] = 'Reply User <reply@example.com'
|
||||
end
|
||||
end
|
||||
|
||||
let(:mail_with_original_sender_header) do
|
||||
Mail.new do
|
||||
from 'Sender <sender@example.com>'
|
||||
to 'Inbox <inbox@example.com>'
|
||||
subject :header
|
||||
body 'Hi'
|
||||
header['Reply-To'] = 'Reply User <reply@example.com'
|
||||
header['X-Original-Sender'] = 'Forwarded Sender <forwarded.sender@example.com>'
|
||||
end
|
||||
end
|
||||
|
||||
let(:mail_with_invalid_original_sender_header) do
|
||||
Mail.new do
|
||||
from 'Sender <sender@example.com>'
|
||||
to 'Inbox <inbox@example.com>'
|
||||
subject :header
|
||||
body 'Hi'
|
||||
header['Reply-To'] = 'Reply User <reply@example.com'
|
||||
header['X-Original-Sender'] = 'not an email address'
|
||||
end
|
||||
end
|
||||
|
||||
it 'returns nil sender values when from header is malformed' do
|
||||
presenter = described_class.new(mail_with_malformed_from)
|
||||
|
||||
expect(presenter.original_sender).to be_nil
|
||||
expect(presenter.sender_name).to be_nil
|
||||
expect(presenter.notification_email_from_chatwoot?).to be(false)
|
||||
end
|
||||
|
||||
it 'falls back to from header when reply_to is malformed' do
|
||||
presenter = described_class.new(mail_with_malformed_reply_to)
|
||||
expect(presenter.original_sender).to eq('sender@example.com')
|
||||
end
|
||||
|
||||
it 'uses parsed X-Original-Sender value when available' do
|
||||
presenter = described_class.new(mail_with_original_sender_header)
|
||||
expect(presenter.original_sender).to eq('forwarded.sender@example.com')
|
||||
end
|
||||
|
||||
it 'falls back to from when X-Original-Sender is invalid' do
|
||||
presenter = described_class.new(mail_with_invalid_original_sender_header)
|
||||
expect(presenter.original_sender).to eq('sender@example.com')
|
||||
end
|
||||
|
||||
it 'matches notification sender emails case-insensitively' do
|
||||
mail_with_uppercase_sender = Mail.new do
|
||||
from 'Chatwoot <ACCOUNTS@CHATWOOT.COM>'
|
||||
to 'Inbox <inbox@example.com>'
|
||||
subject :header
|
||||
body 'Hi'
|
||||
end
|
||||
|
||||
with_modified_env MAILER_SENDER_EMAIL: 'Chatwoot <accounts@chatwoot.com>' do
|
||||
presenter = described_class.new(mail_with_uppercase_sender)
|
||||
expect(presenter.notification_email_from_chatwoot?).to be(true)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user