1
\$\begingroup\$

Want to show you part of code that I wrote to process search request. User commit search by articles (codes) of products. The goal is to allow user write his search request to textarea element in any possible way: in a row, in a column or separated by any separator (comma, space, semicolon etc.).

<?php
include("db.php"); // Including connection to MySQL

if(!empty($_POST['codes'])){ // Checking user's input from textarea not empty

    $codes = $_POST['codes']; // Moving user's input to variable
    preg_match_all("/\d{6}/", $codes, $codes_array); // Taking all elements that matches regular expression and adding them to the array

    $codes_array = array_unique($codes_array[0]); // Removing duplicates from the array

    if(!empty($codes_array)){ // Checking array not empty

        $codes_list = ""; // Creating new variable

        foreach ($codes_array as $key => $value) { // Processing our array
            $codes_list .= $value . "|"; // Adding values and separator to previously created variable to use it in MySQL query
        }

        $codes_list = rtrim($codes_list, "|"); // Removing last separator from string
        $codes_add = " AND `code` REGEXP '^({$codes_list})$'"; // Creating new variable with part of MySQL query and our elements from the array
    } else {
        $codes_add = ""; // Creating empty variable if our array was empty
    }
}

$query = "SELECT * FROM `table` WHERE `status` = '0'" . $codes_add; // Generating MySQL query string
$result = mysqli_query($link, $query); // Making a request to MySQL
?>

My friend said that it contains vulnerabilities for SQL-injection because I do not filter user's input. But my logic based on a statement that user's input filters at the line with preg_match_all. I'm sure that nothing but elements that consist of 6 digits will be included to the array.

Anyway I want that someone who have an experience with PHP and vulnerabilities took a look at this code and share his opinion about potential risks of using it. Thank You.

\$\endgroup\$
  • \$\begingroup\$ I won't comment on the security of this, but you are really over-commenting your code. Not every line needs a comment, code should be self-explanatory, for example if(!empty($codes_array)){ // Checking array not empty it is pretty obvious reading the if statement that it checks for an empty array, what is the comment telling the reader exactly? Comments should be there for extraordinary explanation, not to map out your thought process, it adds a lot of noise to your code. \$\endgroup\$ – Ron Beyer Feb 4 at 16:06
  • 3
    \$\begingroup\$ "Code Tells You How, Comments Tell You Why" \$\endgroup\$ – BCdotWEB Feb 4 at 16:33
  • \$\begingroup\$ A note about empty(): The input of "0" is ignored (assumed as empty). For string it is better to use isset($_POST['codes']) && strlen($_POST['codes']) to allow the input of "0". \$\endgroup\$ – Wiimm Feb 10 at 9:46
1
\$\begingroup\$

Being as big an advocate of prepared statements as I am, I cannot find any way for the SQL injection here, so your code should be safe.

I could only do the usual routine of making this code less bloated:

$query = "SELECT * FROM `table` WHERE `status` = '0' ";

if(!empty($_POST['codes'])) {
    preg_match_all("/\d{6}/", $_POST['codes'], $codes_array);
    $codes_array = array_unique($codes_array[0]);
    $codes_list = implode(",", $codes_array);
    $codes_add = $codes_list ? " AND `code` IN ($codes_list)" : "";
    $query .= $codes_add;
}
$result = mysqli_query($link, $query);

basically, PHP already has a function to glue array values together called implode() and, based on my understanding of the SQL, I decided to change REGEXP for IN. If I am wrong, then you could simply change it back to regexp.

\$\endgroup\$
  • \$\begingroup\$ You might like to position the preg_match_all() call up in the conditional, just in case the user input only has invalid characters. \$\endgroup\$ – mickmackusa Mar 20 at 10:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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