Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have written some code to simple add/mod/del to a table called person. I would like comments on where this code could be comprising on security. I have used the PDO library in php.

GetPerson.php ( use this to retrieve details of a person using ajax )

<?php


include '../classes/person.php';

include '../classes/avaaz_mysql_db.php';

include '../config.php';

try
{
    if(isset($_POST['id']))
    {
        $id = $_POST['id'];

        //clean variable //clean further
        settype($id, 'string');
        $id = strip_tags($id);

        //decode
        $id = base64_decode($id);

        //remove salt string
        $id = str_replace($config['salt_string'], '', $id);

        //clean again
        settype($id, 'integer');

        //get the person details and output as json
        $db = new avaaz_mysql_db($config['db_connection_string'],$config['db_user_name'],$config['db_password']);
        $row = $db->get_person($id);

        $arr_peron_details = $row[0];
        //clean data returned from db

        foreach ($arr_peron_details as $key => $value)
        {
            $arr_peron_details[$key] = htmlentities($value);
        }



        echo json_encode(array($arr_peron_details));
    }

}
catch (Exception $e)
{
    echo "Getting person details: ".$e->getMessage();
}

?>

Person.php ( class that represents a person)

<?php

    class person
    {
        private $id;
        private $first_name;
        private $last_name;
        private $country;
        private $city;
        private $address;
        private $email;

        public function set_id($tmp_id)
        {
                    //data type checking and validation happens here

            settype($tmp_id, "integer");

            if((!isset($tmp_id)) || $tmp_id==0)
            {
                throw new Exception('Incorrect person id');
            }

            $this->id = $tmp_id;
        }

        public function get_id()
        {
            return $this->id;
        }

        public function set_first_name($tmp_first_name)
        {
            //data type checking and validation happens here
            settype($tmp_first_name, "string");


            if((!isset($tmp_first_name)) || $tmp_first_name=='')
            {
                throw new Exception('First name cannot be empty');
            }

            $tmp_first_name = strip_tags($tmp_first_name);

            if(!$this->is_only_letters_and_spaces($tmp_first_name))
            {
                throw new Exception('Only letters and spaces allowed in first name');
            }


            $this->first_name = $tmp_first_name;
        }

        public function set_last_name($tmp_last_name)
        {
            //data type checking and validation happens here        
            settype($tmp_last_name, "string");

            if(!isset($tmp_last_name) || $tmp_last_name=='')
            {
                throw new Exception('Last name cannot be empty');
            }

            $tmp_last_name = strip_tags($tmp_last_name);

            if(!$this->is_only_letters_and_spaces($tmp_last_name))
            {
                throw new Exception('Only letters and spaces allowed in last name');
            }

            $this->last_name = $tmp_last_name;
        }

        public function set_country($tmp_country)
        {
            //data type checking and validation happens here
            settype($tmp_country, "string");

            if(!isset($tmp_country) || $tmp_country=='')
            {
                throw new Exception('Country cannot be empty');
            }

            $tmp_country = strip_tags($tmp_country);

            if(!$this->is_only_letters_and_spaces($tmp_country))
            {
                throw new Exception('Only letters and spaces allowed in country');
            }

            $this->country = $tmp_country;
        }

        public function set_city($tmp_city)
        {
            //data type checking and validation happens here
            settype($tmp_city, "string");

            if(!isset($tmp_city) || $tmp_city=='')
            {
                throw new Exception('City cannot be empty');
            }

            $tmp_city = strip_tags($tmp_city);

            if(!$this->is_only_letters_and_spaces($tmp_city))
            {
                throw new Exception('Only letters and spaces allowed in city');
            }

            $this->city = $tmp_city;
        }

        public function set_address($tmp_address)
        {
            //data type checking and validation happens here
            settype($tmp_address, "string");

            if(!isset($tmp_address) || $tmp_address=='')
            {
                throw new Exception('Address cannot be empty');
            }

            $tmp_address = strip_tags($tmp_address);

            $this->address = $tmp_address;
        }

        public function set_email($tmp_email)
        {
            //data type checking and validation happens here
            settype($tmp_email, "string");

            if(!isset($tmp_email) || $tmp_email=='')
            {
                throw new Exception('Email cannot be empty');
            }

            $tmp_email = strip_tags($tmp_email);

            //check if first name contains only letters 
            if(!$this->is_valid_email($tmp_email))
            {
                throw new Exception('Invalid email address');
            }
            $this->email = $tmp_email;
        }

        public function get_first_name()
        {
            return $this->first_name;
        }

        public function get_last_name()
        {
            return $this->last_name;
        }

        public function get_country()
        {
            return $this->country;
        }

        public function get_city()
        {
            return $this->city;
        }

        public function get_address()
        {
            return $this->address;
        }

        public function get_email()
        {
            return $this->email;
        }

        //can later be moved to another helper classs
        public function is_only_letters_and_spaces($string)
        {
            return preg_match('~^[a-zA-Z ]*$~', $string);
        }

        //can later be moved to another helper classs
        public function is_valid_email($email)
        {
            if(!filter_var($email, FILTER_VALIDATE_EMAIL))
            {
                return false;
            }
            else
            {
                return true;
            }
        }

    }


    ?>

