Dynamic Product Page and SQL Injection Vulnerabilities

Question

New programmer here, I asked a similar question last night but thought I should make a new question with updated attributes.

The code below is based on an old tutorial that I have been modifying. When I run a test script off the mysql connection script it shows me that it does connect to the database and shows the one test table I created to set this up. Table was made with PHPmyadmin and has the Primary key as id.

The variables in the lower part of the document come back as undefined, I'm assuming because it is not validating or inserting the data from my table correctly? I've tried removing the while loop and seem to get the same result each time. Does anyone have any other ideas on this one?
Secondly I just recently found out about SQL Injection and have tried modifying the code in such a way to adhere to these sanitation protocols. Could some nice fellow inform me of anything I am doing incorrectly or something further I need to make the code more secure?

<?php
if (isset($_GET['id'])) {
    include 'storescripts/mysqli.php';
    $id = preg_replace('#[^0-9]#i', '', $_GET['id']); 
    $query = $mysqli->prepare("SELECT * FROM products WHERE id='?' LIMIT 1");
    $query->bind_param("s", $id);
    $query->execute();
    $sql = $query->get_result()->fetch_assoc();
    $productCount = mysqli_num_rows($sql);
    if ($productCount > 0) {
        while($row = mysqli_fetch_array($sql)){ 
             $product_name = $row["product_name"];
             $price = $row["price"];
             $details = $row["details"];
             $category = $row["category"];
             $subcategory = $row["subcategory"];
         }
         
    } else {
        echo "That item does not exist.";
        exit();
    }
        
} else {
    echo "Data to render this page is missing.";
    exit();
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title><?php echo $product_name; ?></title>
<link rel="stylesheet" href="style/style.css" type="text/css" media="screen" />
</head>
<body>
<div align="center" id="mainWrapper">
  <?php include_once("template_header.php");?>
  <div id="pageContent">
  <table width="100%" border="0" cellspacing="0" cellpadding="15">
  <tr>
    <td width="19%" valign="top"><img src="inventory_images/<?php echo $id; ?>.jpg" width="142" height="188" alt="<?php echo $product_name; ?>" /><br />
      <a href="inventory_images/<?php echo $id; ?>.jpg">View Full Size Image</a></td>
    <td width="81%" valign="top"><h3><?php echo $product_name; ?></h3>
      <p><?php echo "$".$price; ?><br />
        <br />
        <?php echo "$subcategory $category"; ?> <br />
<br />
        <?php echo $details; ?>
<br />
        </p>
      <form id="form1" name="form1" method="post" action="cart.php">
        <input type="hidden" name="pid" id="pid" value="<?php echo $id; ?>" />
        <input type="submit" name="button" id="button" value="Add to Shopping Cart" />
      </form>
      </td>
    </tr>
</table>
  </div>
  <?php include_once("template_footer.php");?>
</div>
</body>
</html>

you really don't need a preg_replace for the id, but if you want nobody will hinder you. It is unclear what means variable in the lower part, this and that you didn't tell us the table layout nobody can help you — nbk, Oct 10 '20 at 00:12
thank you, i updated the post somewhat, let me know what else is needed if anything. Just starting on this stuff, lol. — Beau, Oct 10 '20 at 00:35
SELECT * is bad programming, as you should select columns that you really need. and still i don't get which variable you mean one of the $row? — nbk, Oct 10 '20 at 00:38
the quotes in the SQL do not mean something like "here is my value" but have certain meaning of defining a string. There are no strings in your query, hence there must be no quotes either — Your Common Sense, Oct 10 '20 at 00:55
@npk - it's supposed to grab the data from all columns and dynamically render a product page in the html section below, but I cannot for the life of me get the variables in the output to work. I think it has something to do with translating the primary key to the php to populate the variables. — Beau, Oct 10 '20 at 02:45

Bill Karwin · Answer 1 · 2020-10-10T17:11:36.430

The problem is here:

SELECT * FROM products WHERE id='?' LIMIT 1

You must not put the parameter placeholder inside quotes. It should be like this:

SELECT * FROM products WHERE id=? LIMIT 1

Parameter placeholders don't need to be in quotes, even if the data type of your parameter is a string. If a ? inside quotes were a parameter placeholder, then how would you ever write an SQL query that used a literal question mark as part of some text? :-)

Your code to fetch the results is a little bit confusing. Here's another way to write it:

$query->execute();
$result = $query->get_result();
$row = $result->fetch_assoc();
if ($result->num_rows > 0) {
    $product_name = $row["product_name"];
    $price = $row["price"];
    $details = $row["details"];
    $category = $row["category"];
    $subcategory = $row["subcategory"];
} else {
    echo "That item does not exist.";
    exit();
}

Your query will only return 0 or 1 rows, so you only need to fetch once. No need to use a while loop to fetch more.

You do need to fetch at least once for num_rows to return more than 0, because it doesn't know if there are any rows to fetch until you have tried to fetch at least one. But you don't need a new variable for that, because it's already available in the num_rows variable.

Besides that issue, I have another minor recommendation:

Instead of this:

$id = preg_replace('#[^0-9]#i', '', $_GET['id']);

Use this:

$id = (int) $_GET['id'];

It's faster code, and it will also avoid mistakes if someone inputs a string like '123abc789'. Your code will return the value 123789, but is this correct? Casting using (int) will return only the leading numeric part, so the value would be 123.

If you're trying to parse numbers that might have thousands separators, I'd suggest using https://www.php.net/manual/en/numberformatter.parse.php

Dynamic Product Page and SQL Injection Vulnerabilities

1 Answers1