From 1d67c849cfb29326074d62ac1f97ddf4056edce9 Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Fri, 22 Jun 2012 18:29:28 -0400 Subject: [PATCH] spamassassin: add explicit default reject_type consolidate the two data_post methods into one (more linear, simpler) more informative log message add new headers to top of headers (not bottom (consistent MTA behavior)) --- plugins/spamassassin | 54 +++++++++++++++++----------- t/plugin_tests/spamassassin | 70 +++++++++++++++++++------------------ 2 files changed, 70 insertions(+), 54 deletions(-) diff --git a/plugins/spamassassin b/plugins/spamassassin index 1101f8e..2d7d2e5 100644 --- a/plugins/spamassassin +++ b/plugins/spamassassin @@ -141,6 +141,7 @@ use warnings; use Qpsmtpd::Constants; use Qpsmtpd::DSN; + use Socket qw(:DEFAULT :crlf); use IO::Handle; @@ -155,12 +156,14 @@ sub register { if ( ! defined $self->{_args}{reject} && defined $self->{_args}{reject_threshold} ) { $self->{_args}{reject} = $self->{_args}{reject_threshold}; }; + if ( ! defined $self->{_args}{reject_type} ) { + $self->{_args}{reject_type} = 'perm'; + }; - $self->register_hook('data_post', 'check_spam_reject'); - $self->register_hook('data_post', 'check_spam_munge_subject'); + $self->register_hook('data_post', 'data_post_handler'); } -sub hook_data_post { +sub data_post_handler { my ($self, $transaction) = @_; return (DECLINED) if $self->is_immune(); @@ -180,7 +183,8 @@ sub hook_data_post { my $headers = $self->parse_spamd_response( $SPAMD ) or return (DECLINED); $self->insert_spam_headers( $transaction, $headers, $username ); - return (DECLINED); + $self->munge_subject( $transaction ); + return $self->reject( $transaction ); }; sub select_spamd_username { @@ -361,52 +365,62 @@ sub print_to_spamd { $self->log(LOGDEBUG, "check_spam: finished sending to spamd"); }; -sub check_spam_reject { +sub reject { my ($self, $transaction) = @_; my $sa_results = $self->get_spam_results($transaction) or do { - $self->log(LOGNOTICE, "skip: no spamassassin results"); + $self->log(LOGNOTICE, "skip, no results"); return DECLINED; }; my $score = $sa_results->{score} or do { - $self->log(LOGERROR, "skip: error getting spamassassin score"); + $self->log(LOGERROR, "skip, error getting score"); return DECLINED; }; my $ham_or_spam = $sa_results->{is_spam} eq 'Yes' ? 'Spam' : 'Ham'; + my $status = "$ham_or_spam, $score"; + my $learn; + if ( $sa_results->{autolearn} ) { + $learn = "learn=". $sa_results->{autolearn}; + }; my $reject = $self->{_args}{reject} or do { - $self->log(LOGERROR, "skip: reject not set ($ham_or_spam, $score)"); + $self->log(LOGERROR, "skip, reject disabled ($status, $learn)"); return DECLINED; }; if ( $score < $reject ) { - $self->log(LOGINFO, "pass, $ham_or_spam, $score < $reject"); - return DECLINED; - }; + if ( $ham_or_spam eq 'Spam' ) { + $self->log(LOGINFO, "fail, $status < $reject, $learn"); + return DECLINED; + } + else { + $self->log(LOGINFO, "pass, $status < $reject, $learn"); + return DECLINED; + } + } + $self->connection->notes('karma', $self->connection->notes('karma') - 1); # default of media_unsupported is DENY, so just change the message - $self->log(LOGINFO, "deny, $ham_or_spam, $score > $reject"); - return Qpsmtpd::DSN->media_unsupported("spam score exceeded threshold"); + $self->log(LOGINFO, "deny, $status, > $reject, $learn"); + return ($self->get_reject_type(), "spam score exceeded threshold"); } -sub check_spam_munge_subject { +sub munge_subject { my ($self, $transaction) = @_; + my $sa = $self->get_spam_results($transaction) or return; my $qp_num = $self->{_args}{munge_subject_threshold}; - my $sa = $self->get_spam_results($transaction) or return DECLINED; my $required = $sa->{required} || $qp_num or do { $self->log(LOGDEBUG, "skipping munge, no user or qpsmtpd pref set"); - return DECLINED; + return; }; - return DECLINED unless $sa->{score} > $required; + return unless $sa->{score} > $required; my $subject_prefix = $self->qp->config('subject_prefix') || '*** SPAM ***'; my $subject = $transaction->header->get('Subject') || ''; $transaction->header->replace('Subject', "$subject_prefix $subject"); - - return DECLINED; } sub get_spam_results { @@ -465,7 +479,7 @@ sub _cleanup_spam_header { $old_header_name = ($old_header_name =~ s/^X-//) ? "X-Old-$old_header_name" : "Old-$old_header_name"; for my $header ( $transaction->header->get($header_name) ) { - $transaction->header->add($old_header_name, $header) if $action eq 'rename'; + $transaction->header->add($old_header_name, $header, 0) if $action eq 'rename'; $transaction->header->delete($header_name); } } diff --git a/t/plugin_tests/spamassassin b/t/plugin_tests/spamassassin index 67018b4..dfe6409 100644 --- a/t/plugin_tests/spamassassin +++ b/t/plugin_tests/spamassassin @@ -25,9 +25,9 @@ sub register_tests { $self->register_test('test_connect_to_spamd', 4); $self->register_test('test_parse_spam_header', 10); - $self->register_test('test_get_spam_results', 19); - $self->register_test('test_check_spam_munge_subject', 4); - $self->register_test('test_check_spam_reject', 2); + $self->register_test('test_get_spam_results', 20); + $self->register_test('test_munge_subject', 4); + $self->register_test('test_reject', 2); } sub test_connect_to_spamd { @@ -43,38 +43,38 @@ sub test_connect_to_spamd { $self->{_args}{spamd_socket} = '/var/run/spamd/spamd.socket'; my $SPAMD = $self->connect_to_spamd(); if ( $SPAMD ) { - ok( $SPAMD, "connect_to_spamd, socket"); - + ok( $SPAMD, "socket"); + $self->print_to_spamd( $SPAMD, $message, $length, $username ); shutdown($SPAMD, 1); # close our side of the socket (tell spamd we're done) my $headers = $self->parse_spamd_response( $SPAMD ); #warn Data::Dumper::Dumper($headers); - ok( $headers, "connect_to_spamd, socket response\n"); + ok( $headers, "socket response\n"); } else { - ok( 1 == 1, "connect_to_spamd, socket connect FAILED"); - ok( 1 == 1, "connect_to_spamd, socket response FAILED"); + ok( 1 == 1, "socket connect FAILED"); + ok( 1 == 1, "socket response FAILED"); }; # Try a TCP/IP connection $self->{_args}{spamd_socket} = '127.0.0.1:783'; $SPAMD = $self->connect_to_spamd(); if ( $SPAMD ) { - ok( $SPAMD, "connect_to_spamd, tcp/ip"); + ok( $SPAMD, "tcp/ip"); #warn Data::Dumper::Dumper($SPAMD); $self->print_to_spamd( $SPAMD, $message, $length, $username ); shutdown($SPAMD, 1); # close our side of the socket (tell spamd we're done) my $headers = $self->parse_spamd_response( $SPAMD ); #warn Data::Dumper::Dumper($headers); - ok( $headers, "connect_to_spamd, tcp/ip response\n"); + ok( $headers, "tcp/ip response\n"); } else { - ok( 1 == 1, "connect_to_spamd, tcp/ip connect FAILED"); - ok( 1 == 1, "connect_to_spamd, tcp/ip response FAILED"); + ok( 1 == 1, "tcp/ip connect FAILED"); + ok( 1 == 1, "tcp/ip response FAILED"); }; }; -sub test_check_spam_reject { +sub test_reject { my $self = shift; my $transaction = $self->qp->transaction; @@ -83,17 +83,17 @@ sub test_check_spam_reject { # message scored a 10, should pass $self->{_args}{reject} = 12; $transaction->notes('spamassassin', { is_spam => 'Yes', score => 10 } ); - my $r = $self->check_spam_reject($transaction); - cmp_ok( DECLINED, '==', $r, "check_spam_reject, $r"); - + my $r = $self->reject($transaction); + cmp_ok( DECLINED, '==', $r, "r: $r"); + # message scored a 15, should fail $self->{_args}{reject} = 12; $transaction->notes('spamassassin', { is_spam => 'Yes', score => 15 } ); - ($r) = $self->check_spam_reject($transaction); - cmp_ok( DENY, '==', $r, "check_spam_reject, $r"); + ($r) = $self->reject($transaction); + cmp_ok( DENY, '==', $r, "r: $r"); }; -sub test_check_spam_munge_subject { +sub test_munge_subject { my $self = shift; my $transaction = $self->qp->transaction; @@ -103,31 +103,31 @@ sub test_check_spam_munge_subject { $self->{_args}{munge_subject_threshold} = 5; $transaction->notes('spamassassin', { score => 6 } ); $transaction->header->add('Subject', $subject); - $self->check_spam_munge_subject($transaction); + $self->munge_subject($transaction); my $r = $transaction->header->get('Subject'); chomp $r; - cmp_ok($r, 'eq', "*** SPAM *** $subject", "check_spam_munge_subject +"); + cmp_ok($r, 'eq', "*** SPAM *** $subject", "+"); $transaction->header->delete('Subject'); # cleanup $self->{_args}{munge_subject_threshold} = 5; $transaction->notes('spamassassin', { score => 3 } ); $transaction->header->add('Subject', $subject); - $self->check_spam_munge_subject($transaction); + $self->munge_subject($transaction); $r = $transaction->header->get('Subject'); chomp $r; - cmp_ok($r, 'eq', $subject, "check_spam_munge_subject -"); + cmp_ok($r, 'eq', $subject, "-"); $transaction->header->delete('Subject'); # cleanup $transaction->notes('spamassassin', { score => 3, required => 4 } ); $transaction->header->add('Subject', $subject); - $self->check_spam_munge_subject($transaction); + $self->munge_subject($transaction); $r = $transaction->header->get('Subject'); chomp $r; - cmp_ok($r, 'eq', $subject, "check_spam_munge_subject -"); + cmp_ok($r, 'eq', $subject, "-"); $transaction->header->delete('Subject'); # cleanup $transaction->notes('spamassassin', { score => 5, required => 4 } ); $transaction->header->add('Subject', $subject); - $self->check_spam_munge_subject($transaction); + $self->munge_subject($transaction); $r = $transaction->header->get('Subject'); chomp $r; - cmp_ok($r, 'eq', "*** SPAM *** $subject", "check_spam_munge_subject +"); + cmp_ok($r, 'eq', "*** SPAM *** $subject", "+"); }; sub test_get_spam_results { @@ -145,15 +145,17 @@ sub test_get_spam_results { $r_ref->{hits} = delete $r_ref->{score}; # SA v2 compat }; my $r2 = _reassemble_header($r_ref); - cmp_ok( $h, 'eq', $r2, "get_spam_results ($h)" ); + cmp_ok( $h, 'eq', $r2, $h ); # this time it should be cached $r_ref = $self->get_spam_results($transaction); - next if $h =~ /hits=/; # caching is broken for SA v2 headers + if ( $h =~ /hits=/ ) { + ok( 1 ); + next; + }; # caching is broken for SA v2 headers $r2 = _reassemble_header($r_ref); - cmp_ok( $h, 'eq', $r2, "get_spam_results ($h)" ); + cmp_ok( $h, 'eq', $r2, $h ); }; - }; sub test_parse_spam_header { @@ -161,11 +163,11 @@ sub test_parse_spam_header { foreach my $h ( @sample_headers ) { my $r_ref = $self->parse_spam_header($h); - if ( $h =~ /hits=/ ) { + if ( $h =~ /hits=/ ) { $r_ref->{hits} = delete $r_ref->{score}; # SA v2 compat }; my $r2 = _reassemble_header($r_ref); - cmp_ok( $h, 'eq', $r2, "parse_spam_header ($h)" ); + cmp_ok( $h, 'eq', $r2, $h ); }; }; @@ -181,7 +183,7 @@ sub test_message { return <<'EO_MESSAGE' To: Fictitious User From: No Such -Subject: jose can you see, by the dawns early light? +Subject: jose can you see, by the dawns early light? What so proudly we. EO_MESSAGE