2
\$\begingroup\$

I am trying to write my first script that checks users against a database (MySQL right now) and if they aren't in the database, registers them. I've tried my best to sanitize and validate the form data before setting variables to be used from it. I would love any constructive criticism or critiques on any aspect of my code.

Database schema:

users(id, type, username, password, email, first_name, last_name, date_created)

config.inc.php

<?php
/**     
 * Title:            config.inc.php
 * Created by:       M. David Kay III
 * Contact:          xxxxxxxxxx@gmail.com
 * Created:          August 19th, 2013
 * Last modified:    August 21th, 2013
 *
 * Configuration file does the following things:
 * 1 - Defines systemwide settings so changing them is easier.
 * 2 - Defines constants used by many other scripts
 * 3 - Starts the session
 * 4 - Defines homebrew Error handling
 * 5 - Defines a redirection function.
 */

#########################
####### SETTINGS ########
$live = false;      // variable used to dictate how errors are handled. 
                    // also used for payment services to see if we're testing or using them.
$contact_email = 'xxxxxxxxx@gmail.com';
#########################

#########################
####### CONSTANTS #######
define('BASE_URI', '/home6/usr/public_html/mock/');         // constant variable of parent web root directory, to be changed upon deployment.
define('BASE_URL', 'www.projectstack.org/mock/');                             // constant variable of web url, to be changed upon deployment.
define('MYSQL', BASE_URI. 'includes/mysql.inc.php');                          // constant variable of mysql config include file, to be changed upon deployment.
#########################

session_start();    // starts the session, yay!
                    // since we are worried about logged in users, this starts the
                    // tracking process.

/* error handling function:
 * $e_number = error type in numerical format
 * $e_message = the error message itself stored as string
 * $e_file = name of file in which error occurred
 * $e_line = line the error occurred on 
 * $e_vars = an array of all variables in existence when error occurred.
 */

function my_error_handler($e_number, $e_message, $e_file, $e_line, $e_vars) {
    global $live, $contact_email;   // makes the live and contact_email variables global in scope.
    $message = "An error occurred in script '$e_file' on line $e_line: \n$e_message\n";
    // error message with a backtrace array(everything that happened up to that point)
    $message .= "<pre>".print_r(debug_backtrace(),1)."</pre>\n";
    //1 (or true) in print_r() makes function return value, instead of print it.
    $message .= "<pre>".print_r($e_vars,1)."</pre>\n";
    if(!$live){
        echo '<div class="error">'.nl2br($message).'</div>';    // if site isnt live, display error in browser
    } else {
        error_log($message,1,$contact_email,'From:mdavidkay3@gmail.com');  // if site is live, email (1) error log to $contact_email
                                                                           // second argument sets where the error_log goes. 
        //if site is live, this will show a generic error msg, if error isnt a notice
        if($e_number != E_NOTICE){
            echo '<div class="error">A system error occurrd. We apologize for the inconvenience.</div>';
        }
    }   // end of $live IF-ELSE
    return true;
}       // end of my_error_handler() definition.


set_error_handler('my_error_handler');       // sets PHPs default error handler to my defined function

// function purpose: to create form inputs, test to see if theyve been previously filled in, and return errors on improper form submission
// function arguments: name given to element, type of element [text, password, textarea], and array of errors.
function create_form_input($name, $type, $errors) {
    $value = false;
    if(isset($_POST[$name])) $value = $_POST[$name];                        // checks to see if name value was submitted, and if so, assigns it to $value.
    if($value && get_magic_quotes_gpc()) $value = stripslashes($value);    // strips extraneous slashes from $value, only if Magic Quotes is enabled.

//checking the input type:
if(($type=='text')||($type=='password')) {
    echo '<input type="'.$type.'" name="'.$name.'" id="'.$name.'"';     // shell of the input element, with type, name, and id.
    if($value) echo 'value="'.htmlspecialchars($value).'"';              // if value has already been submitted, cleans value of special characters and puts it back in form.
    if(array_key_exists($name, $errors)) {                              // checks to see if there were any errors, and if so, will produce the proper error report from the error array
        echo 'class="error" /><span class="error">'.$errors[$name].'</span>';
    } else {
        echo ' />';
    }
} elseif($type=='textarea') {                                           // checks to see if element was a textarea instead
    if(array_key_exists($name, $errors)) echo '<span class="error">'.$errors[$name].'</span>';  // if there was an error in form submission, this will show error msg
    echo "<textarea name='$name' id='$name' class='$name' rows=5 cols=75";
    if(array_key_exists($name, $errors)) {
        echo ' class="error">';
    } else {
        echo '>';
    }
    if($value) echo $value;                 // checks to see if there was data for textarea submitted, and if there was, redisplays it. 
    echo '</textarea>';
}   //end of primary IF-ELSE      
}       //end of create_form_input()  

