[FIX] mail: limit reply-to length to prevent miss-folding
Context In a previous fix https://github.com/odoo/odoo/pull/83276 , the `_notify_get_reply_to_formatted_email` method was introduced to prevent edge-cases where the cypthon `email` library might incorrectly fold the “Reply-To” email header. We identified an other corner case where DKIM signature verification might fail on the recipient’s end depending on the tech stack used to verify the DKIM signature vs the one used to DKIM sign it on the sending end. This might be related to how RFC5322 and RFC6376 interact with each other : In RFC5322 defines folding white spaces as follows : ``` FWS = ([*WSP CRLF] 1*WSP) / obs-FWS with obsolete FWS = 1*WSP *(CRLF 1*WSP) ``` While RFC6376 uses : `FWS = [*WSP CRLF] 1*WSP ` Based on this, it seems that for proper header content folding, the specifications expects at least one WSP (space or tab) before a CRLF. Currently when using the `email` cpython library to handle email objects, we observed that when the header value for the “Reply-To” is longer than 68 characters, it will return a folded string representation adding a linebreak after the colon. Example: `Reply-To:\r\n "Marc R.Long Name Jonhson" <catchall@very.long.subdomain@example.com>\r\n` Notice that the there is no WSP between the colon character an `\r\n`. It seems that in this corner case, certain DKIM verification tech stacks (from tests Microsoft Outlook and Rspamd) will miss-read the “Reply-to” header as empty, while others correct for it (Gmail). This in returns leads to the DKIM signature verification failing. As it is impossible to test every possible combination of DKIM tech stacks in the email ecosystem and that until the `email` cypython library handles this corner case correctly, this fix tries to preformat the “Reply-To” more defensively. We also print a warning log if the `record_email` alone is longer than 68 characters (as it will not be folded), inviting the user to shorten it to prevent DKIM verification issues. Unit test fixing: - shortened alias name to prevent the 68 character limit from being triggered during the test_notification_reply_to_batch performance test (as we are not testing the 68 char here) - changed language in test_mail_message_values_fromto_long_name to reflect the new 68 character limit and mutted logger, as it now print a warning message Considerations for the future This PR only prevents the “Reply-To” from being malformed. In theory, the miss-folding could happen to any email header constructed using the `email` cpython library. In practice, the probability of the “To” and “From” being affected is low, as they usually don’t exceed a total of 78 characters as per RFC. Nevertheless if the future shows that this might be a bigger issue, one should think about: * On Odoo side: add a more robust header value formatting applied to all affected headers before the email gets sent out * On Python’s side: work with the `email` library maintainer to find a longterm solution opw-3826296 fixed test and conflicts in 17.0 FW closes odoo/odoo#161014 X-original-commit: 894f981d04195861e757c052b7d10155cb5975b4 Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
This commit is contained in:
jorv-odoo
@@ -8,6 +8,9 @@ from markupsafe import Markup
|
||||
from odoo import api, models, tools, _
|
||||
from odoo.addons.mail.tools.alias_error import AliasError
|
||||
|
||||
import logging
|
||||
|
||||
_logger = logging.getLogger(__name__)
|
||||
|
||||
class BaseModel(models.AbstractModel):
|
||||
_inherit = 'base'
|
||||
@@ -290,9 +293,13 @@ class BaseModel(models.AbstractModel):
|
||||
a bad management of quotes (missing quotes after refold). This appears
|
||||
therefore only when having quotes (aka not simple names, and not when
|
||||
being unicode encoded).
|
||||
Another edge-case produces a linebreak (CRLF) immediately after the
|
||||
colon character separating the header name from the header value.
|
||||
This creates an issue in certain DKIM tech stacks that will
|
||||
incorrectly read the reply-to value as empty and fail the verification.
|
||||
|
||||
To avoid that issue when formataddr would return more than 78 chars we
|
||||
return a simplified name/email to try to stay under 78 chars. If not
|
||||
To avoid that issue when formataddr would return more than 68 chars we
|
||||
return a simplified name/email to try to stay under 68 chars. If not
|
||||
possible we return only the email and skip the formataddr which causes
|
||||
the issue in python. We do not use hacks like crop the name part as
|
||||
encoding and quoting would be error prone.
|
||||
@@ -301,8 +308,14 @@ class BaseModel(models.AbstractModel):
|
||||
complete name in formataddr. Otherwise fallback on 'company_id'
|
||||
of self or environment company;
|
||||
"""
|
||||
# address itself is too long for 78 chars limit: return only email
|
||||
if len(record_email) >= 78:
|
||||
length_limit = 68 # 78 - len('Reply-To: '), 78 per RFC
|
||||
# address itself is too long : return only email and log warning
|
||||
if len(record_email) >= length_limit:
|
||||
_logger.warning('Notification email address for reply-to is longer than 68 characters. '
|
||||
'This might create non-compliant folding in the email header in certain DKIM '
|
||||
'verification tech stacks. It is advised to shorten it if possible. '
|
||||
'Record name (if set): %s '
|
||||
'Reply-To: %s ', record_name, record_email)
|
||||
return record_email
|
||||
|
||||
if not company:
|
||||
@@ -315,9 +328,9 @@ class BaseModel(models.AbstractModel):
|
||||
name = f"{company.name} {record_name}" if record_name else company.name
|
||||
|
||||
formatted_email = tools.formataddr((name, record_email))
|
||||
if len(formatted_email) > 78:
|
||||
if len(formatted_email) > length_limit:
|
||||
formatted_email = tools.formataddr((record_name or company.name, record_email))
|
||||
if len(formatted_email) > 78:
|
||||
if len(formatted_email) > length_limit:
|
||||
formatted_email = record_email
|
||||
return formatted_email
|
||||
|
||||
|
||||
@@ -1557,6 +1557,10 @@ class TestMailgateway(MailCommon):
|
||||
@mute_logger('odoo.addons.mail.models.mail_thread', 'odoo.models.unlink', 'odoo.addons.mail.models.mail_mail')
|
||||
def test_message_process_reply_to_new_thread(self):
|
||||
""" Test replies not being considered as replies but use destination information instead (aka, mass post + specific reply to using aliases) """
|
||||
# shorten company name to prevent 68 character formatting from
|
||||
# triggering and making the assert missmatch.
|
||||
# See _notify_get_reply_to_formatted_email method
|
||||
self.user_employee.company_id.name = "Forced"
|
||||
first_record = self.env['mail.test.simple'].with_user(self.user_employee).create({'name': 'Replies to Record'})
|
||||
record_msg = first_record.message_post(
|
||||
subject='Discussion',
|
||||
|
||||
@@ -141,11 +141,12 @@ class TestMessageValues(MailCommon):
|
||||
'<img src="/web/image/{attachment.id}?access_token={attachment.access_token}" alt="image0" width="2"></p>'.format(attachment=msg.attachment_ids[0])
|
||||
)
|
||||
|
||||
@mute_logger('odoo.models.unlink')
|
||||
@mute_logger('odoo.models.unlink', 'odoo.addons.mail.models.models')
|
||||
@users('employee')
|
||||
def test_mail_message_values_fromto_long_name(self):
|
||||
""" Long headers may break in python if above 78 chars as folding is not
|
||||
done correctly (see ``_notify_get_reply_to_formatted_email`` docstring
|
||||
""" Long headers may break in python if above 68 chars for certain
|
||||
DKIM verification stacks as folding is not done correctly
|
||||
(see ``_notify_get_reply_to_formatted_email`` docstring
|
||||
+ commit linked to this test). """
|
||||
# name would make it blow up: keep only email
|
||||
test_record = self.env['mail.test.container'].browse(self.alias_record.ids)
|
||||
@@ -158,17 +159,17 @@ class TestMessageValues(MailCommon):
|
||||
})
|
||||
reply_to_email = f"{test_record.alias_name}@{self.alias_domain}"
|
||||
self.assertEqual(msg.reply_to, reply_to_email,
|
||||
'Reply-To: use only email when formataddr > 78 chars')
|
||||
'Reply-To: use only email when formataddr > 68 chars')
|
||||
|
||||
# name + company_name would make it blow up: keep record_name in formatting
|
||||
self.company_admin.name = "Company name being about 33 chars"
|
||||
test_record.write({'name': 'Name that would be more than 78 with company name'})
|
||||
test_record.write({'name': 'Being more than 68 with company name'})
|
||||
msg = self.env['mail.message'].create({
|
||||
'model': test_record._name,
|
||||
'res_id': test_record.id
|
||||
})
|
||||
self.assertEqual(msg.reply_to, formataddr((test_record.name, reply_to_email)),
|
||||
'Reply-To: use recordname as name in format if recordname + company > 78 chars')
|
||||
'Reply-To: use recordname as name in format if recordname + company > 68 chars')
|
||||
|
||||
# no record_name: keep company_name in formatting if ok
|
||||
test_record.write({'name': ''})
|
||||
@@ -177,7 +178,7 @@ class TestMessageValues(MailCommon):
|
||||
'res_id': test_record.id
|
||||
})
|
||||
self.assertEqual(msg.reply_to, formataddr((self.env.user.company_id.name, reply_to_email)),
|
||||
'Reply-To: use company as name in format when no record name and still < 78 chars')
|
||||
'Reply-To: use company as name in format when no record name and still < 68 chars')
|
||||
|
||||
# no record_name and company_name make it blow up: keep only email
|
||||
self.env.user.company_id.write({'name': 'Super Long Name That People May Enter "Even with an internal quoting of stuff"'})
|
||||
@@ -186,15 +187,15 @@ class TestMessageValues(MailCommon):
|
||||
'res_id': test_record.id
|
||||
})
|
||||
self.assertEqual(msg.reply_to, reply_to_email,
|
||||
'Reply-To: use only email when formataddr > 78 chars')
|
||||
'Reply-To: use only email when formataddr > 68 chars')
|
||||
|
||||
# whatever the record and company names, email is too long: keep only email
|
||||
test_record.write({
|
||||
'alias_name': 'Waaaay too long alias name that should make any reply-to blow the 78 characters limit',
|
||||
'alias_name': 'Waaaay too long alias name that should make any reply-to blow the 68 characters limit',
|
||||
'name': 'Short',
|
||||
})
|
||||
self.env.user.company_id.write({'name': 'Comp'})
|
||||
sanitized_alias_name = 'waaaay-too-long-alias-name-that-should-make-any-reply-to-blow-the-78-characters-limit'
|
||||
sanitized_alias_name = 'waaaay-too-long-alias-name-that-should-make-any-reply-to-blow-the-68-characters-limit'
|
||||
msg = self.env['mail.message'].create({
|
||||
'model': test_record._name,
|
||||
'res_id': test_record.id
|
||||
|
||||
@@ -739,10 +739,13 @@ class TestBaseAPIPerformance(BaseMailPerformance):
|
||||
@users('admin', 'employee')
|
||||
@warmup
|
||||
def test_notification_reply_to_batch(self):
|
||||
# overwrite company name to keep it short/simple
|
||||
# and not trigger the 68 character reply_to formatting
|
||||
self.env.user.company_id.name = "Forced"
|
||||
test_records_sudo = self.env['mail.test.container'].sudo().create([
|
||||
{'alias_name': 'alias.test.%s.%d' % (self.env.user.name, index),
|
||||
{'alias_name': 'a.%s.%d' % (self.env.user.name, index),
|
||||
'customer_id': self.customer.id,
|
||||
'name': 'Test_%d' % index,
|
||||
'name': 'T_%d' % index,
|
||||
} for index in range(10)
|
||||
])
|
||||
|
||||
|
||||
Reference in New Issue
Block a user