Login and User Information Requests

Posted on

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 of if($user != FALSE)?

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

Leave a Reply

Your email address will not be published. Required fields are marked *