Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] tools.mail: ignore original email during encapsulation #79979

Closed
wants to merge 1 commit into from

Conversation

odony
Copy link
Contributor

@odony odony commented Nov 17, 2021

When the system broadcasts an email response to document followers, if the config parameters mail.force.smtp.from or mail.dynamic.smtp.from are defined, it will rewrite the From address to avoid spoofing the sender's domain.

For example, if the mail.catchall.domain is set to example.com and an email response comes from:

"John D" <john@doe.com>

it will rewrite it to:

"John D (john@doe.com)" <notifications@example.com>

This will make sure the system never sends outgoing email for an external domain, as it has no authority for doing so, and that could break mail filtering/authentication rules (SPF, DMARC, etc.)

During this "encapsulation rewrite step", both the original Sender name and their email are preserved, and put into the quoted "name" field of the rewritten address. It seems sensible to preserve as much information as possible about the original sender.

Unfortunately, the inclusion of the Sender email in the final name makes it appear to some inbox providers as if the message is trying to deceptively impersonate another person (as many phishing schemes would).
As of November 2021 GMail at least does this, and will hide the name in the UI when it happens. It will keep only the rewritten email, which is not very useful in the case of a notification (even though it's more technically correct, of course).

This patch removes the original email from the rewritten notification, keeping only the name, considering that the email is not the most important part, and it's better to have one of the two than none.

So after the patch, the rewritten address is now:

"John D" <notifications@example.com>

When there is no name in the original address, we keep only the local part of the email, to avoid the same display issue. The recipient will have to identify the sender based on the context / past messages.

@robodoo
Copy link
Contributor

robodoo commented Nov 17, 2021

Pull request status dashboard

@odony odony requested a review from std-odoo November 17, 2021 17:57
@C3POdoo C3POdoo requested a review from a team November 17, 2021 17:58
@C3POdoo C3POdoo added the RD research & development, internal work label Nov 17, 2021
@pedrobaeza
Copy link
Collaborator

Shouldn't this be applied to v13 as well as the system parameters exists in that version?

@odony odony requested review from tde-banana-odoo and removed request for a team November 17, 2021 18:03
@odony odony marked this pull request as draft November 17, 2021 18:04
@odony
Copy link
Contributor Author

odony commented Nov 17, 2021

Shouldn't this be applied to v13 as well as the system parameters exists in that version?

Certainly yes, though at this point I'm mostly testing the idea ;)

It seems GMail does not do this all the time, but only when the mail went through an external domain (e.g. when it displays a "via ..." line), or when the mail is generally not trusted. In that case an optional config might be preferrable 🤔

@pedrobaeza
Copy link
Collaborator

I don't see bad to hide the email address also for privacy questions, so this should be if not always, at least by a configuration option.

@pedrobaeza
Copy link
Collaborator

And also as strategy: showing your email tempts receivers to take that email and write directly to that one instead of replying and passing through Odoo hub.

odoo/tools/mail.py Outdated Show resolved Hide resolved
odoo/tools/mail.py Outdated Show resolved Hide resolved
odoo/tools/mail.py Outdated Show resolved Hide resolved
odoo/addons/base/tests/test_mail.py Outdated Show resolved Hide resolved
Copy link
Contributor

@std-odoo std-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi :)

Good catch for this one :) what about still including the email but with [at] instead of @ ? (so gmail might not detect the name as an email and remove it)

odoo/tools/mail.py Outdated Show resolved Hide resolved
@odony odony force-pushed the 14.0-encapsulate-noemail-odo branch from 1c9ea1e to c260f98 Compare December 7, 2021 17:47
@odony odony marked this pull request as ready for review December 15, 2021 12:36
When the system broadcasts an email response to document followers,
if the config parameters `mail.force.smtp.from` or
`mail.dynamic.smtp.from` are defined, it will rewrite the `From`
address to avoid spoofing the sender's domain.

For example, if the `mail.catchall.domain` is set to `example.com` and
an email response comes from:

   "John D" <john@doe.com>

it will rewrite it to:

   "John D (john@doe.com)" <notifications@example.com>

This will make sure the system never sends outgoing email for an external
domain, as it has no authority for doing so, and that could
break mail filtering/authentication rules (SPF, DMARC, etc.)

During this "encapsulation rewrite step", both the original Sender name
and their email are preserved, and put into the quoted "name" field of
the rewritten address. It seems sensible to preserve as much information
as possible about the original sender.

Unfortunately, the inclusion of the Sender email in the final name makes
it appear to some inbox providers as if the message is trying to
deceptively impersonate another person (as many phishing schemes would).
As of November 2021 GMail at least does this, and will hide the name in
the UI when it happens. It will keep only the rewritten email, which is not
very useful in the case of a notification (even though it's more
technically correct, of course).

This patch removes the original email from the rewritten notification,
keeping only the name, considering that the email is not the most
important part, and it's better to have one of the two than none.

So after the patch, the rewritten address is now:

   "John D" <notifications@example.com>

When there is no name in the original address, we keep only the local
part of the email, to avoid the same display issue. The recipient will
have to identify the sender based on the context / past messages.
@odony odony force-pushed the 14.0-encapsulate-noemail-odo branch from c260f98 to d9cc4c5 Compare December 15, 2021 12:38
@odony
Copy link
Contributor Author

