Infinite loop on login redirect

Question

I have a login script which generally works for me, but occasionally goes into an infinite loop on redirecting after checking the cookie stored for login. The browser will report something like the following: "Firefox has detected that the server is redirecting the request for this address in a way that will never complete." Others have reported this issue too. Below are the key elements of the login process. I wonder if someone can see what the issue is with this process/script.

Thanks,

Nick

First at the top of each page that is protected:

<?php
session_start();

$_SESSION['url'] = $_SERVER['REQUEST_URI'];

require('login/config.php');
require('login/functions.php'); 

if (allow_access(Users) != "yes")
{ 
include ('login/check_login.php'); 
exit;
}
?>

Then in check_login.php:

<?
session_start();

//check to see if the user already has an open session
if (($_SESSION[user_name] != "") && ($_SESSION[password] != ""))
{

    header("Location:$_SESSION[redirect]");
    exit;
}

$lr_user = $_COOKIE['lr_user'];
$lr_pass = $_COOKIE['lr_pass'];

//check to see if cookies have been set previously
if(($lr_user != "") && ($lr_pass != ""))
{
    header("Location:/login/redirect.php");
    exit;
}

//if neither is true, redirect to login
    header("Location:/login/login.php");

?>

Then, in redirect.php:

<?
session_start();

//require the functions file
require ("config.php");
require ("functions.php");

$lr_user = $_COOKIE['lr_user'];
$lr_pass = $_COOKIE['lr_pass'];

//check to see if cookies are already set, remember me
if ((!$lr_user) || (!$lr_pass))
{

$username = $_POST[username];
$password = $_POST[password];

}else{

$username = $lr_user;
$password = $lr_pass;

}

//sets cookies to remember this computer if the user asks to
if ($_POST[remember] == "Yes")
{
setcookie("lr_user", $username, $duration, "/", $domain);
setcookie("lr_pass", $password, $duration, "/", $domain);
}

//sets session variables
sess_vars($base_dir, $server, $dbusername, $dbpassword, $db_name, $table_name, $username, $password);

if(isset($_SESSION['url']))
  $_SESSION[redirect] = $_SESSION['url']; // holds url for last page visited.
else 
   $_SESSION[redirect] = "/index.php"; // default page for

//redirects the user    
header("Location:$_SESSION[redirect]");
?>

functions.php

<?php

//function to get the date
function last_login()
{
    $date = gmdate("Y-m-d");
    return $date;
}

//function that sets the session variable
function sess_vars($base_dir, $server, $dbusername, $dbpassword, $db_name, $table_name, $username, $password)
{

    //make connection to dbase
    $connection = @mysql_connect($server, $dbusername, $dbpassword)
                or die(mysql_error());

    $db = @mysql_select_db($db_name,$connection)
                or die(mysql_error());

    $sql = "SELECT * FROM $table_name WHERE username = '$username' and password = password('$password')";

    $result = @mysql_query($sql, $connection) or die(mysql_error());


    //get the number of rows in the result set
    $num = mysql_num_rows($result);

    //set session variables if there is a match
    if ($num != 0) 
    {
        while ($sql = mysql_fetch_object($result)) 
        {
            $_SESSION[first_name]   = $sql -> firstname;
            $_SESSION[last_name]    = $sql -> lastname; 
            $_SESSION[user_name]    = $sql -> username;       
            $_SESSION[password]     = $sql -> password;
            $_SESSION[group1]       = $sql -> group1;
            $_SESSION[group2]       = $sql -> group2;
            $_SESSION[group3]       = $sql -> group3;
            $_SESSION[pchange]      = $sql -> pchange;  
            $_SESSION[email]        = $sql -> email;
            $_SESSION[redirect]     = $sql -> redirect;
            $_SESSION[verified]     = $sql -> verified;
            $_SESSION[last_login]   = $sql -> last_login;
        }
    }else{
        $_SESSION[redirect] = "$base_dir/errorlogin.php";
    }
}


//functions that will determine if access is allowed
function allow_access($group)
{
    if ($_SESSION[group1] == "$group" || $_SESSION[group2] == "$group" || $_SESSION[group3] == "$group" ||
        $_SESSION[group1] == "Administrators" || $_SESSION[group2] == "Administrators" || $_SESSION[group3] == "Administrators" ||
        $_SESSION[user_name] == "$group")
        {
            $allowed = "yes";
        }else{
            $allowed = "no";
        }
    return $allowed;
}