mysql.inc.php

<?php
/**     
 * Title:            mysql.inc.php
 * Created by:       M. David Kay III
 * Contact:          xxxxxxxxxx@gmail.com
 * Created:          August 20th, 2013
 * Last modified:    August 20th, 2013
 *
 * MySQL connection Configuration file does the following things:
 * 1 - Defines database connection constants
 * 2-  Creates the PDO database resource for connectivity used by scripts
 * 3 - Defines a hash compare function for passwords. 
 */
 /*
  *      Setting up the connection to the database that many scripts will be using.
  *      Defines multiple information for database connection, user, pw, host, & db name.
  */
DEFINE('DB_USER','user');        // constant variable for database user, to be changed when going to deployment
DEFINE('DB_PASSWORD','pass');    // constant variable for database user password, to be changed when going to deployment
DEFINE('DB_HOST','localhost');      // constant variable for database host, to be changed when going to deployment
DEFINE('DB_NAME','database');       // constant variable for database name, to be changed when going to deployment

$dsn = 'mysql:host=' . DB_HOST . ';dbname=' . DB_NAME . ';charset=utf8';
//we are implicilty selecting the utf8 charset for the database connection

$opt = array();
//optional parameters to be sent to the PDO object

//trying the database connection, if anything goes wrong, throw an Exception
//and alert the end User in a friendly way.
try {
    if(!($dbc = new PDO($dsn, DB_USER, DB_PASSWORD))){      // Creates the $dbc variable object so we can
                                                            // have a connection to the database.
                                                            // uses PDO functions.
    throw new Exception;
    }

}
catch (PDOException $e) {
    echo '<p>Could not connect to the database. Please contact the system administrator.</p>';
}

//function purpose: turn user-supplied pws into secure hashes to be passed to the DB and back
//function inputs: $password - the plain-text password entered into forms by the user.
function get_password_hash($password) {
    return hash_hmac('sha256', $password, 'ivegotalovelybunchofcoconuts-NOTUSED', true);
    //hash_hmac()'s 4 arguments: algorithm to use, data to be hashed, hash key, true = raw binary & false = hex characters
}  

register.php

<?php
/**     
 * Title:            register.php
 * Created by:       M. David Kay III
 * Contact:          xxxxxxxxxxx@gmail.com
 * Created:          August 19th, 2013
 * Last modified:    August 21th, 2013
 *
 * Register script does the following things:
 * 1 - Checks for submission by POST method
 * 2 - Checks each of the form inputs against a REGEX & validation
 *   a - If valid input, trim & store in short variable names
 *       i - Create hash value of password to store in database.
 *   b - If invalid input, add error to error array. 
 * 3 - Check email and username against database
 *   a - If neither are dupes, add user into database
 *   b - If email or username are dupes, let user know
 */
require('./includes/config.inc.php');
require(MYSQL);

$reg_errors = array();          // creates a variable array to catch all the errors that could be generated.

