I am using PDO for the first time in my project. In my previous project someone suggested me to use PDO as my queries were wide open to inject. I am pasting a sample code of my project. Can you please let me know how safe my code and query is?

try {
    $pid = $_GET['pid'];
    $conn = new PDO("mysql:host=" . $DB_HOST . ";dbname=" . $DB_NAME, $DB_USER, $DB_PASS);
    $sql = "SELECT * FROM packages_to_be_shipped_on_bremail_address WHERE package_id=$pid";

    $q = $conn->prepare($sql);

    $q->bindColumn(1, $pid);
    $q->bindColumn(2, $custemail);
    $q->bindColumn(3, $shipcompany);
    $q->bindColumn(4, $deliverydate);
    $q->bindColumn(5, $trackingid);
    $q->bindColumn(6, $destaddress);
    $q->bindColumn(7, $destzip);
    $q->bindColumn(8, $status);
    $q->bindColumn(9, $rate);
    $q->bindColumn(10, $rates_sent_status);
    $q->bindColumn(11, $customer_response);

        echo "Shipping Company - ".$shipcompany."<br /><hr />";
        echo "Expected Delivery Date - ".$deliverydate."<br /><hr />";
catch(PDOException $e) {
    echo $e->getMessage();
  • 1
    \$\begingroup\$ It is still wide open to sql injection. This is never sanitized $pid = $_GET['pid']; and is used directly in your query. \$\endgroup\$
    – bumperbox
    Commented May 6, 2014 at 7:37
  • \$\begingroup\$ This might help, stackoverflow.com/questions/60174/… \$\endgroup\$
    – bumperbox
    Commented May 6, 2014 at 7:38
  • \$\begingroup\$ Just another note, I would name the columns in your select. If you ever alter the table, the column order may not match what you expect, and it will probably silently fail. \$\endgroup\$
    – bumperbox
    Commented May 6, 2014 at 7:39
  • \$\begingroup\$ @bumperbox what is the solution of if i alter the column order? \$\endgroup\$ Commented May 6, 2014 at 8:32
  • \$\begingroup\$ @RajeshVishnani: cf my answer: use $stmt->fetch(PDO::FETCH_ASSOC), which will return each row as an associative array, where the column (field) names are the indexes: $row = $stmt->fetch(PDO::FETCH_ASSOC); echo $row['package_id']; will always work, regardless of column/field order... \$\endgroup\$ Commented May 6, 2014 at 12:28

1 Answer 1


Not safe at all, I'm afraid. Sure, you're calling PDO::prepare, to create a prepared statement, and that's all fine and dandy, but you are directly concatenating unsanitized user data into the string form which the query is prepared:

 $pid = $_GET['pid'];
 $sql = "SELECT * FROM packages_to_be_shipped_on_bremail_address WHERE package_id=$pid";

What if I were to pass any of the following values as "pid":

1 OR package_id > 0;

That would mean that, in the first case, you're executing the query:

SELECT * FROM packages_to_be_shipped_on_bremail_address WHERE package_id = 1 OR package_id > 0;
-- which translates to:
SELECT * FROM packages_to_be_shipped_on_bremail_address;

The second case is, in a way, even more subtle:

SELECT * FROM packages_to_be_shipped_on_bremail_address WHERE package_id = ?

This is a perfectly valid string to be passed to the PDO::prepare method. The ? is a placeholder for a value that will be bound later on in the code (at least, that's what PDO, and MySQL expects).
That means that your code will won't see anything wrong with the query, but once PDOStatement::execute is called, it'll throw an exception saying there is a problem. Something along the lines of "PDOStatement::execute expects one parameter, received null"
Just check the docs, and check the examples of how PDO::prepare should be used. What you're after should be:

$stmt = $pdo->prepare(
    'SELECT *
     FROM packages_to_be_shipped_on_bremail_address
     WHERE package_id = :pid'
        ':pid' => $pid
while($row = $stmt->fetch(PDO::FETCH_OBJ))//or FETCH_ASSOC
    echo 'Shipping Company - ', $row->shipcompany,
          '<br><hr>Expected Delivery Date ', $row->deliverydate, '<br><hr>';

Before you ask: Yes, those are comma's in the echo statement. echo is a language construct that works pretty similarly to C++'s std::COUT. You can push multiple values to it, separated by comma's.
The upshot is that PHP can then skip the string concatenation step, making the statement more efficient.
The downside? Well, there isn't one AFAIK.

Anyway: How does this produce safer queries? Well, that's easy: you send the query, without the user-supplied data to the server. MySQL can already compile and optimize the query. Often, MySQL will even be able to pre-fetch some of the data you need.
When you execute the query, and provide the data that makes up the WHERE clause (or other parts of your query), then MySQL can escape them properly, and since the query is already compiled, there's no real danger of the user supplied data being able to alter the query itself. This is just a short, very basic, and incomplete explanation of prepared statements. For more details: ask Google.

Other Niggles
There are some other issues that should be fixed, too:

$pid = $_GET['pid'];

What if $_GET isn't set, or the pid parameter wasn't passed? In that case, PHP will emit a notice (undefined index or something). Simply check before you attempt to assign:

if (!isset($_GET['pid']))
    exit();//or do something else
$pid = $_GET['pid'];

Avoid SELECT * FROM queries as much as you can. Sure SELECT * is quick and easy to write, but it does what it says on the tin: it selects every field. You only seem to be using 2 fields per row.
Why, then, do you feel compelled to select everything? Wouldn't:

SELECT shipcompany, deliverydate FROM your_wacky_tbl_name WHERE package_id = :pid

Work, too? As an added bonus: it's easier to see/understand for anyone maintaining this code what the query does, and what it selects. If you need some other field later on, then you can simply add that to the query.

Next, you've wrapped everything in a try block. Probably expecting PDO to throw up exceptions if any of the calls fail (PDO::prepare, PDOStatement::execute and PDOStatement::fetch or even that PDOStatement::bindColumn call that really serves no purpouse here, IMO).

This isn't going to work as you expect it to, because you haven't set PDO's error-mode so that it will throw exceptions. To do so, either pass an array to the constructor or call PDO::setAttribute:

$pdo = new PDO($dsn, $user, $pass,

See a list of attributes that you can set, either by passing them as an assoc array to the constructor, or by calling PDO::setAttribute.

Pokémon Exception Handling
At the moment, you're just blindly catching any of the possible PDOExceptions that might be thrown. This is called Pokémon Exception Handling, when you "gotta catch 'em all". Another term you could use for this is code smell.

Depending on what part of your code that encounters problems, you should handle that problem differently: if the new PDO() line failed, then stop there but whatever you do: You don't echo the message to the user, as it is likely to contain information about your server.
Log the exception, and either throw up a new, more general RuntimeException('Server says no') exception.

If the PDOStatement calls fail, you can be a tad more specific in the error message you send to the user, but again: you don't show them the PDOException message. That message is likely to reveal information on your database or table.
Log the message, and present the user with a polite message, saying that you couldn't find his/her $_GET['pid'] product.

Basically, if you're going to work with exceptions, then have a handler registered that logs the exceptions that are thrown and present the user with a generic error message, hiding your server layout as much as possible.
It's easy to do, and increases the overall security of your application a bit.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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