4
\$\begingroup\$

I am building a social network with PHP, HTML and CSS and a bit of JS. I want to know how secure my login and register code is.

Register html:

    <input id="uname" type="text" onblur="this.value=removeSpaces(this.value);" name="username" required placeholder="Username..."><p/>
        <br>
    <input id="pww" type="password" onblur="this.value=removeSpaces(this.value);" name="pw" required placeholder="Password..."><p />
        <br>
    <input id="cpw" type="password" onblur="this.value=removeSpaces(this.value);" name="pw2" required placeholder="Confirm Password..."><p />
        <br>

    <button type="submit" id="bt1" name="Register">Register</button>

</form>

Login html:

    <input type="text" id="luname" onblur="this.value=removeSpaces(this.value);" required name="username" placeholder="Username...">
    
    <input type="password" id="lpw" onblur="this.value=removeSpaces(this.value);" required name="pw" placeholder="Password..." >

    <button id="bt2" type="submit" name="signin">Login</button>

</form>

Register code:

<?php

error_reporting(-1); // reports errors

// check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) {
    
    // looks like a hack, send to index.php
    header('Location: index.php');
    die();
}
    
require 'config/connect.php';

//Initializing variable. "" When you want to append stuff later. 0 When you want to add numbers later
$username = ''; 
$pw = ''; 
$pw2 = ''; 

$_SESSION['username'] = $username; // stores username in session variable

$success = array(); // holds success messages
$errors = []; // holds error messages

if ($pw !== $pw2) {

    //$errors[] = "The passwords do not match.";

}

if (!$errors) {
    //An SQL statement template is created and sent to the database
    $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
    // This function binds the parameters to the SQL query and tells the database what the parameters are.
    $stmt->bind_param("s", $_POST['username']);
    // the database executes the statement.
    $stmt->execute();
    // Associative arrays are arrays that use named keys that you assign to them.
    $row = $stmt->get_result()->fetch_assoc();

    if ($row && $row['username'] == $_POST['username']) {

        //$errors[] = "<p id='exists'>Username exists</p>";

        $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 34%;right: 30%;position: absolute;">Username exists</p></b>';
        header('Location: index.php');
    }
}

if (!$errors) {

    $pw = password_hash($pw, PASSWORD_BCRYPT, array('cost' => 14));

    $stmt = $conn->prepare("INSERT INTO users (username, pw) VALUES(?, ?)");
    $stmt->bind_param("ss", $_POST['username'], $pw );
    $stmt->execute();

    $_SESSION["username"] = $_POST['username'];
    header('Location: profile.php');
    die();

} else {
    
    // The foreach construct provides an easy way to iterate over arrays. 
    foreach ($errors as $error) {
        echo "$error <br /> \n";
    }
    echo '<a href="index.php" id="exists">Try again</a><br />';
}
?>

Login code:

<?php

error_reporting(-1); // reports errors

require 'config/connect.php';

$username = '';
$_SESSION['username'] = $username;

//check if form is submitted
//if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['signin'])) {

    // looks like a hack, send to index.php
    //header('Location: index.php');
    //die();
//}

if (empty($_POST["username"])) {

    //echo 'Fill in username to sign in. <a href= index.php>Try again</a><br />';
    //die();

    $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 15%;right: 38%;position: absolute;">Username cannot be empty</p></b>';
    header('Location: index.php');
}

if (empty($_POST["pw"])) {

    // echo 'Fill in password to sign in. <a href= index.php>Try again</a><br />';
    // die();

    $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 15%;right: 38%;position: absolute;">Password cannot be empty</p></b>';
    header('Location: index.php');
}

$sql = "SELECT pw FROM users WHERE username = ?";
$stmt = mysqli_prepare($conn, $sql);

if ( !$stmt ) {

    echo mysqli_error($conn);
    die();
}

$stmt->bind_param('s', $_POST['username']);

if ( !$stmt->execute() ) {

    echo mysqli_error($conn);
    die();
}
// we found a row with that username, 
// now we need to check the password is correct

// get the password from the row
$stmt->bind_result($hashed_pwd); // Binds variables to a prepared statement for result storage
$stmt->fetch(); // Fetch results from a prepared statement into the bound variables

if ( password_verify($_POST['pw'], $hashed_pwd) ) {

    // password verified
    $_SESSION["username"] = $_POST['username'];
    header('Location: profile.php');

} else {
//echo 'Incorrect username or Password. <a href= index.php>Try again</a><br />';

    $_SESSION['error'] = '<b><p style="color: #fff; font-size: 25px; top: 15%;right: 30%;position: absolute;">Incorrect username or Password.</p></b>';
    header('Location: index.php');
}
?>
\$\endgroup\$
  • \$\begingroup\$ Welcome to code review, you can improve the question by making the title more about what the code does and leave the actual question about security in the body of the question. There are many questions that start off with How secure is my .... A better title would be Social network login and registration forms. \$\endgroup\$ – pacmaninbw Jun 18 at 22:24
  • \$\begingroup\$ Please tell us why you are disallowing spaces in the password fields? Mutating the user's password (silently) will lead to user frustration when they try to login with spaces in the future. Why do you unconditionally save a blank username to the SESSION array? Why is $errors an array if can only have a max size of 1? Showing mysqli_error() messages to the end user is never secure practice. \$\endgroup\$ – mickmackusa Jun 18 at 22:53
  • \$\begingroup\$ @mickmackusa ok I fixed all the other problems but is this good ? $errors = ''; instead of holding it as an array ? \$\endgroup\$ – John H Jun 19 at 0:51
  • \$\begingroup\$ I suppose so. (I guess my questions were actually a review.) \$\endgroup\$ – mickmackusa Jun 19 at 0:53
  • \$\begingroup\$ @mickmackusa Lol I guess so. I allowed user's to add spaces to their passwords. I removed the session at the start of the file. And I fixed the $errors. Is there a security problem with allowing users to add spaces to passwords or usernames ? \$\endgroup\$ – John H Jun 19 at 0:56
1
\$\begingroup\$
  • Allowing spaces in usernames and passwords represents absolutely no security risk -- so let your users do it if they like. Well, if you don't want usernames to have spaces, that's a personal choice. In fact, by silently removing spaces, you stand the chance of irritating your users because they will struggle to log in if the saved username or password doesn't match what they [thought they] typed.

  • I don't see any reason to save a blank username string to the $_SESSION array. If you are going to save the username to the SESSION, then only added it after it is validated.

  • Because your script can only produce a single error message, then it could be argued that employing an array to contain this data is an overly complex data structure. Unless you plan on extending the volume of error messages to be displayed to the user, just save the variable as a string.

  • Never display mysqli_error() messages in production code. You never want to give bad-actors any "useful" information.

  • I don't recommend saving html in your SESSION array. Save the lean text and maybe save meta data on the text (was it a successful or failed outcome). If html markup & styling is necessary, that should be generated by the view/template and css should be in an external style sheet.

  • if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) { is more simply written as if (!isset($_POST['Register'])) {.

  • The check if $row['username'] == $_POST['username'] is silly. Of course, the two values will be identical -- you just pulled the qualifying match from the database! Instead, just return the COUNT() from your query, this way you will always have a result set and you can just check if the result set's lone column value is truthy/falsey to determine if the username is already taken.

| improve this answer | |
\$\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.