//checks to see if the form had already been submitted.
if($_SERVER['REQUEST_METHOD']=='POST') {
    //checks for a first name entry between 2-20 characters, case insensitive, allowing for a space, . , ' , or -.
    //if no entry for first name, error msg added to error array.
    if(preg_match('/^[A-Z\'.-]{2,20}$/i',$_POST['first_name']) && isset($_POST['first_name']) && is_string($_POST['first_name'])){
        $fn = trim($_POST['first_name']);
    } else {
        $reg_errors['first_name'] = 'Please enter your first name!';
    }

    //checks for a last name entry between 2-40 characters, case insensitive, allowing for a space, . , ' , or -.
    //if no entry for last name, error msg added to error array.
    if(preg_match('/^[A-Z\'.-]{2,40}$/i',$_POST['last_name']) && isset($_POST['K=last_name']) && is_string($_POST['last_name'])){
        $ln = trim($_POST['last_name']);
    } else {
        $reg_errors['last_name'] = 'Please enter your last name!';
    }

    //checks for a username entry between 5-30 characters, case insensitive, allowing for only numbers and letters.
    //if no entry for username, error msg added to error array.
    if(preg_match('/^[A-Z0-9]{5,30}$/i',$_POST['username']) && isset($_POST['username']) && is_string($_POST['username'])){
        $u = trim($_POST['username']);
    } else {
        $reg_errors['username'] = 'Please enter a desired user name!';
    }

    //checks for an email entry that conforms to email address syntax
    //if no entry for email, error msg added to error array.
    if(filter_var($_POST['email'], FILTER_VALIDATE_EMAIL) && isset($_POST['email']) && is_string($_POST['email'])) {
        $e = trim($_POST['email']);
    } else {
        $reg_errors['email'] = "Please enter a valid email address!";
    }

    //checks for a password entry that conforms to the requirements given on the form.
    //if no entry for password, error msg added to error array.
    //if the two passwords do not match, error msg added to error array.
    if(preg_match('/^(\w*(?=\w*\d)(?=\w*[a-z])(?=\w*[A-Z])\w*){6,20}$/',$_POST['pass1']) && isset($_POST['pass1']) && is_string($_POST['pass1'])){
        if($_POST['pass1']==$_POST['pass2']) {
            $p = get_password_hash(trim($_POST['pass1']));
            //hashes the password from plaintext for storage in database
        } else {
            $reg_errors['pass2'] = 'Your passwords did not match each other.';
        }
    } else {
        $reg_errors['pass1'] = "Please enter a valid password!";
    }

    //if no errors are present in array, make sure email and username arent in DB yet.
    if(empty($reg_errors)) {
        //prepared statement for checking if email or username is already registered
        try
        {
            $stmt = $dbc->prepare("SELECT email, username FROM `users` WHERE email = :email OR username = :username");
            $stmt->bindParam(':email', $e);             //email parameter
            $stmt->bindParam(':username', $u);          //username parameter
            $stmt->setFetchMode(PDO::FETCH_NUM);
            $stmt->execute();
            $count = $stmt->rowCount();
        }
        catch(PDOException $e)
        {
            echo 'Couldnt query database. ' . $e->getMessage();
        }

        if($count==0) {                                // no dupes of email or user, so let's add them into the DB!
            $stmt->closeCursor();                               //frees the database connection for a new statement to go through
            try {
                $stmt = $dbc->prepare("INSERT INTO `users` (`id`,`type`,`username`,`email`,`pass`,`first_name`,`last_name`,`date_created`) 
            VALUES(NULL,'member',?,?,?,?,?,ADDDATE(NOW())");
                $stmt->bindParam(1, $u);
                $stmt->bindParam(2, $p);
                $stmt->bindParam(3, $e);
                $stmt->bindParam(4, $fn);
                $stmt->bindParam(5, $ln);
                $stmt->setFetchMode(PDO::FETCH_NUM);
                $stmt->execute();
                $rows = $stmt->fetchAll();
            } catch (PDOException $e)
            {
                echo $e->getMessage();
            }


            //asks DB if a new row was created, and if so, thanks user for 
            //registration on the site & sends an email to their email.
            //if query doesnt work, an error is triggered
            if($rows==1) {
                include('./includes/header.inc.php');
                echo "<h3>Thanks!</h3><p>Thank you for registering! This is just
                    a mock up site at the time being, so your account isn't going 
                    to do much right now. Eventually, there will be an Authorize.net
                    payment processing routine going on here.</p>";


                $body = "Thanks for registering at PTRoster Mock Up. We greatly appreciate your interest in our services.\n\n";
                mail($_POST['email'],'Registration Confirmation for PTRoster Mock',$body,'From:thisone@thatone.org');
                include('./includes/footer.inc.php');
                exit(); 
            } else {
                echo "<p>Something is going on, trying to figure it out.</p>";
            }
        } else {
            trigger_error("You could not be registered due to a system error. We apologize for any inconvenience.");
        }
    } else {                    // both username and email might be already used in DB, and error msgs are generated for array.
        if($rows==2) {          // this checks to make sure both entries are dupes        
            $reg_errors['email'] = 'This email address has already been registered. If you have forgotten your password, use the link to the right to have your password sent to you.';
            $reg_errors['username'] = 'This username has already been registered. Please try another.';
        } else {                //this checks to see which of the two (email or username) is already in DB if both arent dupes.
            $row = $stmt->fetchAll();
            if(($row[0] == $_POST['email']) && ($row[1] == $_POST['username'])) {   //both match entries in DB
                $reg_errors['email'] = 'This email address has already been registered. If you have forgotten your password, use the link to the right to have your password sent to you.';
                $reg_errors['username'] = 'This username has already been registered with this email address. If you have forgotten your password, use the link to the right to have your password sent to you.';
            } elseif($row[0]==$_POST['email']) {        // email match
                    $reg_errors['email'] = 'This email address has already been registered. If you have forgotten your password, use the link to the right to have your password sent to you.';
            } elseif($row[1]==$_POST['username']) {     // username match
                $reg_errors['username'] = 'This username has already been registered. Please try another one.';
            }
        }       // end of $rows==2 ELSE
    }           // end of $rows == 0 IF
               //  end of empty($reg_errors) IF.
} else {                   // end of the main form submission conditional.
require('./includes/header.inc.php');
echo '<p>You did not fill out the form for registration. Please <a href="?page=join-now">go back</a> and fill out the form.</p>';
}      

?><h3>Register</h3>

<form action="register.php" method="post" accept-charset="utf-8" style="padding-left:100px;">
<p><label for="first_name"><strong>First Name</strong></label><br/><?php create_form_input('first_name', 'text', $reg_errors); ?></p>
<p><label for="last_name"><strong>Last Name</strong></label><br/><?php create_form_input('last_name', 'text', $reg_errors); ?></p>
<p><label for="username"><strong>Desired Username</strong></label><br/><?php create_form_input('username', 'text', $reg_errors); ?><small>Only letters and numbers are allowed.</small></p>
<p><label for="email"><strong>Email Address</strong></label><br/><?php create_form_input('email', 'text', $reg_errors); ?></p>
    <p><label for="pass1"><strong>Password</strong></label><br/><?php create_form_input('pass1', 'password', $reg_errors); ?><small>Must be between 6 and 20 characters long, with at least one lowercase letter, one uppercase letter, and one number.</small></p>
    <p><label for="pass2"><strong>Confirm Password</strong></label><br/><?php create_form_input('pass2', 'password', $reg_errors); ?></p>
    <input type="submit" name="submit_button" value="Next &rarr;" id="submit_button" class="formbutton" />
</form>
<?php
include('./includes/footer.inc.php');
?>
\$\endgroup\$
3
  • \$\begingroup\$ Do you view your error logs when you run this? It may be worth it to define variables you first define in try-catch blocks to dummy values before you enter the try{}. After the first try statement in if(empty($reg_errors)) the next try{} block where you call $stmt->closeCursor() could throw up in your logs if you caught PDOException in the previous block. And other variables like $count where you depend on them later. \$\endgroup\$ – asafreedman Aug 21 '13 at 23:02
  • \$\begingroup\$ @asafreedman I never knew that variables created in try-catch blocks were not in-scope for later use. Wow. Is there a clean way to make a PDO statement outside of try blocks, or rather just go ahead and treat each new try-catch block as it's own instantiation. Thank you for your comment. \$\endgroup\$ – alephalpha0 Aug 22 '13 at 4:55
  • \$\begingroup\$ It's not that they're not in-scope for later. It's that there's a change they wont be. If you catch an exception all the code that comes after that line, in the same try-catch block, that threw it will not be ran. If you've defined a variable in a try-catch that you want later you could see some odd behavior. It might be easier to just check for null if you think you'll run up against that situation in PHP. A java compiler would tell you at compile time but we don't get that luxury with php. \$\endgroup\$ – asafreedman Aug 22 '13 at 16:00
2
\$\begingroup\$

Good job using the database library with parameterized statements.

Your error messages need to be more detailed. For example, the user would not know what you consider to be a valid password.

Your register.php is long. Consider breaking it up into functions.

For error checking, try to consistently put the error-handling code right after the if.

if (OK != some_operation()) {
    // Handle error case
} else {
    // Handle normal case
}

One reason is that the error-handling code is usually shorter, so you can glance over it and get it out of the way. Also, the error-handling often involves bailing out early. Then, you can avoid nesting if-elses.

if (OK != some operation()) {
    return ERROR_CODE_1;
}
if (OK != another_operation()) {
    return ERROR_CODE_2;
}
...

Within the if-condition, it would make more sense to check

if (!(isset($var) && isstring($var) && fits_requirements($var))) {
    // Handle error
}

than to check

if (!(fits_requirements($var) && isset($var) && isstring($var))) {
    // Handle error
}

since the former can take advantage of the short-circuit behaviour of &&.

\$\endgroup\$
1
  • \$\begingroup\$ I never considered the logic behavior of my conditionals, thanks for pointing out a way to better "arm" them. I'll be breaking down the sanitizing and validation into separate functions. Thanks for the tips! \$\endgroup\$ – alephalpha0 Aug 26 '13 at 14:44

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.