Is this code written safe from attacks?

Question

I have small question. I don't know much about JavaScript (jQuery) or AJAX and I don't know whether I'm doing it safe from XSS Attacks, SQL injection and that stuff. I think the PHP with MySQL would be written right. The code works, I have added it below.

This is PHP with database call which is called by AJAX

  if(isset($_POST["userLoginSubmit"]) == true){
    if(!isset($_SESSION["userID"])){
      userLogin(filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL), $_POST["password"]);
    }
  }

function userLogin($email, $password){
    $return = [
      "email" => "",
      "password" => "",
      "error" => false
    ];

    if(empty($email)){
      $return["email"] = "empty";
      $return["error"] = true;
    }

    if(empty($password)){
      $return["password"] = "empty";
      $return["error"] = true;
    }

    if($return["error"] == true){
      echo(json_encode($return));
      return false;
    }

    $dbConnect = new DbConnect;
    $dbDoConnect = $dbConnect->doConnectFromOutside();
    $selectCommand = "SELECT *
                      FROM users
                      WHERE email = :email LIMIT 1";

    $data = $dbDoConnect->prepare($selectCommand);
    $data->bindValue(":email", $email, PDO::PARAM_STR);
    if(!$data->execute()){
      $return['error'] = "connection";
      echo(json_encode($return));
      return false;
    }

    if($data->rowCount() > 0){
      $selectedData = $data->fetch(PDO::FETCH_ASSOC);
    } else{
      $return["email"] = "free";
      echo(json_encode($return));
      return false;
    }

    if(!password_verify($password, $selectedData["password"])){
      $return["password"] = "badPassword";
      echo(json_encode($return));
      return false;
    }

    $_SESSION["userID"] = $selectedData["userID"];
    $_SESSION["firstName"] = $selectedData["firstName"];
    $_SESSION["lastName"] = $selectedData["lastName"];
    $_SESSION["email"] = $selectedData["email"];
    $return["error"] = "success";
    echo(json_encode($return));
    return true;
  }

This is login form

<div class='signIn'>
                    <i class='fas fa-times fa-2x accountPopupSwitch'></i>
                    <h1>přihlášení</h1>
                    <p>NEMÁM ŮČET A&nbsp;<a href='#' class='changeForm preventDefault'>CHCI HO VYTVOŘIT</a></p>
                    <form method='POST' name='userLogin' id='userLogin'>
                        <input type='email' id='userLoginEmail' name='email' placeholder='Váš email' class='removableVal'>
                        <label for='userLoginEmail' id='userLoginEmailLabel' class='removableVal'></label>
                        <input type='password' id='userLoginPassword' name='password' placeholder='Heslo' class='removableVal'>
                        <label for='userLoginPassword' id='userLoginPasswordLabel' class='removableVal'></label>
                        <p id='userLoginMessage'></p>
                        <button type='submit' id='userLoginSubmit'>PŘIHLÁSIT SE</button>
                    </form>
                        <p>NEMÁM ŮČET A&nbsp;<a href='#' class='changeForm preventDefault'>CHCI HO VYTVOŘIT</a></p>
                </div>

And this is AJAX which is submitted when user try to log in

