Skip to content

Commit 5e46b81

Browse files
martinknafvemartinknafve
authored
Use empty envelope address where possible (#514)
1 parent 5a5015b commit 5e46b81

10 files changed

Lines changed: 125 additions & 41 deletions

File tree

‎hmailserver/source/Server/Common/AntiSpam/SpamTestSpamAssassin.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ namespace HM
6666
// SpamAssassin default rules and custom rules also rely on Return-Path header being present
6767
// We delete this header again after SpamAssassin checking has completed
6868
std::vector<std::pair<AnsiString, AnsiString>> fieldsToWrite;
69-
fieldsToWrite.push_back(std::make_pair("Return-Path", pTestData->GetEnvelopeFrom()));
69+
String sEnvelopeFrom = pTestData->GetEnvelopeFrom();
70+
AnsiString sReturnPath = sEnvelopeFrom.IsEmpty() ? "<>" : "<" + sEnvelopeFrom + ">";
71+
fieldsToWrite.push_back(std::make_pair("Return-Path", sReturnPath));
7072

7173
TraceHeaderWriter writer;
7274
writer.Write(sFilename, pMessage, fieldsToWrite);

‎hmailserver/source/Server/Common/Util/EmailAllUsers.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ namespace HM
103103
String sFrom;
104104
sFrom.Format(_T("\"%s\" <%s>"), sFromName.c_str(), sFromAddress.c_str());
105105

106-
oMessageData.SetReturnPath("");
107106
oMessageData.SetFrom(sFrom);
108107
oMessageData.SetSubject(sSubject);
109108
oMessageData.SetBody(sBody);

‎hmailserver/source/Server/SMTP/LocalDelivery.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,9 @@ namespace HM
320320
{
321321
std::vector<std::pair<AnsiString, AnsiString> > fieldsToWrite;
322322

323-
fieldsToWrite.push_back(std::make_pair("Return-Path", pMessage->GetFromAddress()));
323+
String sFromAddress = pMessage->GetFromAddress();
324+
AnsiString sReturnPath = sFromAddress.IsEmpty() ? "<>" : "<" + sFromAddress + ">";
325+
fieldsToWrite.push_back(std::make_pair("Return-Path", sReturnPath));
324326

325327
if (Configuration::Instance()->GetSMTPConfiguration()->GetAddDeliveredToHeader())
326328
fieldsToWrite.push_back(std::make_pair("Delivered-To", sOriginalAddress));

‎hmailserver/source/Server/SMTP/RuleApplier.cpp‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -249,10 +249,7 @@ namespace HM
249249
// We need to update the SMTP envelope from address, if this
250250
// message is forwarded by a user-level account.
251251
std::shared_ptr<CONST Account> pAccount = CacheContainer::Instance()->GetAccount(rule_account_id_);
252-
String sMailerDaemonAddress = MailerDaemonAddressDeterminer::GetMailerDaemonAddress(pMsg);
253-
if (pMsg->GetFromAddress().IsEmpty())
254-
pMsg->SetFromAddress(sMailerDaemonAddress);
255-
else if (pAccount && IniFileSettings::Instance()->GetRewriteEnvelopeFromWhenForwarding())
252+
if (pAccount && IniFileSettings::Instance()->GetRewriteEnvelopeFromWhenForwarding() && !pMsg->GetFromAddress().IsEmpty())
256253
pMsg->SetFromAddress(pAccount->GetAddress());
257254

258255
// Add new recipients
@@ -406,19 +403,14 @@ namespace HM
406403

407404
std::shared_ptr<Account> emptyAccount;
408405

409-
// Send a copy of this email.
406+
// Reply to the email
410407
std::shared_ptr<Message> pMsg = std::shared_ptr<Message>(new Message());
411408
pMsg->SetState(Message::Delivering);
412409

413410
String newMessageFileName = PersistentMessage::GetFileName(pMsg);
414411

415-
// check if this us a user-level account rule or global rule.
416-
std::shared_ptr<CONST Account> pAccount = CacheContainer::Instance()->GetAccount(rule_account_id_);
417-
418412
std::shared_ptr<MessageData> pNewMsgData = std::shared_ptr<MessageData>(new MessageData());
419413
pNewMsgData->LoadFromMessage(newMessageFileName, pMsg);
420-
if (!pAccount)
421-
pNewMsgData->SetReturnPath("");
422414
pNewMsgData->GenerateMessageID();
423415
pNewMsgData->SetTo(sReplyRecipientAddress);
424416
pNewMsgData->SetFrom(pAction->GetFromName() + " <" + pAction->GetFromAddress() + ">");
@@ -429,18 +421,12 @@ namespace HM
429421
pNewMsgData->IncreaseRuleLoopCount();
430422
pNewMsgData->Write(newMessageFileName);
431423

432-
// We need to update the SMTP envelope from address, if this
433-
// message is replied to by a user-level account.
434-
if (pAccount)
435-
pMsg->SetFromAddress(pAccount->GetAddress());
436-
437424
// Add recipients.
438425
bool recipientOK = false;
439426
RecipientParser recipientParser;
440427
recipientParser.CreateMessageRecipientList(sReplyRecipientAddress, pMsg->GetRecipients(), recipientOK);
441428

442429
PersistentMessage::SaveObject(pMsg);
443-
444430
}
445431

446432
bool

‎hmailserver/source/Server/SMTP/SMTPDeliverer.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -285,18 +285,16 @@ namespace HM
285285

286286
sErrMsg.Replace(_T("%MACRO_RECIPIENTS%"), sCollectedErrors);
287287

288-
// Send a copy of this email.
288+
// Submit an error log (delivery failed)
289289
std::shared_ptr<Message> pMsg = std::shared_ptr<Message>(new Message());
290290
pMsg->SetState(Message::Delivering);
291-
pMsg->SetFromAddress("");
292291

293292
const String newFileName = PersistentMessage::GetFileName(pMsg);
294293

295294
std::shared_ptr<MessageData> pNewMsgData = std::shared_ptr<MessageData>(new MessageData());
296295
pNewMsgData->LoadFromMessage(newFileName, pMsg);
297296

298297
// Required headers
299-
pNewMsgData->SetReturnPath("");
300298
pNewMsgData->GenerateMessageID();
301299
pNewMsgData->SetSentTime(Time::GetCurrentMimeDate());
302300

‎hmailserver/source/Server/SMTP/SMTPForwarding.cpp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
#include "../Common/Persistence/PersistentMessage.h"
1515

16-
#include "../common/Util/MailerDaemonAddressDeterminer.h"
1716

1817
#include "RecipientParser.h"
1918

@@ -84,10 +83,7 @@ namespace HM
8483
// Create a copy of the message
8584
std::shared_ptr<Message> pNewMessage = PersistentMessage::CopyToQueue(pRecipientAccount, pOriginalMessage);
8685

87-
String sMailerDaemonAddress = MailerDaemonAddressDeterminer::GetMailerDaemonAddress(pNewMessage);
88-
if (pNewMessage->GetFromAddress().IsEmpty())
89-
pNewMessage->SetFromAddress(sMailerDaemonAddress);
90-
else if (IniFileSettings::Instance()->GetRewriteEnvelopeFromWhenForwarding())
86+
if (IniFileSettings::Instance()->GetRewriteEnvelopeFromWhenForwarding() && !pNewMessage->GetFromAddress().IsEmpty())
9187
pNewMessage->SetFromAddress(pRecipientAccount->GetAddress());
9288

9389
pNewMessage->SetState(Message::Delivering);

‎hmailserver/source/Server/SMTP/SMTPVacationMessageCreator.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,10 @@ namespace HM
8585
}
8686

