From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Nigel van Keulen" <nigel2392@gmail.com>, <pmg-devel@lists.proxmox.com>
Subject: Re: [pmg-devel] SPAM: [PATCH pmg-api 1/1] fix: 2971, to DKIM signing for OOO messages by using postmaster@<domain> as a sender if it is not present.
Date: Wed, 22 May 2024 14:05:32 +0200 [thread overview]
Message-ID: <D1G5RK3KIX8M.S76SRDH42RGJ@proxmox.com> (raw)
In-Reply-To: <20240518145855.670-2-nigel2392@gmail.com>
hey thanks for contributing to PMG!
i just wanted to provide some quick feedback on the code you sent in,
but please make sure to follow our development guidelines and also note
that if you haven't already, you need to sign a harmony CLA [1].
some notes on the commit message:
a) it is to long, we generally enforce a limit of 70 characters per
line. please provide a short summary in the first line and if needed
more context in a second paragraph.
b) i am assumig OOO stands for "out of office", however your patch
applies to all mails as long as they don't have a sender. so this
commit message is somewhat confusing.
On Sat May 18, 2024 at 4:58 PM CEST, Nigel van Keulen wrote:
> ---
> src/PMG/RuleDB/Accept.pm | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/PMG/RuleDB/Accept.pm b/src/PMG/RuleDB/Accept.pm
> index e3e39a7..8a6261f 100644
> --- a/src/PMG/RuleDB/Accept.pm
> +++ b/src/PMG/RuleDB/Accept.pm
> @@ -101,8 +101,16 @@ sub execute {
> PMG::Utils::remove_marks($entity);
>
> if ($dkim->{sign}) {
> +
> + my $mailSender = $msginfo->{sender};
> + if ($mailSender eq '') {
> + my $mailDomain = $msginfo->{domain};
> + $mailSender = "postmaster\@$mailDomain";
i wonder if more or less hardcoding a sender for mails here makes sense.
it might be neater to make this configurable somehow.
i also wonder if this adds much functionality in reality. most users may
be able to just use the `dkim-use-domain` to use the sender from the
envelop instead.
> + syslog('info', "%s: No sender found, using default sender: %s", $queue->{logid}, $mailSender);
note that this line is too long. lines should not be longer than 100
characters. you can wrap this line like so to fit that limit
```pl
syslog(
'info',
"%s: No sender found, using default sender: %s",
$queue->{logid},
$mailSender
);
```
this would be in accordance with our style guide [2]. also note that
you did not correctly indent your code. please follow the indentation
guidelines in the style guide as well.
> + }
> +
> eval {
> - $entity = PMG::DKIMSign::sign_entity($entity, $dkim, $msginfo->{sender});
> + $entity = PMG::DKIMSign::sign_entity($entity, $dkim, $mailSender);
> };
> if ($@) {
> syslog('warning',
[1]: https://pmg.proxmox.com/wiki/index.php/Developer_Documentation#Software_License_and_Copyright
[2]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings
_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
next prev parent reply other threads:[~2024-05-22 12:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-18 14:58 [pmg-devel] SPAM: [PATCH pmg-api 0/1] *** Fix OOO e-mail's DKIM signatures *** Nigel van Keulen
2024-05-18 14:58 ` [pmg-devel] SPAM: [PATCH pmg-api 1/1] fix: 2971, to DKIM signing for OOO messages by using postmaster@<domain> as a sender if it is not present Nigel van Keulen
2024-05-22 12:05 ` Shannon Sterz [this message]
[not found] ` <CAD-rDhfxenLiYSiTVQjWCdc=G0ooKirUQVR6GJDFJmhU_jESug@mail.gmail.com>
[not found] ` <D1H4170KM7OL.7HNFYXHGMV1H@proxmox.com>
2024-05-23 15:06 ` Shannon Sterz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D1G5RK3KIX8M.S76SRDH42RGJ@proxmox.com \
--to=s.sterz@proxmox.com \
--cc=nigel2392@gmail.com \
--cc=pmg-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox