3
\$\begingroup\$

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"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
<img src="#" height="50" width="40"/>&nbsp;&nbsp;
</div>
\$\endgroup\$
3
\$\begingroup\$

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");
  1. 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.
  2. 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']
  3. Instead of $reg = @$_POST['reg']; and if($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.
  4. 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

  1. 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.
  2. It's not a good idea to store the password in your session. See this on security.

in profile.php

  1. try to use a better variable name than $get and $check. maybe something like $row and $query would be more suitable.
  2. And you already know this but stop using mysql_* functions and change that over to PDO with prepared statements with param binding.
  3. 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; to echo(htmlentities($username));
\$\endgroup\$
3
  • \$\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\$ Mar 13 '13 at 12:44
  • \$\begingroup\$ Also for the defining part, how would I implement that with an if statement? \$\endgroup\$ Mar 13 '13 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\$ Mar 13 '13 at 14:25
0
\$\begingroup\$

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']);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.