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; 
 }
?>

• Your Common Sense: It looks safe but it's twice the size it should be. For example in your other question you were using fetchAll() but now for some reason you don't
• mickmackusa: You are intentionally looking for a literal % at the end of the second param? $param2."%"; And a separate question: what is the desired query if someone wants to search for the string 0?
• John Beasley: @YourCommonSense - Are you referring to another question I asked on StackExchange? In regards to not using fetchAll(), I wasn't thinking about it. I wrote the script using fetch() and I got back the necessary results.
• Peilonrayz: What does your code do? I get you're looking for vulnerabilities when interacting with SQL but a short description goes a long way. Additionally site policy is to be a short synopsis of your code, not what you want out of a review. Please edit your code to ammend these issues.
• mickmackusa: Is this only the target of an ajax call? You are not writing to the database. Do you need to use POST? Do you actually mean to use LIKE on both conditions in the WHERE?

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.

• mickmackusa: Maybe it would be best if you posted an answer to talk about the error reporting. I don't think I can predict your preferred handling. I think I'd like to see it. In my own project, I would only want a handling technique that would not damage the UX. This way you can advise about the setAttribute() placement as well -- which I fully agee with.
• mickmackusa: My bad -- semicolon inside of $response. And as YCS said, please move $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); to inside your include file.
• mickmackusa: @JohnBeasley you are correct. Fixed that too. Nah, don't bother posting on SO, it's my responsibility to fix my dodgy snippet.
• mickmackusa: I'll be awake for a few more minutes.
• mickmackusa: @JohnBeasley I've updated my answer.