Re: [pmg-devel] [RFC pmg-api] trim Message-ID when parsing E-mail

[Hannes Duerr <h.duerr@proxmox.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3364 bytes --]


On 4/17/25 16:31, Stoiko Ivanov wrote:
> Thanks for the patch!
>
> generally looks ok - and could be merged - some nits/questions inline (can
> fixup most upon applying):
>
> On Mon, 14 Apr 2025 17:36:56 +0200
> Hannes Duerr<h.duerr@proxmox.com> wrote:
>
>> when we currently parse emails we do not remove trailing newlines from
>> the message-id. The consequence of this is that if you use the rule
>> system macro __MSGID__, there is also a newline at the end of the
>> string. This in turn leads to problems if you create a rule and want to
>> add something after the message ID.
> As we talked off-list about this I know the context - but mentioning the
> case of - modifying the message-id, after stripping <> and the domain to
> not leak information  would help people in the future remembering why this
> was added.
yes good point, i will add it in a v2
>> Signed-off-by: Hannes Duerr<h.duerr@proxmox.com>
>> ---
>>
>> Notes:
>>      I'm not 100% sure whether we can simply trim the newline without any
>>      problems. I have searched through the code a bit, but I have not come
>>      across any problematic areas. We use the msgid for the log messages, but
> I was surprised that the newline might be there at all - but the docs of
> MIME::Head (https://metacpan.org/pod/MIME::Head) say so:
> NOTE: The header(s) returned may end with a newline. If you don't want this, then chomp the return value.
>
> Else I looked through our code and also think that this is read once, and
> then used in a log-message and for adding it as template variable
> https://git.proxmox.com/?p=pmg-api.git;a=blob;f=src/bin/pmg-smtp-filter;h=32bad7b881733147e5defe64a590dc709669dd1a;hb=HEAD#l184
>
>
>>      it is in the last position there and a newline is set. This means that
>>      the log message does not change
>>      If anyone has any objections or other ideas on how to tackle the
>>      problem, I'd be interested to hear them
> How did you test this - I gave it a quick spin with a modify field object
> (adding a X-my-msgid header) and it looked ok - but out of curiosity?
I constructed the setup described by the customer:
I sent messages from a domain x, stripped the domain from the Message-ID 
with postfix header-checks and added another domain y with an header 
attribute (modify field) and the __MSGID__ macro
I also checked the log messages when sending some other arbitrary mails.
>>   src/PMG/MailQueue.pm | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PMG/MailQueue.pm b/src/PMG/MailQueue.pm
>> index 4e37cb9..2af95bd 100644
>> --- a/src/PMG/MailQueue.pm
>> +++ b/src/PMG/MailQueue.pm
>> @@ -4,6 +4,7 @@ use strict;
>>   use warnings;
>>   
>>   use PVE::SafeSyslog;
>> +use PVE::Tools qw (trim);
>>   use MIME::Parser;
>>   use IO::File;
>>   use Encode;
>> @@ -394,7 +395,7 @@ sub parse_mail {
>>       PMG::MIMEUtils::fixup_multipart($entity);
>>   
>>       if ((my $idcount = $entity->head->count ('Message-Id')) > 0) {
>> -	$self->msgid ($entity->head->get ('Message-Id', $idcount - 1));
>> +	$self->msgid (trim($entity->head->get ('Message-Id', $idcount - 1)));
> nit: when touching this you could remove the space `msgid(...)` vs
> `msgid (...)`

good point, will incorporate it in the v2

The v2 can be found here: 
https://lore.proxmox.com/pmg-devel/20250423084549.20411-1-h.duerr@proxmox.com/T/#u 


[-- Attachment #1.2: Type: text/html, Size: 5189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel