Jump to content

Please review my code


WWW_9hub_Net

Recommended Posts

Could someone please look at the code below and leave your feedback.

 

// grab the connecting IP address. //
$connecting_ip = get_ipaddress();
if (empty($connecting_ip)) {
    echo "Could not determine IP address.";
    exit;

}

// Determine if connecting IP address is allowed to connect to timesheet


    if ($restrict_ips == "yes") {
        $size = count($allowed_networks);
for ($x = 0; $x < $size; $x++) {
            $is_allowed = ip_range($allowed_networks[$x],

$connecting_ip);
            if ($is_allowed) {
                break;
            }
        }


if (! $is_allowed) {
        echo "You are not authorized to view this page.";
        exit;

        }

        unset($x);
        unset($size);
        unset($is_allowed);
    }

function get_ipaddress() {
    if (! empty($_SERVER['REMOTE_ADDR'])) {
        $direct_ip  = $_SERVER['REMOTE_ADDR'];
    } else {
        $direct_ip  = '';
    }

    if (! empty($_SERVER['HTTP_X_FORWARDED'])) {
        $proxy_ip = $_SERVER['HTTP_X_FORWARDED'];
    } elseif (! empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
        $proxy_ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
    } elseif (! empty($_SERVER['HTTP_FORWARDED'])) {
        $proxy_ip = $_SERVER['HTTP_FORWARDED'];
    } elseif (! empty($_SERVER['HTTP_X_FORWARDED'])) {
        $proxy_ip = $_SERVER['HTTP_X_FORWARDED'];
    } else {
        $proxy_ip  = '';
    }



// Returns the true IP if it has been found, else false //
    if (! empty($proxy_ip)) {
        $is_ip = preg_match('/'.VALID_IP_ADDRESS_REGEX.'/', $proxy_ip, $regs);
        if ($is_ip and (count($regs) > 0)) {



// True IP behind a proxy
            return $regs[0];
        } else {
           

// Can't define IP: there is a proxy but we don't have information about the true IP
            return false;
        }
    }


// True IP without proxy
    return $direct_ip;
}

 

I had problem with the script when there is proxy server in place.

 

Thanks in advance

 

 

 

 

Link to comment
https://forums.phpfreaks.com/topic/188764-please-review-my-code/
Share on other sites

I'd recommend looking at a Indent style and stick to it, and place your code within

PHP

tags.

 

As for your code, It looks fairly excessive regarding extracting the IP from X-FORWARDED*, but it looks functional. You should define your functions before they are used, you are calling it before it is ever defined (get_ipaddress())

Thanks for the suggestion. I will make a note of it.

 

Is there any other simpler way to handle same situation? Extracting ip address when there is a proxy.

 

What sort of proxy are you wanting to get? Most (such as gov't/company/squid) proxy types may release an X-FORWARDED-FOR header, Nearly no web proxy in existance does that, and the *-TO headers won't be set as it isn't within it's own IP location table.

 

    $ipString=@getenv("HTTP_X_FORWARDED_FOR");

    $addr = explode(",",$ipString);

    return $addr[sizeof($addr)-1];

 

Should be sufficient ennough.

As for your code, It looks fairly excessive regarding extracting the IP from X-FORWARDED*, but it looks functional. You should define your functions before they are used, you are calling it before it is ever defined (get_ipaddress())

 

I'm defining get_ipaddress in the functions.php and connecting_ip in header.php. You mean I have to define 'VALID_IP_ADDRESS_REGEX' ?

 

Yeah I did forget to define it in functions.php

// Input validating regular expressions

define('VALID_IP_ADDRESS_REGEX', '^((?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:[.](?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3})|(\s*((([0-9A-Fa-f]{1,4}{7}
(([0-9A-Fa-f]{1,4})|)|(([0-9A-Fa-f]{1,4}{6}
(:|((25[0-5]|2[0-4]\d|[01]?\d{1,2})(\.(25[0-5]|2[0-4]\d|
[01]?\d{1,2})){3})|(:[0-9A-Fa-f]{1,4})))|
(([0-9A-Fa-f]{1,4}{5}(((25[0-5]|2[0-4]\d|[01]?\d{1,2})
(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|
((:[0-9A-Fa-f]{1,4}){1,2})))|(([0-9A-Fa-f]{1,4}{4}
(:[0-9A-Fa-f]{1,4}){0,1}(((25[0-5]|2[0-4]\d|[01]?\d{1,2})
(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|
((:[0-9A-Fa-f]{1,4}){1,2})))|(([0-9A-Fa-f]{1,4}{3}
(:[0-9A-Fa-f]{1,4}){0,2}(((25[0-5]|2[0-4]\d|[01]?\d{1,2})
(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|((:[0-9A-Fa-f]{1,4}){1,2})))|
(([0-9A-Fa-f]{1,4}{2}(:[0-9A-Fa-f]{1,4}){0,3}(((25[0-5]|2[0-4]\d|
[01]?\d{1,2})(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|
((:[0-9A-Fa-f]{1,4}){1,2})))|(([0-9A-Fa-f]{1,4}(:[0-9A-Fa-f]{1,4}){0,4}
(((25[0-5]|2[0-4]\d|[01]?\d{1,2})(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|
((:[0-9A-Fa-f]{1,4}){1,2})))|(:[0-9A-Fa-f]{1,4}){0,5}(((25[0-5]|2[0-4]\d|
[01]?\d{1,2})(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})?)|
((:[0-9A-Fa-f]{1,4}){1,2})))|(((25[0-5]|2[0-4]\d|[01]?\d{1,2})
(\.(25[0-5]|2[0-4]\d|[01]?\d{1,2})){3})))(%.+)?\s*)$');

 

 

 

I was aware of that, I just meant in the original code

$connecting_ip = get_ipaddress();

Came before:

function get_ipaddress() {

    if (! empty($_SERVER['REMOTE_ADDR'])) {

        $direct_ip  = $_SERVER['REMOTE_ADDR'];

    } else {

        $direct_ip  = '';

    }

 

Was the original code just combined or have you not spotted that mistake?

 

Thats one of the strange things with PHP. You can refer to a function if its declared after its called..

For example.

woot('Buddski');

function woot($str) {
echo $str.' got Wooted..<br/>';
}

will return "Buddski got Wooted..";

Makes for some really confusing code..

 

Thats one of the strange things with PHP. You can refer to a function if its declared after its called..

For example.

 

You know, I don't think I knew that, that's what you get when you come from a VB.NET background.. Assumed as variables couldn't be called before, functions couldn't either.

Archived

This topic is now archived and is closed to further replies.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.