Bug #1316

get_ip and $_SERVER['HTTP_X_FORWARDED_FOR

Added by Jesse Labrocca over 3 years ago. Updated about 3 years ago.

Status:ClosedStart date:11/17/2010
Priority:NormalDue date:
Assignee:Tom Moore% Done:

100%

Category:Other
Target version:1.6.4
Reproducibility:Always Database Type:
Reported In MyBB Version:1.6.0 Database Version:
PHP Version: SQA assignments:Stefan T.
Browser:

Description

In inc/functions.php the logic is wrong for get_ip().

function get_ip()
{
    if(isset($_SERVER['REMOTE_ADDR']))
    {
        $ip = $_SERVER['REMOTE_ADDR'];
    }
    elseif(isset($_SERVER['HTTP_X_FORWARDED_FOR']))
    {

The elseif should only be if.

History

#1 Updated by Ryan Gordon over 3 years ago

Doesn't seem wrong to me. Can you explain why?

#2 Updated by Jesse Labrocca over 3 years ago

Because $_SERVER['REMOTE_ADDR'] is assigned as well as $_SERVER['HTTP_X_FORWARDED_FOR']. So once it knows that $_SERVER['REMOTE_ADDR'] it will not do the the elseif. The current code would only work if $_SERVER['HTTP_X_FORWARDED_FOR'] is set and $_SERVER['REMOTE_ADDR'] is not.

PM me at MyBB.com if you need example script running on one of my sites. I went over this with Dennis. I'm using a new reverse proxy service and without this code change the wrong IP is logged.

if (condition A){
}elseif(condition B){
}

Condition A exists in ALL cases so Condition B is never executed. Hope that clarifies this.

#3 Updated by Jesse Labrocca over 3 years ago

Here from php.net

The elseif statement is only executed if the preceding if expression and any preceding elseif expressions evaluated to FALSE, and the current elseif expression evaluated to TRUE.

#4 Updated by Ryan Gordon over 3 years ago

I understand how if and elseif logic statements work, what I am not completely convinced of is that the order of execution should be changed.

Currently $_SERVER['REMOTE_ADDR'] takes precedence over $_SERVER['HTTP_X_FORWARDED_FOR']. You're asking me to reverse that. I just need a good explanation to have confidence that this change won't cause any regressions (rather, for me to take the risk of regressions with this change)

#5 Updated by Jesse Labrocca over 3 years ago

I always seem bad at explaining but I think most of my bug reports have come out valid.

I'll try to explain again.

If member is using a proxy the $_SERVER['HTTP_X_FORWARDED_FOR'] is set as well as the $_SERVER['REMOTE_ADDR']. Since $_SERVER['REMOTE_ADDR'] is checked first the if statement is true and elseif check for $_SERVER['HTTP_X_FORWARDED_FOR'] is never logged which is the true ip. It's the whole purpose of that code.

Go test this under the right conditions yourself. I'm terrible at explaining but I do a lot of testing to make sure my conclusions are right.

I'll do another explain too. This is how you have the code.

a= 1
b=2

if (a=1){
echo "true";
}elseif(b=2){
echo "false";
}

False will never happen because once you have the first true statement the other elseif's don't execute. $_SERVER['REMOTE_ADDR'] is ALWAYS TRUE.

#6 Updated by Ryan Gordon over 3 years ago

Alright, that makes more sense. But do we want the get_ip() function to use the $_SERVER['HTTP_X_FORWARDED_FOR'] if it's set from a proxy? I'll do some more research into it and see what I can come up with.

#7 Updated by Jesse Labrocca over 3 years ago

Here is how this works.

['HTTP_X_FORWARDED_FOR'] is sent by a proxy service (squid or nginx) and the $_SERVER['REMOTE_ADDR'] variable is actually the proxy service IP. So anytime ['HTTP_X_FORWARDED_FOR'] is used the REMOTE_ADDR is there too.

The purpose of get_ip() function is a great one. I'm sure it's very old legacy code but it's still vital. Unfortunately it's never worked right.

I'm now using a reverse proxy service with does give me the HTTP_X_FORWARDED_FOR variable in the header which is the actual members IP. Very important for features to work (banning and logging) properly. The old code will always log the proxy ip REMOTE_ADDR in every circumstance.

The fix is the most simplest of changes.

function get_ip() {
if(isset($_SERVER['REMOTE_ADDR'])) {
$ip = $_SERVER['REMOTE_ADDR'];
}
if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {

That will do it. $ip will be the remote but if the HTTP_X_FORWARDED_FOR isset then the $ip is checked and changed. Feel free to test yourself but I've done so in numerous circumstances.

#8 Updated by zinga burga over 3 years ago

No, the logic shouldn't be changed. To handle internal proxies, perhaps check that the remote address is an internal one (127.0.0.1 and other common subnets).

I purposely changed the logic to prevent forging headers effectively allowing forged IPs on the majority of webservers. X-Forwarded-For may be set by an internal proxy, but there is no guarantee that that is the case. It can very easily be set by someone trying to forge their address.

#9 Updated by Jesse Labrocca over 3 years ago

Logic doesn't work as-is. That's why it should be changed. Your argument that headers could be forged are null since the IP could be forged anyways and you might as well take away the entire function anyways.

The function was meant to log the HTTP_X_FORWARDED_FOR if it existed. It doesn't. That's the bug. You're arguing about changing the feature. I'm arguing about fixing a bug.

If you have a reverse proxy situation you need to log HTTP_X_FORWARDED_FOR.

#10 Updated by zinga burga over 3 years ago

Jesse Labrocca wrote:

Logic doesn't work as-is. That's why it should be changed. Your argument that headers could be forged are null since the IP could be forged anyways and you might as well take away the entire function anyways.

You clearly do NOT know how IP forging works. It operates at a webserver level and is extremely hard to do. Just Google it and educate yourself a bit before making silly comments, please.
Or, if you're confident about forging IPs, please demonstrate to me how you do it. Good luck cracking a CSPRNG - I'd like to see you try...

The function was meant to log the HTTP_X_FORWARDED_FOR if it existed. It doesn't. That's the bug. You're arguing about changing the feature. I'm arguing about fixing a bug.

It used to, but I changed it so that it doesn't. (I left the useless other code there because MyBB staff don't seem to like excessive change)

If you have a reverse proxy situation you need to log HTTP_X_FORWARDED_FOR.

Not all proxies use that header. nginx, by default, uses Real-IP, for example. In the end, you'll never be able to catch all instances. As I suggested, an improvement could be to check whether REMOTE_ADDR is from an internal address, and disregard it if so. This fixes up the problem with internal proxies, and WILL FIX YOUR PROBLEM. Your solution only fixes it by introducing a security hole.

#11 Updated by Jesse Labrocca over 3 years ago

So what's the purpose of the get_ip function if you're not going to even consider the code that uses HTTP_X_FORWARDED_FOR. Why not just completely remove it since it doesn't work?

It used to work as you say but you changed it and now it doesn't. Not all proxies will use HTTP_X_FORWARDED_FOR but if you're using it yourself for a reverse proxy to your webserver then you will.

I think you're making this personal because this is your code change and it's me reporting it. Ridiculous you're defending non-working code because you didn't like how it worked. This bug isn't about IP forging. You're the first guy to mention that's possible not me.

[quote]X-Forwarded-For may be set by an internal proxy[/quote]

And if it is then your code doesn't work. That's the problem and that's the bug. If I have it set by my own proxy server it's not properly logged because the logic is wrong.

Do you get it now?

#12 Updated by Jesse Labrocca over 3 years ago

nginx doesn't use HTTP_X_FORWARDED_FOR so then remote_addr will be logged
Squid and mod_proxy (apache reverse proxy) do use HTTP_X_FORWARDED_FOR but with your code the wrong IP is logged.

Take a step back and examine the code and what I'm saying in this bug report. I think you completely misunderstand what's going on.

When in your view should HTTP_X_FORWARDED_FOR be logged? Because as code stands it's never logged even though the code is there to do it.

#13 Updated by Jesse Labrocca over 3 years ago

I hate I can't edit my posts in this.

I'm reading your post 20 times Zinga. It appears you knowingly altered the code thinking it's better. I think you believe that all reverse proxies are internal load balancers but that's not always the case. How this code becomes a security issue I'll never know. I guess someone could creatively forge HTTP_X_FORWARDED_FOR so their real IP won't be logged. To me that's not a real security issue.

Here is situation that this came about.

Cloudflare is offering a free service to do reverse proxy. It includes a variety of kick ass features including load balancing and caching. Go check it out. Pretty neat stuff. However all IPs since using the service were Cloudflare remote_addr. They do send real IP via HTTP_X_FORWARDED_FOR. I was going to either make a plugin or core file edit to make sure HTTP_X_FORWARDED_FOR is logged but after reviewing get_ip() I found this bug because the code is there to do this already but the logic is wrong.

Essentially it sounds like you've removed a feature purposely and hid it as a simple code it. Using other reverse proxy for your site such as squid or mod_proxy will result in the same problems I'm having. And not all load balancers and proxies are going to be internal IPs. As a matter of fact it's likely they won't be if you're geo targetting traffic.

Can you explain how this is a security issue? Yes it opens a doorway for someone to send forged HTTP_X_FORWARDED_FOR and maybe even bypass a ban. Maybe the answer is a setting like "Enable Reverse Proxy" which will then do the HTTP_X_FORWARDED_FOR check.

#14 Updated by Ryan Gordon over 3 years ago

Zinga's correct in saying that forging the actual IP is essentially impossible. X_FORWARDED_FOR is just an attribute but if you don't have the right IP assigned by the router, assigned from the ISP then your PC internet connection isn't going anywhere.

Making the X_FORWARDED_FOR hold precedence over REMOTE_ADDR will make IP logging essentially redundant because X_FORWARDED_FOR could be any arbitrary string.

Jesse Labrocca wrote:

So what's the purpose of the get_ip function if you're not going to even consider the code that uses HTTP_X_FORWARDED_FOR. Why not just completely remove it since it doesn't work?

That is a valid point but separate from this bug report. If that's what you want to report then feel free to report it and we'll fix that instead.

Jesse, what a realistic solution would be is:
Check the REMOTE_ADDR against a list of known proxies. If it's a known, good, proxy then you know to use the X_FORWARDED_FOR instead. Unfortunately this is not something that MyBB can realistically implement. You'll have to write a plugin or make core code modifications if you want this change implemented.

#15 Updated by Jesse Labrocca over 3 years ago

Okay Ryan. Fix it how you want. I've done my job at reporting buggy code.

I'm done.

#16 Updated by zinga burga over 3 years ago

Jesse Labrocca wrote:

So what's the purpose of the get_ip function if you're not going to even consider the code that uses HTTP_X_FORWARDED_FOR. Why not just completely remove it since it doesn't work?

It does work. It just doesn't work the way you like it to.

It used to work as you say but you changed it and now it doesn't. Not all proxies will use HTTP_X_FORWARDED_FOR but if you're using it yourself for a reverse proxy to your webserver then you will.

I use an internal proxy myself. I'm quite aware of how they work, thank you.

I think you're making this personal because this is your code change and it's me reporting it. Ridiculous you're defending non-working code because you didn't like how it worked. This bug isn't about IP forging. You're the first guy to mention that's possible not me.

And I think you're making false claims based on your assumptions and perhaps your own misguided beliefs.
Yes, I do defend my edits. I think that's only natural. I do admit I didn't consider internal proxies when I made the edit, but I can't reverse it now. I've made my suggestion WHICH FIXES YOUR PROBLEM, so I really don't see why you're complaining so much.
If you ask me, you're the one defending your changes, which I've stated over and over again, open a security hole. Again, I don't mind you defending it, as long as you're reasonable about it. I've mentioned the problems in your suggestion, but you've failed to mention any problems in mine. Thus, you're just attacking me for the sake of yourself.

For your later point, I think a setting is a reasonable option, although if MyBB checks for internal IPs, I think only like a very small number of people need it.

And if it is then your code doesn't work. That's the problem and that's the bug. If I have it set by my own proxy server it's not properly logged because the logic is wrong.

What do they call a counter argument against the wrong point? Was it a straw-man again?

As said, if you have some elaborate proxy scheme, there's no way it's going to be automatically handled by a server side script. Again, X-Forwarded-For can be set by:
- internal server proxy
- client side proxy
- someone trying to forge their IP
For the second point, there have been reports of people claiming they've got people coming in with an IP of "127.0.0.1", which is a a reason for the change made. There is no guarantee that the header is set by an internal server whatsoever.
I think if you're setting up some elaborate proxy scheme, it's really your responsibility to get the script working with it.

#17 Updated by Jesse Labrocca over 3 years ago

You're arguing a feature change. I'm arguing a bug. You're no longer on dev team so it's no longer your decision how features work. Fact is that as written the code does not work as it's expected and the logic is wrong.

I'll expect MyBB to deal with this reported bug in it's code base.

#18 Updated by Andreas Klauer over 3 years ago

I wouldn't like X-Forwarded-For since anyone can spoof it. Yes you can also hide your IP with a proxy but at least you need a proxy - with X-Forwarded-For all you need is a firefox plugin. Features like the ip banlist would also become useless against unwanted bots and spammers. So I'm against making that change.

Solution 1, store all IPs instead of deciding for one, since it's not possible to determine the correct one. (big brother)

Solution 2, add a proxy trust system in addition to the ban list system, and then pick the first best ip.

/**
 * Fetch the IP address of the current user.
 *
 * @return string The IP address.
 */
function get_ip()
{
    $consider = implode(' ', array($_SERVER['REMOTE_ADDR'], // client
                                   $_SERVER['HTTP_X_FORWARDED_FOR'], // squid
                                   $_SERVER['HTTP_X_REAL_IP'], // nginx
                                   $_SERVER['HTTP_X_CLIENT_IP'], // lolwut
                                   $_SERVER['HTTP_CLIENT_IP'], // mystery header
                                   // add more crap here
                            ));

    // grab ip addresses
    preg_match_all("#\\b(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\b#",
                   $consider, $addresses);

    $ip = '';

    // pick first address that isn't complete bullshit
    foreach($addresses[0] as $key => $val)
    {
        // Set the new best IP.
        $ip = $val;

        if(is_banned_ip($ip)) // optional, also breaks the installer
        {
            // fuck
            break;
        }

        // Check for server or local or private IP.
        if($ip == $_SERVER['SERVER_ADDR']
           || is_trusted_proxy($ip) // not implemented, copy the banlist feature
           || preg_match("#^(10|127|172\\.(1[6-9]|2[0-9]|3[0-1])|192.168)\\.#", $ip))
        {
            // See if we can find something better in the next loop.
            continue;
        }

        break;
    }

    return $ip;
}

Solution 3, outsource all of this to a plugin.

/**
 * Fetch the IP address of the current user.
 *
 * @return string The IP address.
 */
function get_ip()
{
    global $plugins;

    $ip = '127.0.0.1';

    if(isset($_SERVER['REMOTE_ADDR']))
    {
        $ip = $_SERVER['REMOTE_ADDR'];
    }

    // Outsource proxy crap. :)
    $plugins->run_hooks_by_ref("get_ip", $ip);

    return $ip;
}

Plugins can already do 3) even without the hook, but this way it would look more officially intended...

#19 Updated by Tom Moore over 3 years ago

It's obvious that there are many different solutions to this issue - some with many levels of protection against others. I'm much in favour of leaving the main function as is, and including a plugin hook for authors to easily create their own level of protection (while leaving out risks to 'default' behaviour).

#20 Updated by Jesse Labrocca over 3 years ago

I personally would prefer a plugin hook. Then there is a solution for those who run reverse proxy or other convoluted situations. But leaving the code as-is should not be an option since the logic is wrong and most of the function doesn't even work. So redundant code should imho be removed.

#21 Updated by zinga burga over 3 years ago

Jesse Labrocca wrote:

You're arguing a feature change. I'm arguing a bug.

Yes, you're ranting on about a problem, whereas I'm suggesting a solution, whilst you're simply being absurd and ignoring the suggested solution.
My guess is that it's because you feel butthurt that your own suggestion is full of problems.

You're no longer on dev team so it's no longer your decision how features work.

And thanks for providing proof of how much butthurt you are. I never said what the decision would be, I only put forward my suggestion, which, so far, no-one (including you) has shown any problem with. On the other hand, 3 people (all of whom are more knowledgeable than you) have shown problems with your suggestion, and you blatantly refuse to acknowledge them.

But in either case, if you want to go down the this silly off-topic and irrelevant path of yours, you have never been on the dev team, so you certainly are in no more position to make decisions here than I am. I am glad you have never been on the dev team - with an attitude like yours, refusing to accept that you could be wrong, is certainly not a trait any real developer has. And you certainly are very wrong in many cases - I'm sure I don't need to point this out to you. I don't expect everyone to be very knowledgeable, however, I get tired of people like you who BS a lot.
If anything, me having actually been on the dev team gives me far greater insight into the ideas behind MyBB than you.

Fact is that as written the code does not work as it's expected and the logic is wrong.

No, fact is, the code works exactly how I intended it to work, and that's because the logic is correct.
If you had some other expectation in mind, than I had, then I apologise for not being able to read your mind and meet them.

Andreas Klauer wrote:

Plugins can already do 3) even without the hook, but this way it would look more officially intended...

A comment would be suffice then. No need for any additional hook as you've mentioned

<?php
function ipcrap_info() {
 return array(
  'name' => 'poo'
 );
}
$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_REAL_IP']; //or whatever is appropriate

#22 Updated by Tom Moore over 3 years ago

Seriously - you have to do this here? It's a development tracker, not a message forum to argue in.

/**
 * Fetch the IP address of the current user.
 *
 * @return string The IP address.
 */
function get_ip()
{
    global $plugins;

    $ip = 0;
    if(isset($_SERVER['REMOTE_ADDR']))
    {
        $ip = $_SERVER['REMOTE_ADDR'];
    }

    $plugins->run_hooks("get_ip");

    return $ip;
}

Ultimately, this function may be in use where checking the IP of the users might not be always accessible from another plugin hook (we're looking at places like this internally) while preserving what MyBB did before. By providing one here, it does open it up to a multitude of options for plugin authors - either checking external SPAM databases such as StopForumSpam much easier or your own IP checking facilities.

Or we could continue to argue the case for HTTP_CLIENT_IP vs. HTTP_X_FORWARDED_FOR vs. REMOTE_ADDR...

#23 Updated by zinga burga over 3 years ago

Tom Moore wrote:

Seriously - you have to do this here? It's a development tracker, not a message forum to argue in.

I'm sure that development needs a bit of a debate. After all, there's not always a magic solution is there?
But anyway, I provide a solution, and all Jesse Labrocca does is pointlessly protest against me. He should take his antics elsewhere.

Ultimately, this function may be in use where checking the IP of the users might not be always accessible from another plugin hook (we're looking at places like this internally) while preserving what MyBB did before. By providing one here, it does open it up to a multitude of options for plugin authors - either checking external SPAM databases such as StopForumSpam much easier or your own IP checking facilities.

Hook is unnecessary IMO. All that can be done without it, as I shown above. No need to unnecessarily invoke more load on the server.
(or, please provide more hooks throughout the whole script, it would be much more appreciated that way =P)

Or Jesse Labrocca could continue to argue the case for HTTP_CLIENT_IP vs. HTTP_X_FORWARDED_FOR vs. REMOTE_ADDR...

^ Corrected.

#24 Updated by Andreas Klauer over 3 years ago

Plugins can already do 3) even without the hook, but this way it would look more officially intended...

Messing with $_SERVER directly is a hack, the hook would be the clean, and thus preferred solution. That's why I mentioned it in the first place.

Hook is unnecessary IMO.

If it leads to a plugin that handles proxies in great detail (with customizable settings, list of trusted proxies and all), something that would never make it into core, I'm all for it. The function is only called once per request, so no much harm done in terms of server load. Only have to be careful about the installer (not sure if $plugins is available in the installer - $cache turned out to be unavailable, or at least banned_ip() which uses the cache caused an error when called from get_ip() within the installer).

The real question is:

Would anyone actually write such a plugin? :)

Sounds like something that could be done in a day (code is pretty much above already and the trust list can be done by copying the ip banlist feature), even so I'm not sure if I would actually care enough.

#25 Updated by zinga burga over 3 years ago

Andreas Klauer wrote:

Messing with $_SERVER directly is a hack, the hook would be the clean, and thus preferred solution. That's why I mentioned it in the first place.

There are millions of things which could be considered a "hack" (of course, depending on the person evaluating it), but I'd hardly call looping through a usually empty array per function call any cleaner.
My posted code is a very simple one liner that does the job, and has no potential consequences. It'll also have the side effect of patching any "bad" plugins which try to read REMOTE_ADDR rather than use the function.

$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_REAL_IP'];

vs (using Andreas' function, Tom's is broken)
$plugins->add_hook('get_ip', 'whoopy_ip');
function whoopy_ip(&$ip)
{
  $ip = $_SERVER['HTTP_X_REAL_IP'];
}

How is the former worse off? Some personal religious belief?

The function is only called once per request, so no much harm done in terms of server load.

I agree, but many hooks I suggested got rejected because of "too much load" (lolwot). As you said, this would rarely be used, and there are many many more places which would be MUCH more useful (DB query for example) to have hooks. Yes, it's possible to hook into the DB without hooks, but if you consider overwriting $_SERVER a hack, overwriting the DB class would simply be an abomination in your book.

Only have to be careful about the installer (not sure if $plugins is available in the installer - $cache turned out to be unavailable, or at least banned_ip() which uses the cache caused an error when called from get_ip() within the installer).

Which is why it's good to stick to something that already works (ie current code).

#26 Updated by Jesse Labrocca over 3 years ago

I'm past this argument. I'll make changes on my own. But I found a bug and this is a LEGITIMATE BUG. How you fix it is your call.

Found this well working function. Very short and sweet but could be a base for working code. Works on a reverse proxy.

function real_ip(){
if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])){
$_SERVER['REMOTE_ADDR']=$_SERVER['HTTP_X_FORWARDED_FOR'];
}elseif(isset($_SERVER['HTTP_X_REAL_IP'])){
$_SERVER['REMOTE_ADDR']=$_SERVER['HTTP_X_REAL_IP'];
}
return $_SERVER['REMOTE_ADDR'];
}

