From 9faa1e1903ec9a762eb329bb864e632b44ddf1e1 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 12:03:49 -0500 Subject: [PATCH 1/6] Fix clamdscan configuration handling - Honor configured 'false' values - Treat 'scan_all' consistently with other options--permitting 0, 1, 'yes', or 'no' as values --- plugins/virus/clamdscan | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/plugins/virus/clamdscan b/plugins/virus/clamdscan index 5a990e7..5621cc6 100644 --- a/plugins/virus/clamdscan +++ b/plugins/virus/clamdscan @@ -136,10 +136,14 @@ sub register { } # Set sensible defaults - $self->{_args}{deny_viruses} ||= 'yes'; $self->{_args}{max_size} ||= 1024; - $self->{_args}{scan_all} ||= 1; - for my $setting ('deny_viruses', 'defer_on_error') { + if ( ! defined $self->{_args}{deny_viruses} ) { + $self->{_args}{deny_viruses} = 'yes'; + } + if ( ! defined $self->{_args}{scan_all} ) { + $self->{_args}{scan_all} = 1; + } + for my $setting (qw( deny_viruses defer_on_error scan_all )) { next unless $self->{'_args'}{$setting}; if (lc $self->{'_args'}{$setting} eq 'no') { $self->{'_args'}{$setting} = 0; From 8344bf8439b1cbb9d0dcfa7df3a964e9fb177953 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 12:56:32 -0500 Subject: [PATCH 2/6] Working toward functional virus/clamdscan tests. --- Makefile.PL | 9 +++++---- t/plugin_tests/virus/clamdscan | 24 ++++++++++-------------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/Makefile.PL b/Makefile.PL index 7d455d3..0835dfb 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -17,9 +17,10 @@ WriteMakefile( 'Net::IP' => 0, 'Time::HiRes' => 0, 'IO::Socket::SSL' => 0, + 'ClamAV::Client' => 0, # virus/clamdscan # Dev/Test modules - 'Test::More' => 0, - 'Test::Output' => 0, + 'Test::More' => 0, + 'Test::Output' => 0, # modules for specific features 'Mail::DKIM' => 0, 'File::Tail' => 0, # log/summarize, log/watch @@ -49,8 +50,8 @@ sub MY::postamble { qq[ testcover : \t cover -delete && \\ - HARNESS_PERL_SWITCHES=-MDevel::Cover \$(MAKE) test && \\ - cover + HARNESS_PERL_SWITCHES=-MDevel::Cover \$(MAKE) test && \\ + cover ] } diff --git a/t/plugin_tests/virus/clamdscan b/t/plugin_tests/virus/clamdscan index 8deb1a3..bd432ac 100644 --- a/t/plugin_tests/virus/clamdscan +++ b/t/plugin_tests/virus/clamdscan @@ -8,11 +8,13 @@ use Qpsmtpd::Constants; sub register_tests { my $self = shift; - eval 'use ClamAV::Client'; ## no critic (Stringy) - if ( ! $@ ) { + SKIP: { + eval 'use ClamAV::Client'; ## no critic (Stringy) + skip "Could not load ClamAV::Client", 4 + if $@; $self->register_test('test_register', 3); $self->register_test('test_get_clamd', 1); - }; + } $self->register_test('test_err_and_return', 2); $self->register_test('test_get_filename', 1); $self->register_test('test_set_permission', 1); @@ -24,8 +26,8 @@ sub test_register { my $self = shift; ok( $self->{_args}{deny_viruses} eq 'yes', "deny_viruses"); - ok( $self->{_args}{max_size} == 128, "max_size"); - ok( $self->{_args}{scan_all} == 0, "scan_all"); + ok( $self->{_args}{max_size} == 1024, "max_size"); + ok( $self->{_args}{scan_all} == 1, "scan_all"); }; sub test_err_and_return { @@ -74,16 +76,10 @@ sub test_is_too_big { sub test_is_not_multipart { my $self = shift; - my $tran = shift || $self->qp->transaction(); + my $tran = $self->qp->transaction(); ok( $self->is_not_multipart(), "not_multipart" ); - - if ( $tran->header ) { - $tran->header->add('Content-Type', 'multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"'); - ok( ! $self->is_not_multipart(), "not_multipart" ); - } - else { - ok( 1 ); - } + $tran->header->add('Content-Type', 'multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"'); + ok( ! $self->is_not_multipart(), "not_multipart" ); } From 44cafde7d4155971d2495b249dcec2aee3c477b9 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 13:48:19 -0500 Subject: [PATCH 3/6] More complete clamdcscan tests * construct our own Qpsmtpd::Transaction object for testing, so we're sure to have a pristine state * Move some logic into a should_scan() sub, to help separate scan_all from is_not_multipart (since the multipart state of a message has nothing to do with any configuration parameters) --- plugins/virus/clamdscan | 18 ++++++++++------ t/plugin_tests/virus/clamdscan | 39 ++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/plugins/virus/clamdscan b/plugins/virus/clamdscan index 5621cc6..2b1cfc3 100644 --- a/plugins/virus/clamdscan +++ b/plugins/virus/clamdscan @@ -140,7 +140,7 @@ sub register { if ( ! defined $self->{_args}{deny_viruses} ) { $self->{_args}{deny_viruses} = 'yes'; } - if ( ! defined $self->{_args}{scan_all} ) { + if ( ! $self->{_args}{scan_all} ) { $self->{_args}{scan_all} = 1; } for my $setting (qw( deny_viruses defer_on_error scan_all )) { @@ -161,8 +161,7 @@ sub data_post_handler { $self->log(LOGINFO, "skip, naughty"); return (DECLINED); } - return (DECLINED) if $self->is_too_big($transaction); - return (DECLINED) if $self->is_not_multipart($transaction); + return DECLINED if ! $self->should_scan($transaction); my $clamd = $self->get_clamd() or return $self->err_and_return("Cannot instantiate ClamAV::Client"); @@ -328,8 +327,6 @@ sub is_not_multipart { my $self = shift; my $transaction = shift || $self->qp->transaction; - return if $self->{'_args'}{'scan_all'}; - return 1 if !$transaction->header; # Ignore non-multipart emails @@ -340,5 +337,14 @@ sub is_not_multipart { return 1; } - return; + return 0; +} + +sub should_scan { + my $self = shift; + my $tran = shift; + return 0 if $self->is_too_big($tran); + return 1 if $self->{_args}{scan_all}; + return 0 if $self->is_not_multipart($tran); + return 1; } diff --git a/t/plugin_tests/virus/clamdscan b/t/plugin_tests/virus/clamdscan index bd432ac..0aa450f 100644 --- a/t/plugin_tests/virus/clamdscan +++ b/t/plugin_tests/virus/clamdscan @@ -4,6 +4,8 @@ use strict; use warnings; use Qpsmtpd::Constants; +use Qpsmtpd::Transaction; +use Mail::Header; sub register_tests { my $self = shift; @@ -20,6 +22,7 @@ sub register_tests { $self->register_test('test_set_permission', 1); $self->register_test('test_is_too_big', 2); $self->register_test('test_is_not_multipart', 2); + $self->register_test('test_should_scan',4); } sub test_register { @@ -64,22 +67,40 @@ sub test_get_clamd { sub test_is_too_big { my $self = shift; - my $tran = shift || $self->qp->transaction(); + my $tran = Qpsmtpd::Transaction->new(); $self->{_args}{max_size} = 8; - $tran->{_body_size} = (7 * 1024 ); - ok( ! $self->is_too_big( $tran ), "is_too_big"); + $tran->{_body_size} = 7 * 1024; + ok( ! $self->is_too_big( $tran ), "is_too_big 1"); - $tran->{_body_size} = (9 * 1024 ); - ok( $self->is_too_big( $tran ), "is_too_big"); + $tran->{_body_size} = 9 * 1024; + ok( $self->is_too_big( $tran ), "is_too_big 2"); } sub test_is_not_multipart { my $self = shift; - my $tran = $self->qp->transaction(); + my $tran = Qpsmtpd::Transaction->new(); - ok( $self->is_not_multipart(), "not_multipart" ); - $tran->header->add('Content-Type', 'multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"'); - ok( ! $self->is_not_multipart(), "not_multipart" ); + ok( $self->is_not_multipart($tran), "not_multipart 1" ); + $tran->header( Mail::Header->new( [ + 'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"' + ] ) ); + ok( ! $self->is_not_multipart($tran), "not_multipart 2" ); } +sub test_should_scan { + my $self = shift; + my $trans = Qpsmtpd::Transaction->new(); + $trans->{_body_size} = 1; + $self->{_args}{scan_all} = 1; + ok( $self->should_scan($trans), "Should scan small message, scan_all=1"); + $self->{_args}{scan_all} = 0; + ok( ! $self->should_scan($trans), "Should not scan small message, scan_all=0"); + $trans->{_body_size} = 99999999999; + ok( !$self->should_scan($trans), "Should not scan large message" ); + $trans->{_body_size} = 1; + $trans->header( Mail::Header->new( [ + 'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"' + ] ) ); + ok( $self->should_scan($trans), "Should not scan multi-part message" ); +} From ba3122bd8211dc1b3d9745d845c9d3cbfe5f5577 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 13:51:19 -0500 Subject: [PATCH 4/6] Style cleanup * no more 'return ()' * Explicit 'return 0' --- plugins/virus/clamdscan | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/plugins/virus/clamdscan b/plugins/virus/clamdscan index 2b1cfc3..3fb220c 100644 --- a/plugins/virus/clamdscan +++ b/plugins/virus/clamdscan @@ -140,7 +140,7 @@ sub register { if ( ! defined $self->{_args}{deny_viruses} ) { $self->{_args}{deny_viruses} = 'yes'; } - if ( ! $self->{_args}{scan_all} ) { + if ( ! defined $self->{_args}{scan_all} ) { $self->{_args}{scan_all} = 1; } for my $setting (qw( deny_viruses defer_on_error scan_all )) { @@ -159,7 +159,7 @@ sub data_post_handler { if ($self->connection->notes('naughty')) { $self->log(LOGINFO, "skip, naughty"); - return (DECLINED); + return DECLINED; } return DECLINED if ! $self->should_scan($transaction); @@ -196,19 +196,19 @@ sub data_post_handler { $self->adjust_karma(-1); if ($self->{_args}{deny_viruses}) { - return (DENY, "Virus found: $found"); + return DENY, "Virus found: $found"; } $transaction->header->add('X-Virus-Found', 'Yes', 0); $transaction->header->add('X-Virus-Details', $found, 0); - return (DECLINED); + return DECLINED; } $self->log(LOGINFO, "pass, clean"); $transaction->header->add('X-Virus-Found', 'No', 0); $transaction->header->add('X-Virus-Checked', "by $version on " . $self->qp->config('me'), 0); - return (DECLINED); + return DECLINED; } sub assemble_message { @@ -226,9 +226,9 @@ sub err_and_return { if ($message) { $self->log(LOGERROR, $message); } - return (DENYSOFT, "Unable to scan for viruses") + return DENYSOFT, "Unable to scan for viruses" if $self->{_args}{defer_on_error}; - return (DECLINED, "skip"); + return DECLINED, "skip"; } sub get_filename { @@ -320,7 +320,7 @@ sub is_too_big { } $self->log(LOGDEBUG, "data_size, $size"); - return; + return 0; } sub is_not_multipart { From 3d977738270c688f5a8113872d36e390dbaa6f61 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 13:57:15 -0500 Subject: [PATCH 5/6] Reverse sense of is_not_multipart to is_multipart. --- plugins/virus/clamdscan | 12 ++++++------ t/plugin_tests/virus/clamdscan | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/plugins/virus/clamdscan b/plugins/virus/clamdscan index 3fb220c..808d307 100644 --- a/plugins/virus/clamdscan +++ b/plugins/virus/clamdscan @@ -323,21 +323,21 @@ sub is_too_big { return 0; } -sub is_not_multipart { +sub is_multipart { my $self = shift; my $transaction = shift || $self->qp->transaction; - return 1 if !$transaction->header; + return 0 if !$transaction->header; # Ignore non-multipart emails - my $content_type = $transaction->header->get('Content-Type') or return 1; + my $content_type = $transaction->header->get('Content-Type') or return 0; $content_type =~ s/\s/ /g; if ($content_type !~ m!\bmultipart/.*\bboundary="?([^"]+)!i) { $self->log(LOGNOTICE, "skip, not multipart"); - return 1; + return 0; } - return 0; + return 1; } sub should_scan { @@ -345,6 +345,6 @@ sub should_scan { my $tran = shift; return 0 if $self->is_too_big($tran); return 1 if $self->{_args}{scan_all}; - return 0 if $self->is_not_multipart($tran); + return 0 if ! $self->is_multipart($tran); return 1; } diff --git a/t/plugin_tests/virus/clamdscan b/t/plugin_tests/virus/clamdscan index 0aa450f..b7b2344 100644 --- a/t/plugin_tests/virus/clamdscan +++ b/t/plugin_tests/virus/clamdscan @@ -21,7 +21,7 @@ sub register_tests { $self->register_test('test_get_filename', 1); $self->register_test('test_set_permission', 1); $self->register_test('test_is_too_big', 2); - $self->register_test('test_is_not_multipart', 2); + $self->register_test('test_is_multipart', 2); $self->register_test('test_should_scan',4); } @@ -77,15 +77,15 @@ sub test_is_too_big { ok( $self->is_too_big( $tran ), "is_too_big 2"); } -sub test_is_not_multipart { +sub test_is_multipart { my $self = shift; my $tran = Qpsmtpd::Transaction->new(); - ok( $self->is_not_multipart($tran), "not_multipart 1" ); + ok( ! $self->is_multipart($tran), "is_multipart 1" ); $tran->header( Mail::Header->new( [ 'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"' ] ) ); - ok( ! $self->is_not_multipart($tran), "not_multipart 2" ); + ok( $self->is_multipart($tran), "is_multipart 2" ); } sub test_should_scan { From f2bcd3736a75d1b952429f412bfb180a15fb9e5d Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Tue, 16 Sep 2014 14:04:37 -0500 Subject: [PATCH 6/6] New tests to cover new handling of register-time configuration. --- t/plugin_tests/virus/clamdscan | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/t/plugin_tests/virus/clamdscan b/t/plugin_tests/virus/clamdscan index b7b2344..bbcbf3f 100644 --- a/t/plugin_tests/virus/clamdscan +++ b/t/plugin_tests/virus/clamdscan @@ -14,7 +14,7 @@ sub register_tests { eval 'use ClamAV::Client'; ## no critic (Stringy) skip "Could not load ClamAV::Client", 4 if $@; - $self->register_test('test_register', 3); + $self->register_test('test_register', 6); $self->register_test('test_get_clamd', 1); } $self->register_test('test_err_and_return', 2); @@ -28,9 +28,17 @@ sub register_tests { sub test_register { my $self = shift; - ok( $self->{_args}{deny_viruses} eq 'yes', "deny_viruses"); - ok( $self->{_args}{max_size} == 1024, "max_size"); - ok( $self->{_args}{scan_all} == 1, "scan_all"); + ok( $self->{_args}{deny_viruses}, "deny_viruses 1"); + is( $self->{_args}{max_size}, 1024, "max_size 1"); + ok( $self->{_args}{scan_all}, "scan_all 1"); + + my $qp = $self->qp; + + # Re-initialize the plugin with some different options + $self->register($qp,qw( scan_all 0 max_size 200 deny_viruses no)); + ok( ! $self->{_args}{deny_viruses}, "deny_viruses 2"); + is( $self->{_args}{max_size}, 200, "max_size 2"); + ok( !$self->{_args}{scan_all}, "scan_all 2"); }; sub test_err_and_return {