Login Form Code

PHP
1

Hi,

My below code is for logging a user in. Do you see any issues with it? How can it be improved or made more efficient?

Or is it correct? Thanks and appreciate any feedback!!!

Code at the top of php page…


<?php
session_start();
/***********************
 declare variables
***********************/
$username = $_POST['username'];
$password = $_POST['password'];
$submit = $_POST['submit'];

// declare an empty error array
$error_message = array();

// if form submitted
if(isset($submit))
{
 		 /*************************
		  Form Error Checking
		 *************************/
     if(!$username || $username == "")
     {
        $error_message['username'] = 'Please Enter Username';
     }
     if(!$password || $password == "")
     {
        $error_message['password'] = 'Please Enter Password';
     }
		 /*************************
		  End Form Checking
		 *************************/

    /*********************************
     If No Form Errors
    *********************************/
    if(count($error_message) == 0)
    {
        // include database connection
        include('includes/db_connect.php');
        
        // perform query
        $query = "SELECT * FROM ".
        			 	 "tnlname ".
        				 "WHERE username= '$username' ".
        				 "AND password= '$password'";
        $result = mysql_query($query) or die(mysql_error());
        $num_results = mysql_num_rows($result);

        if ($num_results > 0)
        {
         	 // if they are in the database register the user
        	 $HTTP_SESSION_VARS['valid_user'] = $username;		 
        }// end if

	} // end if count
}// end isset $submit
?>

And in the body…


<?php
if ((isset($submit)) && (count($error_message) == 0))
{
 	 // check if session variable registered
	 if (isset($HTTP_SESSION_VARS['valid_user']))
	 {
	    $HTTP_SESSION_VARS['valid_user'] = $username;
echo '<p>You are logged in as: '.$HTTP_SESSION_VARS['valid_user'].'</p>';
session_register('valid_user');
			

echo '<br /><ul>'.
'<li><a href="fileupload.php">Upload File</a></li> '.
'</ul>';
echo '<a href="index.php">Return to Login Page</a></p>';		
			}

		}
		else {
			echo '<p>Incorrect Username/Password<br />';
			echo '<a href="index.php">Go Back</a></p>'; 
		}
}
else
{
	session_destroy();

?>
<form action="index.php" name="frm_login" id="frm_login" method="post">

		<fieldset>
					<legend>Login Form - Members Area</legend>
					
					<p>
					<?php if(isset($error_message['username'])) { echo  $error_message['username']; }?>
					<label for="username">Username:</label>
					<input type="text" name="username" id="username" value="<?php if(isset($username)) { echo $username; } ?>" /></p>

					<p>
					<?php if(isset($error_message['password'])) { echo  $error_message['password']; }?>
					<label for="password">Password:</label>
					<input type="password" name="password" id="password" value="<?php if(isset($password)) { echo $password; } ?>" /></p>

					<p><input type="submit" name="submit" id="submit" class="btn" value="Login" /></p>
		</fieldset>

</form>
<?php
}// end else
?>

            </div>

Also in a subsequent page I am testing, on hitting the IE browser back button I get “Webpage has expired error”. Any ideas how to fix this?


// this code at top of page...
<?php
session_start();
?>


// this code in body...
<?php
		 // check if session variable registered
  	 if (isset($HTTP_SESSION_VARS['valid_user']))
  	 {
  	    $username=$HTTP_SESSION_VARS['valid_user'];
echo '<p>You are logged in as: '.$HTTP_SESSION_VARS['valid_user'].'</p>';
session_register('valid_user');
				
echo '<p>Welcome</p>';				
								
		 }
?>
2

mysql_real_escape_string does require a connection to the database, which is why it’s not a good idea to escape the data when you receive it. The mysql_real_escape_string part really should be delayed as long as possible, to where the database parts are done.

An example of this can be seen on the documentation page for mysql_real_escape_string.

3

I have no idea without running your code.

For debugging purposes, post the output from this:


 
echo 'Username before = '.$_POST['username'].'<br />Password before = '.$_POST['password'];
 
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
 
'Username after = '.$username.'<br />Password after = '.$password; die();
4

Using a framework will not get rid of these issues. Also, its best to know how to attend to these problems manually, rather than simply letting a 3rd party script handle it for you.

5

Why don’t you use cakePHP or other framework? It will help you get rid of these problems.

6

The sticky thread for coding tips was updated recently with info about handling input and output that should help a lot here.

7

Use mysql_real_escape_string() only when you are about to enter it into the database. Ideally you should use prepared statements, but this will suffice.

If you plan on outputting this data onto the webpage then I would suggest htmlspecialchars().

If you plan on using user inputted data for some shell argument then the following would also suffice: escapeshellarg()

8

Fields are now returned empty after I enter correct login. Why is this?


$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
9

Without trying it on your code, I don’t know.

What happens when you try it on your code?

10

So is this correct?


$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);

Does it prevent the SQL Injection attack below?


// We didn't check $_POST['password'], it could be anything the user wanted! For example:
$_POST['username'] = 'aidan';
$_POST['password'] = "' OR ''='";

How does it deal with this password entry?

Thanks.

11

Just having a quick look through your code I would suggest sanitising all user inputs using mysql_real_escape_string() to reduce the vulnerability of your code to [URL=“http://unixwiz.net/techtips/sql-injection.html”]sql injection

12

From PHP 4.1.0 $HTTP_SESSION_VARS is deprecated

13

Thanks!