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");
      }

    });
  });

The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. — BCdotWEB, Jun 24 '20 at 8:58

mickmackusa · Accepted Answer · 2020-06-25 01:06:30Z

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.
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.
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).
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.
Combine if(isset($_POST["userLoginSubmit"]) == true){ if(!isset($_SESSION["userID"])){
to become if (isset($_POST["userLoginSubmit"]) && !isset($_SESSION["userID"])) {.
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));
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.
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.
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.
I don't think that I would bother with rowCount(), I'd just check if the fetched result set was null.

Simplify the SESSION declaration block like this:

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

$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.
I don't see it in your question, but I hope you are calling start_session() somewhere.

Hrmm, now I am second guessing myself about struckthough point 1. I ran a test in my local environment where location.href is used after a successful login. The login credentials remained in the developer tools after the new page was loaded and even after I navigated to additional pages. Am I testing this incorrectly @Kazz? — mickmackusa, Jun 25 '20 at 2:20
Ah I guess the Network tab would need to have been left open while logging in. That's not something that the average user would do. — mickmackusa, Jun 25 '20 at 2:27
there is an option to record network requests, which is by default enabled, once you open the network tab in developer tools, you can see even classic form submit there, which persist through requests. As you said that's not what regular users do. — Kazz, Jun 25 '20 at 7:10

Kazz · Accepted Answer · 2020-06-24 23:04:35Z

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>

I think a deliberate amount of sanitizing/validating is important prior to adding data to the database to prevent second order attacks. medium.com/@fiddlycookie/… — mickmackusa, Jun 25 '20 at 1:10
You are right @mickmackusa, I hope he use prepared statements across whole application, not just on registration/login, which do avoid this kind of attack without sanitizing input. — Kazz, Jun 25 '20 at 6:56
And what if someone write <script> or <?php and saves it into the database with recensions and after that he will load recensions and that code will be there? I tried it and it did not do nothing, but maybe it is because these recensions printing prepend function. Script was cloced after recension text and some divs and php commented. — Tomáš Kretek, Jun 25 '20 at 13:18
And if I write <script>alert('Hello! I am an alert box!');</script> it will do it. — Tomáš Kretek, Jun 25 '20 at 13:28
@TomášKretek As I wroted above "Use sanitizing whenever you printing user input on your pages (XSS attack)", that means that if you print for example the script with alert you wroted above you do it as: htmlspecialchars($userinput) and then it become harmless text. If you do sanitizing before inserting to the db, you may end up doing it more times (edit for example) and < will end up beeing < so just before printing is good enough, if you use some templates for example latte its done automatically. The <?php do nothing to your app because you would have to eval() it . — Kazz, Jun 25 '20 at 14:35

Stack Exchange Network

current community

your communities

more stack exchange communities

Is this code written safe from attacks?

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged javascript php security ajax pdo or ask your own question.

Hot Network Questions

Is this code written safe from attacks?

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascript php security ajax pdo or ask your own question.

Related

Hot Network Questions