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;
//or:
?
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'
);
$stmt->execute(
array(
':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,
array(
PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
)
);
//or
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
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.
$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\$