Login and User Information Requests

Question

This code basically connects to a database, sets login success and failure pages, queries the database for user details, checks if user is active, sets session value and redirects accordingly.

Can you have a look? What do you think of it? Any suggestions?

<?php
session_start();
// Connect to the database
try {
 $db = new PDO('mysql:host=localhost; dbname=database', 'username', 'password');
 $db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
 $db->exec("SET NAMES 'utf8'");
} catch(Exception $e) {
 exit;
}
// Set URL for successful login and unsuccessful login
if($_POST['page']) {
 $success = $_POST['page'];
} else {
 $success = 'http://website.com/members';
}
$fail = 'http://website.com/login';
// Check if login came from my server
if($_SERVER['SERVER_NAME'] != 'website.com') {
 header('Location: ' . $fail);
}
// Check if a user is trying to login
if($_POST) {
 // Query the users details
 try {
 $user_query = $db->prepare('SELECT * FROM users LEFT JOIN zones ON user_timezone = zone_id WHERE user_email = ? AND user_pass = ?');
 $user_query->bindParam(1, $_POST['username'], PDO::PARAM_STR, 50);
 $user_query->bindParam(2, $_POST['password'], PDO::PARAM_STR, 15);
 $user = $user_query->execute();
 $user = $user_query->fetch(PDO::FETCH_ASSOC);
 } catch(Exception $e) {
 exit;
 }
 // Make sure account is active
 if($user['user_active'] != 1) {
 header('Location: ' . $fail . '?error=2');
 exit;
 }
 // Make sure user exists
 if($user != FALSE) {
 $_SESSION['uid'] = $user['user_id'];
 $_SESSION['utz'] = $user['zone_name'];
 header('Location: ' . $success);
 } else {
 header('Location: ' . $fail . '?error=1');
 exit;
 }
} else {
 header('Location: ' . $fail . '?error=4');
 exit;
}
?>

• Mathieu Guindon: You mean improve a piece of code right? Or you're a code jazzman?
• Malachi: where did you get this code at?
• Ravi Kumar: Are you trying to improve someone else's code or this is the code which you implemented ?

Answer 1

Just few quick things come to my mind:

• The way try/catch is used is to exit on error, which would happen anyway. Maybe you need to send some info to the user here instead.

• Are you passing raw user input to the database, without validation? This would be clear security issue.

• Why not if($user) instead of if($user != FALSE)?

• What is the purpose to set headers and then exit without any request?

• AD7six: Prepared statements mean sql injection is already accounted for. Setting location headers and exiting is the normal way to redirect a user. Wrapping the header-and-exit lines in a function is a simple example how the code could be improved.