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\$

closed as off-topic by 200_success Feb 5 '16 at 21:09

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

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();
?>
\$\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.
\$\endgroup\$

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