Return to Answer

• 1

• 11k
• 31
• 49

• 11k
• 31
• 49

You want to make your code as secure as possible. Good. You want your code to be as clean as possible: Great. How are you doing? Well, there's some work left to be done, I'm affraid.

Magic quotes
I have very little to say about this, except for: RTFM (it's in the security section BTW):

This feature has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 5.4.0.

Just replace the code that deals with magic quotes with a couple of ini_set's, that ammount to:

magic_quotes_gpc = Off
magic_quotes_runtime = Off
magic_quotes_sybase = Off

PDO
After connecting, in the pointless try-catch block (more on that later), you set the error-mode:

$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

Only to repeat this call after the try-catch again. That's not clean code, that's clutter. Besides, wouldn't it be cleaner to connect like this:

$pdo = new PDO( 'sqlsrv:server=tcp:'.$host.',1433;Database='.$dbname,
 (string) $username,
 (string) $password,
 array(
 PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
 PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
 )
 );

Setting your attributes in one fell swoop? this way, I can see what attributes are being set and what server is being connected to. Why, then, am I not catching an exception, that might be thrown here? Simply because your catch block is a die: if the connection fails, your app fails, why catch what cannot be saved?

Other niggles:

Redirecting: header("Location: login.php"); is not standard, it'll redirect to the login.php script in your pwd (present working directory). The best way to redirect still is:

header("Location: http://yourdomain.com/login.php", true, 301);//redirect permanently
return;//or exit... I hate die

Wrapping your selects in a try-catch=>die is just a waste of space, just select, if it throws an exception, let the app crash, and get to debugging. When INSERT INTO fails, however, that's a different story. You have to use try-catch there, if you want your code to be as safe as it possibly can be, but you'll have to use transactions, and rollback in case of an exception:

try
{
 $pdo->beginTransaction();
 $pdo->exec($insertStmt);
 $pdo->commit();
}
catch(PDOException $e)
{
 $pdo->rollBack();//revert any changes made during last transaction
 throw $e;//rethrow exception, let it go... don't call die
}

require is not safe
Well, it's not the safest option available to you. require_once is. They do exactly the same thing, except for one thing: require_once (as its name suggests) will make sure the file wasn't included already. That makes it a tad slower, but a whole lot safer. Use require_once, then!

globals are bad
global variables are not safe. Ever. Period. Use functions, classes, namespaces and all that, to avoid name-conflicts.

error settings
If this were production code, I hope you'd set display_errors to 0, right? clients shouldn't be able to see what errors your code contains, that's not safe. setting the error_reporting(E_ALL); is good, but consider: error_reporting(E_STRICT|E_ALL); or error_reporting(-1);, and "go for zero" (as in no notices, warnings or errors)