PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Sep 19, 2017 8:25 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 6 posts ] 
Author Message
 Post subject: a first attempt
PostPosted: Sun Feb 17, 2013 1:37 pm 
Offline
Forum Newbie

Joined: Thu May 24, 2012 12:14 pm
Posts: 6
This is my first attempt at a registration script, I am sure the list of problems is a long one.
Thanks in advance.

Rab

Syntax: [ Download ] [ Hide ]
// Get values from form
$username=$_POST['username'];
$password=$_POST['password'];
$confirm=$_POST['confirm'];
$firstname=$_POST['firstname'];
$lastname=$_POST['lastname'];
$email=$_POST['email'];
$address=$_POST['address'];
$address2=$_POST['address2'];
$city=$_POST['city'];
$state=$_POST['state'];
$zip=$_POST['zip'];
$phone=$_POST['phone'];
$dob=$_POST['dob'];

// Strip any escape characters
$username = stripslashes($username);
$password = stripslashes($password);
$confirm = stripslashes($confirm);
$firstname = stripslashes($firstname);
$lastname = stripslashes($lastname);
$email = stripslashes($email);
$address = stripslashes($address);
$address2 = stripslashes($address2);
$city = stripslashes($city);
$state = stripslashes($state);
$zip = stripslashes($zip);
$phone = stripslashes($phone);
$dob = stripslashes($dob);

//Check for empty fields
if(empty($_POST['username'])){
   echo (USERNAME_BLANK_ERROR);
}
if(empty($_POST['password'])){
   echo (PASSWORD_BLANK_ERROR);
}
if(empty($_POST['confirm'])){
   echo (CONFIRM_BLANK_ERROR);
}
if(empty($_POST['firstname'])){
   echo (FIRSTNAME_BLANK_ERROR);
}
if(empty($_POST['lastname'])){
   echo (LASTNAME_BLANK_ERROR);
}
if(empty($_POST['email'])){
   echo (EMAIL_BLANK_ERROR);
}
if(empty($_POST['address'])){
   echo (ADDRESS_BLANK_ERROR);
}
if(empty($_POST['city'])){
   echo (CITY_BLANK_ERROR);
}
if(empty($_POST['state'])){
   echo (STATE_BLANK_ERROR);
}
if(empty($_POST['zip'])){
   echo (ZIP_BLANK_ERROR);
}
if(empty($_POST['phone'])){
   echo (PHONE_BLANK_ERROR);
}
if(empty($_POST['dob'])){
   echo (DOB_BLANK_ERROR);
}

// Verify field lengths
if(strlen($username) < 6){
        echo (USERNAME_SHORT_ERROR);
}
if(strlen($username) > 15){
        echo (USERNAME_LONG_ERROR);
}
if(strlen($password) < 6){
        echo (PASSWORD_SHORT_ERROR);
}
if(strlen($password) > 15){
        echo (PASSWORD_LONG_ERROR);
}
if(strlen($firstname) < 2){
        echo (FIRSTNAME_SHORT_ERROR);
}
if(strlen($firstname) > 25){
        echo (FIRSTNAME_LONG_ERROR);
}
if(strlen($lastname) < 2){
        echo (LASTNAME_SHORT_ERROR);
}
if(strlen($laststname) > 25){
        echo (LASTNAME_LONG_ERROR);
}
if(strlen($email) < 10){
        echo (EMAIL_SHORT_ERROR);
}
if(strlen($email) > 100){
        echo (EMAIL_LONG_ERROR);
}
if(strlen($address) < 5){
        echo (ADDRESS_SHORT_ERROR);
}
if(strlen($address) > 200){
        echo (ADDRESS_LONG_ERROR);
}
if(strlen($city) < 3){
        echo (CITY_SHORT_ERROR);
}
if(strlen($city) > 22){
        echo (CITY_LONG_ERROR);
}
if(strlen($zip) < 5){
        echo (ZIP_SHORT_ERROR);
}
if(strlen($zip) > 5){
        echo (ZIP_LONG_ERROR);
}

// Compare Passwords
if ($password != $confirm) {
        echo(PASSWORD_MATCH_ERROR);
}

// Validate Phone Number
if( !preg_match("/^([1]-)?[0-9]{3}-[0-9]{3}-[0-9]{4}$/i", $phone) ) {
    echo (PHONE_VALIDATE_ERROR);
}

// Validate Email Address
if( !preg_match("/^[_a-z0-9-]+(.[_a-z0-9-]+)*@[a-z0-9-]+(.[a-z0-9-]+)*(.[a-z]{2,3})$/i", $email) ) {
    echo (EMAIL_VALIDATE_ERROR);
}

// Hash password and validate hash
$hash = $hasher->HashPassword($password);

if(strlen($hash) <= 20){
        echo (HASH_ERROR);
}

