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

closes core#4070 - respect default return path #25309

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Jan 10, 2023

https://lab.civicrm.org/dev/core/-/issues/4070

Overview

Administer » CiviMail » Mail Accounts allows you to specify a default return path header for your emails. However, FlexMailer ignores it.

Replication Steps

  • Set up your bounce address account, setting the Return-Path value.
  • Send a CiviMail.
  • Check the Return-Path header.

Before

Same value as if you had no Return-Path set.

After

Return-Path matches the default set on the mail account.

@civibot civibot bot added the master label Jan 10, 2023
@civibot
Copy link

civibot bot commented Jan 10, 2023

(Standard links)

@MegaphoneJon
Copy link
Contributor Author

@spalmstr While working on this issue, I saw your post on answers.microsoft.com asking about VERP support.

If you a) apply this patch, b) set a default Return-Path of your bounce address in Administer » CiviMail » Mail Accounts then you don't need VERP - the bounces will be sent to the Return-Path address, and there's an alternate header X-CiviMail-Bounce that contains the original VERP address, which Civi will process.

@spalmstr
Copy link
Contributor

@MegaphoneJon Wonderful, that is great, but did this patch actually get applied as the check failed? I did a

composer update

earlier today on my development environment that pulls, I think dev:master, and I didn't see that the patch had been applied.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@MegaphoneJon
Copy link
Contributor Author

test this please

@spalmstr
Copy link
Contributor

spalmstr commented Feb 2, 2023

I have at last got round to doing some testing and verified that the Return Path is set to the one expected. However, Exchange overwrites this with the From address. So, if you are sending from a different From address to the default return path, bounces end up in that From address's inbox, not the one configured to receive bounced emails. This is, I suspect, a failing of Exchange, not CiviCRM.

@MegaphoneJon
Copy link
Contributor Author

@spalmstr I assume that means you are both sending from Exchange and your bounce address is an Exchange mailbox?

I tested this sending via Postfix but with the bounce address being an Exchange mailbox, and that worked, just want to confirm we didn't get different results for the same test.

@spalmstr
Copy link
Contributor

spalmstr commented Feb 3, 2023

I tested using a different From address going via MS Exchange. In each case, the Return-Path was set to the From address, not the Return-Path set by CiviCRM. Both From and Return-Path addresses are MS Exchange ones. This is a bit disappointing - with physical mail there may be different address to which to send undelivered mail to the address of the sender. It means that we have to make sure that all CiviMail jobs are sent from the same address. The CiviCRM bounce tracking is useful for tracking.

I had hoped that since MS Exchange Plus Addressing became the default that we could have Return-Paths of the form [email protected], but it seems not to be possible. I tweaked the code to set the Return-Path to that, but as noted above, it made no difference.

Thanks, though, for your efforts. It looks a useful change.

@totten
Copy link
Member

totten commented Feb 21, 2023

If I'm reading correctly, it sounds like the testing showed that (1) the patch does get Civi to behave well wrt setting Return-Path but (2) some MTAs (MS Exchange) don't respect the requested Return-Path.

Aside: The conception of CRM_Core_BAO_MailSettings::defaultReturnPath() feels weird -- like it probably works in the case where 1 === #outbound mail-servers === #inbound mail-servers (which is probably typical - but doesn't match the multiplicity of configuration records). Regardless, in grepping, this seems to be the approach in pre-existing senders -- and the patch simply makes flexmailer behave like the rest of Civi.

Those issues don't sound like they should block 25309. I'm gonna flag merge ready - but if @spalmstr or @MegaphoneJon see any issue with my understanding, then we can pull back.

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Feb 21, 2023
@spalmstr
Copy link
Contributor

I have no issue. It seems Exchange tweaks the Return Path.

@demeritcowboy demeritcowboy merged commit 5a6309a into civicrm:master Feb 27, 2023
@MegaphoneJon MegaphoneJon deleted the flexmailer-return-path branch June 22, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants