Does this PDO process look protected from SQL injection?

Question

Does this process look protected from SQL injection attacks?

Is there something I could possibly change to make it more protected?

<?php
  include("../include/database.php");

  if(isset($_POST['details']))
  {
    $values = $_POST['details'];

    $param1 = htmlspecialchars(trim($values['param1']));
    $param2 = htmlspecialchars(trim($values['param2']));

    $conditions = [];
    $parameters = []; 

    if (!empty($param1))
    {
        $conditions[] = 'COLUMN1 LIKE ?';
        $parameters[] = $param1."%";
    }
    if (!empty($param2))
    {
        $conditions[] = 'COLUMN2 = ?';
        $parameters[] = $param2."%";
    }

    try {
        $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $select = "SELECT COLUMN1, COLUMN2 FROM table";

        if ($conditions)
        {
            $select .= " WHERE ".implode(" AND ", $conditions);
        }          

        $stmt = $conn->prepare($select);
        $stmt->execute($parameters);

        $out = array();

        while ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
            $out[] = $row;
        }

        echo json_encode($out);
    }
    catch(PDOException $e)
    {
        echo "Error: " . $e->getMessage();
    }

    $conn = null; 
  }
?>

Answer 1

It looks like your script is part of a basic ajax searching technique.

• $_POST is typically used when writing to the database (or when there is a distinct reason that $_GET is unsuitable). Since you are merely SELECTing data, just use $_GET.
• if you are going to validate your incoming data, don't bother acquiring resources until after you have validated the incoming data and determined that it qualifies for a trip to the database. If you are going to default to SELECTing the whole table, then it doesn't matter.
• it looks like if params 1 and 2 are missing, you are happy to perform a full table SELECT. So why deny the full table SELECT if details isn't declared?
• empty() is greedy -- it is looking for any falsey value. Even the string 0 - which has a length of 1 - is deemed empty. Perhaps do a strlen() check instead.
• I don't think that trimming should be forced on the data if part of a search string -- maybe the user wants to include the space.
• don't htmlspecialchars() as an attempt to improve security. The prepared statement is going to protect you from string injections. This call should be used when printing to screen, not querying the db.
• I think you have a typo in your second WHERE condition in that you mean to use a second LIKE but you have used = and kept the % appended to the value.
• I recommend that you design your table names and column names as lowercase strings to differentiate them from MYSQL keywords.
• you are not performing an data manipulations on the result set in this layer, so it will be more direct to fetchAll()
• always provide a response string from this script; ideally every response should be json so that your response receiving script can be simpler.
• never show end users the raw error message. Give them a vague indication of an error and nothing more.
• As @YourCommonSense commented, you should move your $conn->setAttribute call your include file.
• Normal execution of your script will not be generating any errors. Catching the errors will prevent the logging of the errors. I recommend removing the try catch block. For continued researching, start here.

Code:

$conditions = [];
$parameters = [];   

foreach (['param1' => 'COLUMN1', 'param2' => 'COLUMN2'] as $param => $column) {
    if (isset($_GET['details'][$param]) && strlen($_GET['details'][$param])) {
        $conditions[] = $column . " LIKE ?";
        $parameters[] = $_GET['details'][$param] . "%";
    }
}

include("../include/database.php");
$select = "SELECT COLUMN1, COLUMN2 FROM table";
if ($conditions) {
    $select .= " WHERE " . implode(" AND ", $conditions);
}          
$stmt = $conn->prepare($select);
$stmt->execute($parameters);
echo json_encode($stmt->fetchAll(PDO::FETCH_ASSOC));

---

In your ajax call, do not tell declare the response type as html -- it's not html, it is json. In doing so, you won't need to parse it.