// Insert User into database
$sql="INSERT INTO $users(user_username, user_password, user_email, user_firstname, user_lastname, user_address, user_address2, user_city, user_state, user_zip, user_phone, user_dob)VALUES('$username', '$hash', '$firstname', '$lastname', '$address', '$address2', '$city', '$state', '$zip', '$phone', '$dob')";
$result=mysql_query($sql);

if($result){
echo (REGISTRATION_SUCCESS);
echo "<BR>";
echo "<a href='reg_thankyou.php'>Back to main page</a>";
}

else {
echo (REGISTRATION_ERROR);
}
?>
<?php
// close connection
mysql_close();
?>
 


Top
 Profile  
 
 Post subject: Re: a first attempt
PostPosted: Sun Feb 17, 2013 5:03 pm 
Offline
Spammer :|
User avatar

Joined: Wed Oct 15, 2008 2:35 am
Posts: 6545
Location: WA, USA
1. The checks that fields are not empty should come at the very beginning. If any of those fail then the rest of the code should not execute at all.
2. stripslashes() is not adequate for escaping data. See #5.
3. A better way to validate a phone number is to strip out all non-digits and validate length. You can then reformat it if you want.
4a. The email check is okay, but if you don't want to turn away uncommon email addresses then you should use a more generic regex.
4b. Optionally, functions like getmxrr and dns_get_record will allow you to check for a valid domain name. Keep in mind that if someone wants to put in a fake email address they can do that very easily and you can't really know for sure that it works (unless you actually try to email them).
5. The mysql extension and mysql_* functions are deprecated. You should not use them for any new code. Switch to something like PDO or mysqli; besides better performance overall, both offer prepared statements which mean you don't have to try to escape strings yourself.


Top
 Profile  
 
 Post subject: Re: a first attempt
PostPosted: Sun Feb 17, 2013 7:54 pm 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Some of these were already mentioned, I first opened this before the other post, just was doing other work in the middle of writing this.

The stripslashes should be done conditionally. What if you move to a server that doesn't use a "crutch" for poor programming. See http://www.php.net/manual/en/function.g ... es-gpc.php for example.

Kinda sloppy to go back and forth between using $_POST and the variables you already assigned those $_POST values. Keep it uniform.

IMO, it is best to always do all the "logic" before you output anything, keep that for last, you are echoing out errors in the middle.
Here is how I do it, I set up and array for errors ($aryErr), then for each error in your code, do something like
Syntax: [ Download ] [ Hide ]
$aryErr['username'] = USERNAME_BLANK_ERROR;

Then when you are all done, you can check count($aryErr) to see if there were any errors. If they were, in the appropriate place you can do something like:
Syntax: [ Download ] [ Hide ]
<p>
  Please check the following and try again:<br><br>
  <?php echo implode('<br>',$aryErr); ?>
</p>

Note, you do not need to give an index to the errors, I just use that in habit in case I'm doing a form where they want the error to display right above the input, then you can get them.

Also, combine your checks. Your code the way it is, if I leave Username blank, I will get BOTH the error USERNAME_BLANK_ERROR and USERNAME_SHORT_ERROR. Try something like:
Syntax: [ Download ] [ Hide ]
if (empty($username)) {
  $aryErr['username'] = USERNAME_BLANK_ERROR;
}
elseif (strlen($username)<6 || strlen($username)>16) {
  $aryErr['username'] = USERNAME_SIZE_ERROR;
  // something like "Username needs to be 6-16 characters long"
}

Technically, the Email To Short error, not needed, as your regex will force the minimum. I in fact know a few people where I used to work, that has 3 letter domain, where they use only 1 letter for e-mail, so they would fail this step even with a valid email.

For phone number, assuming from other fields that this is for US, you should also check for other items.
Area Code and Prefix cannot begin with 0
Area Code and Prefix cannot end in 11
Area Code and Prefix cannot be 555
There is nothing to allow for people who still enter their phone number as (330) 555-1212 [See, there is that dang 555... LOL]

You are allowing duplicates, not sure your requirements, but if you want unique, you should be also checking the database for them to already be used. For example, in the $username block I gave you above, add this to the end:
Syntax: [ Download ] [ Hide ]
else {
  // Database should already be established, assigned to $db
  $rs = mysqli_query($db,'SELECT `username` FROM `'.$users.'` WHERE `username`="'.mysqli_real_escape_string($db,$username).'" LIMIT 1');
  if ($rs && mysqli_num_rows($rs)>0) {
    $aryErr['username'] = USERNAME_ALREADY_EXISTS_ERROR;
  }
}

Speaking of mysql code, you should be migrating away from mysql_ based functions, as that set is depreciated, and will be left out of an upcoming version of PHP.

Also, for your insert statement, you need to be properly escaping the data (the way I just did it above, or using prepared statements via mysqli or PDO) else, you are wide open for SQL injection attacks.

If you are validating everything else, might as well do Date of Birth as well.