#27 Updated by Andreas Klauer over 3 years ago

zinga burga wrote:

It'll also have the side effect of patching any "bad" plugins which try to read REMOTE_ADDR rather than use the function.
How is the former worse off?

What you call "bad" plugins could be intentional. For example a plugin that wants to log both proxy and client IPs. With the original REMOTE_ADDR lost that is not possible anymore. That's why I said it's a hack. Yes, purely academic and perfectionist case.

Some personal religious belief?

I'm not religious about it. I use hacks myself. Funny you should mention that DB example because Google SEO does something very similar. Not to mention the core edits. However, that doesn't mean I'd say no if someone came along and offered a better alternative. Unfortunately with MyBB, better alternatives are few and far between.

#28 Updated by zinga burga over 3 years ago

Andreas Klauer wrote:

What you call "bad" plugins could be intentional. For example a plugin that wants to log both proxy and client IPs. With the original REMOTE_ADDR lost that is not possible anymore. That's why I said it's a hack. Yes, purely academic and perfectionist case.

I would usually say to change the priorities, but unfortunately these can't be modified at plugin load.

Either case, this is a rather weird situation (to have a plugin that overwrites an address and another one which logs them). For one, it would probably make a lot more sense to rely on webserver logs than use a script level plugin. But if that wasn't the case, one would imagine that this would probably be more logical to combine into a single script, rather than have separate plugins. I can't see a big demand for this, and to handle the proxy situation, it's probably going to need to be hand crafted anyway.
And I still wouldn't call it a hack. You're essentially saying that the internal proxy isn't the client - the client is actually someone behind the proxy. That's what you want, and exactly what you say.
Personally, mine is much simpler, and looks cleaner to me.
Also:
- it requires no modification in MyBB (in line with what appears to be the policy for MyBB development)
- I'd still use my solution regardless of whether the hook is added or not, because it works on older versions of MyBB, runs faster and has fewer lines of code (and personally much easier to understand)

