current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

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?

share|improve this question

closed as off-topic by 200_success♦ Feb 5 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.
up vote 3 down vote accepted

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();
?>
share|improve this answer
up vote 1 down vote
  • 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.
share|improve this answer

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