2
\$\begingroup\$

I really like to use PDO because it's simple and easy to make a safe query, i prepared all queries and used placeholders, i think it's safe but i'm not sure at all, and I'm thinking if i used trim() the right way. What you guys think? Any doubt please ask me on the comments section.

login.php

<?php

if($_SERVER['REQUEST_METHOD'] == 'POST'){ 

$email = trim($_POST['email']);

try{
  $Query = "SELECT * FROM users WHERE email = :email";
  $statement = $conn->prepare($Query);
  $statement->bindValue(':email', $email);
  $statement->execute();
  $user = $statement->fetch(PDO::FETCH_ASSOC);    
  $RowCount = $statement->rowCount();
} catch (PDOerrorInfo $e){}

  if( $RowCount == 0 ){
   // User doesn't exist
    $_SESSION['message'] = "Não existe um usuário com este e-mail.";
    header("location: error.php");

  } else{ // User exists

      if( password_verify($_POST['password'], $user['password'])){
        $_SESSION['email']  = $user['email'];
        $_SESSION['first_name'] = $user['first_name'];
        $_SESSION['last_name'] = $user['last_name'];
        $_SESSION['username'] = $user['username'];
        $_SESSION['img'] = $user['img'];
        $_SESSION['active'] = $user['active'];
        $_SESSION['logged_in'] = true;
        header("location: ../index.php");
      } else {
          $_SESSION['message'] = "Senha incorreta, tente novamente!";
          header("location: error.php");
        }      
    }  
}

register.php

<?php

$img = rand(0,30);
$first_name = trim($_POST['first_name']);
$last_name = trim($_POST['last_name']);
$username = trim($_POST['username']);
$email = trim($_POST['email']);
$password = password_hash($_POST['password'], PASSWORD_BCRYPT);
$hash = md5( rand(0,1000) );

// Check if user with that email already exists
$result = $conn->prepare("SELECT * FROM users WHERE email = :email");
$result->bindParam(':email', $email);
$result->execute();
$RowCount = $result->rowCount();

if ( $RowCount > 0 ) { 
    $_SESSION['message'] = 'Já existe um usuário com este e-mail!';
    header("location: error.php");   
} else {
    $sql = "INSERT INTO users (first_name, last_name, username, img, email, password, hash) VALUES (:first_name, :last_name, :username, :img, :email, :password, :hash)";
    $sql = $conn->prepare($sql);
    $sql->bindParam(':first_name', $first_name);
    $sql->bindParam(':last_name', $last_name);
    $sql->bindParam(':username', $username);
    $sql->bindParam(':img', $img);
    $sql->bindParam(':email', $email);
    $sql->bindParam(':password', $password);
    $sql->bindParam(':hash', $hash);
    $sql->execute();

}

forgot.php

<?php 
require 'db.php';
session_start();
if ( $_SERVER['REQUEST_METHOD'] == 'POST' ) 
{   
    $email = trim($_POST['email']);
    $result = $conn->prepare("SELECT * FROM users WHERE email = :email");
    $result->bindValue(':email', $email);
    $result->execute();
    $RowCount = $result->rowCount();

    if ( $RowCount == 0 )
    { 
        $_SESSION['message'] = "Não existe um usuário com este e-mail.";
        header("location: error.php");
    }
    else {
        $user = $result->fetch(PDO::FETCH_ASSOC);      
        $email = $user['email'];
        $hash = $user['hash'];
        $first_name = $user['first_name'];

        $_SESSION['message'] = "<p>Link de confirmação enviado para <span>$email</span>"
        . " clique no link para resetar a sua senha!</p>";

        $to      = $email;
        $subject = 'Resetar senha - AnimeFire';
        $message_body = '
        Olá '.$first_name.' :),

        Você solicitou o resete de sua senha.

        Clique no link para resetar:

        https://localhost/login-system/reset.php?email='.$email.'&hash='.$hash;  

        mail($to, $subject, $message_body);

        header("location: success.php");
  }
}
?>

reset.php

<?php
require 'db.php';
session_start();
if( isset($_GET['email']) && !empty($_GET['email']) AND isset($_GET['hash']) && !empty($_GET['hash']) )
{
    $email = trim($_GET['email']); 
    $hash = trim($_GET['hash']); 

    $result = $conn->prepared("SELECT * FROM users WHERE email = :email AND hash = :hash");
    $result->bindValue(':email', $email);
    $result->bindValue(':hash', $hash);
    $result->execute();
    $RowCount = $result->rowCount();

    if ( $RowCount == 0 )
    { 
        $_SESSION['message'] = "A conta já foi verificada ou o URL é inválido!";
        header("location: error.php");
    }
}else {
    $_SESSION['message'] = "A verificação falhou :/ tente novamente!";
    header("location: error.php");  
}
?>

reset_password.php

<?php
require 'db.php';
session_start();
if ($_SERVER['REQUEST_METHOD'] == 'POST') { 
    if ( $_POST['newpassword'] == $_POST['confirmpassword'] ) { 

        $new_password = password_hash($_POST['newpassword'], PASSWORD_BCRYPT);        
        $email = trim($_GET['email']); 
        $hash = trim($_GET['hash']); 

        $sql = $conn->prepare("UPDATE users SET password = :new_password, hash = :hash WHERE email = :email");
        $sql->bindValue(':new_password', $new_password);
        $sql->bindValue(':hash', $hash);
        $sql->bindValue(':email', $email);
        $sql->execute();

        if ( $conn->prepare($sql) ) {

        $_SESSION['message'] = "Sua senha foi resetada com sucesso ^^";
        header("location: success.php");    

        }
    } else {
        $_SESSION['message'] = "As senhas não estão iguais, tente novamente!";
        header("location: error.php");    
    }
}
?>

profile.php

<?php
if (empty($_SESSION['email'])) {
  $_SESSION['message'] = "Você precisa estar logado para vizualizar esta página!";
  header("location: error.php");    
}
else {
  $first_name = $_SESSION['first_name'];
  $last_name = $_SESSION['last_name'];
  $email = $_SESSION['email'];
  $username = $_SESSION['username'];
  $img = $_SESSION['img'];
}
?>
<img src="img/avatar/<?= $img ?>.jpg">
<h3 ><?= $username ?></h3>   
<h6 >Nome: <?= $first_name.' '.$last_name ?></h6>
<h6 >Email: <?= $email ?></h6>
\$\endgroup\$
  • \$\begingroup\$ I don't see anything you need to change. You might want to consider passing your variables directly into the execute instead of binding them separately. For example use this when retrieving email: $statement->execute([':email', $email]); \$\endgroup\$ – Dave Nov 24 '18 at 12:33
2
\$\begingroup\$

The trim() function usage is OK.

The biggest problem here is a hash security. A permanent md5( rand(0,1000) ); hash is anything but security. It's so easily guessable that you can count it doesn't exist at all.

Password reminder hashes are generated per request, each time anew. And it should be something less predictable, http://php.net/manual/en/function.random-bytes.php for example.

Other issues are so common that they made into my list of Top 10 PHP delusions:

There is also a strange code snippet in reset_password.php, checking the result of prepare to test the success of the previous query makes no sense. Besides, given your error reporting is right, there is no need to check for the success at all, just do your redirect right away:

  $sql->bindValue(':email', $email);
  $sql->execute();
  $_SESSION['message'] = "Sua senha foi resetada com sucesso ^^";
  header("location: success.php");    
  exit;

it is also a very good habit to always add exit after every Location header call in your code, as a header itself doesn't mean thet the code execution has been stopped.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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