IPBlacklistFilterStrategy and Use X-Forwarded-For

[trac@…]

I am running tracd behind a local Apache proxy.
this means that all the IP addresses trac sees are 127.0.0.1, however, Apache adds the header X-Forwarded-For that says what was the original IP address.
this cause a problem to the IPBlacklistFilterStrategy.
I suggest that the IPBlacklistFilterStrategy will take the X-Forwarded-For header into consideration.
if the ip address is a LAN address (127.0.0.1, 10.0.0.*, 192.168.* etc) it should ignore it and get the next one from the XFF header.

I have logic that does this for my plugin which can be used:
​http://firestats.cc/browser/branches/firestats-1.1/integration/trac/firestats/hitlogger.py

[anonymous]

Index: tracspamfilter/filters/ip_blacklist.py
===================================================================
--- tracspamfilter/filters/ip_blacklist.py      (revision 8323)
+++ tracspamfilter/filters/ip_blacklist.py      (working copy)
@@ -35,7 +35,7 @@
         overall karma of a submission.""")

     servers = ListOption('spam-filter', 'ip_blacklist_servers',
-                         'bsb.empty.us, sc.surbl.org', doc=
+                         'bsb.empty.us, multi.surbl.org', doc=
         """Servers used for IP blacklisting.""")

     # IFilterStrategy implementation

[anonymous]

Aargh, That comment was for #8032. Sorry. The ticket name is so equal :-)

[dstoecker]

In r9890.

[cboos]

r9890 doesn't look optimal, as it changes the existing interface in a very intrusive way (r9891, r9892) and therefore will break the other plugins relying on it.

I think that in this situation, the X-Forwarded-For should rather be used to replace req.remote_addr, as the original one is useless (we could still keep that in req._forwarded_by_addr eventually).

So if we would do the check done in [9890#file2] in Request.remote_addr​, then we could support this feature in a transparent way. Also, all the other uses of remote_addr would benefit from this fix as well (e.g. storing the IP of wiki authors in wiki changes, it doesn't help to store 127.0.0.1 either).

What do you think?

[anonymous]

I had also another reason to do the intrusive change regarding the IP parameter. Actually the IP is used by the filters to do decisions, so it also should be passed as explicit parameter (even if that means an interface change - are there really other plugins relying on spamfilter test/train API?) It was not so clean as I wished, but nothing is for free. The train() interface does a hack with setting REMOTE_ADDRESS to work around this missing argument.

If only for that bug report I would have replaced it inline where needed.

Handling X-Forwarded-For in Trac itself is nevertheless a good idea I would think. It must not necessarily mean to revert the IP argument :-)

[cboos]

Replying to anonymous:

I had also another reason to do the intrusive change regarding the IP parameter. Actually the IP is used by the filters to do decisions, so it also should be passed as explicit parameter (even if that means an interface change - are there really other plugins relying on spamfilter test/train API?)

Ah, you're right, I confused that with the other interface. Yeah, this is mainly an internal interface I suppose.

It was not so clean as I wished, but nothing is for free. The train() interface does a hack with setting REMOTE_ADDRESS to work around this missing argument.

author and ip could be keys in a submitter dictionary, for example.

If only for that bug report I would have replaced it inline where needed.

Handling X-Forwarded-For in Trac itself is nevertheless a good idea I would think. It must not necessarily mean to revert the IP argument :-)

Ok, I'll do this.

Oh, just a quick note though: for your commit messages, please take the habit of using the SpamFilter: prefix for the first line (take a look at log:/plugins for examples).

[omry]

guys, keep in mind that X-Forwarded-For can be easily be spoofed by spam bots.

each proxy appends itself the the X-Forwarded-For, but the user should be able to control exactly how this is handled.

my original bug report was about having tracd behind apache proxy. since in this case we know that there is an apache proxy we can safely strip the first (or is it the last?) ip address from the XFF list.

I think the implementation I suggested may be too naive for the general case.

[dstoecker]

I made it configurable in r9934. I think we should leave it there for some time and if it works as wanted, then maybe move it to Request directly.

[Pawel Tecza <ptecza@…>]

Hello there!

I have a standalone tracd which runs behind Nginx proxy. It sets a proper X-Real-IP and X-Forwarded-For headers, but it seems that BlogSpam? filter sets negative karma for a new tickets. I can see something like that on the admin/spamfilter/monitor page:

BlogSpamFilterStrategy (-5): BlogSpam says content is spam (badip:state/blacklist.d/127.0.0.1)

Below details about my Trac box:

• Debian 6.0 Squeeze
• Python 2.6.6
• Trac 0.12.2-1~bpo60+1
• TracSpamFilter?-0.4.8dev_r10882

I've resolved the issue by hacking tracd source code:

--- trac/web/standalone.py.old	2011-12-12 14:01:33.546973494 +0000
+++ trac/web/standalone.py	2011-12-12 14:00:50.554973513 +0000
@@ -112,6 +112,11 @@
     server_version = 'tracd/' + VERSION
 
     def address_string(self):
+        # Get remote address of client if tracd is behind a proxy
+        ip = self.headers.get("X-Real-IP") or \
+             self.headers.get("X-Forwarded-For")
+        if ip:
+            return ip
         # Disable reverse name lookups
         return self.client_address[:2][0]

Now tracd shows me a good remote IP address of client and I don't have a problem with a karma.