$("#userLogin").submit(function(event){
    event.preventDefault();
    var email = $("#userLoginEmail").val(),
    password = $("#userLoginPassword").val();
    $("#userLoginSubmit").html("<i class='fas fa-sync'></i>");

    $.ajax({
      method: "POST",
      url: "accountPopupFunctions.php",
      data: {
        email: email,
        password: password,
        userLoginSubmit: true
      },
      dataType: "json",
      success: function(loginResults){
        $("#userLoginPasswordLabel, #userLoginEmailLabel, #userLoginMessage").removeClass("error").html("");
        $("#userLoginPassword").val("");
        $("#userLoginSubmit").html("PŘIHLÁSIT SE");

        if(loginResults["email"] == "empty") $("#userLoginEmailLabel").addClass("error").html("nevyplněný email");
        if(loginResults["email"] == "free") $("#userLoginEmailLabel").addClass("error").html("neexistující email");
        if(loginResults["password"] == "empty") $("#userLoginPasswordLabel").addClass("error").html("nevyplněné heslo");
        if(loginResults["password"] == "badPassword") $("#userLoginPasswordLabel").addClass("error").html("špatné heslo");
        if(loginResults["error"] == "connection") $("#userLoginMessage").addClass("error").html("Chyba v připojení (#003).");
        if(loginResults["error"] == "success"){
          localStorage.setItem('logged', true);
          localStorage.removeItem('loggedOut');
          localStorage.removeItem('loggedPopup');
          localStorage.removeItem('unlogedPopup');
          $(location).attr("href", "index.php");
        }

      },
      error: function(){
        $("#userLoginMessage").addClass("error").html("Něco se pokazilo (#003).");
        $("#userLoginSubmit").html("PŘIHLÁSIT SE");
      }

    });
  });

Answer 1

1. First and foremost, I don't really like the idea of an ajax login because imagine the user is on a public device and logs in then steps away from the computer and someone else pulls up to the browser, opens up the Developer tools > Network > XHR > Headers and gets an eyeful of the login credentials that were sent off for logging in. Is it a bit "tinfoil hat"? Is it the user's responsibility? Yeah, maybe, but there will be users that will be blissfully unaware of this vulnerability. Once a bad actor can infiltrate the parts of your system that are password protected, they may be able to cause more significant trouble in the application. See @Kazz's argument against my claim which I find correct and invalidates my concern.

2. I agree with @slepic, don't bother sanitizing user input. If they are fouling up their submitted data and you "fix" it for them, then you may be damaging their UX because what they typed is not being entered into your system as they expected.

3. I prefer to write my file assets (includes / function declarations) at the top of my file unless there is a logical reason to postpone the deed. Perhaps you don't want to load/declare certain assets until the user's submission has passed qualifying checkpoints. In doing so, the "flow" of your script will be uninterrupted visually (to the human developer).

4. In all places where you are writing == true, just omit it. The loose comparison on true is the same as the expression with the last 7 characters.