avaaz_mysql_db.php ( database layer )

<?php

include_once('person.php');

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */
class avaaz_mysql_db
{
    //used to validate sory by variable
    private $fields = array('first_name','last_name','country','city','address','email');
    private $connection_string;
    private $user_name;
    private $password;

    public function avaaz_mysql_db($tmp_connection_string,$tmp_user_name,$tmp_password)
    {
        $this->connection_string = $tmp_connection_string;
        $this->user_name = $tmp_user_name;
        $this->password = $tmp_password;
    }

    public function add_person($person)
    {

        $first_name = '';
        $last_name = '';
        $country = '';
        $city = '';
        $address = '';
        $email = '';

        try
        {
            //check if a person with the same name already exists
            if($this->does_same_name_exist($person))
            {
                throw new Exception('Person with the same name already exists');
            }

            //check if a person with the same email already exists
            if($this->does_email_exist($person))
            {
                throw new Exception('Person with the same email already exists');
            }

            //prepare db and pdo statement
            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $stmt = $db->prepare("INSERT INTO persons (first_name, last_name, country, city, address, email ) VALUES (:first_name, :last_name, :country, :city, :address, :email )");
            $stmt->bindParam(':first_name', $first_name);
            $stmt->bindParam(':last_name', $last_name);
            $stmt->bindParam(':country', $country);
            $stmt->bindParam(':city', $city);
            $stmt->bindParam(':address', $address);
            $stmt->bindParam(':email', $email);

            // insert one row
            $first_name = $person->get_first_name();
            $last_name = $person->get_last_name();
            $country = $person->get_country();
            $city = $person->get_city();
            $address = $person->get_address();
            $email = $person->get_email();
            $stmt->execute();
        }
        catch(Exception $e)
        {
            throw new Exception($e->getMessage());
        }

    }

    public function edit_person($person)
    {
        $first_name = '';
        $last_name = '';
        $country = '';
        $city = '';
        $address = '';
        $email = '';


        //check if a person with the same name already exists
        try 
        {
            if($this->does_same_name_exist($person))
            {
                throw new Exception('Person with the same name already exists');
            }

            //check if a person with the same email already exists
            if($this->does_email_exist($person))
            {
                throw new Exception('Person with the same email already exists');
            }

            //db and pdo statements
            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $stmt = $db->prepare("update persons set first_name=:first_name, last_name=:last_name, country=:country, city=:city, address=:address, email=:email where id = :person_id");
            $stmt->bindParam(':first_name', $first_name);
            $stmt->bindParam(':last_name', $last_name);
            $stmt->bindParam(':country', $country);
            $stmt->bindParam(':city', $city);
            $stmt->bindParam(':address', $address);
            $stmt->bindParam(':email', $email);
            $stmt->bindParam(':person_id', $person_id);

            // insert one row
            $person_id = $person->get_id();
            $first_name = $person->get_first_name();
            $last_name = $person->get_last_name();
            $country = $person->get_country();
            $city = $person->get_city();
            $address = $person->get_address();
            $email = $person->get_email();

            $stmt->execute();
        }
        catch(Exception $e)
        {
            throw new Exception($e->getMessage());
        }


    }

    public function delete_person($person)
    {
        try 
        {
            //check if a person with the same first name and last name already exists
            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $id = $person->get_id();
            $statement = $db->prepare("delete from persons where id = :id");
            $statement->bindParam('id', $id);
            $statement->execute();
        }
        catch(Exception $e)
        {
            throw new Exception($e->getMessage());
        }

    }

