0
\$\begingroup\$

My code is working however it seems to be using old outdated php version so less secure and I'm still new to programming so I'd be more than thankful if someone shows me how an improved updated and secured version would look like

My code:

Login.php:

<!DOCTYPE html>
<html>
<head>
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel="stylesheet" href="Styles/Bootstrap.css">
    <link rel="stylesheet" href="Styles/Font-Awesome.css">
    <link rel="stylesheet" href="Styles/General.css">
    <link rel="stylesheet" href="Styles/Login.css">
    <link rel="icon" href ="favicon.ico" type="image/x-icon" /> <link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />

    <title>Register & Login</title>
</head>
<body>

      
    <a class="btn btn-danger" href="index.html">Cancel</a>

    <div>
        <h1 class="px20">Please Login after you Register if you don't have an account.</h1>
        <div>
        <div class="form-box">
            <div class="button-box">
                <div id="btn"></div>
                    <button type="button" class="toggle-btn" onclick="Login()">
                        Login
                    </button>
                    <button type="button" class="toggle-btn" onclick="Register()">
                        Register
                    </button>
            </div>

            <form action="validation.php" method="POST" id="Login" class="input-group">
                    <input type="Email" name="Email" class="input-field" placeholder="Email" required>
                    <input type="Password" name="Password" id="Password" class="input-field" placeholder="Password" required>

                    <i class="far fa-eye" id="togglePassword"></i>

                    <button type="submit" class="submit-btn">Login</button>
                </form>

                <form action="registeration.php" method="POST" id="Register" class="input-group">
                    <input type="Email" name="Email" class="input-field" placeholder="Email" required>
                    <input type="text" name="Username" class="input-field" placeholder="Username" required>
                    <input type="Password" name="Password" class="input-field" id="Password2" placeholder="Password" required>

                    <i class="far fa-eye" id="togglePassword2"></i>

                    <button type="submit" class="submit-btn">Register</button>
                </form>


        </div>
    </div>
    </div>

    <script src="Scripts/Login.js"></script>
</body>
</html>

Registeration.php:

<?php

session_start();

$con = mysqli_connect('host','db_name','pass');

mysqli_select_db($con, 'table');

$Email = $_POST['Email'];
$Pass = $_POST['Password'];
$User = $_POST['Username'];

$s = "select * from Users where Email = '$Email'";

$result = mysqli_query($con, $s);

$num = mysqli_num_rows($result);

if($num == 1){
    echo "<p align='center'> <font color=White  size='50pt'>Email Already in use</font> </p>";
    
;}else{
    $Date = date('Y-m-d H:i:s');
    $reg= " insert into Users(Username, Email, Password, Creation_Date, VIP, Admin) values ('$User', '$Email' , '$Pass', '$Date', 'NO', 'NO')";
    mysqli_query($con, $reg);
    echo "<p align='center'> <font color=White  size='50pt'>Registeration successful</font> </p>";
}


?>




<html>

<head>
<meta http-equiv="refresh" content="4;URL=login.php">
    <link rel="stylesheet" href="Styles/General.css">
    <link rel="stylesheet" href="Styles/Background.css">
    <link rel="icon" href ="favicon.ico" type="image/x-icon" /> <link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />

    <title>Register & Login</title>
</head>

<body class="Blue-Black">
    <h1>Please wait, you will be automatically redirected to the login page.</h1>
</body>

</html>

Validation.php:

<?php

session_start();

$con = mysqli_connect('host','dbname','pass');

mysqli_select_db($con, 'table');

$Email = $_POST['Email'];
$Pass = $_POST['Password'];

$s = "select * from Users where Email = '$Email' && Password = '$Pass' ";

$result = mysqli_query($con, $s);

$num = mysqli_num_rows($result);

if($num == 1){
    $_SESSION['Email'] = $Email;
    $Date = date('Y-m-d H:i:s');
    $update = "UPDATE Users SET Last_Login = '$Date' WHERE Users.Email = '$Email'";
    mysqli_query($con, $update);
    $UpdateLastLogin = "UPDATE Users SET Login_Times = Login_Times + 1 where Email  = '$Email'";
    mysqli_query($con, $UpdateLastLogin);
    header('location:home.php');
}else{
    header('location:login.php');
}

?>
\$\endgroup\$
2
  • \$\begingroup\$ Your problem is that you never use the idea you offered. You just diligently copy and paste the exact part of the code provided, but never implement solutions, used in this code, on other parts of your application \$\endgroup\$ Commented Sep 16, 2023 at 7:09
  • \$\begingroup\$ i try to understand the codes i copy but if i don't i ask again till i understand the part that i don't i'd be lying if i said i understand every single part of the code but who's gonna explain it all for me? and im just doing this for a project i don't use php much anywhere else not yet at least i still need to learn it properly and im aware of that \$\endgroup\$
    – Bebo
    Commented Sep 16, 2023 at 7:12

1 Answer 1

3
\$\begingroup\$

Yes indeed, your approach is severely outdated. Such a code nowadays is frowned upon. Here are some rules for the better code

  • Each script should consist of four parts
    1. Initialization. Usually it's done in a separate file that's either includes the current script, or being included into it.
    2. Input validation (if any)
    3. Data manipulation.
    4. Output (if any)
  • Security should be your foremost concern
    • in SQL, always use parameters instead of PHP variables for any database interaction
    • passwords must be hashed using password_hash()
    • all output in HTML must be done through htmlspecialchars()
  • User's convenience is a good thing to think of
    • for example, in case of user error, showing the error just on a white screen is not very convenient
    • already entered data is better to be kept filled too

