Hey all, I’m new to MySql and PHP. Never the less I have written a simple program. It would be great to hear some constructive critisism from other php programmers about “good practices” I’m utilizing and “bad practices”.
The code: A simple casino type game where you log in. You will be able to buy chips(for free currently) and gamble a number guessing game. That is pretty much all I have so far.
I would like to know if I’m able to post my code here to be critiqued before I do so. The code is kind of lengthy (11 .php, 1 sql, 1 .css)
Thanks!
-Derek
Hi dadam88, welcome to the forums,
And thanks for asking.
You can make a post and attach the files as a “zip” or renamed with the “txt” extension if you would like.
Well here it is, please let me know any “No No’s” I’m doing and “Yes Yes’s!”.
One note, is that I know my login process is not secured, so no need to comment on that, I just want to know if I’m practicing good practices and if not, how.
Hi Derek,
Lets start with login.php
<?php
include "connect.php";
$username = $_POST['username'];
$password = $_POST['password'];
?>
<link href="style.css" rel="stylesheet" type="text/css" />
<div align="center">
<?
if($username&&$password)
{
$query = mysql_query("SELECT * FROM users WHERE username='$username'");
$numrows = mysql_num_rows($query);
if ($numrows != 0) //username found
{
while ($row = mysql_fetch_assoc($query))
{
$dbusername =$row['username'];
$dbpassword = $row['password'];
}
if($password==$dbpassword&&$username==$dbusername) //check if password matches database pasword and username
{
echo "You're in! <br><a href ='casino.php'>Click</a> here to enter the casino.";
$_SESSION['username']=$username;
}
else
{
die("incorrectpass<br><a href ='index.php'>Try again</a>");
}
}
else
{
die("You do not exsist!<br><a href ='index.php'>Try again</a>");
exit();
}
}
else
{
die("you did not enter a username/password");
exit();
}
?>
Always escape user supplied data prior to using it in an SQL query, this will prevent “SQL Injection”.
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
Instead if iterating through all users, just ask the database to find you one with the right credentials.
<?php
$sql = "SELECT username FROM users WHERE username = '$username' AND password = '$password' LIMIT 1";
$res = mysql_query($sql);
if(1 === mysql_num_rows($res))
{
#user found
$user = mysql_fetch_assoc($res);
$_SESSION['username'] = $user['username'];
}
else
{
#no matching user
}
In addition to this, you’re also storing the users password in “plain text”, you should at the very least hash it using SHA1. This would change the above code only slightly, but offer much better security.
<?php
$sql = "SELECT username FROM users WHERE username = '$username' AND password = SHA1('$password') LIMIT 1";
$res = mysql_query($sql);
if(1 === mysql_num_rows($res))
{
#user found
$user = mysql_fetch_assoc($res);
$_SESSION['username'] = $user['username'];
}
else
{
#no matching user
}
Notice the change in the $sql string?
Always escape user supplied data prior to using it in an SQL query, this will prevent “SQL Injection”.
I had just read about this, thanks for reminding me.
Instead if iterating through all users, just ask the database to find you one with the right credentials.
Much appreciated, I clearly see that was inefficient.
In addition to this, you’re also storing the users password in “plain text”, you should at the very least hash it using SHA1. This would change the above code only slightly, but offer much better security.
I wasn’t focusing on integrating password security yet. I have not heard about SHA1, but I have heard and played around with MD5, opinions? 
They’re essentially the same thing for this purpose, but SHA1 uses a different algorithm, and is longer, so there’s less chance of collisions (different strings resulting in the same hash).
So, is this better?
Is this redundant? The reason why I just using select username because I want to do a check if the username exists. If it doesn’t exist I see no point in executing anymore code. If I select password too, I cant error out a correct message when one or the other is wrong.
$query = mysql_query("SELECT username FROM users WHERE username='$username'");
Do I only need to query for the password the second time?
$query = mysql_query("SELECT username, password FROM users WHERE username='$username' AND password='$password' LIMIT 1");
<?php
include "connect.php";
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
?>
<link href="style.css" rel="stylesheet" type="text/css" />
<div align="center">
<?
if($username&&$password)
{
$query = mysql_query("SELECT username FROM users WHERE username='$username'");
$numrows = mysql_num_rows($query);
if ($numrows == 1) //username exsists
{
$query = mysql_query("SELECT username, password FROM users WHERE username='$username' AND password='$password' LIMIT 1");
if (mysql_num_rows($query) == 1) //username and password match
{
$user = mysql_fetch_assoc($query);
echo "You're in! <br><a href ='casino.php'>Click</a> here to enter the casino.";
$_SESSION['username']= $user['username'];
}
else
{
die("incorrectpass<br><a href ='index.php'>Try again</a>");
}
}
else
{
die("You do not exsist!<br><a href ='index.php'>Try again</a>");
exit();
}
}
else
{
die("you did not enter a username/password");
exit();
}
?>
Ask yourself if you want to be that specific. It could be of great help for someone that’s trying to hack an account. While the legitimate owner of the account should know his account name, and a more generic ‘Login failed, check username and/or password’ should be enough.
And if you want to have two different messages, you can always select both username and password in the first query, and then check the password retrieved with the one inserted by the user. No need for two queries.