Also, for state, to make it easier on user (and keep uniform data in database) I always give them a drop down to choose from:
Syntax: [ Download ] [ Hide ]
$arySate = array('State'=>array(
  'AL'=>'Alabama','AK'=>'Alaska','AZ'=>'Arizona','AR'=>'Arkansas','CA'=>'California',
  'CO'=>'Colorado','CT'=>'Connecticut','DE'=>'Delaware','DC'=>'Dist. of Columbia','FL'=>'Florida',
  'GA'=>'Georgia','HI'=>'Hawaii','ID'=>'Idaho','IL'=>'Illinois','IN'=>'Indiana','IA'=>'Iowa',
  'KS'=>'Kansas','KY'=>'Kentucky','LA'=>'Louisiana','ME'=>'Maine','MD'=>'Maryland',
  'MA'=>'Massachusetts','MI'=>'Michigan','MN'=>'Minnesota','MS'=>'Mississippi','MO'=>'Missouri',
  'MT'=>'Montana','NE'=>'Nebraska','NV'=>'Nevada','NH'=>'New Hampshire','NJ'=>'New Jersey',
  'NM'=>'New Mexico','NY'=>'New York','NC'=>'North Carolina','ND'=>'North Dakota','OH'=>'Ohio',
  'OK'=>'Oklahoma','OR'=>'Oregon','PA'=>'Pennsylvania','RI'=>'Rhode Island','SC'=>'South Carolina',
  'SD'=>'South Dakota','TN'=>'Tennessee','TX'=>'Texas','UT'=>'Utah','VT'=>'Vermont','VA'=>'Virginia',
  'WA'=>'Washington','WV'=>'West Virginia','WI'=>'Wisconsin','WY'=>'Wyoming')
);

Then later, something such as:
Syntax: [ Download ] [ Hide ]
<label for="state">State: </label>
<select id="state" name="state">
  <?php foreach($aryState as $abbr=>$full): ?>
    <option value="<?php echo $abbr; ?>"><?php echo $full; ?></option>
  <?php endforeach; ?>
</select>

You are doing a check of the length of password's hash, and then echoing out an error. What exactly will the user be able to do to change that error? A good hash system you shouldn't need to check. Also it is advisable to have a SALT and PEPPER for the (SALT being per site, PEPPER being per user, usually something non-changing in the database (ie if you track "Created TimeStamp", or "Created IP"), or a random Pepper you generate every time they save a new password)

That is about it for now. Time to take wife to movies...

Oh yeah, last thing, I also use code to prevent emails from "Temporary" services, here is the code I currently use:
Syntax: [ Download ] [ Hide ]
list($user,$domain)=split('@',$aryData['Email']);
if (in_array($domain,array(
    'sharklasers.com','guerrillamailblock.com','guerrillamail.com','guerrillamail.net','guerrillamail.biz',
    'guerrillamail.ord','guerrillamail.de','disposableinbox.com','mailinator.com','fakeinbox.com',
    'tempemail.net','rppkn.com','anonymbox.com','mailmetrash.com'
  ))) {
    $aryErr['Email'] = 'Sorry, we do not allow &quot;Temporary&quot; Emails';
}


Hope this helps.

-Greg


Top
 Profile  
 
 Post subject: Re: a first attempt
PostPosted: Sun Feb 17, 2013 9:02 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13427
Location: New York, NY, US
We don't often see Fortran programmers coming to PHP! :drunk:

I agree with all the above, definitely do all of that. From a modularity point of view, you have an array to check $_POST, why not keep you field definition information in an array too. For example:
Syntax: [ Download ] [ Hide ]
$fields = array(
    'username' => array(
        'value' => '',
        'required' => true,
        'minlen' => 6,
        'maxlen' => 15,
        'regex' => '/[A-Za-z0-9]*/',
        ),
    'password' => array(
    );
 

It would allow you to have standard forms code and generate/validate the form from configuration data. Beyond that you an OO solution would have an array of field objects that could each have a chain of validators and filters.

_________________
(#10850)


Top
 Profile  
 
 Post subject: Re: a first attempt
PostPosted: Sun Feb 17, 2013 9:33 pm 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Just noticed a typo in the code i gave for the states, I accidently double wrapped the data as an array:

$aryState = array('State'=>array( ... ));

it needs to be

$aryState = array( ... );

Technically on my system, anything in a SELECT list are contained inside $aryList, and I didn't fully adjust the code over to a single array.


Top
 Profile  
 
 Post subject: Re: a first attempt
PostPosted: Wed Oct 23, 2013 1:54 am 
Offline
Forum Contributor
User avatar

Joined: Sun Nov 13, 2005 8:35 am
Posts: 212
Location: Folkestone, Kent, UK
To save time getting all the post variables at the begining you could try
Syntax: [ Download ] [ Hide ]
foreach ($_POST as $key => $value) {
                                $$key = stripslashes($value);
                                        }


this will automatically convert you $_POST['name'] to $name and stripslashes for you


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 6 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group