Problem
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;
}
?>
Solution
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 ofif($user != FALSE)
? -
What is the purpose to set headers and then exit without any request?