[RESOLVED] Am I using PDO correctly here?

[mcapone888]

I am a PHP hobbyist. I stepped away for a few years, and now I am back. When I left, I was using mysql_query. I have now read PDO is all the rage. I am now teaching myself PDO using online tutorials.

If someone who was experienced with PDO wouldn't mind, could you please check the below code I created utilizing PDO to validate a simple user name and password authorization script? It seems to work for me. I would just like to know if I am doing something wrong, and if my script is secured as it can be. I would very much appreciate it. Thanks.

<?php
include_once 'includes/connect.php';
session_start();


// username and password sent from form
$myusername = $_POST['myusername'];
$mypassword = $_POST['mypassword'];


//scrub against SQL Injection - Is this still needed now that I use PDO???
$myusername = strip_tags($myusername);
$mypassword = strip_tags($mypassword);
$myusername = mysql_real_escape_string($myusername);
$mypassword = mysql_real_escape_string($mypassword);


//check to see if user name AND password exists in Database
$stmt = $db->prepare("SELECT * FROM userinfo WHERE UserUserName = ? AND UserPassword = ?");
$stmt->execute(array($myusername, $mypassword));
$accountValid = $stmt->fetchAll(PDO::FETCH_ASSOC);

if($accountValid)
{
     //Valid User Name and Password - Get User ID, and then set Session Variables
     $stmt2 = $db->prepare("SELECT UserID FROM userinfo WHERE UserUserName = ? AND UserPassword = ?");
     $stmt2->execute(array($myusername, $mypassword));
     $userid = $stmt2->fetchColumn();

 $_SESSION['username'] = $myusername;
 $_SESSION['userid'] = $userid;

 echo "Correct Username and Password: " . $_SESSION['username'] . " " . $_SESSION['userid'];
}
else
{    

     //Invalid User name or Password
     echo "Wrong Username or Password";
}
?>

[bradgrafelman]

Few things I notice:

1. You should always check to see if external variables exist (e.g. via [man]isset/man or [man]empty/man before accessing them. For example, rather than this:

   $foo = $_POST['foo'];

   I tend to do something like this:

   $foo = isset($_POST['foo']) ? $_POST['foo'] : '';

2. Why are you using [man]strip_tags/man?

3. You shouldn't be using any mysql_*() functions if you're using PDO (or MySQLi).

   Instead, either use the equivalent PDO::quote() method (link: [man]pdo.quote[/man]) or use prepared statements. Since you're already using prepared statements with bounded parameters, there's no need to do any sanitizing (that's the whole point of using bounded parameters in the first place 😉).

4. Why are you using two queries that SELECT the exact same row(s)? Get rid of the second and change the first from a 'SELECT *' (which you should almost never use, by the way) to SELECT the UserID column instead.

   Also note that it would probably make more sense to use PDOStatement::fetch() (link: [man]pdostatement.fetch[/man]) rather than fetchAll() since you presumably only expect at most one row to be matched.

5. In general, you're missing some error checking. What happens if the query can't be prepared or executed?

[NogDog]

bradgrafelman;11007375 wrote:

...
[*]In general, you're missing some error checking. What happens if the query can't be prepared or executed?[/list]

One approach you can take is to run PDO with its exception mode enabled.

$db = new PDO( /* your connection string */ );
$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

Then you can wrap your entire application (e.g. the main script) in a try/catch block, and if a PDO exception is encountered, handle it in one place (instead of having to test each prepare() and execute() to see if it ran okay):

<?php
try {
  /* your application code */
}
catch(PDOException $e) {
  error_log(print_r($e, true));
  /* invoke a generic error page for database errors */
}
catch(Exception $e) {
  /* handle other exceptions here */
}

[mcapone888]

Thank you Nog and Brad. These comments were very helpful, and I have implemented all of them. Thanks.