[RESOLVED] PHP and Mysql Login

[Foxman]

I made a self-submitting user log in script for my site, and no matter what I do, it just refreshes the page when the form is submitted. My script is fairly long, but I see nothing wrong with it, and no parsing errors come up on the page. My database connections are in config.php, and thats all that's in config.php. I am using sessions for the site as well for user info, such as uid. I use a cookie set by other pages if you aren't logged in, so the form redirects back to that page once you log in, and if you never visited a page before logging in, you get redirected to the index. Here is my script:

<? include("scripts/config.php"); ?>
<?
session_start();
if(isset($_SESSION['uid'])){
	echo "You are already logged in. If you logged in as the incorrect user, please <a href=\"logout\" title=\"Logout Of Site\">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a>";
}
elseif(!isset($_SESSION['uid'])){
	$self = $_SERVER['PHP_SELF'];
	$checksc = "<script language=\"javascript\">
function validate(){
	var Check = 0;
	if(document.form.username.value == ''){
		alert('Please enter a valid username.');
		return false; Check = 1
	}
	if(document.form.password.value == ''){
		alert('Please enter a valid password.');
		return false; Check = 1
	}
    if(Check == 0){
		document.form.send.disabled = true;
		return true;
	}
}
</script>";
	$formt = "<form action=\"$self\" method=\"post\" onsubmit=\"return validate();\" name=\"form\">
	Enter Your Username: <input type=\"text\" name=\"username\" title=\"Enter your username here.\" value=\"$username\"/><br>
	Enter Your Password: <input type=\"password\" name=\"password\" title=\"Enter your password here.\" value=\"$password\"/><label title=\"Show your password as text.\"><input type=\"checkbox\" name=\"showhide\" onclick=\"if(document.form.showhide.checked==true) document.form.password.type='text';if(document.form.showhide.checked==false) document.form.password.type='password';\"/>Show your password</label><br>
	<input type=\"submit\" value=\"Submit\" title=\"Log In\" name=\"send\"/>
</form>If you do not have a account, please <a href=\"register\" title=\"Register as a Member (Registration closes on DATE)\">register</a>.<br>If you are having trouble logging in, please email <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a> for help.";
	$send = $_POST['send'];
	$username = $_POST['username'];
	$password = $_POST['password'];
	if($send){
		if(isset($username) && isset($password)){
			$query=@mysql_query("SELECT * FROM members WHERE username=$username AND password=$password") or die(mysql_error());
			$num = mysql_num_rows($query);
			if($num != 0){
				$mrid=@mysql_query("SELECT uid FROM members WHERE username=$username AND password=$password") or die(mysql_error());
				$_SESSION['uid'] = $mrid;
				if(!isset($_COOKIE['redirect'])){
					header("Location: index.php");
				}
				else{
					header("Location:". $_COOKIE['redirect'] . "");
				}
			}
			else{
				echo "$checksc
				You have entered a incorrect or invalid username or password. Please try again.<br>
				$formt";
			}
		}
		elseif($password == "" || $username == ""){
			echo "$checksc
			";
			if($username == ""){
				echo "You did not enter a username.<br>";
			}
			if($password == ""){
				echo "You did not enter a password.<br>";
			}
			echo "
			$formt";
		}
	}
	else{
		echo "$checksc
		$formt";
	}
}
?>

The javascript is for form validation but some people do not have javascript enabled in their browser.

[bradgrafelman]

No offense, just being honestly blunt here, but there's a lot wrong with your code, so bear with me here...

1. I would recommend using the full <?php ?> tags versus the short tags; IIRC, short tags aren't available in PHP6 anymore, and they aren't considered good coding practice anyway.

2. These lines:

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

   Have 2 things wrong with them. One, if the variables weren't POSTed (e.g. form hasn't been submitted), you'll generate a notice saying that you're accessing an undefined index of the $_POST array. Two, later on, you check if $username and $password are set. Well of course they're set, you just set them (erroneously or not)! This is why you should...

