Edgewall Software
Modify

Opened 18 years ago

Closed 14 years ago

Last modified 13 years ago

#5199 closed enhancement (fixed)

IPBlacklistFilterStrategy and Use X-Forwarded-For

Reported by: trac@… Owned by:
Priority: normal Milestone: plugin - spam-filter
Component: plugin/spamfilter Version:
Severity: normal Keywords: apache proxy x-forwarded-for
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

Attachments (0)

Change History (11)

comment:1 by Christian Boos, 18 years ago

Component: generalspamfilter
Milestone: not applicable
Owner: changed from Jonas Borgström to Matthew Good

comment:2 by anonymous, 15 years ago

Owner: changed from Matthew Good to Christian Boos
Summary: IPBlacklistFilterStrategy and Use X-Forwarded-For[PATCH] IPBlacklistFilterStrategy and Use X-Forwarded-For
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

comment:3 by anonymous, 15 years ago

Summary: [PATCH] IPBlacklistFilterStrategy and Use X-Forwarded-ForIPBlacklistFilterStrategy and Use X-Forwarded-For

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

comment:4 by Christian Boos, 15 years ago

Milestone: not applicablespam-filter-plugin
Owner: Christian Boos removed

comment:5 by Dirk Stöcker, 14 years ago

Resolution: fixed
Status: newclosed

In r9890.

comment:6 by Christian Boos, 14 years ago

Keywords: apache proxy x-forwarded-for added
Resolution: fixed
Status: closedreopened

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?

comment:7 by anonymous, 14 years ago

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 :-)

in reply to:  7 comment:8 by Christian Boos, 14 years ago

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).

comment:9 by omry, 14 years ago

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.

comment:10 by Dirk Stöcker, 14 years ago

Resolution: fixed
Status: reopenedclosed

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.

comment:11 by Pawel Tecza <ptecza@…>, 13 years ago

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.