user login/validation script

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
itp
Forum Commoner
Posts: 67
Joined: Fri Jun 15, 2007 6:50 am

user login/validation script

Post by itp »

I cobled together a simple user validation/login script using a mySQL database and PHP sessions. I seem to work OK but I'm not sure that it is as secure/safe or fast as it can be. It seems a bit slower in Firefox than IE. Comments welcome.

Code: Select all

<?php
session_start();   

$logoff  	  = (empty($_REQUEST["logoff"]))  	    ?  ""  :  $_REQUEST["logoff"]       ;
$userName  	  = (empty($_REQUEST["userName"]))   	?  ""  :  $_REQUEST["userName"]      ;
$userPassword = (empty($_REQUEST["userPassword"]))  ?  ""  :  $_REQUEST["userPassword"]  ;
$userSession  = (empty($_SESSION["userSession"]))   ?  ""  :  $_SESSION["userSession"]   ;
$message  = "";

// set error reporting
error_reporting(E_ALL); 
ini_set('display_errors', 1); 
     
// user makes attempt to login  
if ( $userName <> '' && $userPassword <> '')
{
	$valid_user = fnValidateUser($userName,$userPassword);
	if ($valid_user==1)
	{
		$_SESSION['userSession'] = $userName;
		$_REQUEST["userName"] ='';
		$userSession  = $userName;
		$message  = '';
	}
	else
	{
		$_SESSION['userSession'] = '';
		$_REQUEST["userName"] ='';
		$_REQUEST["logoff"] ='';
		$userSession  = '';
		$message  = 'Username/password incorrect';
	}	 
}

// show login screen if logout requested or not currently logged in
if ($logoff == 'Y' || $userSession == '')
{
	if ($userSession <> '')
	{
		session_destroy();
		$userSession ='';
		$_SESSION["userSession"] ='';
	}
	$_REQUEST["logoff"] ='';
	$_REQUEST["userName"] ='';
	fnGetlogin($message);
}
 
// successfully signed in
if ($userSession <> '')
{
	echo $userSession . ' is logged in <br>';
	echo "<a href = 'secure3.php?logoff=Y' >logoff</a>  ";
	echo "<a href = 'secure4.php' >another page</a>";
}
 
// show login screen 
function fnGetlogin($message)
{

echo "<p align='center'><font color=red>" . $message . "</font></p>";
echo <<<EOT
	<form action="secure3.php" method="post">
	<p align="center">Please login to access this document.</p>
	<table align="center" border="0">
	 <tr>
	  <th>
	Username:
	  </th>
	  <th>
	<input type="text" name="userName">
	  </th>
	 </tr>
	 <tr>
	  <th>
	Password:
	  </th>
	  <th>
	<input type="password" name="userPassword">
	  </th>
	 </tr>
	 <tr>
	  <th colspan="2" align="right">
	<input type="submit" value="Login">
	</form>
	  </th>
	 </tr>
	</table>
	</body>
	</html>
EOT;
}

// function to validate username & password against SQL database
function fnValidateUser($userName,$userPassword)
{
	// get database parameters
	include_once("include2.php");

	//echo $userName . $userPassword. '<br><br><br><br>';
	$db = mysql_connect($host, $user, $password) or die("Could not connect."); 
	if(!$db) 
		die("no db");
	if(!mysql_select_db($database,$db))
	 	die("No database selected.");

	$sql = mysql_query("SELECT password FROM member WHERE username = '$userName'");
	$fetch_em = mysql_fetch_array($sql);
	$numrows = mysql_num_rows($sql);
	 
	if($numrows != "0" & $userPassword == $fetch_em["password"]) 
	{
		$valid_user = 1;
	}
	else 
	{
		$valid_user = 0;
	}
 	return $valid_user;
}
 
?>
Include2.php contains the following code...

Code: Select all

<?php

if ($_SERVER['DOCUMENT_ROOT'] =='/var/www/html')
{

 $host = 'localhost'     ;
 $user = 'xxx'         ;
 $password = "xxx"   ;
 $database = "xxx"     ;
 $file1 = "xxxx" ;
}

