Re: [pmg-devel] [PATCH pmg-api v3 7/8] pmgqm: handle smtputf8 data

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 11/23/22 10:23, Stoiko Ivanov wrote:
> $data->{pmail} is both used in the template rendering ('Spam Report for
> $pmail'), and as content for the To header, which need different
> treatment. Thus introduce 'pmail_raw' additionally.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>   src/PMG/CLI/pmgqm.pm | 24 +++++++++++++-----------
>   src/PMG/Utils.pm     |  7 ++++---
>   2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/src/PMG/CLI/pmgqm.pm b/src/PMG/CLI/pmgqm.pm
> index dbec8ef..7293579 100755
> --- a/src/PMG/CLI/pmgqm.pm
> +++ b/src/PMG/CLI/pmgqm.pm
> @@ -2,6 +2,7 @@ package PMG::CLI::pmgqm;
>   
>   use strict;
>   use Data::Dumper;
> +use Encode qw(encode);
>   use Template;
>   use MIME::Entity;
>   use HTML::Entities;
> @@ -17,6 +18,7 @@ use PVE::SafeSyslog;
>   use PVE::Tools;
>   use PVE::INotify;
>   use PVE::CLIHandler;
> +use PVE::JSONSchema qw(get_standard_option);
>   
>   use PMG::RESTEnvironment;
>   use PMG::Utils;
> @@ -57,7 +59,7 @@ sub get_item_data {
>       }
>   
>       $item->{envelope_sender} = $ref->{sender};
> -    $item->{pmail} = $ref->{pmail};
> +    $item->{pmail} = encode_entities(PMG::Utils::try_decode_utf8($ref->{pmail}));
>       $item->{receiver} = $ref->{receiver} || $ref->{pmail};
>   
>       $item->{date} = strftime("%F", localtime($ref->{time}));
> @@ -157,11 +159,10 @@ __PACKAGE__->register_method ({
>       parameters => {
>   	additionalProperties => 0,
>   	properties => {
> -	    receiver => {
> +	    receiver => get_standard_option('pmg-email-address', {
>   		description => "Generate report for a single email address. If not specified, generate reports for all users.",
> -		type => 'string', format => 'email',
>   		optional => 1,
> -	    },
> +	    }),
>   	    timespan => {
>   		description => "Select time span.",
>   		type => 'string',
> @@ -175,11 +176,10 @@ __PACKAGE__->register_method ({
>   		enum => ['short', 'verbose', 'custom'],
>   		optional => 1,
>   	    },
> -	    redirect => {
> +	    redirect => get_standard_option('pmg-email-address', {
>   		description => "Redirect spam report email to this address.",
> -		type => 'string', format => 'email',
>   		optional => 1,
> -	    },
> +	    }),
>   	    debug => {
>   		description => "Debug mode. Print raw email to stdout instead of sending them.",
>   		type => 'boolean',
> @@ -280,7 +280,7 @@ __PACKAGE__->register_method ({
>   	    "ORDER BY pmail, time, receiver");
>   
>   	if ($target) {
> -	    $sth->execute($target);
> +	    $sth->execute(encode('UTF-8', $target));
>   	} else {
>   	    $sth->execute();
>   	}
> @@ -302,16 +302,18 @@ __PACKAGE__->register_method ({
>   	};
>   
>   	while (my $ref = $sth->fetchrow_hashref()) {
> -	    if ($creceiver ne $ref->{pmail}) {
> +	    my $decoded_pmail = PMG::Utils::try_decode_utf8($ref->{pmail});
> +	    if ($creceiver ne $decoded_pmail) {
>   
>   		$finalize->() if $data;
>   
>   		$data = clone($global_data);
>   
> -		$creceiver = $ref->{pmail};
> +		$creceiver = $decoded_pmail;
>   		$mailcount = 0;
>   
> -		$data->{pmail} = $creceiver;
> +		$data->{pmail} = encode_entities($decoded_pmail);
> +		$data->{pmail_raw} = $ref->{pmail};
>   		$data->{managehref} = "$protocol_fqdn_port/quarantine";
>   		if ($data->{authmode} ne 'ldap') {
>   		    $data->{ticket} = PMG::Ticket::assemble_quarantine_ticket($data->{pmail});
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index cc30e67..5c9e873 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -1143,12 +1143,13 @@ sub rfc1522_to_html {
>   	    my ($d, $cs) = @$r;
>   	    if ($d) {
>   		if ($cs) {
> -		    $res .= encode_entities(decode($cs, $d));
> +		    $res .= encode('UTF-8', decode($cs, $d));
>   		} else {
> -		    $res .= encode_entities($d);
> +		    $res .= $d;
>   		}
>   	    }
>   	}
> +	$res = encode_entities(decode('UTF-8', $res));

this change is not really explained in the commit message
and is a bit confusing

couldn't we simply do:

encode_entities(decode_rfc1522($enc))

?

afaics is rfc1522_to_html mostly the same as decode_rfc1522
but with an 'encode_entities' after decoding


>       };
>   
>       $res = $enc if $@;
> @@ -1257,7 +1258,7 @@ sub finalize_report {
>   
>       my $top = MIME::Entity->build(
>   	Type    => "multipart/related",
> -	To      => $data->{pmail},
> +	To      => $data->{pmail_raw},
>   	From    => $mailfrom,
>   	Subject => bencode_header(decode_entities($title)));
>