odony commented Dec 15, 2021

Thanks for the feedback! Removed the cruft, dropped the useless option to keep the old behavior (don't think it's useful indeed), and updated the commit msg/PR desc and tests.

I think we can proceed with this small fix if nobody has any blocking issue with it. It will definitely help decrease the risk of being detected as a spam or a tentative spoof by the various email service providers.

PS: For the 15.0 port, the logic for triggering the email rewrite is different (based on the from_filter of the mail server rather than only ICPs), but the code for the rewrite is the same, so the patch will apply cleanly in tools.mail. Of course the tests won't ;)

@odony
Copy link
Contributor Author

odony commented Dec 15, 2021

what about still including the email but with [at] instead of @ ? (so gmail might not detect the name as an email and remove it)

I chose to only keep the local part of the email instead, e.g if we only have an email foo@bar.com, then the rewritten name-email pair will become "foo" <notifications@example.com>.
I think it's safer than trying to disguise the @ sign and being eventually detected as a spoofing attempt, in some future update of the secret heuristics of email service providers :)

@odony
Copy link
Contributor Author

odony commented Dec 15, 2021

@robodoo r+ please

robodoo pushed a commit that referenced this pull request Dec 15, 2021
When the system broadcasts an email response to document followers,
if the config parameters `mail.force.smtp.from` or
`mail.dynamic.smtp.from` are defined, it will rewrite the `From`
address to avoid spoofing the sender's domain.

For example, if the `mail.catchall.domain` is set to `example.com` and
an email response comes from:

   "John D" <john@doe.com>

it will rewrite it to:

   "John D (john@doe.com)" <notifications@example.com>

This will make sure the system never sends outgoing email for an external
domain, as it has no authority for doing so, and that could
break mail filtering/authentication rules (SPF, DMARC, etc.)

During this "encapsulation rewrite step", both the original Sender name
and their email are preserved, and put into the quoted "name" field of
the rewritten address. It seems sensible to preserve as much information
as possible about the original sender.

Unfortunately, the inclusion of the Sender email in the final name makes
it appear to some inbox providers as if the message is trying to
deceptively impersonate another person (as many phishing schemes would).
As of November 2021 GMail at least does this, and will hide the name in
the UI when it happens. It will keep only the rewritten email, which is not
very useful in the case of a notification (even though it's more
technically correct, of course).

This patch removes the original email from the rewritten notification,
keeping only the name, considering that the email is not the most
important part, and it's better to have one of the two than none.

So after the patch, the rewritten address is now:

   "John D" <notifications@example.com>

When there is no name in the original address, we keep only the local
part of the email, to avoid the same display issue. The recipient will
have to identify the sender based on the context / past messages.

closes #79979

Signed-off-by: Olivier Dony <odo@odoo.com>
@robodoo robodoo closed this Dec 15, 2021
@robodoo robodoo temporarily deployed to merge December 15, 2021 14:09 Inactive
@fw-bot
Copy link
Contributor

fw-bot commented Dec 19, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #81473

3 similar comments
@fw-bot
Copy link
Contributor

fw-bot commented Dec 20, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #81473

@fw-bot
Copy link
Contributor

fw-bot commented Dec 21, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #81473

@fw-bot
Copy link
Contributor

fw-bot commented Dec 22, 2021

This pull request has forward-port PRs awaiting action (not merged or closed): #81473

@fw-bot fw-bot deleted the 14.0-encapsulate-noemail-odo branch December 29, 2021 14:46
odooaktiv pushed a commit to odooaktiv/odoo that referenced this pull request Jun 13, 2022
When the system broadcasts an email response to document followers,
if the config parameters `mail.force.smtp.from` or
`mail.dynamic.smtp.from` are defined, it will rewrite the `From`
address to avoid spoofing the sender's domain.

For example, if the `mail.catchall.domain` is set to `example.com` and
an email response comes from:

   "John D" <john@doe.com>

it will rewrite it to:

   "John D (john@doe.com)" <notifications@example.com>

This will make sure the system never sends outgoing email for an external
domain, as it has no authority for doing so, and that could
break mail filtering/authentication rules (SPF, DMARC, etc.)

During this "encapsulation rewrite step", both the original Sender name
and their email are preserved, and put into the quoted "name" field of
the rewritten address. It seems sensible to preserve as much information
as possible about the original sender.

Unfortunately, the inclusion of the Sender email in the final name makes
it appear to some inbox providers as if the message is trying to
deceptively impersonate another person (as many phishing schemes would).
As of November 2021 GMail at least does this, and will hide the name in
the UI when it happens. It will keep only the rewritten email, which is not
very useful in the case of a notification (even though it's more
technically correct, of course).

This patch removes the original email from the rewritten notification,
keeping only the name, considering that the email is not the most
important part, and it's better to have one of the two than none.

So after the patch, the rewritten address is now:

   "John D" <notifications@example.com>

When there is no name in the original address, we keep only the local
part of the email, to avoid the same display issue. The recipient will
have to identify the sender based on the context / past messages.

closes odoo#79979

Signed-off-by: Olivier Dony <odo@odoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants