2

I am converting a sql query to prepared statement to protect it from sql injection.

I am trying to use the information from this question

I do not get any errors but no data is received. I even console.log(html);

Originally I was using this (which works well.)

<?php

$db_host = "localhost";
$db_user = "";
$db_pass = "";
$db_name = "";

try
{
     $DB_con = new PDO("mysql:host={$db_host};dbname={$db_name}",$db_user,$db_pass);
     $DB_con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch(PDOException $exception)
{
     echo $exception->getMessage();
}
?>

$limit  = (intval($_GET['limit']) != 0) ? $_GET['limit'] : 5;
$offset = (intval($_GET['offset']) != 0) ? $_GET['offset'] : 0;

$sql = "SELECT * FROM wuno_inventory WHERE 1 ORDER BY id ASC LIMIT $limit OFFSET $offset";
try {
    $stmt = $DB_con->prepare($sql);
    $stmt->execute();
    $results = $stmt->fetchAll();
}
catch (Exception $ex) {
    echo $ex->getMessage();
}
if (count($results) > 0) {
    foreach ($results as $res) {
        echo '<tr class="invent">';
        echo '<td>' . $res['wuno_product'] . '</td>';
        echo '<td>' . $res['wuno_alternates'] . '</td>';
        echo '<td>' . $res['wuno_description'] . '</td>';
        echo '<td>' . $res['wuno_onhand'] . '</td>';
        echo '<td>' . $res['wuno_condition'] . '</td>';
        echo '</tr>';
    }
}
?> 

And now that I am trying to make secure as suggested in the answer I referenced above I am doing this,

$limit  = (intval($_GET['limit']) != 0) ? $_GET['limit'] : 5;
$offset = (intval($_GET['offset']) != 0) ? $_GET['offset'] : 0;

$stmt = $DB_con->prepare("SELECT * FROM wuno_inventory WHERE 1 ORDER BY id ASC LIMIT :limit, :offset");
$stmt->bindValue(':limit', (int) trim($_GET['limit']), PDO::PARAM_INT);
$stmt->bindValue(':offset', (int) trim($_GET['offset']), PDO::PARAM_INT);
try {
    $stmt->execute();
    $results = $stmt->fetchAll();
}
catch (Exception $ex) {
    echo $ex->getMessage();
}

No data is received when I change the query to this. How can I bind limit and offset to remove it from the statement and make my query more secure.

I have also tried this,

 $stmt->bindParam(':limit', (int) trim($_GET['limit']), PDO::PARAM_INT);
 $stmt->bindParam(':offset', (int) trim($_GET['offset']),PDO::PARAM_INT);




<script type="text/javascript">
jQuery(document).ready(function($) {
var busy = true;
var limit = 5;
var offset = 0;
var itemID = $("#itemID").val();
var assetPath = "<?php echo $assetPath ?>";
var searchPath = "<?php echo $searchPath ?>"; 

function displayRecords(lim, off) {
  jQuery.ajax({
          type: "GET",
          async: false,
          url: assetPath,
          data: "limit=" + lim + "&offset=" + off,
          cache: false,
          beforeSend: function() {
            $("#loader_message").html("").hide();
            $('#loader_image').show();
          },
          success: function(html) {
console.log(html);
            $("#productResults").append(html);
            $('#loader_image').hide();
            if (html === null) {
             $("#loader_message").html('<button data-atr="nodata" class="btn btn-default" type="button">No more records.</button>').show();
            } else {
console.log(html);
             $("#loader_message").html('Loading... <img src="../wp-content/uploads/2016/02/loading.gif" alt="Loading" alt="Loading">').show();
            }
            window.busy = false;
          }
        });
}

(function($) {
$(document).ready(function() {
if (busy === true) {
  displayRecords(limit, offset);
  busy = false;
}
});
})( jQuery );



(function($) {
$(document).ready(function() {
$(window).scroll(function() {
          if ($(window).scrollTop() + $(window).height() > $("#productResults").height() && !busy) {
            offset = limit + offset;
         displayRecords(limit, offset);

          }
});
});
})( jQuery );
});
</script>

HTML

<table id="prods" class="display table center-table" width="100%" >
                <thead>  
                        <tr>  
                            <th>Product #</th>  
                            <th>Alternate #</th>  
                            <th>Description</th>  
                            <th>On Hand</th>  
                        <th>Condition</th>
                        </tr>  
                    </thead>  

                <tbody id="productResults"> 

                </tbody>

            </table>
Community
  • 1
  • 1
wuno
  • 9,547
  • 19
  • 96
  • 180

1 Answers1

1

Do all your variable manipulation prior to the bindValue calls. (Trim is probably unnecessary as you're casting to an int.)

try
{
     $DB_con = new PDO("mysql:host={$db_host};dbname={$db_name}",$db_user,$db_pass);
     $DB_con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch(PDOException $exception)
{
     echo $exception->getMessage();
}

$limit  = (intval($_GET['limit']) != 0) ? (int) $_GET['limit'] : 5;
$offset = (intval($_GET['offset']) != 0) ? (int) $_GET['offset'] : 0;


try {
    $stmt = $DB_con->prepare("SELECT * FROM wuno_inventory LIMIT :limit OFFSET :offset");
    $stmt->bindValue(':limit', $limit, PDO::PARAM_INT);
    $stmt->bindValue(':offset', $offset, PDO::PARAM_INT);
    $stmt->execute();
    $results = $stmt->fetchAll();
}
catch (Exception $ex) {
    echo $ex->getMessage();
}
if (count($results) > 0) {
    foreach ($results as $res) {
        echo '<tr class="invent">';
        echo '<td>' . $res['wuno_product'] . '</td>';
        echo '<td>' . $res['wuno_alternates'] . '</td>';
        echo '<td>' . $res['wuno_description'] . '</td>';
        echo '<td>' . $res['wuno_onhand'] . '</td>';
        echo '<td>' . $res['wuno_condition'] . '</td>';
        echo '</tr>';
    }
}
Matt
  • 5,315
  • 1
  • 30
  • 57
  • weird. I have it settup to load 5 rows. And on scroll loads more. After adding your answer the page does not load any data but it does load data if I scroll. – wuno Feb 19 '16 at 02:35
  • You'll have to post more code showing how your scrolling loader is working for me to troubleshoot. – Matt Feb 19 '16 at 02:36
  • Sure I am curious though why would it work on scroll and not on load because of us binding instead of putting the limit and offset in the query? – wuno Feb 19 '16 at 02:37
  • I added the ajax at the bottom of the question. – wuno Feb 19 '16 at 02:40
  • Is it a safe assumption that $assetPath is calling the PHP code in my answer? – Matt Feb 19 '16 at 02:42
  • Yes it is. Sorry I did not clarify – wuno Feb 19 '16 at 02:42
  • Your busy logic is keeping it from loading the initial results I think. Pull up the javascript console, go to network and XHR and see if the initial request is made or only after you scroll. You could also add a `console.log(html);` inside your success to see when it loads. – Matt Feb 19 '16 at 02:45
  • I used console.log on html. Nothing shows up till I scroll. – wuno Feb 19 '16 at 02:48
  • I do agree my logic is trash. I actually just asked a question on Stack about that. I do not understand how to handle things like this. I would really love to come out of this with that knowledge. These are the last two issues with my program and I am finished. – wuno Feb 19 '16 at 02:49
  • That proves that your logic for the initial load is incorrect. Can you move everything to a single document.ready check? – Matt Feb 19 '16 at 02:49
  • Well it is in a wordpress site. So I have to wrap everything. I can move it all real quick. Also remember it was working just fine if I leave my PHP the way it was with the limit and offset inside the sql statement. It is not till I started trying to bind it that these problems started. – wuno Feb 19 '16 at 02:51
  • That is very odd to me that it worked before. I would have expected the same problem before/after. – Matt Feb 19 '16 at 02:52
  • Ya i swear I can even revert back right now to be sure. But yes its worked till just now when I started trying to make things more secure. – wuno Feb 19 '16 at 02:52
  • Yes I just checked. When I revert back to the first code block in my question everything works like a charm. – wuno Feb 19 '16 at 02:53
  • Sorry, I'm out of ideas! I can't figure out why it would work before and not after. You must have more code, how does `$results` get converted to json? – Matt Feb 19 '16 at 02:55
  • It is not. I will show you the rest of the code right now. – wuno Feb 19 '16 at 03:01
  • Please look at the first code block. I updated it with all the code. At one point I was trying to retrieve json but I had trouble trying to get the data from the json array. So I am just using echo on the raw data and taking the whole response as html in the ajax function. – wuno Feb 19 '16 at 03:03
  • I added the HTML to the bottom – wuno Feb 19 '16 at 03:42
  • I'm at a loss, seems like it should be working. Nothing in the php error log? – Matt Feb 19 '16 at 03:43
  • This is all. But this is always there because I dont have mbstring PHP Startup: Unable to load dynamic library '/usr/local/lib/php/extensions/no-debug-non-zts-20121212/php_mbstring.dll' - /usr/local/lib/php/extensions/no-debug-non-zts-20121212/php_mbstring.dll: cannot open shared object file: No such file or directory in Unknown on line 0 – wuno Feb 19 '16 at 03:45
  • I just recreated your example locally without the PHP and it works fine. The HTML is appended on the first load. Can you post the full PHP file? – Matt Feb 19 '16 at 03:49
  • I added the connection file in place of the include for you. At the top. That is all the rest of the code for the file path. Is there really no other way to accomplish this same thing? For binding the variables to get them out of the statement? Could we not use ?, ? then add that to execute(array) – wuno Feb 19 '16 at 03:53
  • I've updated the code. This is working for me locally. – Matt Feb 19 '16 at 04:05
  • I moved the (int) cast to where you're setting the variable and I swapped the `:limit, :offset` to `LIMIT :limit OFFSET :offset` – Matt Feb 19 '16 at 04:13