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 .