8787

88-
// Send a copy of this email.
88+
// Reply to the email
8989
std::shared_ptr<Message> pMsg = std::shared_ptr<Message>(new Message());
9090

9191
pMsg->SetState(Message::Delivering);
92-
pMsg->SetFromAddress(recipientAccount->GetAddress());
9392

9493
const String newFileName = PersistentMessage::GetFileName(pMsg);
9594

‎hmailserver/source/Server/SMTP/SMTPVirusNotifier.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,17 @@ namespace HM
6666
sErrMsg.Replace(_T("%MACRO_SENT%"), String(header.GetRawFieldValue("Date")));
6767
sErrMsg.Replace(_T("%MACRO_SUBJECT%"), sOriginalSubject);
6868

69-
// Send a copy of this email.
69+
// Send a notification
7070
std::shared_ptr<Message> pMsg = std::shared_ptr<Message>(new Message());
7171

7272
pMsg->SetState(Message::Delivering);
73-
pMsg->SetFromAddress("");
7473

7574
std::shared_ptr<Account> account;
7675

7776
std::shared_ptr<MessageData> pNewMsgData = std::shared_ptr<MessageData>(new MessageData());
7877
pNewMsgData->LoadFromMessage(account, pMsg);
7978

8079
// Required headers
81-
pNewMsgData->SetReturnPath("");
8280
pNewMsgData->GenerateMessageID();
8381
pNewMsgData->SetSentTime(Time::GetCurrentMimeDate());
8482