//function to check the length of the requested password
function password_check($min_pass, $max_pass, $pass)
{

    $valid = "yes";
    if ($min_pass > strlen($pass) || $max_pass < strlen($pass))
    {
        $valid = "no";
    }

    return $valid;
}

?>

config.php

<?

//set up the names of the database and table
$db_name ="";
$table_name ="authorize";

//connect to the server and select the database
$server = "localhost";
$dbusername = "";
$dbpassword = "*";

//domain information
$domain = "";

//Change to "0" to turn off the login log
$log_login = "1";

//base_dir is the location of the files, ie http://www.yourdomain/login
$base_dir = "";

//length of time the cookie is good for - 7 is the days and 24 is the hours
//if you would like the time to be short, say 1 hour, change to 60*60*1
$duration = time()+60*60*24*365*10;

//the site administrator\'s email address
$adminemail = "";

//sets the time to EST
$zone=3600*00;

//do you want the verify the new user through email if the user registers themselves?
//yes = "0" :  no = "1"
$verify = "0";

//default redirect, this is the URL that all self-registered users will be redirected to
$default_url = "";

//minimum and maximum password lengths
$min_pass = 8;
$max_pass = 15;


$num_groups = 0+2;
$group_array = array("Users","Administrators");

?>

Answer 1

If you really want to go with this code, I think here is the answer:

For users who have disabled cookies (private browsing), all of your users will stuck because of the lines in check_login.php. Because these values will be never set and will fall into a loop of check_login.php -> login.php -> check_login.php:

$lr_user = $_COOKIE['lr_user']; 
$lr_pass = $_COOKIE['lr_pass']; 

//check to see if cookies have been set previously 
if(($lr_user != "") && ($lr_pass != "")) 
{     
header("Location:/login/redirect.php");     
exit; 
}

//if neither is true, redirect to login        
header("Location:/login/login.php"); 

Bur I really recommend you to go with some readily available session management snippets.

The problems I see with a quick look that:

a) As Steve says, you MUST not save the username and password in cookies EVER. You only save a random session id value.

setcookie("lr_user", $username, $duration, "/", $domain); 
setcookie("lr_pass", $password, $duration, "/", $domain); 

b) Your functions.php is subject to SQL injection.

$sql = "SELECT * FROM $table_name WHERE username = '$username' and 
password = password('$password')";

c) User inputs are never escaped.

$username = $_POST[username];                 
$password = $_POST[password]; 

d) You splitted your session management checks all over the code.

Answer 2

EDIT - try this:

I think the problem is that I assume you are trying to protect 'all' pages which also include the index.php. However you have included the index.php as a page in the $_SESSION[redirect] variable. You should think along the lines of:

1. User trys to access a page
2. You should check if they are allowed to
3. If they are allowed to, just let them view the page without interruption
4. If they are not (i.e not logged in) - redirect them to your login page

Your script is trying to still redirect them even if they are allowed to view the page (which is causing the loop problem).

It is a subtle difference, but an important one (especially as you are protecting all pages).

I would try this:

At the top of your protected pages change the bottom snippet to:

if (allow_access(Users) != "yes")
{ 
    include ('login/check_login.php'); 
    check_redirect();
}

In your check_login.php try this:

<?
session_start();

function check_redirect()
{

    //check to see if the user already has an open session
    if (($_SESSION[user_name] != "") && ($_SESSION[password] != ""))
    {
       // just return if they are a valid user (no need to redirect them)
       return;
    }

    $lr_user = $_COOKIE['lr_user'];
    $lr_pass = $_COOKIE['lr_pass'];

    //check to see if cookies have been set previously
    if(($lr_user != "") && ($lr_pass != ""))
    {
       // just return if they are a valid user (no need to redirect them)
       return;
    }

    //if neither is true, redirect to login
    header("Location:/login/login.php");
    die();

}  
?>

Your redirect.php is not needed for soley protecting your pages, I assume you use this with your actual login.php script, therefore:

$_SESSION['url']

Would have stored the page they were trying to get to, and your redirect.php / login.php script should just use this to redirect them back there after successful login.

Lastly the above is untested code but should work better than what you had, let me know how you get on.

---

To be honest it is quite hard to determine exactly what is wrong with your code because there are still a few unknown variables, such as the config and functions files and the function:

if (allow_access(Users) != "yes")

Which I assume Users should be 'Users', likewise unless it was just loosely typed for this question all the variables which you have like $_SESSION[user_name] you MUST make sure you correctly add the apostrophes or you will get notices all over the place (undefined variable, assumed...etc etc) not too mention this could mess up your session data.

