Sanitize variables on form submit

Question

i have this code and i would like to know if I'm sanitising my code correctly (user input) i am practicing my security coding for a system that i am working on and i would like to know if i am doing thing the right way.

If there is any thing that i can improve to get things more solid i would very much like to know about them.

UPDATE

after sum reading i have changed my connection to PDO and if i am understand currently i dont need to sanitize the query?

<?php
require_once 'app/helpers.php';
session_start();
$error = '';


if($_POST){


    
 $itemtype = filter_input(INPUT_POST, 'itemtype', FILTER_SANITIZE_STRING);
 $itemtype = trim($itemtype);
    $display = filter_input(INPUT_POST, 'itemdisplay', FILTER_SANITIZE_STRING);
    $display = trim($display);
    $brand = filter_input(INPUT_POST, 'brand', FILTER_SANITIZE_STRING);
$brand = trim($brand);
    $model = filter_input(INPUT_POST, 'model', FILTER_SANITIZE_STRING);
    $model = trim($model);
    $spec = filter_input(INPUT_POST, 'spec', FILTER_SANITIZE_STRING);
    $spec = trim($spec);
    $sn = filter_input(INPUT_POST, 'sn', FILTER_SANITIZE_STRING);
     $sn = trim($sn);
    $setname =  filter_input(INPUT_POST, 'setname', FILTER_SANITIZE_STRING);
    $setname = trim($setname);
    $itemstat =  filter_input(INPUT_POST, 'itemstat', FILTER_SANITIZE_STRING);
    $itemstat = trim($itemstat);



    if(empty($itemtype)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> תכניס את הפריט לקבוצה לא יפה! </div>';
    }elseif (empty($display)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> אם לא נציג ניתן לו שם איך יקחו אותו? </div>';
     }elseif (empty($brand)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> סליחה... מי יצר את הפריט? </div>';
    }elseif (empty($model)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> רגע...איזה דגם זה? </div>';
    }elseif (empty($spec)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> לא מגיע שתכתוב עליו כמה מילים? </div>';
    }elseif (empty($sn)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button> מספר סידורי זה הכי אחי (ושלא יהיה אותו דבר כמו של פריט אחר...לא נעים..) </div>';
     }elseif (empty($setname)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>  אני חייב להיות בזוגיות...מה שם הסט שלי? </div>';
   }elseif (empty($itemstat)){
        $error = '<div class="alert alert-danger alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>    לאחרונה סיימתי קשר רציני... מה הסטטוס שלי? </div>';
    }else{
        if(!empty(filter_input(INPUT_POST, 'name', FILTER_SANITIZE_STRING)) || !empty(filter_input(INPUT_POST, 'email', FILTER_SANITIZE_STRING)) || !empty($_FILES['file']['name'])) {

            $uploadedFile = '';
            if (!empty($_FILES["file"]["type"])) {
                $fileName = $_FILES['file']['name'];
                $valid_extensions = array("jpeg", "jpg", "png");
                $temporary = explode(".", $_FILES["file"]["name"]);
                $file_extension = end($temporary);
                if ((($_FILES["file"]["type"] == "image/jpg") || ($_FILES["file"]["type"] == "image/jpeg")) && in_array($file_extension, $valid_extensions)) {
                    $sourcePath = $_FILES['file']['tmp_name'];
                    $targetPath = "items-img/" . $fileName;
                    if (move_uploaded_file($sourcePath, $targetPath)) {
                        
                        $uploadedFile = $fileName ;
                       
                    }
                }
            }

        }
      $stm = $link -> prepare("INSERT INTO item (item_desc,display,brand,model,spec,sn,set_name,status,item_pic) VALUES ('$itemtype','$display','$brand','$model','$spec','$sn','$setname','$itemstat','$uploadedFile')");

$stm->execute(array('item_desc' => $itemtype , 'display' => $display ,'brand' => $brand ,'model' => $model ,'item_desc' => $itemtype ,'spec' => $spec ,
    'sn' => $sn ,'set_name' => $setname ,'status' => $itemstat ,'name' => $uploadedFile ));
 
        $error = '<div class="alert alert-success alert-dismissable">
     <button type="button" class="close" data-dismiss="alert" aria-hidden="true">×</button>    יש לנו פריט חדש! </div>';


}
}
?>


<div>
    <?= $error ?>

</div>

Please learn about SQL injection and prepared statement, your query is wide open — Jules R, Dec 19 '18 at 09:00
https://codereview.stackexchange.com is where this question should be posted — Isaac, Dec 19 '18 at 09:00
No problem. Posted an answer on here for you anyway to save you some time. I'd still suggest posting it on there though (after using prepared statements) so they give you more in depth feedback. Also changed some of the readability of your question so it should be easier for people to read:D — Isaac, Dec 19 '18 at 09:08

score 2 · Accepted Answer · answered Dec 19 '18 at 09:07

You're sanitising user input and escaping strings. This alone isn't secure. Take a look at using prepared statements. http://php.net/manual/en/mysqli.prepare.php

This means that you prepare the SQL statement, then bind your parameters to that statement, then it is executed and sent to your Dbms. A really good explanation of this (with pretty diagrams) is on the third answer down on this thread. How does a PreparedStatement avoid or prevent SQL injection?

Example code taken from first thread. Try to break it down and understand what it means. It's easy to follow and easy to implement.

function secured_signup($username,$password)
{    
$connection = new mysqli($dbhost,$dbusername,$dbpassword,$dbname);    
if ($connection->connect_error) 
die("Secured");

$prepared = $connection->prepare("INSERT INTO `users` ( `username` , `password`    ) VALUES ( ? , ? ) ; ");
if($prepared==false)
die("Secured");

$result=$prepared->bind_param("ss",$username,$password);
if($result==false)
die("Secured");

$result=$prepared->execute();    
if($result==false)
die("Secured");

$prepared->close();
$connection->close();    
}

Hopefully this helps you on this thread. But I'd still suggest posting it to code review on the stack exchange

@drorshalit If used correctly you don't need to sanitise input with PDO. Here's a good thread detailing why https://stackoverflow.com/questions/4364686/how-do-i-sanitize-input-with-pdo — Isaac, Dec 19 '18 at 12:15

Sanitize variables on form submit

1 Answers1