SA Bugzilla – Bug 6500
[review] clear_originating_ip_headers seems to be broken
Last modified: 2011-05-18 19:42:48 UTC
Created attachment 4811 [details] Only convert configured originating_ip headers to synthetic Received headers It seems the clear_originating_ip_headers config option seems to be broken. The four default originating_ip_headers (X-Yahoo-Post-IP, X-Originating-IP, X-Apparently-From, X-SenderIP) are always converted into synthetic Received headers, even if the list of originating ip headers is cleared (or modified) with the clear_originating_ip_headers and/or originating_ip_headers options. This happens in Mail/SpamAssassin/Message/Metadata/Received.pm. The attached patch changes it so only the headers that are configured as originating_ip_headers are converted into synthetic Received headers.
Created attachment 4812 [details] Debug output of testcase These are the files I used to test this. It contains a test email and four debug output runs of spamassassin -D < test_mail.txt: two runs with stock SA 3.3.1 (one without, and one with the clear_originating_ip_headers option set) and two runs with a patched SA 3.3.1 (again one without and one with the clear_originating_ip_headers option set). test_mail.txt sa_3.3.1_a_without_clear.txt sa_3.3.1_b_with_clear.txt sa_3.3.1patched_a_without_clear.txt sa_3.3.1patched_b_with_clear.txt
The test mail contains the following header: X-Originating-Ip: [121.6.137.76] The IP address in this header is subjected to RBL lookups. I would expect that, when I use the clear_originating_ip_headers option, this would not happen. However, the unpatched version does RBL lookups of this IP with and without the clear_originating_ip_headers option. With the patch, if clear_originating_ip_headers is specified, the IP address from this header is not subjected to RBL lookups. If I misunderstood the effect of clear_originating_ip_headers, please let me know. Regards, roel
I agree, it doesn't make sense to have two sets of 'originating' header field names used in two places: in SA::Message/Metadata/Received.pm and in the DNSEval.pm plugin, especially when both sets are identical by default, but one is hard-wired and the other is configurable. Better to use the same configurable set to be used in both places for consistency and flexibility, like your patch provides. (I'm not sure whether the DNSEval.pm plugin needs this special handling of 'originating' header fields at all, considering that they are already converted to synthetic Received header fields by the Received.pm. But that's probably a topic for some other PR. One could also hope these would be inserted into the Received trace list in the position where they occur in the header section instead of being appended at the end. A yet another PR I guess). +1 for your patch trunk: Bug 6500: clear_originating_ip_headers seems to be broken Sending lib/Mail/SpamAssassin/Message/Metadata/Received.pm Committed revision 1056466.
Tentatively setting target to 3.3.2. Opinions? Votes?
+1 for 3.3 no doubt
> Tentatively setting target to 3.3.2. Opinions? Votes? This looks like a slam dunk. I like the reused of @{$permsgstatus->{main}->{conf}->{originating_ip_headers}} rather than restating the non-standard headers. I'm +1 but always like to see test cases added especially since we have at least 1 test-case in Comment 1. Regards, KAM
Created attachment 4873 [details] Added a test and a sample mail trunk: Bug 6500: clear_originating_ip_headers seems to be broken - added a test Adding t/data/nice/orig_ip_hdr.eml Adding t/originating_ip_hdr.t Committed revision 1099843.
branch 3.3: Bug 6500: clear_originating_ip_headers seems to be broken - added a test Adding t/data/nice/orig_ip_hdr.eml Adding t/originating_ip_hdr.t Committed revision 1099844.
> Adding t/data/nice/orig_ip_hdr.eml > Adding t/originating_ip_hdr.t Adding these two filenames to MANIFEST: trunk: Bug 6500: clear_originating_ip_headers seems to be broken - updating MANIFEST Sending MANIFEST Committed revision 1099853. 3.3: Bug 6500: clear_originating_ip_headers seems to be broken - updating MANIFEST Sending MANIFEST Committed revision 1099851.
So I guess this can be closed now.
Actually I never committed the fix to 3.3, so here it goes: 3.3: Bug 6500: clear_originating_ip_headers seems to be broken Sending lib/Mail/SpamAssassin/Message/Metadata/Received.pm Committed revision 1099867.
The 'make disttest' is failing (as run by Jenkins), apparently because it lacks any local rules. Let's work around this: --- t/originating_ip_hdr.t (revision 1100004) +++ t/originating_ip_hdr.t (working copy) [...] +# the 'originating_ip_headers X-Originating-IP' is normally in a default +# set of originating_ip_headers, but not when 'make disttest' is run; +# let's just make it explicit: +# tstlocalrules (q{ + originating_ip_headers X-Originating-IP branch 3.3: Bug 6500: a workaround for "make disttest" in t/originating_ip_hdr.t Sending t/originating_ip_hdr.t Committed revision 1100008. trunk: Bug 6500: a workaround for "make disttest" in t/originating_ip_hdr.t Sending t/originating_ip_hdr.t Committed revision 1100009.
See also Bug 6591 .