Also, if you're on an internal proxy, then logging the client IP is probably useless, unless you have multiple proxies.

#29 Updated by Ryan Gordon over 3 years ago

Jesse Labrocca wrote:

I'm past this argument. I'll make changes on my own. But I found a bug and this is a LEGITIMATE BUG. How you fix it is your call.

Found this well working function. Very short and sweet but could be a base for working code. Works on a reverse proxy.

function real_ip(){
if(isset($_SERVER['HTTP_X_FORWARDED_FOR'])){
$_SERVER['REMOTE_ADDR']=$_SERVER['HTTP_X_FORWARDED_FOR'];
}elseif(isset($_SERVER['HTTP_X_REAL_IP'])){
$_SERVER['REMOTE_ADDR']=$_SERVER['HTTP_X_REAL_IP'];
}
return $_SERVER['REMOTE_ADDR'];
}

So what's going to happen when everyone at HF finds out that they can spoof their IP address by simply setting the HTTP_X_FORWARDED_FOR attribute to something fake?

#30 Updated by Tom Moore over 3 years ago

  • Status changed from New to Assigned
  • Assignee set to Tom Moore
  • Target version set to 1.6.2

#31 Updated by Tom Moore over 3 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 0 to 100

Applied in changeset r5356.

#32 Updated by Tom Moore over 3 years ago

