2
\$\begingroup\$

I'm working on implementing a pagination system with search functionality using PHP and PDO. The code I've written seems to work, but I want to ensure that I'm following best practices and avoiding potential security vulnerabilities like SQL injection.

The goal of the script is to retrieve user records from a database based on various search criteria and display the results in a paginated manner. I'd appreciate it if someone could review my code and provide suggestions for improvement.

Context: I've shared a part of my PHP code that handles pagination and search. The code constructs SQL queries based on user input and utilizes PDO prepared statements to mitigate SQL injection risks.

Specific Concerns:

Are there any security vulnerabilities in my code that I should be aware of? Am I properly using prepared statements and binding values to prevent SQL injection? Could you suggest any improvements to make the code more readable or efficient? Are there any potential performance issues that I should address? How can I ensure that my pagination system works correctly and is scalable for a large number of records? I would greatly appreciate any feedback or guidance on how to improve my code to follow best practices and enhance the overall quality of my application.

Thank you in advance for your help!

 <?
if ($_SERVER["REQUEST_METHOD"] == "POST") {
  $serializedData = http_build_query($_POST);
  parse_str($serializedData, $serializedDataParsed);
  $searchUsername = $serializedDataParsed['username'];
  $searchE_mail = $serializedDataParsed['e_mail'];
  $searchMob_tel = $serializedDataParsed['mob_tel'];
  $searchStatusName = $serializedDataParsed['statusName'];
  $searchDate = $serializedDataParsed['date'];
}
if ($_SERVER["REQUEST_METHOD"] == "GET") {
  $excludedParameters = ['page'];
  $filteredGET = array_diff_key($_GET, array_flip($excludedParameters));
  $serializedData = http_build_query($filteredGET);

  $searchUsername = $_GET['username'];
  $searchE_mail = $_GET['e_mail'];
  $searchMob_tel = $_GET['mob_tel'];
  $searchStatusName = $_GET['statusName'];
  $searchDate = $_GET['date'];
}

  if(!$class->isNullOrEmpty($searchUsername)){
    $search .= " and username like :username";
  }
  if(!$class->isNullOrEmpty($searchE_mail)){
    $search .= " and e_mail like :e_mail";
  }
  if(!$class->isNullOrEmpty($searchMob_tel)){
    $search .= " and mob_tel like :mob_tel";
  }
  if(!$class->isNullOrEmpty($searchStatusName)){
    $search .= " and statusName like :statusName";
  }
  if(!$class->isNullOrEmpty($searchDate)){
    $search .= " and date like :date";
  }

  $queryForCount = "SELECT Count(*) FROM (SELECT u.id,u.active,u.username,u.name,u.e_mail,
  u.mob_tel ,s.name_".$lang." statusName,
  date_format(u.datetime, '%Y-%m-%d') date
  FROM ".tb."user u
  left join ".tb."user_status s on s.id = u.status) m
  where active in (1,2)".$search;

  $stmtCount = $pdo->prepare($queryForCount);

  if(!$class->isNullOrEmpty($searchUsername)){
    $stmtCount->bindValue(':username', '%'.$searchUsername.'%');
  }
  if(!$class->isNullOrEmpty($searchE_mail)){
    $stmtCount->bindValue(':e_mail', '%'.$searchE_mail.'%');
  }
  if(!$class->isNullOrEmpty($searchMob_tel)){
    $stmtCount->bindValue(':mob_tel', '%'.$searchMob_tel.'%');
  }
  if(!$class->isNullOrEmpty($searchStatusName)){
    $stmtCount->bindValue(':statusName', '%'.$searchStatusName.'%');
  }
  if(!$class->isNullOrEmpty($searchDate)){
    $stmtCount->bindValue(':date', '%'.$searchDate.'%');
  }
  

  $stmtCount->execute();

  $total = $stmtCount->fetchColumn();
  $pageCount = ceil($total / limit);
  
  $start = $get_page != 0 &&  $pageCount >= $get_page ? ($get_page-1) * limit : 0; 
  $query = "SELECT * FROM (SELECT u.id,u.active,u.username,u.name,u.e_mail,
  u.mob_tel ,s.name_".$lang." statusName,
  date_format(u.datetime, '%Y-%m-%d') date
  FROM ".tb."user u
  left join ".tb."user_status s on s.id = u.status ) m 
  where active in (1,2)".$search;

  $query .= " order by id desc";
  $query .= " limit $start, ".limit;

  $stmt = $pdo->prepare($query);

  if(!$class->isNullOrEmpty($searchUsername)){
    $stmt->bindValue(':username', '%'.$searchUsername.'%');
  }
  if(!$class->isNullOrEmpty($searchE_mail)){
    $stmt->bindValue(':e_mail', '%'.$searchE_mail.'%');
  }
  if(!$class->isNullOrEmpty($searchMob_tel)){
    $stmt->bindValue(':mob_tel', '%'.$searchMob_tel.'%');
  }
  if(!$class->isNullOrEmpty($searchStatusName)){
    $stmt->bindValue(':statusName', '%'.$searchStatusName.'%');
  }
  if(!$class->isNullOrEmpty($searchDate)){
    $stmt->bindValue(':date', '%'.$searchDate.'%');
  }

  $stmt->execute();
  $users = $stmt->fetchAll(PDO::FETCH_ASSOC);
?>
\$\endgroup\$
1
  • \$\begingroup\$ Can you please react to the answers you got? \$\endgroup\$ Aug 19, 2023 at 15:20

2 Answers 2

1
\$\begingroup\$

It's great that you stick with prepared statements, even if it required a much more elaborate code. Luckily, it can be easily improved to the amount that would have been needed with incorrect approach of adding values directly in the query.

The trick is using the great PDO's feature of adding values directly to execute() without explicit binding. Not only it will make just a single block of if's for a single query, but the same block could be used for both queries!

All you need is to create an array for values and add each value in the same if condition, like

$values = [];
if($searchUsername){
    $search .= " and username like :username";
    $values['username'] = "%$searchUsername%";
}
if($searchEmail)){
    $search .= " and e_mail like :e_mail";
    $values['e_mail'] = "%$searchEmail%";
}

and so on. And then, at the time of calling execute(), simply pass $values as a parameter! So it will be

$queryForCount = "...";
$stmt = $pdo->prepare($queryForCount);
$stmt->execute($values);
...
$query = "...";
$stmt = $pdo->prepare($query);
$stmt->execute($values);
$users = $stmt->fetchAll(PDO::FETCH_ASSOC);

Other points of improvement:

  • PHP tags

    • avoid using short open tags. It does no harm to write the full <?php tag, yet it makes your code more portable and adhere to the standard
    • avoid the closing tag if it's the last line in the file. will save you a couple headaches and also will make the code adhere to the standard.
  • not sure what does isNullOrEmpty() method do, but PHP already treats all non-empty values as true and empty values (such as null) as false. There are edge cases though, like 0 is treated as false

  • not sure what's the following code meant to be. It seems that you can replace it with just $serializedData = $_POST or just use $_POST already.

     $serializedData = http_build_query($_POST);
     parse_str($serializedData, $serializedDataParsed);
    
  • besides, you can use $_REQUEST if data can come through both POST and GET. Though only GET must be used for search

  • consider sticking with a single naming standard, either camelCase or snake_case

  • and using consistent indentation, that is - four spaces for each level

  • consider using variable interpolation, it is often less verbose, though it's entirely a matter of taste

The standard I am talking about: https://www.php-fig.org/psr/psr-12/

Are there any security vulnerabilities in my code that I should be aware of? Am I properly using prepared statements and binding values to prevent SQL injection?

You are doing everything right in this regard!

Are there any potential performance issues that I should address?

NOT in the PHP code. All the job is done by the DB here and thus you must address performance issues there. Given you are using LIKE, hardly anything can be done though.

How can I ensure that my pagination system works correctly and is scalable for a large number of records?

When it become large, consider adding an extrenal search engine such as Elastic or Manticore. For the current approach all you can do is to make sure that all search columns are added to the single compound index as to make sure that they are always present in RAM

Also, Katie's answer reminded me of of another suggestion:

Consider implementing another technique for pagination, which doesn't involve the predefined list of pages but just the "Next page" button. It will make the count query unnecessary, though this approach has some its own pitfalls. Like, you will have to include all already shown records' ids into a NOT IN() clause.

But again, such a faceted search is not a MySQL's strong suit, and for this, a dedicated search engine is generally used, and becomes rather obligatory when your data exceeds, say, a million records

\$\endgroup\$
0
\$\begingroup\$

Could you suggest any improvements to make the code more readable or efficient? Are there any potential performance issues that I should address? How can I ensure that my pagination system works correctly and is scalable for a large number of records?

You may indeed encounter scalability issues. But this is hard to tell without knowing table structure, and volume. Proper indexing can help optimize search - to a point. Learn to run an execution plan to find out the current cost of your queries.

Since you are using LIKE, so there is not much you can do here, and your users may search on fields that are not covered by any useful index anyway.

I encourage you to test your application under adverse conditions. You could easily fill up a test database with semi-random values by doing INSERTs in a loop. Then test your code on an inflated database and see if performance remains acceptable. Looking at this code, it seems evident that the more the table grows, the slower your code will be. But do you actually expect to have a lot of users in the table of users?

If you anticipate just a few hundreds of records in that table, then you probably shouldn't worry too much. If you're building the next Twitter, then you need to change your approach completely. It won't scale at all.

One problem is that you are repeating queries. First you get a count, then you run almost the same query again to get the rows. Doing this increases the load on your server, not to mention response time.

It would be more rational to save the results to a temporary table of some sort, or use some caching technique. And then do the count and the paging against that limited, prefetched resultset. Not the whole database.

You may have noticed some forums work just like this, like Vbulletin I think and others. There is a search ID in the URL, which refers to a snapshot of the search generated on the server just for you. It also means that changes in the database made after that search will not be reflected in the results, but this is usually not an issue. The user can still stop browsing the paged results and run a new search.

If your database grows a lot, or you have a lot of concurrent users your server could possibly crawl to a halt. Especially if you are on a cheap, shared hosting plan where resources are capped.

Again, this depends a lot on your usage scenario. If your tables remain small, the code should run fast enough even if it's not "efficient". In fact it's probably not worth indexing small tables at all, due to the way the query optimizer works, and introducing caching would not necessarily bring a noticeable improvement. On the other hand, you can understand why large forums cannot afford to run without caching results.

To sum, you can probably ignore all I said, but keep this in mind if your circumstances or usage needs change.

Also note that Mysql as some reserved keywords and date is one of them:

date_format(u.datetime, '%Y-%m-%d') date

active too:

  where active in (1,2)".$search;

Sometimes, using reserved keywords can cause arcane bugs or parsing errors that are not always easy to understand.

\$\endgroup\$
1
  • \$\begingroup\$ Actually, date and active are non-reserved keywords. \$\endgroup\$ Aug 11, 2023 at 14:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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