    public function get_persons($limit = 10, $offset = 0,  $sort='first_name', $sort_type = 'asc')
    {
        //validate offet, limit, sort, sorttype , try catch should come here
        try
        {
            if($limit>100)
            {
                throw new Exception('Cannot return more than 100 records at a time');
            }
            if(!in_array($sort, $this->fields))
            {
                throw new Exception('Sort field is not in the database');
            }
            if($sort_type!='asc' && $sort_type!='desc')
            {
                throw new Exception('Incorrect sort type');
            }

            //prepare db and prepare statment to get people from the db

            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $statement = $db->prepare("select * from persons order by $sort $sort_type limit $offset,$limit ");
            $statement->execute();
            $rows = $statement->fetchAll();
            return $rows;
        }
        catch(Exception $e)
        {
            throw new Exception($e->getMessage());
        }
    }

    public function does_same_name_exist($person)
    {

        try 
        {
            //check if a person with the same first name and last name already exists
            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $first_name = $person->get_first_name();
            $last_name = $person->get_last_name();

            if($person->get_id()==0 || $person->get_id()=='') //check if any person exists with the same name
            {

                $statement = $db->prepare("select id from persons where first_name =  :first_name and last_name =  :last_name");
            }
            else //check if any person besides the one who's is specified exists with the same name
            {

                $statement = $db->prepare("select id from persons where first_name =  :first_name and last_name =  :last_name and id!=:id");
                $id =  $person->get_id();
                $statement->bindParam('id', $id);
            }

            $statement->bindParam('first_name', $first_name);
            $statement->bindParam('last_name', $last_name);


            $statement->execute();
            $rows = $statement->fetchAll();

            if(sizeof($rows)>=1)
            {
                return true;
            }
            else 
            {
                return false;
            }
        }
        catch (Exception $e)
        {
            throw new Exception($e->getMessage());echo $e->getMessage();
        }
    }

    public function does_email_exist($person )
    {
        //check if a person with the same first name and last name already exists
        try 
        {
            $db = new PDO($this->connection_string,$this->user_name,$this->password);

            if($person->get_id()==0 || $person->get_id()=='')    //check if any person exists with the same name
            {
                $statement = $db->prepare("select id from persons where email =  :person_email");
            }
            else  //check if any person besides the one who's is specified exists with the same name
            {
                $statement = $db->prepare("select id from persons where email =  :person_email and id!=:id");
                $id =  $person->get_id();
                $statement->bindParam('id', $id);
            }

            $email = $person->get_email();
            $statement->bindParam('person_email',$email);
            $statement->execute();
            $rows = $statement->fetchAll();

            if(sizeof($rows)>=1)
            {
                return true;
            }
            else 
            {
                return false;
            }
        }
        catch(Exception $e)
        {
            throw new Exception($e->getMessage());
        }
    }

    public function get_person($id)
    {
        //variable validation happens here
        settype($id,'integer');

        try
        {
            $db = new PDO($this->connection_string,$this->user_name,$this->password);
            $statement = $db->prepare("select * from persons where id= :id");
            $statement->bindParam('id', $id);

            $statement->execute();
            $row = $statement->fetchAll();

            if(sizeof($row)>=1)
            {
                return $row;
            }
            else
            {
                throw new Exception('Person with the id not found');
            }
        }
        catch(Exception $e)
        {
            throw new Exception("get_person: ".$e->getMessage());
        }


    }

}

?>

share|improve this question

1 Answer

up vote 5 down vote

It's been a while since I've done any major php code; so I may be a bit rusty.

GetPerson.php

  • $id validations
    This isn't bad really; mostly because the final state is integer. If the final state was a String, then you would have to add more strip_tags calls after base64_decode and str_repalce.

  • What is the purpose of the salt? 

    $id = str_replace($config['salt_string'], '', $id);

    You do not really check for its existence, or if it's valid. You simply remove it without looking back. A malicious user can more or less ignore it as well.

  • Is there a reason why the salt needs to be part of $_POST['id']?
    It would make validations simpler to have it as a standalone field. Then you could simply do,

    $id = $_POST['id'];
settype($id, 'integer');

  • In general can you elaborate more on the use of a salt here?
    Depending on your intended use, you may be able to improve your implementation. At a high-level, a salt is typically used when hashing a password. As much as possible it needs to be unique. Your implementation however does not appear to be using a unique salt.

  • Top-level catch block 

    catch (Exception $e)  
{  
    echo "Getting person details: ".$e->getMessage();  
}

    1. Exception details sent to user
      While it is convenient for troubleshooting, the exact error message can give too much information. This is especially true for DB errors.
      You code also uses exceptions to relay errors back to the user.

      To avoid disclosing too much data I suggest using a white-list approach.
      Create a new type of exception called BusinessRulesException that you explicitly catch and pass the error message back to the user.
      For the catch-all Exception block you can log the exception internally (log file perhaps), tell the user "an unexpected error occurred", and give some kind of unique reference so they can refer to the exact problem that you recorded internally for analysis.

    2. Unescaped output
      You should call htmlentities() on $e->getMessage(). When it comes to XSS you should code defensively; validating inbound and outbound data. Even for exceptions.

      If you take my above suggestion to use a BusinessRuleExcpeption, make sure you call htmlentities() there are well.