Applied in changeset r5357.

#33 Updated by Jesse Labrocca over 3 years ago

Thank you for this. Very comprehensive solution imho. I'm impressed by the teams willingness to look for long-term solutions that attempt to fill multiple situations.

#34 Updated by Stefan T. over 3 years ago

  • Status changed from Resolved to Closed
  • SQA assignments set to Stefan T.

#35 Updated by Andreas Klauer over 3 years ago

r5256 gives me an error message when installing on localhost.

Finish Setup

Importing user groups...done

Creating Administrator account...
Warning [2] Invalid argument supplied for foreach() - Line: 2891 - File: inc/functions.php

I'm not sure why this code is even executed, $mybb->settings['ip_forwarded_check'] should be 0?

#36 Updated by Stefan T. over 3 years ago

  • Status changed from Closed to Feedback

#37 Updated by zinga burga over 3 years ago

Andreas Klauer wrote:

r5256 gives me an error message when installing on localhost.

[...]

I'm not sure why this code is even executed, $mybb->settings['ip_forwarded_check'] should be 0?

...which is one of the reasons why I suggested not changing anything.
Just introducing more squiggly code and bugs and issues and extra maintenance work.
Much better to put the effort into truly broken features with no simple solution like this one.
(besides, there are various other problems with the change too)

