Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection is flagging valid constructs as SQL injection risks when no such risk exists.

[cschwenz]

Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection should not trigger on either:

    my $sth = $dbh->prepare(q[
        UPDATE `my_table` SET
            `data` = ?
        WHERE
            `id` = ?
    ]);
    $sth->execute( $new_data, $id )
        or confess( "Update my_table failed. ($DBI::errstr)" );

or:

    $dbh->do(q[
        UPDATE `my_table` SET
            `data` = ?
        WHERE
            `id` = ?
    ], undef, $new_data, $id)
        or confess( "Update my_table failed. ($DBI::errstr)" );

The attached test file (which has an additional .txt to get around GitHub brain-damage) will pass when Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection correctly handles the above cases; said test file is released under the Artistic 2.0 license (https://opensource.org/licenses/Artistic-2.0).

test_PreventSQLInjection_flagging_valid_constructs.t.txt

[guillaumeaubert]

The plugin indeed catches "Update my_table failed. ($DBI::errstr)" as a potential risk due to the use of update as the leading word in this string, the double-quoting, and the use of an interpolated variable.

For the plugin to understand that it's not a SQL statement, the solution would be to determine if the token matched is in fact argument in a function call and what function calls are considered safe. The simplest I think would be to check for the parent of the current token in https://github.com/guillaumeaubert/Perl-Critic-Policy-ValuesAndExpressions-PreventSQLInjection/blob/master/lib/Perl/Critic/Policy/ValuesAndExpressions/PreventSQLInjection.pm#L636, and return early if the parent token is a function call listed in a configurable whitelist.

I would be happy to take a pull request if you're interested in implementing this, it's a good feature idea. Please note that all contributions to this module must be under the same terms as Perl 5 itself.

Thank you!