0
\$\begingroup\$

I have the following PHP code but I'm unsure, based on the many things I've read, whether or not this is actually safe from an SQL injection attack.

$mysqli = new mysqli("address.address.address", "username", "password", "database");

/* check connection */
if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

$stmt = $mysqli->prepare('INSERT INTO tablename (value) VALUES (?)');
$stmt->bind_param("s", $val);

$val = $_GET['val'];
$stmt->execute();


$stmt = $mysqli->prepare('SELECT * FROM tablename WHERE value = ?');
$stmt->bind_param('s', $val);

$val = $_GET['val'];

$stmt->execute();

$result = $stmt->get_result();
while ($row = $result->fetch_assoc()) {
    echo $row['id'].'<br />';
}

$stmt->close();

$mysqli->close();

Can anyone offer their opinion on how to improve this, please?

\$\endgroup\$
3
\$\begingroup\$

It looks safe from SQL Injection, your binding and preparing is fine.

This is how I would do it, using the PHP code found here in the manual.

In fact, these calls would benefit from being in their own functions; however this would work the same.

<?php
$mysqli = new mysqli("localhost", "my_user", "my_password", "world");

/* check connection */
if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

$val = $_GET['val'];

/* create a prepared statement */
if ($stmt = $mysqli->prepare("INSERT INTO tablename (value) VALUES (?)")) {

    /* bind parameters for markers */
    $stmt->bind_param("s", $val);

    /* execute query */
    if($stmt->execute()) {
       echo 'INSERTED';
    }
    /* close statement */
    $stmt->close();
}

if ($stmt = $mysqli->prepare("SELECT * FROM tablename WHERE value = ?")) {

    /* bind parameters for markers */
    $stmt->bind_param("s", $val);

    /* execute query */
    if($stmt->execute()) {
        $result = $stmt->get_result();
        while ($row = $result->fetch_assoc()) {
            echo $row['id'].'<br />';
        }
    }

    /* close statement */
    $stmt->close();
}

/* close connection */
$mysqli->close();
?>
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$
  • You avoid sql injection by using bind variables in your sql statements.
  • select * is not is not a good way to select data. Use the list of columns you want to select like select id. Quering more data than necessary may decrease performance and may decrease the security. Even this is currently no problem because you need all columns of the table this may change in the future when the table structure changes.
| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.