Then again, no-one ever listened to my suggestions (until people realised the merits of them like 2 years down the track).
Oh well, perhaps I really shouldn't bother...

#38 Updated by Tom Moore over 3 years ago

You suggested not changing anything because you've foreseen a strange issue when installing? To be honest, it's probably an easy 2 minute fix to resolve the issue Andreas mentions off the top of my head, it's not as though we need to change another system - whereas it brings added benefits to the get_ip function.

You know more than anyone about my willingness to fix those long standing issues and provide a better 'API' for plugin authors, and we've come up with a way to manage these properly and to get more people involved in the development of MyBB. Just like any project, there are hoops to jump through - and these are much easier when pessimism is excluded.

#39 Updated by Tom Moore over 3 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r5377.

#40 Updated by zinga burga over 3 years ago

Tom Moore wrote:

You suggested not changing anything because you've foreseen a strange issue when installing?

Of course not. I did foresee that something would get broken however.

To be honest, it's probably an easy 2 minute fix to resolve the issue Andreas mentions off the top of my head, it's not as though we need to change another system - whereas it brings added benefits to the get_ip function.

Of course, assuming that no further issues are introduced.
But anyway, I can probably point out a bunch of other things which are easier 2 (or maybe 1) minute fixes which have remain unfixed for quite a while.

