Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I created small simple PHP Authentication API. I have a couple of scripts that I use for session, authentication and registration. Since I'm not an experienced backend and PHP developer, I wanted someone more experienced to review my scripts and tell me what I did wrong and what I can improve.

I did not use any framework; this is plain PHP.

User registration:

<?php
require_once '../dbConnect.php';

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password']) && isset($object['firstName']) && isset($object['lastName'])) {
    $email = $object['email'];

    $validationQuery="select * from members where email='$email'";
    $result = $mysqli->query($validationQuery) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    if($member) {
        $message = array('message' => 'Member with provided email address already exist, please use other email.');
        http_response_code(406);
        echo json_encode($message);
    } else {
        session_start();
        $firstName = $object['firstName'];
        $lastName = $object['lastName'];
        $password = password_hash($object['password'], PASSWORD_DEFAULT);

        $registrationQuery = "INSERT INTO members 
                (firstName, lastName, email, password)
                VALUES 
                ('$firstName', '$lastName', '$email', '$password')";

        if ($mysqli->query($registrationQuery) === TRUE) {
            $message = array(
                'message' => 'Registration Successful, you can use your credentials to log in.',
                'memberId' => mysqli_insert_id($mysqli));
            $_SESSION["id"] = $message['memberId'];
            echo json_encode($message);
        }
    }

    $mysqli->close();
} else {
    http_response_code(400);
}
?>

Getting authenticated member from session:

<?php
require_once '../dbConnect.php';
session_start();

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password'])) {

    $email = $object['email'];
    $password = $object['password'];
    $query="select * from members where email='$email'";
    $result = $mysqli->query($query) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    if($member) {
        if (password_verify($object['password'], $member['password'])) {
            $message = array('message' => 'Authentication Successful!');
            $_SESSION["id"] = $member['id'];
            echo json_encode($message);
        } else {
            $message = array('message' => 'Wrong Credentials, Authentication failed!');
            session_destroy();
            http_response_code(400);
            echo json_encode($message);
        }
    } else {
        session_destroy();
        http_response_code(406);
    }

    $mysqli->close();
} else {
    session_destroy();
    http_response_code(400);
}
?>

Getting authenticated member from PHP session cookie

<?php
require_once '../dbConnect.php';
session_start();

if (isset($_SESSION["id"])) {
    $memberId = $_SESSION["id"];
    $query="select id, firstName, lastName, email, profileImage from members where id='$memberId'";

    $result = $mysqli->query($query) or die($mysqli->error.__LINE__);
    $member = mysqli_fetch_assoc($result);

    echo $json_response = json_encode($member);

    $mysqli->close();
} else {
    http_response_code(401);
}

?>

Simple logout script:

<?php

session_start();

if (isset($_SESSION["id"])) {
    $message = array('message' => 'Successful log out!');
    session_destroy();
    echo json_encode($message);
} else {
    echo 'You are not logged in!';
    http_response_code(403);
}

?>
share|improve this question
1  
I have rolled back the last edit. Please see what you may and may not do after receiving answers. – Phrancis Dec 25 '16 at 10:31
    
I would use pdo instead of mysqli. And why using plain PHP? Unless this is just for learning purposes, why reinventing the wheel? – Dan Costinel Dec 28 '16 at 22:23

Ehh, let's look at the biggest issue here: the SQL-Injection vulnerability.

$object = json_decode(file_get_contents("php://input"), true);

if (isset($object['email']) && isset($object['password']) && isset($object['firstName']) && isset($object['lastName'])) {
    $email = $object['email'];

    $validationQuery="select * from members where email='$email'";

All I have to do is provide a bad string in that JSON for email and now I can destroy your database easy.

Solution: prepared statements.

share|improve this answer
    
Can you provide quick and good example since i never used prepared statements ? – Super Mario's Yoshi Dec 23 '16 at 17:05
    
@SuperMario'sYoshi I'll see if I can whip one up here shortly. – EBrown Dec 23 '16 at 17:06
    
Thank you sir, i appreciate it! – Super Mario's Yoshi Dec 23 '16 at 17:07
1  
@SuperMario'sYoshi see this section of the manual for a How-To guide to prepared statements. – Phrancis Dec 24 '16 at 2:24
1  
You can ask a new question if you'd like to get your new code reviewed. – Phrancis Dec 25 '16 at 10:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.