‎hmailserver/test/RegressionTests/Infrastructure/AccountServices.cs‎

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ public void ConfirmSingleReturnPathAfterAccountForward()
3737
// Wait for the auto-reply.
3838
var text = Pop3ClientSimulator.AssertGetFirstMessageText(account2.Address, "test");
3939

40-
Assert.IsFalse(text.Contains("Return-Path: account2@example.test"));
41-
Assert.IsFalse(text.Contains("Return-Path: account1@example.test"));
42-
Assert.IsTrue(text.Contains("Return-Path: original-address@example.test"));
40+
Assert.IsFalse(text.Contains("Return-Path: <account2@example.test>"));
41+
Assert.IsFalse(text.Contains("Return-Path: <account1@example.test>"));
42+
Assert.IsTrue(text.Contains("Return-Path: <original-address@example.test>"));
4343
}
4444

4545
[Test]
4646
[Category("Accounts")]
47-
[Description("Ensure that messges aren't forwarded if they re deleted using a rule.")]
47+
[Description("Ensure that messages aren't forwarded if they re deleted using a rule.")]
4848
public void ConfirmSingleReturnPathAfterRuleForward()
4949
{
5050
// Create a test account
@@ -81,9 +81,9 @@ public void ConfirmSingleReturnPathAfterRuleForward()
8181
// Wait for the auto-reply.
8282
var text = Pop3ClientSimulator.AssertGetFirstMessageText(account2.Address, "test");
8383

84-
Assert.IsFalse(text.Contains("Return-Path: account-a@example.test"));
85-
Assert.IsFalse(text.Contains("Return-Path: account2@example.test"));
86-
Assert.IsTrue(text.Contains("Return-Path: external@example.test"));
84+
Assert.IsFalse(text.Contains("Return-Path: <account-a@example.test>"));
85+
Assert.IsFalse(text.Contains("Return-Path: <account2@example.test>"));
86+
Assert.IsTrue(text.Contains("Return-Path: <external@example.test>"));
8787
}
8888

8989
[Test]
@@ -136,6 +136,8 @@ public void TestAutoReply()
136136
var s = pop3ClientSimulator.GetFirstMessageText(account1.Address, "test");
137137
if (s.IndexOf("Out of office!") < 0)
138138
throw new Exception("ERROR - Auto reply subject not set properly.");
139+
Assert.IsTrue(s.Contains("Return-Path: <>"),
140+
"Vacation reply envelope sender must be empty (<>) to prevent mail loops per RFC 3834.");
139141

140142
account2.VacationMessageIsOn = false;
141143
account2.Save();
@@ -318,7 +320,73 @@ public void WhenForwardingFromAddressShouldBeSetToForwardingAccount()
318320
var message = Pop3ClientSimulator.AssertGetFirstMessageText(list.Address, "test");
319321

320322

321-
Assert.IsTrue(message.Contains("Return-Path: sender@example.test"));
323+
Assert.IsTrue(message.Contains("Return-Path: <sender@example.test>"));
324+
}
325+
326+
[Test]
327+
[Category("Accounts")]
328+
[Description("When forwarding a bounce (MAIL FROM:<>), the null envelope-from must be preserved so the forwarded copy cannot itself generate a bounce loop.")]
329+
public void WhenAccountForwardingBounceMessageShouldPreserveNullEnvelopeFrom()
330+
{
331+
var forwarder = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "forwarder@example.test", "test");
332+
var recipient = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "recipient@example.test", "test");
333+
334+
forwarder.ForwardEnabled = true;
335+
forwarder.ForwardAddress = recipient.Address;
336+
forwarder.ForwardKeepOriginal = true;
337+
forwarder.Save();
338+
339+
// Send with empty envelope-from (MAIL FROM:<>), simulating a bounce/DSN.
340+
var smtp = new SmtpClientSimulator();
341+
smtp.Send("", new System.Collections.Generic.List<string> { forwarder.Address }, "Bounce subject", "Bounce body");
342+
343+
Pop3ClientSimulator.AssertMessageCount(forwarder.Address, "test", 1);
344+
345+
_application.SubmitEMail();
346+
CustomAsserts.AssertRecipientsInDeliveryQueue(0);
347+
348+
var message = Pop3ClientSimulator.AssertGetFirstMessageText(recipient.Address, "test");
349+
Assert.IsTrue(message.Contains("Return-Path: <>"),
350+
"Forwarding a bounce must preserve the null envelope-from to prevent bounce loops.");
351+
}
352+
353+
[Test]
354+
[Category("Accounts")]
355+
[Description("When a rule forwards a bounce (MAIL FROM:<>), the null envelope-from must be preserved so the forwarded copy cannot itself generate a bounce loop.")]
356+
public void WhenRuleForwardsBounceMessageShouldPreserveNullEnvelopeFrom()
357+
{
358+
var account1 = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "rulefwd-src@example.test", "test");
359+
var account2 = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "rulefwd-dst@example.test", "test");
360+
361+
var rule = account1.Rules.Add();
362+
rule.Name = "Forward all";
363+
rule.Active = true;
364+
365+
var criteria = rule.Criterias.Add();
366+
criteria.UsePredefined = true;
367+
criteria.PredefinedField = eRulePredefinedField.eFTMessageSize;
368+
criteria.MatchType = eRuleMatchType.eMTGreaterThan;
369+
criteria.MatchValue = "0";
370+
criteria.Save();
371+
372+
var action = rule.Actions.Add();
373+
action.Type = eRuleActionType.eRAForwardEmail;
374+
action.To = account2.Address;
375+
action.Save();
376+
377+
rule.Save();
378+
379+
// Send with empty envelope-from (MAIL FROM:<>), simulating a bounce/DSN.
380+
var smtp = new SmtpClientSimulator();
381+
smtp.Send("", new System.Collections.Generic.List<string> { account1.Address }, "Bounce subject", "Bounce body");
382+
383+
Pop3ClientSimulator.AssertMessageCount(account1.Address, "test", 1);
384+
_application.SubmitEMail();
385+
CustomAsserts.AssertRecipientsInDeliveryQueue(0);
386+
387+
var message = Pop3ClientSimulator.AssertGetFirstMessageText(account2.Address, "test");
388+
Assert.IsTrue(message.Contains("Return-Path: <>"),
389+
"Rule-based forwarding of a bounce must preserve the null envelope-from to prevent bounce loops.");
322390
}
323391

