Converting from mysqli to prepared statements

Question

I am attempting to convert code from the mysqli_* statements to prepared statements to prevent SQL injection. The following code is what I am attempting to convert (and it currently works correctly):

$details = mysqli_query($linkDB,"SELECT * FROM ".PREFIX."Issues WHERE id='".$_POST['article']."' AND disabled='0' LIMIT 1");
$detail = mysqli_fetch_assoc($details);

Here is my attempt at converting to prepared statments. Any way to make this more concise would be appreciated (since I'm going from 2 lines of code to many):

$SQL = "SELECT * FROM ".PREFIX."Issues WHERE id='?' AND disabled='0' LIMIT 1";
$PRE = mysqli_stmt_init($linkDB);
//if (! $PRE = mysqli_prepare($linkDB, $SQL)) {   (alt attempt)
    if (! mysqli_stmt_prepare($PRE, $SQL)) {
        echo "<f><msg>ERROR: Could not prepare query: ".$SQL.", ".mysqli_error($linkDB)."</msg></f>";
    } else {
        mysqli_stmt_bind_param($PRE, "i", $test);
        $test = $_POST['article'];
        if (! mysqli_stmt_execute($PRE)) {
            echo "<f><msg>ERROR: Could not execute query: ".$SQL.", ".mysqli_error($linkDB)."</msg></f>";
        } else{
            $details = mysqli_stmt_get_result($PRE);
            $detail = mysqli_fetch_assoc($details);
            mysqli_stmt_close($PRE);
        }
}

The above code does not return/store db values in the $detail variable for future processing later in the script. I have tried commenting out the mysqli_stmt_close($PRE) call, but that makes no difference. I appreciate your help!

Dharman · Accepted Answer · 2019-07-23T13:44:35.480

2

The main mistake in your code was the typo with '?' in your query. If the ? is inside quotes it is not treated as a placeholder, but as a literal value.

When using MySQLi you should enable MySQLi exception mode. If you do then there is no longer any need to check the result of each function. You should also use OOP style as it is less verbose and you are less likely to make a silly mistake.

// put this line before you open the MySQLi connection
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

$stmt = $linkDB->prepare('SELECT * FROM '.PREFIX.'Issues WHERE id=? AND disabled=0 LIMIT 1');
$stmt->bind_param('i', $_POST['article']);
$stmt->execute();
$detail = $stmt->get_result()->fetch_assoc();

edited Jul 23 '19 at 13:44

answered Jul 22 '19 at 23:01

Dharman

30,962
25
85
135

I checked the referenced link but it says not to check for errors like "if ($result)". If I don't check for those and report back via email, how will the developers know what the problem is? If php just spits out the error, then that will be shown to the user making the site look unprofessional and our devs in the dark about what the problem is. – user1646428 Jul 23 '19 at 13:30
And it was just the quotes around the ?, thanks for the tip! Awaiting further clarification for the above question... – user1646428 Jul 23 '19 at 13:32
Wouldn't I still have to do a "if ($result)" though to catch it? – user1646428 Jul 23 '19 at 13:33
Dharman, @user1646428 you both are making a big mistake. PHP doesn't "spit" anything. It does exactly what a programmer would tell it to do. Want it on-screen? All right PHP will show it on-screen. Want it logged? No problem it will be logged. Want it emailed? the same. Want to show a custom error page to the user? You get the point: just tell PHP to do so. **Once** in a single place. And then change it if needed. While catching errors inplace will just bloat your code and make the error reporting inflexible. Check [PHP error reporting basics](https://phpdelusions.net/articles/error_reporting) – Your Common Sense Jul 23 '19 at 13:41
@YourCommonSense Yes you are right, maybe I misunderstood what OP wanted here... – Dharman Jul 23 '19 at 13:44
@user1646428 incidentally, it is **your code** "spits the error". So it will be shown to a site user but a developer would have no idea of it. That's exact point I am making here: just *raise* the error and then tell PHP where it should go, **instead of echoing it right away,** as you are doing right now. – Your Common Sense Jul 23 '19 at 13:47
YourCommonSense - great article in your referenced link! @Dharman, thanks for your sample code as that's what I took it as, not necessarily something to implement verbatim (hence my follow-up question about routing error messages that YCS solved). And yes, my OP code was just something that was attempting to be implemented, not the actual production code. I was trying to figure out what was wrong before modifying it accordingly. – user1646428 Jul 23 '19 at 14:02

score 0 · Answer 2 · answered Jul 22 '19 at 21:38

0

I would recommend PDO if you aren't an experienced coder. It's object oriented and fits a modern way of coding PHP.

In your config file you put:

$db = new PDO('mysql:host=localhost;dbname=test', $user, $pass);

And your script:

$statement = $pdo->prepare('SELECT * FROM '.PREFIX.'Issues WHERE id = :id AND disabled = 0 LIMIT 1';
$statement->execute(['id' => $_POST['article']);
$result = $statement->fetch(PDO::FETCH_ASSOC);

answered Jul 22 '19 at 21:38

GJ Nilsen

695
9
27

thanks for the reply. Unfortunately there are other portions of the project that are using procedural calls and not the object oriented style. Instead of re-writing all of that, I would just like to update to prepared statements right now. Any help with that would be appreciated! – user1646428 Jul 22 '19 at 21:53
And just to point out another link I came across for those using PDO https://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection?rq=1 – user1646428 Jul 23 '19 at 13:43

Converting from mysqli to prepared statements

2 Answers2

Linked