5. Combine if(isset($_POST["userLoginSubmit"]) == true){ if(!isset($_SESSION["userID"])){
   to become if (isset($_POST["userLoginSubmit"]) && !isset($_SESSION["userID"])) {.

6. I don't see any reason to return true|false from your function call. I mean, the return value from userLogin() is never used for anything. It probably makes better sense to exit(json_encode($return));

7. I certainly hope that the email column in your table is a UNIQUE KEY because you wouldn't want multiple people sharing an identity. Assuming so, there is reason to explicitly write LIMIT 1.

8. Your prepared statement looks okay. I'd probably not bother declaring the single-use variable -- I'd just write the sql string directly into the prepare() call.

9. connection is a somewhat inappropriate description of the error. It might not be a connection error. But either way, it is good that you are not sending the raw error to the user.

10. I don't think that I would bother with rowCount(), I'd just check if the fetched result set was null.

11. Simplify the SESSION declaration block like this:

    $_SESSION = [
        "userID" => $selectedData["userID"],
        "firstName" => $selectedData["firstName"],
        "lastName" => $selectedData["lastName"],
        "email" => $selectedData["email"],
    ];
    

12. $return["error"] = "success"; is an oxymoron. I think it would be more sensible to set the value to 0 or "" or false. A falsey value will also make checking the value simpler in the javascript.

13. I don't see it in your question, but I hope you are calling start_session() somewhere.

Answer 2

Just in my opinion, take this with grain of salt. You don't need that at all:

$return = [
  "email" => "",
  "password" => "",
  "error" => false
];

Ajax response data type could be just a text, one of these: [bad credentials, error, success], you should validate if email or password is empty before even sending a request, on server side simply check if either of them is empty and if so just print "error" (because if that happen, someone is bypassing your script and sending custom data, therefore you don't have to bother to sending back appropriate error). It's a good practice to not show if an email is registered on your website, so email not present in your database or bad password should be merged to "bad credentials".

Ad to mickmackusa 1) Ajax login is good if you redirect page after successfully login, which you do with: $(location).attr("href", "index.php"); that clear the xhr from developer tools.

So your code could look like this:

if(isset($_POST["userLoginSubmit"]) && !isset($_SESSION["userID"])){
   userLogin($_POST["email"], $_POST["password"]);
}

function userLogin($email, $password){
    if(empty($email) || empty($password)
        || !filter_var($email, FILTER_VALIDATE_EMAIL) // server side email validation
    ){
        exit("error"); // custom data, script bypass
    }

    $dbConnect = new DbConnect;
    $dbDoConnect = $dbConnect->doConnectFromOutside();
    // if(!$dbDoConnect) { exit("connection") } // if you wanna check for connection should be something like that
    $data = $dbDoConnect->prepare("SELECT * FROM users WHERE email = :email"); //  LIMIT 1 not needed because email is unique key 
    $data->bindValue(":email", $email, PDO::PARAM_STR);
    if(!$data->execute()){
        exit("error"); // internal error (bad query)
    }
    
    $selectedData = $data->fetch(PDO::FETCH_ASSOC);
    if(!$selectedData || !password_verify($password, $selectedData["password"])){
        exit("bad-credentials");
    }

    $_SESSION = [
        "userID" => $selectedData["userID"],
        "firstName" => $selectedData["firstName"],
        "lastName" => $selectedData["lastName"],
        "email" => $selectedData["email"],
    ];

    exit("success");
}


$("#userLogin").submit(function(event){
    event.preventDefault();
    var email = $("#userLoginEmail").val(),
    password = $("#userLoginPassword").val();
    if(!email){ // if it's actually email is done by: input type='email'
        $("#userLoginEmailLabel").addClass("error").html("nevyplněný email");
    }
    if(!password){
        $("#userLoginPasswordLabel").addClass("error").html("nevyplněné heslo");
    }
    if(!email || !password) return;
    $("#userLoginSubmit").html("<i class='fas fa-sync'></i>");

    $.ajax({
      method: "POST",
      url: "accountPopupFunctions.php",
      data: {
        email: email,
        password: password,
        userLoginSubmit: true
      },
      dataType: "text",
      success: function(loginResult){
        $("#userLoginPasswordLabel, #userLoginEmailLabel, #userLoginMessage").removeClass("error").html("");
        $("#userLoginPassword").val(""); // btw you don't have to clear password, that's advantage of using ajax login
        $("#userLoginSubmit").html("PŘIHLÁSIT SE");

        if(loginResult == "error") $("#userLoginMessage").addClass("error").html("Něco se pokazilo (#003).");
        if(loginResult == "bad-credentials") $("#userLoginMessage").addClass("error").html("Spatný email nebo heslo.");
        if(loginResult == "success"){
          localStorage.setItem('logged', true);
          localStorage.removeItem('loggedOut'); // these seem not necessary, check for the above instead
          localStorage.removeItem('loggedPopup');
          localStorage.removeItem('unlogedPopup');
          $(location).attr("href", "index.php");
        }

      },
      error: function(){
        $("#userLoginMessage").addClass("error").html("Něco se pokazilo (#003).");
        $("#userLoginSubmit").html("PŘIHLÁSIT SE");
      }

    });
});

Much simpler isn't it ?

You can do it without ajax if you want to, by just printing script that shows the popup on page load (with the error messages).

Use sanitizing whenever you printing user input on your pages (XSS attack), you don't have to sanitize the inputs from post because that's done already by prepared statement (only for SQL !).

By the way, instead: <a href='#' class='changeForm preventDefault'>CHCI HO VYTVOŘIT</a> you can do this: <a href='javascript:;' class='changeForm'>CHCI HO VYTVOŘIT</a>