Perhaps if I offered some advice on your current code, you may be able to try some things which could fix your code.

Multiple redirects / Scripts

I would firstly rewrite your check_login.php and redirect.php scripts - the fact that you have two separate scripts (which could be combined) will always give you problems at some stage, because essentially you are redirecting to a redirect (which isn't logically when you say it out a loud). So firstly rewrite your scripts to one 'auth.php' script. And also simplify your include for pages that require authentication, for example:

<?php
    session_start();

    // use require_once for your login scripts, not 'include' as you want an error to 
    // occur to halt the page processing if the file is not found, include will just
    // give a warning but still continue with the page processing (which you're trying
    // to protect). Lastly '_once' so you don't get multiple inclusions of the same
    // script by accident (which is always import for login / redirect scripts). 
    require_once('login/auth.php');

    // just create one function which will proxy all the other functions / includes
    // you could exclude this function and just use the require_once file for direct 
    // access - but I would say including a function will make it easier to understand.
    check_login();
?>

Now the auth.php file:

<?php
    session_start();

    // if these are required use the _once. I would guess some of these 'functions'
    // may be able to be included within this 'auth.php' file directly?
    require_once('login/config.php');
    require_once('login/functions.php'); 

    // set your variables here
    $_SESSION['url'] = $_SERVER['REQUEST_URI'];

    // the main check login function
    function check_login() 
    {

        // Check if your user is logged in / needs to be logged in
        // perhaps something from your allow_access() function?

        // Do all the checks for session / cookie you should resolve the checks to
        // a simple bool variable (i.e. if the user is valid or not)
        $userIsValid = true || false; // get this from above code

        // Then use the redirect function and pass in the $userIsValid variable
        // which will tell the redirect() function where to redirect to.
        redirect($userIsValid);

    }

    // use a separate function for the redirect to keep it cleaner 
    // not too sure on all the Url's you have floating around in your code as I
    // would think you either want to let them to proceed to the page they were
    // trying to view (if validated) or you want them to login?      
    function redirect($validUser = false)
    {
        // if the user is valid, just return as you don't have to redirect them
        if ( $validUser ) {
            return true;
        }
        // otherwise just redirect them to the login page
        header("Location:/login/login.php");
        die();
    }

?>

Security

You don't need to (and shouldn't!) be storing the actual password in a session, and I would certainly advice against a cookie also. However if you must store the password / username in a cookie, at the very least you must encrypt it using md5() with a salt, etc. So in a nutshell rather than checking $_SESSION['user_name'] and $_SESSION['password'] it could just be something like:

// if the user has no valid session do something:
if ( !isset($_SESSION['id']) ) {


}

Separation of concerns

I'm not sure why your have:

$username = $_POST[username];
$password = $_POST[password];

Within your redirect.php file, are you also using this script for when users login? I don't think it's a good idea if you are (which may be the problem). You should have a separate script to handle all actual login functionality (including the redirect after logging in). The above should only be concerned with a) checking if the user is valid / logged in b) redirecting them if not - essentially protecting your web page(s).

Your Code:

//check to see if the user already has an open session
if (($_SESSION[user_name] != "") && ($_SESSION[password] != ""))
{

    header("Location:$_SESSION[redirect]");
    exit;
}

I'm not sure I get this bit in the context, as your basically redirecting them if they have a valid session? Scratch that I don't really understand the whole check_login.php script, as things are a bit backwards (especially when combined with your redirect.php script). You are checking the same variables again ($lr_user) || (!$lr_pass) in your redirect script and making reference to things that have not even been set in your check_login.php script.

EDIT: Could be solution? Unless I have over looked something the above code block makes reference to $_SESSION['redirect'], I think that should be either $_SESSION['url'] or just don't redirect them. The $_SESSION['redirect'] doesn't get set until the redirect.php script (which may not get called if the session exists).

Final thoughts:

Sorry if this doesn't really answer your question as you would have liked but I really think this would be a good opportunity to take a good look at your script(s) and clean / simplify them. Ideally you should look at this with an OOP approach, i.e. create a session, redirect, login class. But if sticking with plain functions (procedural) make sure your create a clean separation for each script. So in a nutshell:

• Don't repeat yourself, for example why have both $_SESSION['redirect'] and $_SESSION['url'] these are the same value?
• Don't redirect to a redirect (one script should handle this)
• Separate your concerns - Have a login script doing the login process and a authentication / acl script securing your actual pages (don't combine the two).

Hope the above makes sense, but let me know if not.