TaintedSql false positives for prepared PDO statements

[cgocast]

The PHP documentation for PDO::prepare() states

Also, calling PDO::prepare() and PDOStatement::execute() helps to prevent SQL injection attacks by eliminating the need to manually quote and escape the parameters.

Therefore, the following code sample should not raise TaintedSql:

<?php

function deleteBindedUserId(PDO $pdo): void {
    $userId = $_POST['userid'];
    $stmt = $pdo->prepare("delete from users where user_id = :userid");
    $stmt->bindParam(':user_id', $userId);
    $stmt->execute();
}

function deleteConcatenedUserId(PDO $pdo): void {
    $userId = $_POST['userid'];
    $stmt = $pdo->prepare("delete from users where user_id = " . $userId);
    $stmt->execute();
}

[psalm-github-bot]

Hey @cgocast, can you reproduce the issue on https://psalm.dev ?

[danog]

function deleteConcatenedUserId(PDO $pdo): void {
    $userId = $_POST['userid'];
    $stmt = $pdo->prepare("delete from users where user_id = " . $userId);
    $stmt->execute();
}

This code should very much raise a TaintedSql issue :)

The first one however shouldn't, correct.

[cgocast]

@danog Both code are very similar. I do not understand why the second one should raises a TaintedSql while the second should not. Please, can you give me a more detailed explanation ?

[danog]

Psalm is simply warning about the fact that using prepare() does not automagically escape the SQL you pass it: prepare() does not prevent SQL injection if you just concatenate a user-provided string to the query you pass it, you must pass it a prepared statement and separately specify the parameters to execute/bind them to avoid SQL injection.

[weirdan]

The first one however shouldn't, correct.

[cgocast]

This was fixed by PR #10048