Below is example code for the user registration. It's a single file which is split into four parts for sake of presentation

Part one. Initialization

<?php    
require 'init.php';

// Initialize all values so we won't have to always check them for existsnce
$error = ['name' => '', 'email' => '', 'password' => ''];
$input = ['name' => '', 'email' => '', 'password' => ''];

Parts two. Input validation

// if the form was submitted
if ($_SERVER['REQUEST_METHOD'] === 'POST') {

    // input validation

    $name = $input['name'] = trim(filter_input(INPUT_POST, 'name'));
    if (mb_strlen($name) < 3 || mb_strlen($name) > 30) {
        $error['name'] = 'Please enter your name, it must be from 3 to 30 charaters long.';
    }

    $email = $input['email'] = trim(filter_input(INPUT_POST, 'email'));
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        $error['email'] = 'Please enter a valid email address.';
    } else {
        $result = $db->execute_query("SELECT 1 FROM users WHERE email = ?", [$email]);
        if ($result->fetch_row()) {
            $error['email'] = 'Email address already taken.';
        }
    }

    $password = filter_input(INPUT_POST, 'password');
    if (strlen($password) < 6 || strlen($password) > 72) {
        $error['password'] = 'Password must be from 6 to 72 bytes long.';
    }

Part three. Data processing

    // if no errors
    if (implode($error) === '')
    {
        // passwords MUST be hashed using the dedicated function
        $password = password_hash($password, PASSWORD_DEFAULT);
        
        // a parameterized query MUST be used to avoid errors and injections
        $stmt = $db->prepare("INSERT INTO users (name, email, password) VALUES (?,?,?)");
        $stmt->execute([$name, $email, $password]);

        // after each succesful POST request there MUST be a redirect
        header("Location: /index.php");
        // after sending Location header the code MUST be terminated
        die;
    }
}

Part four. Output

require 'templates/layout/header.php';
require 'templates/registration.php';
require 'templates/layout/footer.php';

This code should be only the first iteration though. Innumerable improvements can be made to it. But it could be a good starting point

A minimal version of init.php could be like

<?php
error_reporting(E_ALL);
ini_set('display_errors', 1);

session_start();

if (!file_exists('config.php')) {
    throw new Exception('Please create config.php based on config.sample.php');
}
$config = require 'config.php';

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$db = new mysqli(...$config['db']);
$db->set_charset($config['db_charset']);

config.sample.php

<?php

return [
    'db' => [
        'hostname' => '127.0.0.1',
        'username' => '',
        'password' => '',
        'database' => '',
        'port' => 3306,
    ],
    'db_charset' => 'utf8mb4',
];

The HTML template for the form would be like

<div class="col-md-6 col-md-offset-3">
  <h1 class="mb-5">Registration</h1>

  <form role="form" method="post" action="">

    <div class="form-group<?php if ($error['name']): ?> has-error <?php endif ?>">
      <label class="control-label" for="inputError">Name</label>
      <input type="text" class="form-control" name="name" value="<?= htmlspecialchars($input['name']) ?>">
<?php if ($error['name']): ?>
      <span class="help-block"><?= htmlspecialchars($error['name']) ?></span>
<?php endif ?>
    </div>

    <div class="form-group<?php if ($error['email']): ?> has-error <?php endif ?>">
      <label class="control-label" for="inputError">Email</label>
      <input type="text" class="form-control" name="email" value="<?= htmlspecialchars($input['email']) ?>">
<?php if ($error['email']): ?>
      <span class="help-block"><?= htmlspecialchars($error['email']) ?></span>
<?php endif ?>
    </div>

    <div class="form-group<?php if ($error['password']): ?> has-error <?php endif ?>">
      <label class="control-label" for="password">Password</label>
      <input type="password" class="form-control" name="password">
<?php if ($error['password']): ?>
      <span class="help-block"><?= htmlspecialchars($error['password']) ?></span>
<?php endif ?>
    </div>

    <div class="d-flex justify-content-between">
      <button type="submit" class="btn btn-primary">Register</button>
      <a class="btn btn-outline-primary pull-right" href="login.php">Login</a>
    </div>
  </form>

</div>

The idea here is to display possible validation errors right along with already entered form controls, so the user don't have to enter them again.

\$\endgroup\$
9
  • \$\begingroup\$ ok how about setting the connection to the database? it's also outdated based on what i've heard and i have to use smth called pdo instead but my knowledge is so limited on this topic \$\endgroup\$
    – Bebo
    Commented Sep 14, 2023 at 9:35
  • \$\begingroup\$ is there a way i could contact you sir other than stackexchange cuz i need a little more help pls :) ? \$\endgroup\$
    – Bebo
    Commented Sep 14, 2023 at 9:48
  • 1
    \$\begingroup\$ Like it's already said in my answer, it's using parameters that make your SQL interactions secure. As long as you're using them for all SQL queries, it's safe, with any driver. As long as you don't, no driver will make your code secure. Is it understood? \$\endgroup\$ Commented Sep 14, 2023 at 10:54
  • 2
    \$\begingroup\$ Good question. I added HTML template for the form \$\endgroup\$ Commented Sep 16, 2023 at 5:20
  • 1
    \$\begingroup\$ Yes that's perfect now i understand, i added lots of useless lines to display errors in addition to the ones you did as I didn't use value =htmlspecialchars part \$\endgroup\$
    – Bebo
    Commented Sep 16, 2023 at 5:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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