324392
[Test]

‎hmailserver/test/RegressionTests/Rules/Rules.cs‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1612,10 +1612,46 @@ public void TestReply()
16121612
CustomAsserts.AssertRecipientsInDeliveryQueue(0);
16131613
var message = CustomAsserts.AssertGetFirstMessage(account1, "Inbox");
16141614

1615-
Assert.AreEqual("ruletest2@example.test", message.FromAddress);
1615+
Assert.AreEqual(string.Empty, message.FromAddress);
16161616
Assert.AreEqual("auto-replied", message.get_HeaderValue("Auto-Submitted"));
16171617
}
16181618

1619+
[Test]
1620+
[Description("Auto-reply generated by a rule must use a null envelope-from (Return-Path: <>) to prevent bounce loops per RFC 3834.")]
1621+
public void WhenRuleRepliesEnvelopeFromShouldBeNull()
1622+
{
1623+
var account1 = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "replytest-src@example.test", "test");
1624+
var account2 = SingletonProvider<TestSetup>.Instance.AddAccount(_domain, "replytest-dst@example.test", "test");
1625+
1626+
var rule = account2.Rules.Add();
1627+
rule.Name = "Auto-reply rule";
1628+
rule.Active = true;
1629+
1630+
var ruleCriteria = rule.Criterias.Add();
1631+
ruleCriteria.UsePredefined = true;
1632+
ruleCriteria.PredefinedField = eRulePredefinedField.eFTMessageSize;
1633+
ruleCriteria.MatchType = eRuleMatchType.eMTGreaterThan;
1634+
ruleCriteria.MatchValue = "0";
1635+
ruleCriteria.Save();
1636+
1637+
var ruleAction = rule.Actions.Add();
1638+
ruleAction.Type = eRuleActionType.eRAReply;
1639+
ruleAction.FromAddress = account2.Address;
1640+
ruleAction.FromName = "Auto Reply";
1641+
ruleAction.Subject = "Autoreply";
1642+
ruleAction.Save();
1643+
1644+
rule.Save();
1645+
1646+
SmtpClientSimulator.StaticSend(account1.Address, account2.Address, "Test", "Test message.");
1647+
1648+
CustomAsserts.AssertRecipientsInDeliveryQueue(0);
1649+
1650+
var replyText = Pop3ClientSimulator.AssertGetFirstMessageText(account1.Address, "test");
1651+
Assert.IsTrue(replyText.Contains("Return-Path: <>"),
1652+
"Rule-based auto-reply must use null envelope-from to prevent bounce loops.");
1653+
}
1654+
16191655
[Test]
16201656
[Description("Rule REPLY body must be QP-wrapped to 76 chars per RFC 2045 section 6.7 (issue #171).")]
16211657
public void WhenRuleReplyBodyExceedsQPLineLimitShouldWrapToRfc2045()

0 commit comments

Comments
 (0)