3. Get rid of the three lines I mentioned above (lines 31-33). Then, instead of saying if($send) { you'd do something like if(isset($_POST['send'])) { .

4. Inside that if() statement, you can now define your $username and $password variables. However, just because the form appears to have been submitted doesn't mean that both of these variables were as well. As such, a common way of setting variables to hold POST'ed data is this:

   $username = (isset($_POST['username']) ? $_POST['username'] : '');
   $password = (isset($_POST['password']) ? $_POST['password'] : '');

5. Instead of using [man]isset/man on line 35, I would instead check to make sure that neither variables contain an empty string.

6. Your script is vulnerable to SQL injection attacks. This means that a malicious user can inject SQL commands directly into your query because you're not sanitizing user-supplied data before you place it in your SQL query (with a function like [man]mysql_real_escape_string[/man]). Instead of placing $username or $password directly into your SQL query, you should instead pass them through a function such as [man]mysql_real_escape_string/man first. Nothing hard there - just escape out of the string and concatenate.

7. Why do you run two queries, once to check if any rows are returned, and then yet again to select the data you want? No reason to do this at all - get rid of the second query, and change the "SELECT *" to "SELECT uid" in the first query.

8. If you're comparing a column with a string, make sure you pass SQL a string. As in PHP, MySQL expects strings to be surrounded by quotes. Therefore, you should put quotes in your query around your values for the username and password columns.

9. I assume the mysql_error() calls are for debugging purposes only, and that you will remove them when this site goes live, right? You should never, ever, ever display this kind of error message on a live website.

10. Inside the if($num != 0){ statement (line 38), you have two problems. One, there's that second SQL query that I've previously mentioned is superfluous and is simply wasting resources. Other than that, you're improperly trying to retrieve data from the query. Once you execute a query with [man]mysql_query/man, you need to use a function such as [man]mysql_result/man (if you're only retrieving one column from one row, as you are in this case) or [man]mysql_fetch_assoc/man (if you're retrieving an entire row at a time, as in a loop).

11. The elseif() statement on line 54 can simply be an else statement, no need to use an if() simply to provide the exact inverse of the conditions listed in the if() statement.

[scross]

I'm sorry to be a bit pedantic but you also have some other issues in your code:

1) You include a great deal of html and javascript straight into your php code, making it much more difficult to read, a big problem if you (or others) decide to work upon it. Even if your script is small, it's always good practise to seperate the html and the php code.
2) Use Comments - This will allow you to provide a way for developers to gain a grasp of the function of your code without having to read it through. They might also help you spot logic errors such as the use of two mysql queries when you only need one.
3) Use single quotes for strings if they contain many double quotes. For example:

echo "You are already logged in. If you logged in as the incorrect user, please <a href=\"logout\" title=\"Logout Of Site\">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a>";

Would it not have been easier to use this:

echo 'You are already logged in. If you logged in as the incorrect user, please <a href="logout" title="Logout Of Site">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href="mailto:[e-mail]" title="Email [e-mail] for problems that you are having with logging in.">[e-mail]</a>';

And this is nothing compared to the form, full of hundreds of unneccessary escaped double quotes. I tend to use single quotes for my strings in php so that I don't have to escape double quotes if I have html code in the string.

Other than this (and the adjustments from bradgrafelman) your code seems ok and I'm glad you indented your code 🙂

[bradgrafelman]

Of course, the last step once you get all of this working would be to run the pages through http://validator.w3.org and fix up your HTML! :p

[Foxman]

