#1
  1. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    May 2012
    Posts
    11
    Rep Power
    0

    Critique on first attempt


    This is my first attempt at a Registration Script. Where can I improve it.

    Thanks in advance,
    Rab

    PHP Code:
    // 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();
    ?>
  2. #2
  3. No Profile Picture
    I haz teh codez!
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2003
    Posts
    2,556
    Rep Power
    2338
    Where can I improve it.
    By throwing it all away and starting with this: How to program a basic but secure login system using PHP and MySQL .
    I ♥ ManiacDan & requinix

    This is a sig, and not necessarily a comment on the OP:
    Please don't be a help vampire!
  4. #3
  5. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Hi,

    yeah, that's probably the easiest solution.

    However, since you asked for code critique, some general notes:

    • Please read up on security. It's almost sad that you spend 15 lines just on removing all slashes. There's a reason why PHP has added them, you know? Of course this feature is generally terrible (which is why they've removed it), but circumventing it and leaving the input as it is opens your code to any SQL injection.
    • You need to get your PHP knowledge up to date. The old MySQL extension is long obsolete and will be removed in the long run. Modern alternatives are available since almost 10 years. The PHP developers have even put a big red warning on each page of the PHP manual, but obviously moving people away from this extension is as hard as killing the Internet Explorer 6 (thanks to loads of bad tutorials and old code still floating around on the inernet).
    • You've got lots and lots of repetitious code. Whenever you find yourself writing the same code bits over and over again, try to find a more intelligent approach (like defining everything in an array and then doing the repetitions in a loop).
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  6. #4
  7. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    May 2012
    Posts
    11
    Rep Power
    0
    Jacques1,

    Thanks for your reply. This form is all about me writing and getting familiar with PHP. I am VERY aware I have a lot to learn. I am looking into leaning PDO as we speak. The arrays you speak of were going to be my next conquest, lol.

    Rab

    Comments on this post

    • Jacques1 disagrees : Stop cross-posting, show some respect.
    • requinix agrees : counter
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    I see you've made it your habit to crosspost your threads everywhere and anywhere and let different people tell you the same things. That's extremely disrespectful.

    I've also seen that we already told you about PDO and MySQLi half a year ago. But probably you didn't even read it, because you were busy getting your questions into as much PHP forums as possible. How intelligent.

    Comments on this post

    • aeternus agrees : title is misleading
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  10. #6
  11. For POny!
    Devshed Newbie (0 - 499 posts)

    Join Date
    Apr 2012
    Location
    Amsterdam
    Posts
    416
    Rep Power
    115
    You might want to add [PHPNET="trim"]trim[/PHPNET] also on your $_POST values.

    ALso the first part of your code assume there is a $_POST value,
    PHP Code:
    $username=$_POST['username']; 
    that doesn't have to be true, potentially causing an error. You should add an [PHPNET="if"]if clause[/PHPNET] to make sure the form is submitted.

    this
    PHP Code:
    $username=$_POST['username'];
    //to
    $username stripslashes($username); 
    can be done faster*
    PHP Code:
    $username stripslashes($_POST['username']); 
    *pay attention though to the security sins sticky!!
    Last edited by aeternus; February 17th, 2013 at 03:21 PM.
  12. #7
  13. Jealous Moderator
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,303
    Rep Power
    9400
    Originally Posted by Jacques1
    I see you've made it your habit to crosspost your threads everywhere and anywhere and let different people tell you the same things. That's extremely disrespectful.
    Or perhaps he's trying to get as much input from as many people as possible. You may be offended that not everyone considers a single forum to be the best resource available but it's not a crime.
    If you don't feel like reiterating what others may have said elsewhere then all you have to do is not post.
  14. #8
  15. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Originally Posted by requinix
    If you don't feel like reiterating what others may have said elsewhere then all you have to do is not post.
    I would have, if I had known that this thread is in 10 other forums. I usually don't Google the question before replying to it. I only did after I had a look at his other threads and found out that he always cross-posts.

    What I find disrespectful is when people just dump their question everywhere without linking to the other forums. This means that most of the helpers will just waste their time, because what they write down (sometimes with a lot of effort) has already been said in some other forum.

    I don't like this "it's free, so I'll abuse it" mentality. Of course you might say that nobody forces us to be here and help others. But without people spending their free time on writing replies, you could close down this site.

    And, seriously, do you really believe that getting "input from as many people as possible" actually works? When you post your stuff in 10 forums, you're likely to get the same answers 10 times. And the 2 exceptional replies you'll probably miss, because you've forgotten what you've checked where.

    Cross-posting is rude and stupid. Don't do it.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  16. #9
  17. Jealous Moderator
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,303
    Rep Power
    9400
    Originally Posted by Jacques1
    And, seriously, do you really believe that getting "input from as many people as possible" actually works? When you post your stuff in 10 forums, you're likely to get the same answers 10 times. And the 2 exceptional replies you'll probably miss, because you've forgotten what you've checked where.
    * Here we mentioned Oreo's tutorial and your 6 sins
    * On php freaks they mentioned intval() and arrays
    * On dream.in.code they gave an example on how to use PDO
    * In my post elsewhere I said a bit about validating phone numbers and email addresses
    No one, two, or three places had all that.
  18. #10
  19. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    All three forums (the ones I know of) tell him that he needs to escape his values and get rid of the repetitious code. In fact, we (and probably the other forums as well) told him the exact same things 6 months ago.

    So, yeah, cross-posting seems to help him a lot.

    Might have been a better idea to focus on a single discussion and actually read the replies. But obviously there are different opinions on that ...
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".

IMN logo majestic logo threadwatch logo seochat tools logo