SitePoint Sponsor

User Tag List

Results 1 to 10 of 10
  1. #1
    SitePoint Evangelist sp0om's Avatar
    Join Date
    Feb 2004
    Location
    MN
    Posts
    408
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Critique My Form Validation/Security Code

    Over the past couple of days, I've been adding security features to the forms on a site I'm building. Below is the validation process for $username. Does it look complete? Are there any holes? Should I add anything else?

    PHP Code:
    //make_safe function defined to prevent SQL injection
        
    function make_safe($variable) {
         
    $variable addslashes(trim(mysql_escape_string($variable)));
          return 
    $variable;
        }

    //make_safe function applied to $username
        
    $username =  make_safe($_POST['username']);

    //runs checks to ensure form is filled with only letters and numbers
        
    if((!$username) || (!eregi("^[a-z0-9]+$"$username)) {

            if(!
    $username){
                   echo 
    "Username is a required field <br>";
            }

            if(!
    eregi("^[a-z0-9]+$"$username)){
            echo 
    "Only letters and numbers are allowed in the username field<br>";
            }

        } else { 

    //validation complete
        
    LOGIN CODE 
    Feedback of any kind would be very much appreciated. Thanks.

  2. #2
    Wadge! F4nat1c's Avatar
    Join Date
    Oct 2005
    Location
    South Wales, UK
    Posts
    1,134
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    PHP Code:
    $variable addslashes(trim(mysql_escape_string($variable))); 
    Is that really nesessary?

    Maybe you could limit the amount of characters that the user is allowed to put in. Maybe minimum of 6, and maximum of 15/20.

    But yeah that's a very good piece of validation.
    OMFG SitePoint ROXORZ TEH BIG ONE111!
    Wish you were invisible?

  3. #3
    if ($zee == "Guru") { $zee--;}
    Join Date
    Nov 2005
    Location
    Karachi - Pakistan
    Posts
    1,133
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hi
    Seems good. U can further improve it by telling REGEXP the Maximum and Minimum numbers of Letters/Digits. like if u add ^[a-z0-9]{8}+$ this will only accept 8 letters/digits ^[a-z0-9]{8,20}+$ this will accept anything between 8 to 20 letter/digits. Hope this will add some more flexibility.

  4. #4
    SitePoint Addict devil cat's Avatar
    Join Date
    Apr 2003
    Location
    Reno
    Posts
    344
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    PHP Code:
     $variable addslashes(trim(mysql_escape_string($variable))); 
    This is a bit redundant. addslashes is the less protective version of mysql_escape_string. If you drop addslashes, you will still be okay.

  5. #5
    Wadge! F4nat1c's Avatar
    Join Date
    Oct 2005
    Location
    South Wales, UK
    Posts
    1,134
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by devil cat
    PHP Code:
     $variable addslashes(trim(mysql_escape_string($variable))); 
    This is a bit redundant. addslashes is the less protective version of mysql_escape_string. If you drop addslashes, you will still be okay.
    That's the point i tried to make in my original post .

    Quote Originally Posted by F4nat1c
    PHP Code:
    $variable addslashes(trim(mysql_escape_string($variable))); 
    Is that really nesessary?
    OMFG SitePoint ROXORZ TEH BIG ONE111!
    Wish you were invisible?

  6. #6
    SitePoint Addict devil cat's Avatar
    Join Date
    Apr 2003
    Location
    Reno
    Posts
    344
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I have a habit of not being very specific, and I'm trying to break it. That's all.

  7. #7
    if ($zee == "Guru") { $zee--;}
    Join Date
    Nov 2005
    Location
    Karachi - Pakistan
    Posts
    1,133
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I think when u have DEFINED a REGEX Pattern for Alphabets and Digits only then there is no need to use Add slashes.

  8. #8
    SitePoint Wizard
    Join Date
    Jan 2004
    Location
    3rd rock from the sun
    Posts
    1,005
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I think his point was :
    to use make_safe() to frisk the variable - no matter what you were going to do with it next.

    Then, at the script level, make sure it fits the pattern you expect.

    But wont addslashes screw the regex though?

    If the make_safe() code is sql specific shouldn't it be used after the regex?
    isempty()

  9. #9
    Wadge! F4nat1c's Avatar
    Join Date
    Oct 2005
    Location
    South Wales, UK
    Posts
    1,134
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by paulyG
    I think his point was :
    to use make_safe() to frisk the variable - no matter what you were going to do with it next.

    Then, at the script level, make sure it fits the pattern you expect.

    But wont addslashes screw the regex though?

    If the make_safe() code is sql specific shouldn't it be used after the regex?
    Good point. The code may do a better purpose if written as:

    PHP Code:
    //make_safe function defined to prevent SQL injection
        
    function make_safe($variable) {
        
    $variable trim(mysql_escape_string($variable));
          return 
    $variable;
        }

    //runs checks to ensure form is filled with only letters and numbers
        
    if((!$username) || (!eregi("^[a-z0-9]+$"$username)) {

            if(!
    $username){
                   echo 
    "Username is a required field <br>";
            }

            if(!
    eregi("^[a-z0-9]+$"$username)){
            echo 
    "Only letters and numbers are allowed in the username field<br>";
            }

        } else {

    //make_safe function applied to $username
        
    $username =  make_safe($_POST['username']); {

    //validation complete
        
    LOGIN CODE 
    Or something along those lines. Besides, there's no point applying the function to the variable if it hasn't been set yet.
    OMFG SitePoint ROXORZ TEH BIG ONE111!
    Wish you were invisible?

  10. #10
    SitePoint Zealot
    Join Date
    Aug 2005
    Location
    South Africa
    Posts
    185
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I would rather use the PCRE functions as I feel they are more efficient. You can also use something like the following approach to avoid doing the eregi twice,

    PHP Code:
    $clean = array();
    $errors = array();

    if ( isset(
    $_POST['username']) ) {
        if ( 
    preg_match('/^[a-z0-9]+$/i'$username) ) {
            
    $clean['username'] = $username;
        } else {
            
    array_push($error'Username must be alpha numberic');
        }
    } else {
        
    array_push($error'Username is a required field');
    }

    if ( isset(
    $clean['username']) {
        
    // Do login stuff
    } else {
        foreach ( 
    $errors as $error ) {
            echo 
    "$error";
        }

    I would move/avoid the make_safe function until you get to the database abstraction layer.

    --
    lv


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •