From 143cbeeba759ca3c430618c8c788c9dc6db80963 Mon Sep 17 00:00:00 2001 From: jorv-odoo Date: Tue, 9 Apr 2024 10:03:18 +0200 Subject: [PATCH] [FIX] mail: limit reply-to length to prevent miss-folding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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" \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) --- addons/mail/models/models.py | 25 ++++++++++++++++----- addons/test_mail/tests/test_mail_gateway.py | 4 ++++ addons/test_mail/tests/test_mail_message.py | 21 ++++++++--------- addons/test_mail/tests/test_performance.py | 7 ++++-- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/addons/mail/models/models.py b/addons/mail/models/models.py index 3da323376b8..ddfbb9a4b11 100644 --- a/addons/mail/models/models.py +++ b/addons/mail/models/models.py @@ -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 diff --git a/addons/test_mail/tests/test_mail_gateway.py b/addons/test_mail/tests/test_mail_gateway.py index 9c09aa26afe..bc16c9296fe 100644 --- a/addons/test_mail/tests/test_mail_gateway.py +++ b/addons/test_mail/tests/test_mail_gateway.py @@ -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', diff --git a/addons/test_mail/tests/test_mail_message.py b/addons/test_mail/tests/test_mail_message.py index 32452e8b9aa..ca74c97c9d8 100644 --- a/addons/test_mail/tests/test_mail_message.py +++ b/addons/test_mail/tests/test_mail_message.py @@ -141,11 +141,12 @@ class TestMessageValues(MailCommon): 'image0

'.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 diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 40c3a49cc82..5d7d62e7576 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -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) ])