There are some things that I know that need to be fixed, such as mysql_*
needing to be converted to PDO, and using a better hash. I am working on building a social networking site, and I've been having issues with some of the mysql
like mysql_real_escape_string
and implementing newer techniques. Any criticism and/or help would be very appreciated.
register script
<?
$reg = @$_POST['reg'];
//declaring variables to prevent errors
//registration form
$fn = (!empty($_POST['fname'])) ? $_POST['fname'] : '';
$ln = (!empty($_POST['lname'])) ? $_POST['lname'] : '';
$un = (!empty($_POST['username'])) ? $_POST['username'] : '';
$em = (!empty($_POST['email'])) ? $_POST['email'] : '';
$em2 = (!empty($_POST['email2'])) ? $_POST['email2'] : '';
$pswd = (!empty($_POST['password'])) ? $_POST['password'] : '';
$pswd2 = (!empty($_POST['password2'])) ? $_POST['password2'] : '';
$d = date("y-m-d"); // Year - Month - Day
if ($reg) {
if ($em==$em2) {
// Check if user already exists
$statement = $db->prepare('SELECT username FROM users WHERE username = :username');
if ($statement->execute(array(':username' => $un))) {
if ($statement->rowCount() > 0){
//user exists
echo "Username already exists, please choose another user name.";
exit();
}
}
//check all of the fields have been filled in
if ($fn&&$ln&&$un&&$em&&$em2&&$pswd&&$pswd2) {
//check that passwords match
if ($pswd==$pswd2) {
//check the maximum length of username/first name/last name does not exceed 25 characters
if (strlen($un)>25||strlen($fn)>25||strlen($ln)>25) {
echo "The maximum limit for username/first name/last name is 25 characters!";
}
else
{
//check the length of the password is between 5 and 30 characters long
if (strlen($pswd)>30||strlen($pswd)<5) {
echo "Your password must be between 5 and 30 characters long!";
}
else
{
//encrypt password and password 2 using md5 before sending to database
$pswd = md5($pswd);
$pswd2 = md5($pswd2);
$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING );
$sql = 'INSERT INTO users (username, first_name, last_name, email, password, sign_up_date)';
$sql .= 'VALUES (:username, :first_name, :last_name, :email, :password, :sign_up_date)';
$query=$db->prepare($sql);
$query->bindParam(':username', $un, PDO::PARAM_STR);
$query->bindParam(':first_name', $fn, PDO::PARAM_STR);
$query->bindParam(':last_name', $ln, PDO::PARAM_STR);
$query->bindParam(':email', $em, PDO::PARAM_STR);
$query->bindParam(':password', $pswd, PDO::PARAM_STR);
$query->bindParam(':sign_up_date', $d, PDO::PARAM_STR);
$query->execute();
die("<h2>Welcome to Rebel Connect</h2>Login to your account to get started.");
}
}
}
else {
echo "Your passwords do not match!";
}
}
else
{
echo "Please fill in all fields!";
}
}
else {
echo "Your e-mails don't match!";
}
}
?>
login script
<?
//Login Script
if (isset($_POST["user_login"]) && isset($_POST["password_login"])) {
$user_login = preg_replace('#[^A-Za-z0-9]#i', '', $_POST["user_login"]); // filter everything but numbers and letters
$password_login = preg_replace('#[^A-Za-z0-9]#i', '', $_POST["password_login"]); // filter everything but numbers and letters
$password_login=md5($password_login);
$db = new PDO('mysql:host=localhost;dbname=socialnetwork', 'root', 'abc123');
$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING );
$sql = $db->prepare("SELECT id FROM users WHERE username = :user_login AND password = :password_login LIMIT 1");
if ($sql->execute(array(
':user_login' => $user_login,
':password_login' => $password_login))) {
if ($sql->rowCount() > 0){
while($row = $sql->fetch(PDO::FETCH_ASSOC)){
$id = $row["id"];
}
$_SESSION["id"] = $id;
$_SESSION["user_login"] = $user_login;
$_SESSION["password_login"] = $password_login;
exit("<meta http-equiv=\"refresh\" content=\"0\">");
} else {
echo 'Either the password or username you have entered is incorrect. Please check them and try again!';
exit();
}
}
}
?>
profile.php
<? include("inc/incfiles/header.inc.php");?>
<?
if(isset($_GET['u'])){
$username = mysql_real_escape_string($_GET['u']);
if(ctype_alnum($username)) {
//check user exists
$check = mysql_query("SELECT username, first_name FROM users WHERE username='$username'");
if(mysql_num_rows($check)===1){
$get = mysql_fetch_assoc($check);
$username = $get['username'];
$firstname = $get['first_name'];
}
else
{
echo "<meta http-equiv=\"refresh\" content=\"0; url=http://localhost/tutorial/findfriends/index.php\">";
exit();
}
}
}
?>
<div class="postForm">Post form will go here</div>
<div class="profilePosts">Your posts will go here</div>
<img src="" height="250" width="200" alt="<? echo $username; ?>'s Profile" title="<? echo $username; ?>'s Profile" />
<br />
<div class="textHeader"><? echo $username; ?>'s Profile</div>
<div class="profileLeftSideContent"> Some content about this person's profile </div>
<div class="textHeader"><? echo $username; ?>'s Friends</div>
<div class="profileLeftSideContent">
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
<img src="#" height="50" width="40"/>
</div>
2 Answers 2
for your register script
$ln = (!empty($_POST['lname'])) ? $_POST['lname'] : '';
$un = (!empty($_POST['username'])) ? $_POST['username'] : '';
$em = (!empty($_POST['email'])) ? $_POST['email'] : '';
$em2 = (!empty($_POST['email2'])) ? $_POST['email2'] : '';
$pswd = (!empty($_POST['password'])) ? $_POST['password'] : '';
$pswd2 = (!empty($_POST['password2'])) ? $_POST['password2'] : '';
$d = date("y-m-d");
- use more descriptive variable names. looking at
$ln
,$un
,$em
,$em2
and$d
is a real pain to look at lower in the code. Try$lastname
,$username
, etc. - instead of using
(!empty($_POST['fname']))
make it the reverse so you dont have to do a negate. try using(empty($_POST['fname'])) ? '' : $_POST['fname']
- Instead of
$reg = @$_POST['reg'];
andif($fn && $ln && $un && $em && $em2 && $pswd && $pswd2)
try:$doRegistration = isset($_POST['reg']) && !( empty($_POST['lname']) || empty($_POST['username']) || empty($_POST['email']) || empty($_POST['email2']) || empty($_POST['password']) || empty($_POST['password2']))
. Doing this will avoid opening a database connection to check the username if your not actually going to create an account. Only open a database connection when you have all your data lined up and ready to go. - use constants when ever possible. so for
if(strlen($un)>25||strlen($fn)>25||strlen($ln)>25)
change 25 to something like USERNAME_MAX_LENGTH, FIRSTNAME_MAX_LENGTH and LASTNAME_MAX_LENGTH by defining it like so:define('USERNAME_MAX_LENGTH', 25);
etc. do the same for password length also.
for your login script
- There is no need to filter your password and username with
preg_replace
before you use it in your prepared statement. Just use the pdo::bindParam function and it will escape the input for you. - It's not a good idea to store the password in your session. See this on security.
in profile.php
- try to use a better variable name than
$get
and$check
. maybe something like$row
and$query
would be more suitable. - And you already know this but stop using mysql_* functions and change that over to PDO with prepared statements with param binding.
- Since your not checking for special characters in the username upon registration you need to htmlencode it when you echo it. change all instances of
echo $username;
toecho(htmlentities($username));
-
\$\begingroup\$ Thanks Omar for your answer. Most of what you said seems to make sense (whether or not I understand the reason). I didn't know about not storing the password in my session, and I'm going to start trying to find it. For the login script, all I need to do is remove
preg_replace
and the things that follow it? For the registration bit, I echo the following:echo "Please Fill In All Fields"
. Would I place that after the$doRegistration
? \$\endgroup\$Christopher Bennett– Christopher Bennett2013年03月13日 12:44:47 +00:00Commented Mar 13, 2013 at 12:44 -
\$\begingroup\$ Also for the defining part, how would I implement that with an
if
statement? \$\endgroup\$Christopher Bennett– Christopher Bennett2013年03月13日 12:54:33 +00:00Commented Mar 13, 2013 at 12:54 -
\$\begingroup\$ Just define the constants at the top of the script. You reference their value like a regular variable, only without the dollar sign. \$\endgroup\$jdstankosky– jdstankosky2013年03月13日 14:25:15 +00:00Commented Mar 13, 2013 at 14:25
Instead of duplicating a lot of code (and having to type each name twice!! -> copy paste errors!):
$ln = (!empty($_POST['lname'])) ? $_POST['lname'] : '';
$un = (!empty($_POST['username'])) ? $_POST['username'] : '';
$em = (!empty($_POST['email'])) ? $_POST['email'] : '';
$em2 = (!empty($_POST['email2'])) ? $_POST['email2'] : '';
$pswd = (!empty($_POST['password'])) ? $_POST['password'] : '';
$pswd2 = (!empty($_POST['password2'])) ? $_POST['password2'] : '';
I'd create a utility function somewhere (the name is a bit off, that should be made nicer):
function contentOrEmptyString( $stringInput ) {
return ((!empty($stringInput)) ? $stringInput : '');
}
With that:
$ln = contentOrEmptyString($_POST['lname']);
$un = contentOrEmptyString($_POST['username']);
$em = contentOrEmptyString($_POST['email']);
$em2 = contentOrEmptyString($_POST['email2']);
$pswd = contentOrEmptyString($_POST['password']);
$pswd2 = contentOrEmptyString($_POST['password2']);