Search code examples
phpsecuritypdosql-injection

How to create safe (avoid SQL injection) pagination using PHP and PDO and prepared statement?


I was searching for creating PDO pagination and I found this answer, which vulnerable for SQL injection.

I'm wondering about the easiest way to convert this code to be SQL injection safe.

Update #1 In my case I want to add optional where statement and order by based on the user input

My code is:

<?php
$limit = 2;
$order_by   = filter_input(INPUT_GET, 'order_by');
$order_dir  = filter_input(INPUT_GET, 'order_dir');
$query_research_str = filter_input(INPUT_GET, 'search_str');
$query = "SELECT * FROM kategori";
// If search string
if ($query_research_str) {
    //$dbmanager->where('user_name', '%' . $query_research_str . '%', 'like');
    $query = $query . "where user_name like %".$query_research_str."% ";
}
// If order direction option selected
if ($order_dir) {
    //$dbmanager->orderBy($order_by, $order_dir);
    $query = $query . " order By ".$order_by." ".$order_dir;
}

$s = $db->prepare($query);
$s->execute();
$total_results = $s->rowCount();
$total_pages = ceil($total_results/$limit);

if (!isset($_GET['page'])) {
    $page = 1;
} else{
    $page = $_GET['page'];
}



$starting_limit = ($page-1)*$limit;
$show  = $query." LIMIT $starting_limit, $limit";

$r = $db->prepare($show);
$r->execute();

while($res = $r->fetch(PDO::FETCH_ASSOC)):
?>
<h4><?php echo $res['id'];?></h4>
<p><?php echo $res['nama_kat'];?></p>
<hr>
<?php
endwhile;


for ($page=1; $page <= $total_pages ; $page++):?>

<a href='<?php echo "?page=$page"; ?>' class="links"><?php  echo $page; ?>
 </a>

<?php endfor; ?>

Solution

  • I have several comments.

    $order_by   = filter_input(INPUT_GET, 'order_by');
    $order_dir  = filter_input(INPUT_GET, 'order_dir');
    $query_research_str = filter_input(INPUT_GET, 'search_str');
    

    This doesn't make the variables safe. You have not specified any kind of filter as the third argument to filter_input. The docs say:

    If omitted, FILTER_DEFAULT will be used, which is equivalent to FILTER_UNSAFE_RAW. This will result in no filtering taking place by default.

    In other words, it's exactly as unsafe as using the raw GET variables:

    $order_by   = $_GET['order_by'];
    $order_dir  = $_GET['order_dir'];
    $query_research_str = $_GET['search_str'];
    

    You should use query parameters for values like your search_str, but you can't use query parameters for things that are not SQL values. Like column names and SQL keywords in an ORDER BY clause.

    So how to use these safely? Whitelisting.

    Make an array of columns in your kategori table that are legitimate choices for sorting:

    $columns = ['user_name', 'create_date'];
    

    Then if the input is in this array, it's okay to use. Otherwise use a default.

    $order_by = 'user_name';
    if (array_search($_GET['order_by'], $columns)) {
        $order_by = $_GET['order_by'];
    }
    
    $order_dir = 'ASC';
    if ($_GET['order_dir'] == 'DESC') {
        $order_dir = 'DESC';
    }
    

    This way the values can only be those you have pre-validated in your code. There's no SQL injection possible, because if some different value is input, it won't match any of the values your code allows, and therefore the input will be ignored in favor of your defaults.

    For the query research string, you should add it to an array of params, and pass that when you execute() the query. Don't interpolate unsafe variables directly into your SQL query string.

    $query = "SELECT * FROM kategori WHERE true";
    
    $params = [];
    if ($query_research_str) {
        $query .= " AND user_name LIKE ?";
        $params[] = "%{$query_research_str}%";
    }
    
    $query .= " ORDER BY `{$order_by}` {$order_dir}";
    
    $s = $db->prepare($query);
    $s->execute($params);
    

    I also would have comments about your inefficient paging code. Did you know that before you can call rowCount(), your script has to fetch all the results? So there's no need to run the query again with LIMIT and offset. You already have all the data, so go easy on your database server and just take a slice from the array of results:

    $rows = $s->fetchAll(PDO::FETCH_ASSOC);
    $total_rows = $s->rowCount();
    $page = (int) $_GET['page'] ?? 1;
    $offset = ($page-1) * $limit;
    $end = min($total_rows, $offset + $limit);
    
    for ($i = $offset; $i < $end; $i++) {
    ?>
    <h4><?php echo $row[$i]['id'];?></h4>
    <p><?php echo $row[$i]['nama_kat'];?></p>
    <hr>
    <?php
    }