[PATCH] badmailfromto: added strictures, tests, and

[Matt Simerson]

badmailfromto: added strictures, tests, and

rearranged portions of logic for ease of reading

---
plugins/check_badmailfromto        |   68 ++++++++++++++++++++++++------------
t/plugin_tests/check_badmailfromto |   36 +++++++++++++++++++
2 files changed, 82 insertions(+), 22 deletions(-)
create mode 100644 t/plugin_tests/check_badmailfromto

diff --git a/plugins/check_badmailfromto b/plugins/check_badmailfromto
index a57f6f6..3a39874 100644
--- a/plugins/check_badmailfromto
+++ b/plugins/check_badmailfromto
@@ -17,14 +17,14 @@ Based heavily on check_badmailfrom.

=cut

+use strict;
+use Qpsmtpd::Constants;
+
sub hook_mail {
  my ($self, $transaction, $sender, %param) = @_;

-  my @badmailfromto = $self->qp->config("badmailfromto")
-    or return (DECLINED);
-
-  return (DECLINED) unless ($sender->format ne "<>"
-                           and $sender->host && $sender->user);
+    my @badmailfromto = $self->qp->config("badmailfromto");
+    return DECLINED if $self->is_sender_immune( $sender, \@badmailfromto );

  my $host = lc $sender->host;
  my $from = lc($sender->user) . '@' . $host;
@@ -33,27 +33,51 @@ sub hook_mail {
    $bad =~ s/^\s*(\S+).*/$1/;
    next unless $bad;
    $bad = lc $bad;
-    $self->log(LOGWARN, "Bad badmailfromto config: No \@ sign in $bad") and 
next unless $bad =~ m/\@/;
-    $transaction->notes('badmailfromto', "$bad")
-      if ($bad eq $from)
-      || (substr($bad,0,1) eq '@' && $bad eq "\@$host");
+    if ( $bad !~ m/\@/ ) {
+        $self->log(LOGWARN, 'badmailfromto: bad config, no @ sign in '. $bad);
+        next;
+    };
+    if ( $bad eq $from || (substr($bad,0,1) eq '@' && $bad eq "\@$host") ) {
+        $transaction->notes('badmailfromto', $bad);
+    };
  }
  return (DECLINED);
}

sub hook_rcpt {
-  my ($self, $transaction, $rcpt, %param) = @_;
-  my $recipient = lc($rcpt->user) . '@' . lc($rcpt->host);
-  my $sender = $transaction->notes('badmailfromto');
-  if ($sender) {
-    my @badmailfromto = $self->qp->config("badmailfromto")
-      or return (DECLINED);
-
-    foreach (@badmailfromto) {
-      my ($from, $to) = m/^\s*(\S+)\t(\S+).*/;
-      return (DENY, "mail to $recipient not accepted here")
-        if lc($from) eq $sender and lc($to) eq $recipient;
+    my ($self, $transaction, $rcpt, %param) = @_;
+    my $recipient = lc($rcpt->user) . '@' . lc($rcpt->host);
+    my $sender = $transaction->notes('badmailfromto') or do {
+        $self->log(LOGDEBUG, "pass: sender not listed");
+        return (DECLINED);
+    };
+
+    foreach ( $self->qp->config("badmailfromto") ) {
+        my ($from, $to) = m/^\s*(\S+)\t(\S+).*/;
+        return (DENY, "mail to $recipient not accepted here")
+            if lc($from) eq $sender && lc($to) eq $recipient;
    }
-  }
-  return (DECLINED);
+    $self->log(LOGDEBUG, "pass: recipient not listed");
+    return (DECLINED);
}
+
+sub is_sender_immune {
+    my ($self, $sender, $badmf ) = @_;
+
+    if ( ! scalar @$badmf ) {
+        $self->log(LOGDEBUG, 'skip: empty list');
+        return 1;
+    };
+
+    if ( ! $sender || $sender->format eq '<>' ) {
+        $self->log(LOGDEBUG, 'skip: null sender');
+        return 1;
+    };
+
+    if ( ! $sender->host || ! $sender->user ) {
+        $self->log(LOGDEBUG, 'skip: missing user or host');
+        return 1;
+    };
+
+    return;
+};
diff --git a/t/plugin_tests/check_badmailfromto 
b/t/plugin_tests/check_badmailfromto
new file mode 100644
index 0000000..73d9bb9
--- /dev/null
+++ b/t/plugin_tests/check_badmailfromto
@@ -0,0 +1,36 @@
+#!perl -w
+
+use strict;
+use Data::Dumper;
+
+use Qpsmtpd::Address;
+
+sub register_tests {
+    my $self = shift;
+
+    $self->register_test("test_badmailfromto_is_sender_immune", 5);
+}
+
+sub test_badmailfromto_is_sender_immune {
+    my $self = shift;
+
+    my $transaction = $self->qp->transaction;
+    my $test_email = 'm...@test.com';
+    $transaction->sender( Qpsmtpd::Address->new( "<$test_email>" ) );
+    ok( $self->is_sender_immune( $transaction->sender, [] ), "is_immune, empty 
list");
+
+    $transaction->sender( Qpsmtpd::Address->new( '<>' ) );
+    ok( $self->is_sender_immune( $transaction->sender, ['b...@example.com'] ), 
"is_immune, null sender");
+
+    my $address = Qpsmtpd::Address->new( '<matt@>' );
+    $transaction->sender($address);
+    ok( $self->is_sender_immune( $transaction->sender, ['b...@example.com'] ), 
"is_immune, missing host");
+
+    $address = Qpsmtpd::Address->new( '<@example.com>' );
+    $transaction->sender($address);
+    ok( $self->is_sender_immune( $transaction->sender, ['b...@example.com'] ), 
"is_immune, missing user");
+
+    $transaction->sender( Qpsmtpd::Address->new( '<m...@example.com>' ) );
+    ok( ! $self->is_sender_immune( $transaction->sender, ['b...@example.com'] 
), "is_immune, false");
+};
+
-- 
1.7.9.6