[pmg-devel] [PATCH log-tracker] fix #3758: fix `to` filter with empty NoqueueEntry `to` field

[pmg-devel] [PATCH log-tracker] fix #3758: fix `to` filter with empty NoqueueEntry `to` field

[Mira Limbeck]

We only compared the filter `to` with the Noqueue `to` when both were
not empty, and in turn set the nq.dstatus to DStatus::Invalid when they
didn't match.
But in the case of an empty Noqueue `to`, we have to skip it as well if
the filter `to` is not empty.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
contains 2 rustfmt changes that are unrelated to the issue

 src/main.rs | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index b1d4f8c..62e5ea1 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -762,7 +762,6 @@ fn handle_cleanup_message(msg: &[u8], parser: &mut Parser, complete_line: &[u8])
         }
         qe.borrow_mut().cleanup = true;
 
-
         // does not work correctly if there's a duplicate message id in the logfiles
         if let Some(q) = parser.msgid_lookup.remove(msgid) {
             let q_clone = Weak::clone(&q);
@@ -776,8 +775,7 @@ fn handle_cleanup_message(msg: &[u8], parser: &mut Parser, complete_line: &[u8])
                     q.borrow_mut().aq_qentry = Some(Rc::downgrade(&qe));
                 }
             }
-        }
-        else {
+        } else {
             parser.msgid_lookup.insert(msgid.into(), Rc::downgrade(&qe));
         }
     }
@@ -908,8 +906,9 @@ impl SEntry {
                     || (parser.options.exclude_greylist && nq.dstatus == DStatus::Greylist)
                     || (parser.options.exclude_ndr && nq.from.is_empty())
                     || (!parser.options.to.is_empty()
-                        && !nq.to.is_empty()
-                        && find_lowercase(&nq.to, parser.options.to.as_bytes()).is_none())
+                        && ((!nq.to.is_empty()
+                            && find_lowercase(&nq.to, parser.options.to.as_bytes()).is_none())
+                            || nq.to.is_empty()))
                 {
                     nq.dstatus = DStatus::Invalid;
                 }
-- 
2.30.2

[pmg-devel] [PATCH api] fix #3758: allow empty `to` in noqueue case

[Mira Limbeck]

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
 src/PMG/API2/MailTracker.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PMG/API2/MailTracker.pm b/src/PMG/API2/MailTracker.pm
index 187decf..b8b25ad 100644
--- a/src/PMG/API2/MailTracker.pm
+++ b/src/PMG/API2/MailTracker.pm
@@ -171,13 +171,14 @@ my $run_pmg_log_tracker = sub {
 		$entry->{client} = $1;
 	    } elsif ($line =~ m/^CTIME:\s+([0-9A-F]+)$/) {
 		# ignore ?
-	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]+)>$/) {
+	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]*)>$/) {
 		my $e = {};
 		$e->{client} = $entry->{client} if defined($entry->{client});
 		$e->{time} = hex($1) - $timezone;
 		$e->{id} = $2;
 		$e->{dstatus} = $3;
 		$e->{from} = $4;
+		die "empty to address only allowed in NOQUEUE case\n" if !$5 && $e->{dstatus} ne 'N';
 		$e->{to} = $5;
 		push @$list, $e;
 	    } elsif ($line =~ m/^LOGS:$/) {
-- 
2.30.2

[pmg-devel] applied: [PATCH api] fix #3758: allow empty `to` in noqueue case

[Stoiko Ivanov]

Huge thanks for addressing this so quickly!

Applied and tested with a problematic log.

On Wed,  1 Dec 2021 16:41:58 +0100
Mira Limbeck <m.limbeck@proxmox.com> wrote:

> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
>  src/PMG/API2/MailTracker.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PMG/API2/MailTracker.pm b/src/PMG/API2/MailTracker.pm
> index 187decf..b8b25ad 100644
> --- a/src/PMG/API2/MailTracker.pm
> +++ b/src/PMG/API2/MailTracker.pm
> @@ -171,13 +171,14 @@ my $run_pmg_log_tracker = sub {
>  		$entry->{client} = $1;
>  	    } elsif ($line =~ m/^CTIME:\s+([0-9A-F]+)$/) {
>  		# ignore ?
> -	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]+)>$/) {
> +	    } elsif ($line =~ m/^TO:([0-9A-F]+):(T[0-9A-F]+L[0-9A-F]+):([0-9A-Z]):\s+from <([^>]*)>\s+to\s+<([^>]*)>$/) {
>  		my $e = {};
>  		$e->{client} = $entry->{client} if defined($entry->{client});
>  		$e->{time} = hex($1) - $timezone;
>  		$e->{id} = $2;
>  		$e->{dstatus} = $3;
>  		$e->{from} = $4;
> +		die "empty to address only allowed in NOQUEUE case\n" if !$5 && $e->{dstatus} ne 'N';
>  		$e->{to} = $5;
>  		push @$list, $e;
>  	    } elsif ($line =~ m/^LOGS:$/) {

Re: [pmg-devel] applied: [PATCH api] fix #3758: allow empty `to` in noqueue case

[Thomas Lamprecht]

On 01.12.21 19:13, Stoiko Ivanov wrote:
> Applied and tested with a problematic log.

could make sense to add such one to the regression test harness?

[pmg-devel] applied: [PATCH log-tracker] fix #3758: fix `to` filter with empty NoqueueEntry `to` field

[Stoiko Ivanov]

applied, and followed-up with a commit massaging in one of the loglines
producing this into our tests

Thanks!

On Wed,  1 Dec 2021 16:41:57 +0100
Mira Limbeck <m.limbeck@proxmox.com> wrote:

> We only compared the filter `to` with the Noqueue `to` when both were
> not empty, and in turn set the nq.dstatus to DStatus::Invalid when they
> didn't match.
> But in the case of an empty Noqueue `to`, we have to skip it as well if
> the filter `to` is not empty.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> contains 2 rustfmt changes that are unrelated to the issue
> 
>  src/main.rs | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/src/main.rs b/src/main.rs
> index b1d4f8c..62e5ea1 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -762,7 +762,6 @@ fn handle_cleanup_message(msg: &[u8], parser: &mut Parser, complete_line: &[u8])
>          }
>          qe.borrow_mut().cleanup = true;
>  
> -
>          // does not work correctly if there's a duplicate message id in the logfiles
>          if let Some(q) = parser.msgid_lookup.remove(msgid) {
>              let q_clone = Weak::clone(&q);
> @@ -776,8 +775,7 @@ fn handle_cleanup_message(msg: &[u8], parser: &mut Parser, complete_line: &[u8])
>                      q.borrow_mut().aq_qentry = Some(Rc::downgrade(&qe));
>                  }
>              }
> -        }
> -        else {
> +        } else {
>              parser.msgid_lookup.insert(msgid.into(), Rc::downgrade(&qe));
>          }
>      }
> @@ -908,8 +906,9 @@ impl SEntry {
>                      || (parser.options.exclude_greylist && nq.dstatus == DStatus::Greylist)
>                      || (parser.options.exclude_ndr && nq.from.is_empty())
>                      || (!parser.options.to.is_empty()
> -                        && !nq.to.is_empty()
> -                        && find_lowercase(&nq.to, parser.options.to.as_bytes()).is_none())
> +                        && ((!nq.to.is_empty()
> +                            && find_lowercase(&nq.to, parser.options.to.as_bytes()).is_none())
> +                            || nq.to.is_empty()))
>                  {
>                      nq.dstatus = DStatus::Invalid;
>                  }