Person.php

  • Many of your set methods check for 0-length string before calling strip_tags.
    Theoretically it may be possible for a malicious user to submit some HTML tags, have them stripped out, and end up with a 0-length value which you try to disallow.

    I would rewrite this as,

    public function set_first_name($tmp_first_name)  
{  
    settype($tmp_first_name, "string");  
    $tmp_first_name = strip_tags($tmp_first_name);  
    if((!isset($tmp_first_name)) || $tmp_first_name=='')  
    {  
        throw new Exception('First name cannot be empty');  
    }  

    if(!$this->is_only_letters_and_spaces($tmp_first_name))  
    {  
        throw new Exception('Only letters and spaces allowed in first name');  
    }  
    $this->first_name = $tmp_first_name;  
}

    Note that I moved strip_tags to before the first if statement.
    This same problem exists for the following methods as well.

    • set_last_name($tmp_last_name)
    • set_country($tmp_country)
    • set_city($tmp_city)

    • set_address($tmp_address)

    • set_email($tmp_email) has the same problem, although is_valid_email will return false on a 0-length value. I would still move the strip_tags call as detailed above though.

  • The method is_only_letters_and_spaces will evaluate true for 0-length strings.

    If this is expected I would rename the method to is_only_letters_and_spaces_allow_empty_string()

    If this is not expected I would rewrite as,

    public function is_only_letters_and_spaces($string)
 {
    return preg_match('^[a-zA-Z ]+$', $string);
 }

    Note the change to the regex, replaced * (zero or more) with + (one or more).

    I also removed the ~ (tilde) delimiters. As you are not specifying any options, I find this easier to read. That's more of a preference though.

avaaz_mysql_db.php

  • Improper validation of $limit and $offset
    You do not properly validate $limit and $offset before their direct inclusion in the SQL.
    I would suggest rewriting this method as, 

    public function get_persons($limit = 10, $offset = 0, $sort='first_name', $sort_type = 'asc')
{
    try
    {
        settype($limit, 'integer'); // explicit cast removes non-int values
        settype($offset, 'integer'); // explicit cast removes non-int values
        settype($sort, 'string');
        settype($sort_type, 'string');

        if($limit <= 0 || $limit>100) 
        {
            $limit = 10;
        }
        if($offset < 0) {
            $offset = 0;
        }
        if(!in_array($sort, $this->fields))
        {
            $sort = 'first_name';
        }
        if($sort_type!='asc' && $sort_type!='desc')
        {
            $sort_type = 'asc';
        }

        $db = new PDO($this->connection_string,$this->user_name,$this->password);
        $statement = $db->prepare("select * from persons order by $sort $sort_type limit $offset,$limit ");
        $statement->execute();
        $rows = $statement->fetchAll();
        return $rows;
    }
    catch(Exception $e)
    {
        throw new Exception($e->getMessage());
    }
}

    Note that I explicitly cast $limit and $offset as integers and added some range validations as well.

    I also changed the validations to reset the value to its default, instead of throwing an exception.
    In your original code you employ a fail-fast methodology when handling incorrect parameters.
    In this scenario I find it easier/better to just reset the value, but this may not work for your specification or expected implementation.

  • bindParam() missing leading : (colon) on parameter

    $statement->bindParam('id', $id);

    should be rewritten as, 

    $statement->bindParam(':id', $id);

    Note the leading : (colon) in ':id'.

    While the original code does execute without issue, the documentation does not explicitly indicate that this is allowed. It would be better to always use a leading : (colon).

    Some of your methods follow this, while others do not. Affected functions are,

    • public function delete_person($person)
    • public function does_same_name_exist($person)
    • public function does_email_exist($person )
    • public function get_person($id)

  • Potential for Exception details sent to user
    It looks like you left an extra echo in the catch block of does_same_name_exist($person). 

    catch (Exception $e)
{
    throw new Exception($e->getMessage());echo $e->getMessage();
}

    You would never reach it currently because of a throw immediately prior, but I would still remove the echo none-the-less.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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