You know more than anyone about my willingness to fix those long standing issues

That's great however I implore you to understand the issue of priority.
Of course, you're not required to work in any particular order, and if this just happens to take your fancy, so be it, however, any objective assessment of the issue (number of users that actually use it, number of idiots which could set the wrong setting, and number of cases you've actually covered etc etc), considering other active bugs would probably suggest that this particular issue should be very low on the priority list for multiple reasons mentioned in the above walls of text.

and provide a better 'API' for plugin authors

I think this point can be debated, but let's not go there.

and we've come up with a way to manage these properly

Hardly, you haven't considered nginx's (used on quite a number of server setups I must say) default forwarding method in this magical setting you just added, for example.
...c'mon, if you've followed the discussion, we've been through this...

and to get more people involved in the development of MyBB.

???

Just like any project, there are hoops to jump through - and these are much easier when pessimism is excluded.

But a healthy degree of pessimism should always be exercised so that you don't fall over your own optimism (ie not needlessly jumping through hoops you don't have to). Do you agree?

Well, I don't know why I bother. You're just volunteering, and I don't want to over criticise. I hope you at least understand my reasoning, if nothing else, and perhaps incorporate it to benefit yourself or whoever you're trying to help.
If you think I'm just a raving lunatic and should GTFO, well, that's what I'll be doing right now.

Thanks for the fix.

#41 Updated by Tom Moore over 3 years ago

  • Target version changed from 1.6.2 to 1.6.3

You know we can continue this all day. I'm not saying GTFO - Dylan and I carefully considered the impact of the changes after your walls of text and came up with this solution and we know we're not going to satisfy everyone with it.

If there are any of those truly important issues that can be fixed in 2 minutes lying around, then feel free to point them out - and rather than wait for us to figure out them out, they'll get fixed quicker if there's a suitable fixed attached to them too. Most of the issues are fixed randomly - at least, they are by me, but I do check important issues first. If I can fix them in the time I have available, then they'll be fixed.

I do understand the issue of priority, hence why 1.6.2 is imminent. The development changes I mentioned are to truly open up the process, and quite possibly the biggest shift since the whole development became public. You have the best idea of the changes, as you're the one who suggested them to me. Complete information will be available soon.

#42 Updated by Tom Moore over 3 years ago

  • Target version changed from 1.6.3 to 1.6.4

#43 Updated by Stefan T. about 3 years ago

  • Status changed from Resolved to Feedback

After activating, I get this error:

Warning [2] Invalid argument supplied for foreach() - Line: 2893 - File:.../inc/functions.php

#44 Updated by Tom Moore about 3 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r5468.

#45 Updated by Stefan T. about 3 years ago

  • Status changed from Resolved to Feedback

I think, that doesn't fix the issue because $addresses[ 0 ] is still not defined. What about using is_array($addresses[ 0 ])?

#46 Updated by Tom Moore about 3 years ago

Ack, I meant to put the [0] there. What confused me was you said 'activating'. Activating what, exactly? It works fine for me everywhere?

#47 Updated by Stefan T. about 3 years ago

I've got this error after enabling the setting. It is obvious that it doesn't work because I don't use a reverse proxy. But there shouldn't be any error message.

#48 Updated by Tom Moore about 3 years ago

I see. I'll implement the is_array instead.

#49 Updated by Tom Moore about 3 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r5471.

#50 Updated by Stefan T. about 3 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF