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/plugins/virus/clamdscan b/plugins/virus/clamdscan index 5a990e7..808d307 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; @@ -155,10 +159,9 @@ sub data_post_handler { if ($self->connection->notes('naughty')) { $self->log(LOGINFO, "skip, naughty"); - return (DECLINED); + 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"); @@ -193,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 { @@ -223,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 { @@ -317,24 +320,31 @@ sub is_too_big { } $self->log(LOGDEBUG, "data_size, $size"); - return; + return 0; } -sub is_not_multipart { +sub is_multipart { my $self = shift; my $transaction = shift || $self->qp->transaction; - return if $self->{'_args'}{'scan_all'}; - - 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; + return 1; +} + +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_multipart($tran); + return 1; } diff --git a/t/plugin_tests/virus/clamdscan b/t/plugin_tests/virus/clamdscan index 8deb1a3..bbcbf3f 100644 --- a/t/plugin_tests/virus/clamdscan +++ b/t/plugin_tests/virus/clamdscan @@ -4,28 +4,41 @@ use strict; use warnings; use Qpsmtpd::Constants; +use Qpsmtpd::Transaction; +use Mail::Header; sub register_tests { my $self = shift; - eval 'use ClamAV::Client'; ## no critic (Stringy) - if ( ! $@ ) { - $self->register_test('test_register', 3); + SKIP: { + eval 'use ClamAV::Client'; ## no critic (Stringy) + skip "Could not load ClamAV::Client", 4 + if $@; + $self->register_test('test_register', 6); $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); $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); } 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}{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 { @@ -62,28 +75,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 { +sub test_is_multipart { my $self = shift; - my $tran = shift || $self->qp->transaction(); + my $tran = Qpsmtpd::Transaction->new(); - 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 ); - } + ok( ! $self->is_multipart($tran), "is_multipart 1" ); + $tran->header( Mail::Header->new( [ + 'Content-Type: multipart/alternative; boundary="Jx3Wbb8BMHsO=_?:"' + ] ) ); + ok( $self->is_multipart($tran), "is_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" ); +}