froxlor / Froxlor

The server administration software for your needs - The official Froxlor development Git repository
http://www.froxlor.org
GNU General Public License v2.0
1.62k stars 455 forks source link

Traffic Cronjob sends mail violating DMARC rules #1250

Closed alexlehm closed 3 months ago

alexlehm commented 3 months ago

As a rule of thumb: before reporting an issue

Describe the bug lib/Froxlor/Cron/Traffic/ReportsCron.php uses the recipient address of a report as From address which makes the mail undeliverable for strict DMARC domains

E.g. I use username@gmail.com as admin address and that means that the mail will be rejected by gmail since it is not DKIM signed

System information

To Reproduce

  1. Create a admin with an email address of gmail.com (or other strict DMARC domains)
  2. create at least one domain for this admin
  3. wait until 1st of the next month
  4. check deferred mails with mailq, there will be one mail addressed to user@gmail.com which is deferred due to a DMARC reject violation

Expected behavior The mail should use a local email address as From, e.g. webmaster@mydomain.de

d00p commented 3 months ago

use SMTP settings to send emails using valid smtp authentication If you want to use webmaster@mydomain - set the admin-email to that and not user@gmail.com

d00p commented 3 months ago

Also

lib/Froxlor/Cron/Traffic/ReportsCron.php uses the recipient address of a report as From address which makes the mail

is simply not true, see From: https://github.com/froxlor/Froxlor/blob/v2.1/lib/Froxlor/Cron/Traffic/ReportsCron.php#L131 and recipient: https://github.com/froxlor/Froxlor/blob/v2.1/lib/Froxlor/Cron/Traffic/ReportsCron.php#L135

First one is the customer's admin and the second one the customer itself.

d00p commented 3 months ago

If you talk about the mail to the admin itself (which indeed sets the From to the same value as the recipient) - then yes this is intended as for now. A solution might be to set the global system sender-email (and possible smtp setting) as "From" there ...I'll check

d00p commented 3 months ago

You may want to test the following patch:

diff --git a/lib/Froxlor/Cron/Traffic/ReportsCron.php b/lib/Froxlor/Cron/Traffic/ReportsCron.php
index 01828c1d..cd8babaf 100644
--- a/lib/Froxlor/Cron/Traffic/ReportsCron.php
+++ b/lib/Froxlor/Cron/Traffic/ReportsCron.php
@@ -211,7 +211,7 @@ class ReportsCron extends FroxlorCron
                                        $_mailerror = false;
                                        $mailerr_msg = "";
                                        try {
-                                               $mail->SetFrom($row['email'], $row['name']);
+                                               $mail->SetFrom(Settings::Get('panel.adminmail'), Settings::Get('panel.adminmail_defname'));
                                                $mail->Subject = $mail_subject;
                                                $mail->AltBody = $mail_body;
                                                $mail->MsgHTML(nl2br($mail_body));
@@ -297,7 +297,7 @@ class ReportsCron extends FroxlorCron
                                        $_mailerror = false;
                                        $mailerr_msg = "";
                                        try {
-                                               $mail->SetFrom($row['email'], $row['name']);
+                                               $mail->SetFrom(Settings::Get('panel.adminmail'), Settings::Get('panel.adminmail_defname'));
                                                $mail->Subject = $mail_subject;
                                                $mail->Body = $mail_body;
                                                $mail->MsgHTML(nl2br($mail_body));
@@ -472,7 +472,7 @@ class ReportsCron extends FroxlorCron
                                        $_mailerror = false;
                                        $mailerr_msg = "";
                                        try {
-                                               $mail->SetFrom($row['email'], $row['name']);
+                                               $mail->SetFrom(Settings::Get('panel.adminmail'), Settings::Get('panel.adminmail_defname'));
                                                $mail->Subject = $mail_subject;
                                                $mail->AltBody = $mail_body;
                                                $mail->MsgHTML(nl2br($mail_body));
alexlehm commented 3 months ago

Thank you, I have patched that in my server