Closed Bug 448721 Opened 12 years ago Closed 10 years ago

Bring back review IPs


( Graveyard :: Public Pages, enhancement, P3)



(Not tracked)



(Reporter: fligtar, Assigned: sancus)





(2 files, 2 obsolete files)

We should display the IP of the submitter next to each review for admins and editors so we can better detect foul play.
Severity: normal → enhancement
Priority: -- → P3
Target Milestone: --- → 4.x (triaged)
Duplicate of this bug: 463656
Right now, there's no IP tracking at all that I can tell. So that means we don't know the IPs of all the currently posted reviews, and we'll also need to begin tracking them and putting them in the DB.

I'll take this, dunno if I can get it done for 5.4, if not I'll do it in 5.5.
Assignee: nobody → sancus
What's the status of this, Andrei?
Target Milestone: 4.x (triaged) → 5.5
Still planning to do this(and the other bug I've taken) for 5.5, which I believe freezes on January 5th. I'll be working on it in the next few days.
It occurred to me that there's a potential problem here -- I believe you guys are using Zeus now for caching, right? That will obscure the client IP from the usual ways to obtain it, so that means I need to pull it from the http headers.

I found some info here:

Is Zeus correctly passing the IP in the headers(Especially noting the comment about this not working with SSL if you don't have it configured right), and can I rely on that? Maybe clouserw knows?
The X-Forwarded-For header will work fine.  Note that it may be an array and not just an IP.  You should explode it on ',' and then array_pop() the result to get the client address.
OK, thanks!
(In reply to comment #6)
> The X-Forwarded-For header will work fine.  Note that it may be an array and
> not just an IP.  You should explode it on ',' and then array_pop() the result
> to get the client address.

Actually, you want to use array_shift(), not ...pop(). The original client is the first, not last, entry in the list, followed by any intermediary proxies they might have used.
Attached patch Add Review IPs v1 (obsolete) — Splinter Review
This adds ip addresses to reviews. IPs are per-review, rather than being tied to users in any way, so it should permanently store the source IP for every review no matter how it changes. Probably we want to track the IPs that each user uses commonly in some way in the future, to make it easier to compare.

IPs are displayed in the standard review list, where you can also report reviews, etc. It's easy to add the IP display to anywhere else, though.

All current reviews have no IPs associated with them, these will be displayed as right now. I can change that if that's a problem.
Attachment #420105 - Flags: review?(clouserw)
Attached patch Column Addition for IPs (obsolete) — Splinter Review
here's the added column for the reviews table.
Comment on attachment 420105 [details] [diff] [review]
Add Review IPs v1

Thanks, the patch looks pretty good.  Just two things:

1) ___('by %1$s (%2$s) on %3$s') needs an L10n comment to explain all the substitutions (I can help if you need the format)

2) And the reason for the r-; why are you converting the ips to long instead of just dealing with strings?
Attachment #420105 - Flags: review?(clouserw) → review-
I did some reading about storing IP addresses in databases out of curiosity(before I wrote my patch) and the obvious benefit is it takes less space(4 bytes instead of 15) and makes it better for indexing performance, etc. MySQL actually has functions built-in for IP conversion as well(INET_ATON and INET_NTOA). From what I understood it's a common way of storing IPs(although strings are common also).

It's no problem to change it if you prefer I use strings -- but I definitely didn't just do it randomly for no reason :)
I'd rather use strings.  We've had weird errors before with 32 bit vs. 64 bit stuff and this would be no exception.  We don't need to index on these or anything and space isn't a big deal.
Done and Done! Also fixed a dumb bug($isEditor not always being defined) that appeared because some code got erased when I corrected for a conflict this morning.
Attachment #420105 - Attachment is obsolete: true
Attachment #420208 - Flags: review?(clouserw)
Attachment #420106 - Attachment is obsolete: true
Attachment #420208 - Flags: review?(clouserw) → review+
Thanks, r59072 and r59073.  I adjusted the varchar to be 255 because space doesn't matter and I don't want to fight ipv6 if it ever happens.
Closed: 10 years ago
Resolution: --- → FIXED
Verified FIXED on

"by stephend ( on January 5, 2010"
Reopening; ack: should've known that class A IP address looked weird. says my IP address is actually must be Zeus?
Resolution: FIXED → ---
Depends on: 538089
Hmm, maybe the original client is not the first entry in the list as wenzel said? It's difficult for me to debug this since I don't have SVN access(can't push a test to preview) and I dunno what the Zeus X_FORWARDED_FOR array looks like at all.
Your code is fine.  I made this bug dependent on the one that will fix this.
Ah, I see. Well, thanks, I will happily do nothing!
This should be fixed now with 538089 fixed.
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Verified FIXED; now seeing it on preview:

by stephend ( on January 6, 2010
Product: → Graveyard
You need to log in before you can comment on or make changes to this bug.