You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spamassassin.apache.org by jm...@apache.org on 2009/09/24 12:57:18 UTC
svn commit: r818443 - in /spamassassin/trunk: ./ lib/Mail/SpamAssassin/
lib/Mail/SpamAssassin/Conf/ lib/Mail/SpamAssassin/Plugin/ t/
Author: jm
Date: Thu Sep 24 10:57:17 2009
New Revision: 818443
URL: http://svn.apache.org/viewvc?rev=818443&view=rev
Log:
bug 6205: add test to ensure that all config settings are correctly handled when switching between users; add more config setting type metadata to enable those tests to work; and fix URIDetail to store config on the {conf} object, not on the plugin.
Added:
spamassassin/trunk/t/cross_user_config_leak.t
Modified:
spamassassin/trunk/MANIFEST
spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/ReplaceTags.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDetail.pm
spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/WhiteListSubject.pm
Modified: spamassassin/trunk/MANIFEST
URL: http://svn.apache.org/viewvc/spamassassin/trunk/MANIFEST?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/MANIFEST (original)
+++ spamassassin/trunk/MANIFEST Thu Sep 24 10:57:17 2009
@@ -517,3 +517,4 @@
t/spamd_whitelist_leak.t
t/db_awl_perms.t
t/stop_always_matching_regexps.t
+t/cross_user_config_leak.t
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Conf.pm Thu Sep 24 10:57:17 2009
@@ -374,6 +374,7 @@
push (@cmds, {
setting => 'whitelist_from_rcvd',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
unless (defined $value && $value !~ /^$/) {
@@ -389,6 +390,7 @@
push (@cmds, {
setting => 'def_whitelist_from_rcvd',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
unless (defined $value && $value !~ /^$/) {
@@ -500,6 +502,7 @@
push (@cmds, {
command => 'unblacklist_from',
setting => 'blacklist_from',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => \&Mail::SpamAssassin::Conf::Parser::remove_addrlist_value
});
@@ -658,6 +661,7 @@
push (@cmds, {
setting => 'rewrite_header',
+ type => $CONF_TYPE_HASH_KEY_VALUE,
code => sub {
my ($self, $key, $value, $line) = @_;
my($hdr, $string) = split(/\s+/, $value, 2);
@@ -861,6 +865,7 @@
push (@cmds, {
setting => 'report_safe',
default => 1,
+ type => $CONF_TYPE_NUMERIC,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value eq '') {
@@ -945,6 +950,7 @@
push (@cmds, {
setting => 'normalize_charset',
default => 0,
+ type => $CONF_TYPE_BOOL,
code => sub {
my ($self, $key, $value, $line) = @_;
unless (defined $value && $value !~ /^$/) {
@@ -1238,6 +1244,7 @@
push (@cmds, {
setting => 'dns_available',
default => 'test',
+ type => $CONF_TYPE_STRING,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value =~ /^test(?::\s+.+)?$/) {
@@ -1265,6 +1272,7 @@
push (@cmds, {
setting => 'dns_test_interval',
default => 600,
+ type => $CONF_TYPE_NUMERIC,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value !~ /^\d+$/) { return $INVALID_VALUE; }
@@ -1283,6 +1291,7 @@
push (@cmds, {
setting => 'dns_options',
+ type => $CONF_TYPE_HASH_KEY_VALUE,
code => sub {
my ($self, $key, $value, $line) = @_;
my $allowed_opts = "rotate";
@@ -1592,6 +1601,7 @@
push (@cmds, {
setting => 'lock_method',
default => '',
+ type => $CONF_TYPE_STRING,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value !~ /^(nfssafe|flock|win32)$/) {
@@ -1854,6 +1864,7 @@
setting => 'allow_user_rules',
is_priv => 1,
default => 0,
+ type => $CONF_TYPE_BOOL,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value eq '') {
@@ -2586,7 +2597,8 @@
$self->{by_zone}{$zone}{rbl_timeout} = $1+0;
$self->{by_zone}{$zone}{rbl_timeout_min} = $2+0 if defined $2;
}
- }
+ },
+ type => $CONF_TYPE_NUMERIC
});
=item util_rb_tld tld1 tld2 ...
@@ -2732,6 +2744,7 @@
setting => 'bayes_store_module',
is_admin => 1,
default => '',
+ type => $CONF_TYPE_STRING,
code => sub {
my ($self, $key, $value, $line) = @_;
local ($1);
@@ -2918,6 +2931,7 @@
push (@cmds, {
setting => 'user_scores_sql_custom_query',
is_admin => 1,
+ default => undef,
type => $CONF_TYPE_STRING
});
@@ -3762,6 +3776,7 @@
# and now, copy over all the rest -- the less complex cases.
while(my($k,$v) = each %{$source}) {
next if exists $done{$k}; # we handled it above
+ $done{$k} = undef;
my $i = ref($v);
# Not a reference, or a scalar? Just copy the value over.
@@ -3783,6 +3798,13 @@
}
}
+ foreach my $cmd (@{$self->{registered_commands}}) {
+ my $k = $cmd->{setting};
+ next if exists $done{$k}; # we handled it above
+ $done{$k} = undef;
+ $dest->{$k} = $source->{$k};
+ }
+
# scoresets
delete $dest->{scoreset};
for my $i (0 .. 3) {
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm Thu Sep 24 10:57:17 2009
@@ -64,7 +64,11 @@
- $CONF_TYPE_HASH_KEY_VALUE: hash key/value pair,
like "describe" or tflags
-If this is set, a 'code' block is assigned based on the type.
+If this is set, and a 'code' block does not already exist, a 'code' block is
+assigned based on the type.
+
+In addition, the SpamAssassin test suite will validate that the settings
+do not 'leak' between users.
Note that C<$CONF_TYPE_HASH_KEY_VALUE>-type settings require that the
value be non-empty, otherwise they'll produce a warning message.
@@ -88,6 +92,10 @@
Any other values -- including C<undef> -- returned from the subroutine are
considered to mean 'success'.
+It is good practice to set a 'type', if possible, describing how your settings
+are stored on the Conf object; this allows the SpamAssassin test suite to
+validate that the settings do not 'leak' between users.
+
=item default
The default value for the setting. may be omitted if the default value is a
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm Thu Sep 24 10:57:17 2009
@@ -344,6 +344,7 @@
push (@cmds, {
setting => 'whitelist_from_dkim',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
local ($1,$2);
@@ -362,6 +363,7 @@
push (@cmds, {
setting => 'def_whitelist_from_dkim',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
local ($1,$2);
@@ -380,6 +382,7 @@
push (@cmds, {
setting => 'adsp_override',
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_HASH_KEY_VALUE,
code => sub {
my ($self, $key, $value, $line) = @_;
local ($1,$2);
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/ReplaceTags.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/ReplaceTags.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/ReplaceTags.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/ReplaceTags.pm Thu Sep 24 10:57:17 2009
@@ -251,6 +251,7 @@
push(@cmds, {
setting => 'replace_rules',
is_priv => 1,
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_HASH_KEY_VALUE,
code => sub {
my ($self, $key, $value, $line) = @_;
unless (defined $value && $value !~ /^$/) {
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm Thu Sep 24 10:57:17 2009
@@ -580,6 +580,7 @@
push (@cmds, {
setting => 'uridnsbl_skip_domain',
default => {},
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_HASH_KEY_VALUE,
code => sub {
my ($self, $key, $value, $line) = @_;
if ($value =~ /^$/) {
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDetail.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDetail.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDetail.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/URIDetail.pm Thu Sep 24 10:57:17 2009
@@ -14,6 +14,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
# </...@LICENSE>
+#
+# TODO: where are the tests?
=head1 NAME
@@ -129,7 +131,7 @@
}
dbg("config: uri_detail adding ($target $op /$pattern/) to $name");
- $pluginobj->{uri_detail}->{$name}->{$target} = [$op, $pattern];
+ $conf->{parser}->{conf}->{uri_detail}->{$name}->{$target} = "$op /$pattern/";
$added_criteria = 1;
}
@@ -157,7 +159,7 @@
dbg("uri: running $test\n");
- my $rule = $self->{uri_detail}->{$test};
+ my $rule = $permsg->{conf}->{uri_detail}->{$test};
if (exists $rule->{raw}) {
my($op,$patt) = @{$rule->{raw}};
Modified: spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/WhiteListSubject.pm
URL: http://svn.apache.org/viewvc/spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/WhiteListSubject.pm?rev=818443&r1=818442&r2=818443&view=diff
==============================================================================
--- spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/WhiteListSubject.pm (original)
+++ spamassassin/trunk/lib/Mail/SpamAssassin/Plugin/WhiteListSubject.pm Thu Sep 24 10:57:17 2009
@@ -77,6 +77,7 @@
push(@cmds, {
setting => 'whitelist_subject',
default => {},
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
@@ -92,6 +93,7 @@
push(@cmds, {
setting => 'blacklist_subject',
default => {},
+ type => $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST,
code => sub {
my ($self, $key, $value, $line) = @_;
Added: spamassassin/trunk/t/cross_user_config_leak.t
URL: http://svn.apache.org/viewvc/spamassassin/trunk/t/cross_user_config_leak.t?rev=818443&view=auto
==============================================================================
--- spamassassin/trunk/t/cross_user_config_leak.t (added)
+++ spamassassin/trunk/t/cross_user_config_leak.t Thu Sep 24 10:57:17 2009
@@ -0,0 +1,230 @@
+#!/usr/bin/perl
+
+BEGIN {
+ if (-e 't/test_dir') { # if we are running "t/rule_tests.t", kluge around ...
+ chdir 't';
+ }
+
+ if (-e 'test_dir') { # running from test directory, not ..
+ unshift(@INC, '../blib/lib');
+ unshift(@INC, '../lib');
+ }
+}
+
+my $prefix = '.';
+if (-e 'test_dir') { # running from test directory, not ..
+ $prefix = '..';
+}
+
+use lib '.'; use lib 't';
+use SATest; sa_t_init("cross_user_config_leak");
+use Test; BEGIN { plan tests => 6 };
+
+# ---------------------------------------------------------------------------
+# bug 6003
+
+# TODO: we could also do this by having a boolean attribute on the command
+# structure itself to indicate that this test is superfluous. But that's
+# exposing a test-only feature through production code, so right now in my opinion
+# this is cleaner.
+#
+my @ignored_commands = qw(
+
+ score unwhitelist_from unblacklist_from unwhitelist_auth
+ unwhitelist_from_rcvd clear_report_template clear_unsafe_report_template
+ header body uri rawbody full meta test loadplugin tryplugin require_version
+ uri_detail version_tag uridnssub uridnsbl urirhsbl urirhssub urinsrhsbl
+ urinsrhssub urifullnsrhsbl urifullnsrhssub add_header remove_header
+ clear_headers trusted_networks clear_trusted_networks internal_networks
+ clear_internal_networks msa_networks clear_msa_networks bayes_ignore_header
+ report_safe_copy_headers redirector_pattern reuse mimeheader uridnsbl_timeout
+
+);
+
+use strict;
+use warnings;
+require Mail::SpamAssassin;
+
+my $sa = create_saobj({
+ require_rules => 1,
+ local_tests_only => 1,
+ dont_copy_prefs => 1,
+ #debug=>1,
+});
+
+$sa->compile_now(0,1);
+ok($sa);
+
+my %conf_backup;
+$sa->copy_config(undef, \%conf_backup) || die "copy_config failed";
+ok (scalar keys %conf_backup > 2);
+
+# ---------------------------------------------------------------------------
+
+# these need to be pretty improbable so they won't crop up in the defaults
+my $EXPECTED_VAL_STRING = '__test_expected_str';
+my $EXPECTED_VAL_BOOL = 1;
+my $EXPECTED_VAL_BOOL_FALSE = 0;
+my $EXPECTED_VAL_NUMERIC = 9438234;
+my $EXPECTED_VAL_TEMPLATE = '__test_expected_tmpl';
+my $EXPECTED_VAL_HK_KEY = '__test_expected_hk_key';
+my $EXPECTED_VAL_HK_VALUE = '__test_expected_hk_val';
+my $EXPECTED_VAL_ADDRLIST = '__test_expected_foo@bar.com';
+my %expected_val;
+my %ignored_command;
+foreach my $k (@ignored_commands) { $ignored_command{$k}++; }
+
+$sa->read_scoreonly_config("log/user_prefs1");
+set_all_confs($sa->{conf});
+
+$sa->signal_user_changed( { username => "user1", user_dir => "log/user1" });
+ok validate_all_confs($sa->{conf}, 1, 'after first user config read');
+
+$sa->copy_config(\%conf_backup, undef) or die "copy_config failed";
+ok validate_all_confs($sa->{conf}, 0, 'after restoring from backup');
+
+
+$sa->read_scoreonly_config("log/user_prefs2");
+$sa->signal_user_changed( { username => "user2", user_dir => "log/user2" });
+ok validate_all_confs($sa->{conf}, 0, 'after second user config read');
+
+$sa->copy_config(\%conf_backup, undef) or die "copy_config failed";
+ok validate_all_confs($sa->{conf}, 0, 'after second restore from backup');
+exit;
+
+# ---------------------------------------------------------------------------
+
+sub set_all_confs {
+ my ($conf) = @_;
+ foreach my $cmd (@{$conf->{registered_commands}}) {
+ my $k = $cmd->{setting};
+
+ if (!defined $cmd->{type}) {
+ next if $ignored_command{$k};
+ next if ($cmd->{command} && $ignored_command{$cmd->{command}});
+
+ # administrative commands by definition cannot change between users
+ next if ($cmd->{is_admin});
+
+ # attempt to infer types from the default value; if it's a scalar,
+ # we can consider the type to be similarly scalar
+ my $def = $cmd->{default};
+ if (defined $def && ref $def =~ /SCALAR/) {
+ if ("".$def =~ /[^\.\-\d]/) {
+ $cmd->{type} = $Mail::SpamAssassin::Conf::CONF_TYPE_STRING;
+ } else {
+ $cmd->{type} = $Mail::SpamAssassin::Conf::CONF_TYPE_NUMERIC;
+ # we don't actually have to differentiate booleans and numeric,
+ # they're stored the same anyway
+ }
+ }
+
+ # ignore commands defined using custom code; we don't know how/what they
+ # store. Off for now; there's a lot of risk that we'll miss a bug if we
+ # don't pay attention to them anyway. They can be dealt with on a
+ # case-by-case basis using @ignored_commands instead.
+ #
+ ##next if defined $cmd->{code};
+ }
+
+ if (!defined $cmd->{type}) {
+ warn "undef config type for $k".
+ ($cmd->{command} ? " (command=$cmd->{command})" : "");
+ next;
+ }
+
+ if ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_STRING) {
+ $conf->{$k} = $EXPECTED_VAL_STRING;
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_BOOL) {
+ if ($cmd->{default} != $EXPECTED_VAL_BOOL) {
+ $conf->{$k} = $EXPECTED_VAL_BOOL;
+ } else {
+ # we can't use the same value as the default, otherwise we'll
+ # be unable to tell cases where the config has been leaked
+ # from cases where the default is in use
+ $conf->{$k} = $EXPECTED_VAL_BOOL_FALSE;
+ }
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_NUMERIC) {
+ $conf->{$k} = $EXPECTED_VAL_NUMERIC;
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_TEMPLATE) {
+ $conf->{$k} = $EXPECTED_VAL_TEMPLATE;
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_HASH_KEY_VALUE) {
+ $conf->{$k}->{$EXPECTED_VAL_HK_KEY} = $EXPECTED_VAL_HK_VALUE;
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST) {
+ $conf->add_to_addrlist($k, $EXPECTED_VAL_ADDRLIST);
+ }
+ $expected_val{$k} = $conf->{$k};
+ }
+}
+
+my $setting_details;
+my $validation_passed;
+my $settings_should_exist;
+
+sub validate_all_confs {
+ my ($conf, $exist, $stage) = @_;
+
+ $setting_details = '';
+ $validation_passed = 1;
+ $settings_should_exist = $exist;
+
+ foreach my $cmd (@{$conf->{registered_commands}}) {
+ my $k = $cmd->{setting};
+
+ # if the default value is undef, it's a permitted value, obvs
+ next if ($settings_should_exist && !defined $cmd->{default});
+
+ $setting_details = "key='$k' when=$stage";
+ if (!defined $cmd->{type}) {
+ # warn "undef config type for $k"; # already done this
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_STRING) {
+ assert_validation($conf->{$k}, $expected_val{$k});
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_BOOL) {
+ assert_validation($conf->{$k}, $expected_val{$k});
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_NUMERIC) {
+ assert_validation($conf->{$k}, $expected_val{$k});
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_TEMPLATE) {
+ assert_validation($conf->{$k}, $expected_val{$k});
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_HASH_KEY_VALUE) {
+ my $val = $conf->{$k}->{$EXPECTED_VAL_HK_KEY};
+ assert_validation($val, $EXPECTED_VAL_HK_VALUE);
+ }
+ elsif ($cmd->{type} == $Mail::SpamAssassin::Conf::CONF_TYPE_ADDRLIST) {
+ my $val = $conf->{$k}->{$EXPECTED_VAL_ADDRLIST};
+ if (($settings_should_exist && !defined $val)
+ || (!$settings_should_exist && $val))
+ {
+ assert_validation($k, $val, 0); # this will fail, which is what we want
+ }
+ } else {
+ warn "unknown config type: $cmd->{type} for $k";
+ }
+ }
+ return $validation_passed;
+}
+
+sub assert_validation {
+ my ($val, $expected_val) = @_;
+ if ($settings_should_exist && (!defined $val || $val ne $expected_val)) {
+ warn "found=".(defined $val ? "'$val'" : "(none)").
+ " wanted=".(defined $expected_val ? "'$expected_val'" : "(none)").
+ " $setting_details";
+ $validation_passed = 0;
+ }
+ if (!$settings_should_exist && defined($val) && "".$val eq "".$expected_val) {
+ warn "found=".(defined $val ? "'$val'" : "(none)")." wanted=(none)".
+ " $setting_details";
+ $validation_passed = 0;
+ }
+}
+