Thank you both for helping me so much, i used all your advice, and yet it still doesn't work. I didn't exactly understand what you meant by using "mysql_result" in my code. Where should i put it? In the if($num statement or right after the query? I added quotes to the query for $username and $password, but I don't know if I did correctly. I added comments like you guys said also. I'll remove the mysql_error's cause i now get that it can give info to hackers:xbones:. And lastly, I don't know if I changed the if statement on line 34(Now line 41) to be the way you said it should be. Here is my new script:

<?php include("scripts/config.php"); ?>
<?php
session_start();
// If your uid has already been set, tell the user that.
if(isset($_SESSION['uid'])){
	echo "You are already logged in. If you logged in as the incorrect user, please <a href=\"logout\" title=\"Logout Of AVP Showcase\">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a>";
}
// If your uid hasn't been set, proccess or show the form.
elseif(!isset($_SESSION['uid'])){
// Variable for self submitting form.
	$self = $_SERVER['PHP_SELF'];
// Javasript validation for form.
	$checksc = '<script language="javascript">
function validate(){
	var Check = 0;
	if(document.form.username.value == \'\'){
		alert(\'Please enter a valid username.\');
		return false; Check = 1
	}
	if(document.form.password.value == \'\'){
		alert(\'Please enter a valid password.\');
		return false; Check = 1
	}
    if(Check == 0){
		document.form.send.disabled = true;
		return true;
	}
}
</script>';
// Form html tags.
	$formt = '<form action="' . $self . '" method="post" onsubmit="return validate();" name="form">
	Enter Your Username: <input type="text" name="username" title="Enter your username here." value=""/><br>
	Enter Your Password: <input type="password" name="password" title="Enter your password here." value=""/><label title="Show your password as text."><input type="checkbox" name="showhide" onclick="if(document.form.showhide.checked==true) document.form.password.type=\'text\';if(document.form.showhide.checked==false) document.form.password.type=\'password\';"/>Show your password</label><br>
	<input type="submit" value="Submit" title="Log In" name="send"/>
</form>If you do not have a account, please <a href="register" title="Register as a Member (Registration closes on DATE)">register</a>.<br>If you are having trouble logging in, please email <a href="mailto:[e-mail]" title="Email [e-mail] for problems that you are having with logging in.">[e-mail]</a> for help.';
// If the form has been submitted.
	if(isset($_POST['send'])){
		$username = (isset($_POST['username']) ? $_POST['username'] : '');
		$password = (isset($_POST['password']) ? $_POST['password'] : '');
		// If both username and password aren't empty, attempt login.
		if(isset($_POST['username']) && isset($_POST['password'])){
			mysql_real_escape_string($username);
			mysql_real_escape_string($password);
			$query=@mysql_query("SELECT uid FROM members WHERE username=\"$username\" AND password=\"$password\"") or die(mysql_error());
			$num = mysql_num_rows($query);
			// If the number of rows where there is a user id that also has equal password and username as the ones submitted isn't none, loggin and redirect.
			if($num != 0){
				$_SESSION['uid'] = "$query";
				// If the redirection cookie from other pages isn't set, go to the index page.
				if(!isset($_COOKIE['redirect'])){
					header("Location: index.php");
				}
				// Else, redirect to the location that the cookie says.
				else{
					header("Location:". $_COOKIE['redirect'] . "");
				}
			}
			// If the number of rows is equal to 0, say you have entered inccorrect data.
			else{
				echo "$checksc
				You have entered a incorrect or invalid username or password. Please try again.<br>
				$formt";
			}
		}
		// If they didn't enter anything in either form field, say what they didn't enter.
		else{
			echo "$checksc
			";
			// If they didn't enter a username, say that.
			if($username == ""){
				echo "You did not enter a username.<br>";
			}
			// If they didn't enter a password, say that.
			if($password == ""){
				echo "You did not enter a password.<br>";
			}
			echo "
			$formt";
		}
	}
	// If the form has not been submitted, print the form.
	else{
		echo "$checksc
		$formt";
	}
}
?>

[scross]

1) Change:

mysql_real_escape_string($username);
mysql_real_escape_string($password);

to:

$username = mysql_real_escape_string($username);
$password = mysql_real_escape_string($password);

because otherwise you are just losing the output of the mysql_real_escape_string and so your validation doesn't actually work correctly.

2) Change:

$_SESSION['uid'] = "$query";

to:

$result = mysql_fetch_assoc($query);
$_SESSION['uid'] = $result['uid'];

3) Change:

if($username == ""){
    echo "You did not enter a username.<br>";
}
// If they didn't enter a password, say that.
if($password == ""){
    echo "You did not enter a password.<br>";
}

to:

if(empty($username)){
    echo "You did not enter a username.<br>";
}
// If they didn't enter a password, say that.
if(empty($password)){
    echo "You did not enter a password.<br>";
}

4) Finally, change:

elseif(!isset($_SESSION['uid'])){

to:

else{

since the elseif() is completely unneccessary when you just want to handle the opposite condition.

[Foxman]

It still won't work for some reason😕.

<?php include("scripts/config.php"); ?>
<?php
session_start();
// If your uid has already been set, tell the user that.
if(isset($_SESSION['uid'])){
	echo "You are already logged in. If you logged in as the incorrect user, please <a href=\"logout\" title=\"Logout Of AVP Showcase\">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a>";
}
// If your uid hasn't been set, proccess or show the form.
else{
// Variable for self submitting form.
	$self = $_SERVER['PHP_SELF'];
// Javasript validation for form.
	$checksc = '<script language="javascript">
function validate(){
	var Check = 0;
	if(document.form.username.value == \'\'){
		alert(\'Please enter a valid username.\');
		return false; Check = 1
	}
	if(document.form.password.value == \'\'){
		alert(\'Please enter a valid password.\');
		return false; Check = 1
	}
    if(Check == 0){
		document.form.send.disabled = true;
		return true;
	}
}
</script>';
// Form html tags.
	$formt = '<form action="' . $self . '" method="post" onsubmit="return validate();" name="form">
	Enter Your Username: <input type="text" name="username" title="Enter your username here." value=""/><br>
	Enter Your Password: <input type="password" name="password" title="Enter your password here." value=""/><label title="Show your password as text."><input type="checkbox" name="showhide" onclick="if(document.form.showhide.checked==true) document.form.password.type=\'text\';if(document.form.showhide.checked==false) document.form.password.type=\'password\';"/>Show your password</label><br>
	<input type="submit" value="Submit" title="Log In" name="send"/>
</form>If you do not have a account, please <a href="register" title="Register as a Member (Registration closes on DATE)">register</a>.<br>If you are having trouble logging in, please email <a href="mailto:[e-mail]" title="Email [e-mail] for problems that you are having with logging in.">[e-mail]</a> for help.';
// If the form has been submitted.
	if(isset($_POST['send'])){
		$username = (isset($_POST['username']) ? $_POST['username'] : '');
		$password = (isset($_POST['password']) ? $_POST['password'] : '');
		// If both username and password aren't empty, attempt login.
		if(isset($_POST['username']) && isset($_POST['password'])){
			$username = mysql_real_escape_string($username);
			$password = mysql_real_escape_string($password); 
			$query=@mysql_query("SELECT uid FROM members WHERE username=\"$username\" AND password=\"$password\"") or die(mysql_error());
			$num = mysql_num_rows($query);
			// If the number of rows where there is a user id that also has equal password and username as the ones submitted isn't none, loggin and redirect.
			if($num != 0){
				$result = mysql_fetch_assoc($query);
				$_SESSION['uid'] = $result['uid']; 
				// If the redirection cookie from other pages isn't set, go to the index page.
				if(!isset($_COOKIE['redirect'])){
					header("Location: index.php");
				}
				// Else, redirect to the location that the cookie says.
				else{
					header("Location:". $_COOKIE['redirect'] . "");
				}
			}
			// If the number of rows is equal to 0, say you have entered inccorrect data.
			else{
				echo "$checksc
				You have entered a incorrect or invalid username or password. Please try again.<br>
				$formt";
			}
		}
		// If they didn't enter anything in either form field, say what they didn't enter.
		else{
			echo "$checksc
			";
			// If they didn't enter a username, say that.
			if(empty($username)){
				echo "You did not enter a username.<br>";
			}
			// If they didn't enter a password, say that.
			if(empty($password)){
    			echo "You did not enter a password.<br>";
			}
			echo "
			$formt";
		}
	}
	// If the form has not been submitted, print the form.
	else{
		echo "$checksc
		$formt";
	}
}
?>

[Foxman]

Is there an alternative method for validating the form?

[scross]

You also need to change:

if(isset($_POST['username']) && isset($_POST['password'])){

to:

if(!empty($username) && !empty($password)){

because your condition would have always been true and you were also referencing the $_POST values when you should have just used $username and $password. Hopefully it'll work now...

[Foxman]

I changed that but it still doesn't work for some reason. Could it be the way php is configured on my server?

[scross]

Ok, well place:

error_reporting(E_ALL);

at the top of your page (in the php tags, since it's a php function) and comment out those calls to header(). You should then be able to see what is going on. It also helps to use print_r() to print out your variables so you can check they have the value they're supposed to have.

[Foxman]

Okay, I'll try the variable trick, and how do i output error_reporting to header()?

[scross]

Foxman wrote:

how do i output error_reporting to header()?

you what?? 😛

No, add a function call to error_reporting at the top of your code like so:

<?php
error_reporting(E_ALL);
include("scripts/config.php");
session_start();

(I also cleaned up your php tags) Now comment out (just add // before) each of the header() function calls like so:

//header("Location: index.php");

Now you'll be able to see any errors that crop up.

[Foxman]

Okay, i did that and no error messages come up. Thats good, right?

[scross]

No, that's bad, since we still don't know what the problem is. Like I said before, trying outputting some of the variables (like $username and $password) to check their value is what it should be.

[Foxman]

Well, i really can't, because when i submit the form, nothing happens whatsoever, it just refreshes the page and the form shows up.

[scross]

Have you commented out both calls to the header() function?? This is the only thing that would refresh the page.

[Foxman]

Yes. Here is my new code as it stands: (I added some jscript and template tables.)

<?php
error_reporting(E_ALL);
include("scripts/config.php");
session_start();
?>
<script type="text/javascript" src="scripts/scripts?sid=1"></script>
<link rel="stylesheet" href="scripts/scripts?sid=2" type="text/css"/>
<body id="html">
<!-- START big TABLE asset -->
<table cellpadding="0" cellspacing="0" border="0" align="center">
	<tr>
		<td class="big_top"><center><br>Log In</center></td>
	</tr>
		<td class="big_middle"><table align="center" border="0" cellpadding="0" cellspacing="0" width="440" height="100%"><tr><td>
<?php
// If your uid has already been set, tell the user that.
if(isset($_SESSION['uid'])){
	echo "You are already logged in. If you logged in as the incorrect user, please <a href=\"logout\" title=\"Logout Of AVP Showcase\" onclick=\"return log_out();\">logout</a> first and then log back in as the correct user.<br>If you are having log in problems, please email your issue to <a href=\"mailto:[e-mail]\" title=\"Email [e-mail] for problems that you are having with logging in.\">[e-mail]</a>";
}
// If your uid hasn't been set, proccess or show the form.
else{
// Variable for self submitting form.
	$self = $_SERVER['PHP_SELF'];
// Javasript validation for form.
	$checksc = '<script language="javascript">
function validate(){
	var Check = 0;
	if(document.form.username.value == \'\'){
		alert(\'Please enter a valid username.\');
		return false; Check = 1
	}
	if(document.form.password.value == \'\'){
		alert(\'Please enter a valid password.\');
		return false; Check = 1
	}
    if(Check == 0){
		document.form.send.disabled = true;
		return true;
	}
}
</script>';
// Form html tags.
	$formt = '<form action="' . $self . '" method="post" onsubmit="return validate();" name="form">
	Enter Your Username: <input type="text" name="username" title="Enter your username here." value=""/><br>
	Enter Your Password: <input type="password" name="password" title="Enter your password here." value=""/><label title="Show your password as text."><input type="checkbox" name="showhide" onclick="if(this.checked==true) document.form.password.type=\'text\';if(this.checked==false) document.form.password.type=\'password\';"/>Show your password</label><br>
	<input type="submit" value="Submit" title="Log In" name="send"/>
</form>If you do not have a account, please <a href="register" title="Register as a Member (Registration closes on DATE)">register</a>.<br>If you are having trouble logging in, please email <a href="mailto:[e-mail]" title="Email [e-mail] for problems that you are having with logging in.">[e-mail]</a> for help.';
// If the form has been submitted.
	if(isset($_POST['send'])){
		$username = (isset($_POST['username']) ? $_POST['username'] : '');
		$password = (isset($_POST['password']) ? $_POST['password'] : '');
		// If both username and password aren't empty, attempt login.
		if(!empty($username) && !empty($password)){
			$username = mysql_real_escape_string($username);
			$password = mysql_real_escape_string($password); 
			$query=@mysql_query("SELECT uid FROM members WHERE username=\"$username\" AND password=\"$password\"") or die(mysql_error());
			$num = mysql_num_rows($query);
			// If the number of rows where there is a user id that also has equal password and username as the ones submitted isn't none, loggin and redirect.
			if($num != 0){
				$result = mysql_fetch_assoc($query);
				$_SESSION['uid'] = $result['uid']; 
				// If the redirection cookie from other pages isn't set, go to the index page.
				if(!isset($_COOKIE['redirect'])){
					// header("Location: index.php");
				}
				// Else, redirect to the location that the cookie says.
				else{
					// header("Location:". $_COOKIE['redirect'] . "");
				}
			}
			// If the number of rows is equal to 0, say you have entered inccorrect data.
			else{
				echo "$checksc
				You have entered a incorrect or invalid username or password. Please try again.<br>
				$formt";
			}
		}
		// If they didn't enter anything in either form field, say what they didn't enter.
		else{
			echo "$checksc
			";
			// If they didn't enter a username, say that.
			if(empty($username)){
				echo "You did not enter a username.<br>";
			}
			// If they didn't enter a password, say that.
			if(empty($password)){
    			echo "You did not enter a password.<br>";
			}
			echo "
			$formt";
		}
	}
	// If the form has not been submitted, print the form.
	else{
		echo "$checksc
		$formt";
	}
}
?>
</td></tr></table></td>
	<tr>
		<td class="big_bottom"></td>
	</tr>
</table>
<!-- END big TABLE asset -->
</body>

[scross]

After:

$_SESSION['uid'] = $result['uid'];

add:

echo 'Success!';

so you can see whether it worked or not.

[Foxman]

Okay, I put that in and it didn't show up when submitted. Should i remove the if statement for the header()'s?