Are the variables in this code properly escaped?

Question

I think I have properly escaped everything, but yet I'm wondering whether I did the right thing. I remember to read somewhere that we shouldn't filter the input, just the output. In my case, does it mean that I shouldn't use the function sanitize anywhere? And just stick to prepare statements and htmlscpecialchars?

<?php
    #Connection to the database
    function dbcon (){
        try{
            $db = new PDO('mysql:dbname=php_test;host=localhost','root','mysql');
        }
        catch (PDOException $e){
            echo $e->getMessage();
            exit();
        }
        return $db;
    }

    #Sanitize the input for preventing hacking attempts
    function sanitize($data) {
        $data = trim($data);
        $data = stripslashes($data);
        $data = htmlspecialchars($data);
        return $data;
    }

    #Get the list of countries from the DB
    function getCountries() {
        $db = dbcon();
        $query = "SELECT country FROM countries";
        $stmt = $db->prepare($query);
        $stmt->execute();
        $countries = "";
        while ($row = $stmt->fetch()) {
            $countries .= '<option value= "'.$row['country'].'">'.$row['country'].'</option>';
        }
        return $countries;
    }


    $name = $email = $password = $password2 = $country = "";
    $validForm = True;

    #If it's a submission, validate the form
    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        $db = dbcon();

        #Name validation
        $name = sanitize($_POST["name"]);
        if ((strlen($name) < 2) || (strlen($name) > 50)) {
            echo "<span style=\"color: #FF0000;\"> Name must have between 2 and 50 characters </span> <br>";
            $name = "";
            $validForm = False;
        }

        #Email validation
        $email = sanitize($_POST["email"]);
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            echo "<span style=\"color: #FF0000;\"> Check the format of the email </span> <br>";
            $email = "";
            $validForm = False;
        }
        else { #If it's a valid email, check whether or not it's already registered
            $query = "SELECT email FROM users;";
            $stmt = $db->prepare($query);
            $stmt->execute();
            $found = False;
            while (($row = $stmt->fetch()) and (!$found)) {
                if ($row["email"] == $email) {
                    $found = True;
                }
            }
            if ($found) {
                echo "<span style=\"color: #FF0000;\"> This email is already registered </span> <br>";
                $email = "";
                $validForm = False;
            }
        }

        #Password validation
        $password = sanitize($_POST["pass1"]);
        if ((strlen($password) < 6) || (strlen($password) > 20)) {
            echo "<span style=\"color: #FF0000;\"> Password must have between 6 and 20 characters </span> <br>";   
            $validForm = False;
        }
        else { #If it's a valid password, check whether or not both passwords match
            $password2 = sanitize($_POST["pass2"]);
            if ($password != $password2) {
                echo "<span style=\"color: #FF0000;\"> Passwords don't match </span> <br>";
                $validForm = False;
            }
            #If passwords match, hash the password
            else {
                $password = password_hash($password, PASSWORD_DEFAULT);
            }
        }

        #We don't need to validate country because it's retrieved from the DB, but we sanitize it just in case a hacker modified the POST using a proxy
        $country = sanitize($_POST["country"]);

        #All checks done, insert into DB and move to success.php
        if ($validForm) {
            $query = "INSERT INTO users VALUES(:name, :email, :password, :country);";
            $stmt = $db->prepare($query);
            $stmt->bindParam(':name', $name);
            $stmt->bindParam(':email', $email);
            $stmt->bindParam(':password', $password);
            $stmt->bindParam(':country', $country);
            $stmt->execute();
            header("Location: success.php");
        }
    }