// test server
if ($_SERVER['DOCUMENT_ROOT'] == 'C:/xampp/htdocs')
{
 $host = 'localhost'     ;
 $user = 'xxx'         ;
 $password = "xxx"   ;
 $database = "xxx"     ;
 $file1 = "xxxx" ;
  
}



 $LF = chr(13) . chr(10) ;
 $quote = "'";   
 $q = "'";   
 $dq = "\"";   
 $comma = ", "; 


$db = mysql_connect($host, $user, $password) or die("Could not connect."); 
if(!$db) 
	die("no db");
if(!mysql_select_db($database,$db))
 	die("No database selected.");

if(!get_magic_quotes_gpc())
{
  $_GET = array_map('mysql_real_escape_string', $_GET); 
  $_POST = array_map('mysql_real_escape_string', $_POST); 
  $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}
else
{  
   $_GET = array_map('stripslashes', $_GET); 
   $_POST = array_map('stripslashes', $_POST); 
   $_COOKIE = array_map('stripslashes', $_COOKIE);
   $_GET = array_map('mysql_real_escape_string', $_GET); 
   $_POST = array_map('mysql_real_escape_string', $_POST); 
   $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}
 
?>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Your code assumes the super globals are one dimensional arrays. Probably not the best of ideas. ;)
itp
Forum Commoner
Posts: 67
Joined: Fri Jun 15, 2007 6:50 am

Post by itp »

hmmmm, not sure I follow.

I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);


Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

I think the two thing that stand out for me are that the code is not well structured and you do not filter input properly. What kind of comments were you looking for?
(#10850)
Begby
Forum Regular
Posts: 575
Joined: Wed Dec 13, 2006 10:28 am

Post by Begby »

itp wrote:hmmmm, not sure I follow.

I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);


Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?

The problem is that you are running everything through mysql_real_escape_string no matter what it is or where the variable will be used. Sometimes you don't want to escape stuff if you are going to use it in something other than a query.

Also, what if an array gets passed in the post? Your script will bomb. This is what feyd is pointing out.

As for error_reporting, take a look at the comments in php.ini or the documentation on php.net. For development you want to turn on pretty much everything including notices, for a production server you generally do not want any errors/warnings at all.
itp
Forum Commoner
Posts: 67
Joined: Fri Jun 15, 2007 6:50 am

Post by itp »

These are exactly the type of comments that I was looking for. I want to write code
that more than just works. I want well structured, maintainable, scalable
and secure code as well. These things that are not always obvious at first pass.

Begby's comments are very illustrative.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

  • I'd have all your variables that you set at the top be boolean FALSE if the required variable is not found. It seems a bit cleaner, both code-wise and semantically. By doing so, your first if condition can be changed to:

    Code: Select all

    if($userName && $userPassword)
  • Have fnValidateUser() return boolean TRUE or FALSE rather than 1 or 0 - again it seems cleaner. Again, your if condition could then be cleaned up:

    Code: Select all

    if(fnValidateUser($userName,$userPassword)
    {...}
    else
    {...}
  • Is $logoff ever going to exist if the user doesn't want to logoff? If not, you don't need to check for it's value, just it's existence:

    Code: Select all

    if($logoff || !$userSession)
  • Why do you have 2 echo statements one right after the other in fnGelogin(). heredocs can interpret variables, so you can put $message in your heredocs. Strictly speaking, fnGetlogin() should be named fnGetLogin() to be proper camel case.
  • Include2.php: is it just for this application? If so, what are $LF, $quote, $q, $dq, $comma for? They're not used anywhere. Also, you connect to a database in the include2 file, as well as in your main file.
  • If you want to keep the db connection in include2.php, you can get rid of the superfluous if...die condition.

    Code: Select all

    if(!$db)...die("no db");
    is unnecessary
  • ~feyd is correct in that your assumption that $_GET, $_POST, and $_COOKIE are one dimensional. I also personally consider it bad form to overwrite super globals. I treat superglobals as unmodifiable & escape them as necessary.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
Post Reply