?>
<html>
    <head>
    </head>
    <body>
        <!-- Submitting to this very file -->
        <form action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>" method="post">
            <table>
                <tr> <!-- Name -->
                    <td><label for="name">Name:</label></td>
                    <td><input type="text" name="name" value="<?php echo htmlspecialchars($name); ?>" required /></td> 
                    <td><span style="color: #FF0000;">*</span></td> 
                    <td>Between 2 and 50 characters</td>
                </tr>
                <tr> <!-- Email -->
                    <td><label for="email">Email:</label></td>
                    <td><input type="text" name="email" value="<?php echo htmlspecialchars($email); ?>" required/></td> 
                    <td><span style="color: #FF0000;">*</span></td>
                    <td>Must be a valid address</td>
                </tr>
                <tr> <!-- Password -->
                    <td><label for="pass1">Password:</label></td>
                    <td><input type="password" name="pass1" required/></td> 
                    <td><span style="color: #FF0000;">*</span> </td>
                    <td>Between 6 and 20 characters</td>
                </tr>
                <tr> <!-- Confirm password -->
                    <td><label for="pass2">Confirm password:</label></td>
                    <td><input type="password" name="pass2" required/></td>
                    <td><span style="color: #FF0000;">*</span></td>
                    <td>Must be the same as the password</td>
                </tr>
                <tr> <!-- Country -->
                    <td><label for="country">Country:</label></td>
                    <td><select name="country"> <?php echo getCountries(); ?></select></td>
                    <td><span style="color: #FF0000;">*</span></td> 
                </tr>
                <tr>
                    <td><input type="submit"></td>
                </tr>
            </table>
        </form>
    </body>
</html>

Setting a max length to a password is generally bad practise — Crecket, Sep 23 '15 at 11:13
possible duplicate with http://stackoverflow.com/questions/129677/whats-the-best-method-for-sanitizing-user-input-with-php — Awais Tahir, Sep 23 '15 at 11:13
@Crecket yes I know, but this was part of a test which had such requisite — , Sep 23 '15 at 11:13
@yzT Alright, just making sure ;) Abdulla has already given the correct answer btw. Prepare() and BindParam() takes care of sql injection — Crecket, Sep 23 '15 at 11:16
@Crecket according to his answer, I can get rid of the `sanitize` function. But what about `htmlspecialchars`? — , Sep 23 '15 at 11:20
In general you don't need to escape anything as Prepare() tells the server what you want to do before actually doing it. So if someone were to inject code and try to drop your table the server will see that the query is different from what you told the server you were gonna do it will simply not execute it. It doesn't hurt to escape data but its not required in your example — Crecket, Sep 23 '15 at 11:25

score 1 · Accepted Answer · edited May 23 '17 at 11:44

1

Its all done with PDO::prepare

Calling PDO::prepare() and PDOStatement::execute() for statements that will be issued multiple times with different parameter values optimizes the performance of your application by allowing the driver to negotiate client and/or server side caching of the query plan and meta information, and helps to prevent SQL injection attacks by eliminating the need to manually quote the parameters.

and there is anther alternate

PDO::quote

PDO::quote() places quotes around the input string (if required) and escapes special characters within the input string, using a quoting style appropriate to the underlying driver.

To know about XSS Injection Read this Answer too

edited May 23 '17 at 11:44

Community

1
1

answered Sep 23 '15 at 11:11

Abdulla Nilam

36,589
17
64
85

according to your answer, I understand I can get rid of the `sanitize` function, but still I must use the `htmlspecialchars` within the form echoes, right? – Sep 23 '15 at 11:26
@yzT htmlspecialchars turns – Crecket Sep 23 '15 at 11:28

score 1 · Answer 2 · answered Sep 23 '15 at 11:26

1

One thing I always use when cleaning data entered by the user is to use strip_tags()... Specially if the content is to be echoed at some point... you dont want to make easy to inject scripts using your forms. Try to type <script>alert("hello");</scirpt> in one of the text element and submit...

answered Sep 23 '15 at 11:26

Julio Soares

1,200
8
11

using strip_tags() isn't bad or anything. Its not sufficient on its own but it definitely stop people from adding simple stuff like – Crecket Sep 23 '15 at 11:33
@Crecket I didn't say it isn't bad, just unnecessary if I'm using `htmlspecialchars` already – Sep 23 '15 at 11:42
I prefer to use strip_tags, stripslashes and mysql_real_escape_string instead of htmlspecialchars on things I receive from the form and are going to be stored in the DB because I might want to use the content in other ways that not html so i'd like to keep it as 'texty' as possible. I always use it when sending anything from DB or forms to the screen. – Julio Soares Sep 23 '15 at 17:21

Are the variables in this code properly escaped?

2 Answers2