7
\$\begingroup\$

I wrote a simple PHP login form for a website. It has an initialization script, as well as a form to allow a user to login, plus a link to log the user out after logging in.

lib/app_init.php

<?php
declare(strict_types=1);

require_once 'config/db.php';

try {
    $db_conn = new PDO(PDO_DSN, DB_USER, DB_PASS);

    $db_conn->query('ALTER TABLE IF EXISTS author DROP FOREIGN KEY fk_author_role_id');
    $db_conn->query('ALTER TABLE IF EXISTS post DROP FOREIGN KEY fk_post_author_id');
    foreach (['author_role', 'author', 'post'] as $db_table) {
        $db_conn->query("DROP TABLE IF EXISTS $db_table");
    }
    $stmt = 'CREATE TABLE IF NOT EXISTS author_role (' .
        'id int NOT NULL PRIMARY KEY AUTO_INCREMENT, ' .
        'role_name varchar(100) UNIQUE NOT NULL)';
    $db_conn->query($stmt);
    $stmt = 'INSERT INTO author_role (role_name) VALUES ' .
        "('Administrator'), ('Moderator'), ('User')";
    $db_conn->query($stmt);
    $stmt = 'CREATE TABLE IF NOT EXISTS author (' .
        'id int(11) NOT NULL PRIMARY KEY AUTO_INCREMENT, ' .
        'username varchar(255) UNIQUE NOT NULL, ' .
        'passwd varchar(255) NOT NULL, ' .
        'email varchar(255) UNIQUE NOT NULL, ' .
        'created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, ' .
        'role_id int(11) NOT NULL, ' .
        'FOREIGN KEY fk_author_role_id (role_id) REFERENCES author_role (id))';
    $db_conn->query($stmt);
    $stmt = 'INSERT INTO author (username, passwd, email, role_id) ' .
        "VALUES ('lester29', 'test', '[email protected]', 1)";
    $db_conn->query($stmt);
    $stmt = 'CREATE TABLE IF NOT EXISTS post ( ' .
        'id int(11) NOT NULL PRIMARY KEY AUTO_INCREMENT, ' .
        'title varchar(255) UNIQUE NOT NULL, ' .
        'content text NOT NULL, ' .
        'author_id int(11) NOT NULL, ' .
        'created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, ' .
        'updated_at DATETIME NOT NULL ON UPDATE CURRENT_TIMESTAMP, ' .
        'FOREIGN KEY fk_post_author_id (author_id) REFERENCES author (id))';
    $db_conn->query($stmt);

    echo 'Successfully created required tables.';
} catch (PDOException $ex) {
    echo "An error occurred while initializing database: " . $ex->getMessage();
}

lib/user.php

<?php
declare(strict_types=1);

require_once 'config/db.php';

function userExists(string $username, string $password): bool
{
    try {
        $db_conn = new PDO(PDO_DSN, DB_USER, DB_PASS);
        $stmt = 'SELECT username, passwd ' .
            'FROM author ' .
            "WHERE username = ? " .
            "AND passwd = ?";
        $query = $db_conn->prepare($stmt);
        $query->execute([$username, $password]);
        return $query->rowCount() > 0;
    } catch (PDOException $ex) {
        die('An error occurred at auth_user function: ' . $ex->getMessage());
    }
}

footer.php

<footer>
    <p>This website is owned by lester29</p>
</footer>
</body>
</html>

header.php

<?php
session_start();

if (!isset($_SESSION['auth_user'])) {
    $_SESSION['auth_user'] = false;
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>lester29</title>
</head>
<body>
    <header>
        <h1>lester29</h1>
        <p>My first website :-)</p>
        <?php if ($_SESSION['auth_user']): ?>
            <p>You're logged as <?php echo $_SESSION['username']; ?>.</p>
        <?php endif; ?>
    </header>
    <section>
        <nav>
            <ul>
                <li><a href="index.php">Home</a></li>
                <?php if ($_SESSION['auth_user']): ?>
                    <li><a href="transact_user.php?action=logout">Log out</a></li>
                <?php else: ?>
                    <li><a href="login.php">Login</a></li>
                <?php endif; ?>
            </ul>
        </nav>
    </section>
    <main>

index.php

<?php
require_once 'lib/user.php';
require_once 'header.php';
?>
    <section>
        <h2>Posts</h2>
        <?php
        try {
            $db_conn = new PDO(PDO_DSN, DB_USER, DB_PASS);
            $stmt = 'SELECT post.title AS post_title, ' .
                'author.username AS post_author, ' .
                'post.created_at AS post_created, ' .
                'post.content AS post_content ' .
                'FROM post ' .
                'LEFT JOIN author ' .
                'ON post.author_id = author.id';
            /** @var PDOStatement $posts */
            $posts = $db_conn->query($stmt);
            while ($post = $posts->fetch(PDO::FETCH_ASSOC)) {
                ?>
                <article>
                    <h3><?php echo $post['post_title']; ?></h3>
                    <p>Created at <?php echo $post['post_author']; ?> by <?php echo $post['$post_author']; ?></p>
                    <p>
                        <?php echo htmlspecialchars(nl2br($post['post_content'])); ?>
                    </p>
                </article>
                <?php
            }

            if ($posts->rowCount() == 0) {
                echo '<p>No such any posts.</p>';
            }
        } catch (PDOException $ex) {
            echo '<p>An error occurred during retrieving posts: ';
            echo $ex->getMessage();
            echo '</p>';
        }
        ?>
    </section>
<?php
require_once 'footer.php';

login.php

<?php
require_once 'header.php';
?>
    <section>
        <?php if ($_SESSION['auth_user']): ?>
            <h2>Information</h2>
            <p>
                It seems like you're already logged in.
                Do you want to <a href="transact_user.php?action=logout">log out</a>?
            </p>
        <?php else: ?>
            <h2>Sign in</h2>
            <form action="transact_user.php?action=login" method="post">
                <p>
                    <label for="username">Username</label>
                    <input type="text" name="username" id="username">
                </p>
                <p>
                    <label for="password">Password</label>
                    <input type="password" name="password" id="password">
                </p>
                <p>
                    <input type="submit" name="submit_login" value="Login">
                </p>
            </form>
        <?php endif; ?>
    </section>
<?php
require_once 'footer.php';

transact_user.php

<?php
require_once 'header.php';
require_once 'config/db.php';
require_once 'lib/user.php';
?>
    <section>
        <?php
        if (!isset($_GET['action'])) {
            ?>
            <h2>Error</h2>
            <p>No action chosen for user.</p>
            <?php
        } else {
            switch ($_GET['action']) {
                case 'login':
                    if (isset($_POST['submit_login'])) {
                        $username = $_POST['username'];
                        $password = $_POST['password'];
                        if (userExists($username, $password)) {
                            $_SESSION['username'] = $username;
                            $_SESSION['auth_user'] = true;
                            ?>
                            <h2>Success</h2>
                            <p>You're logged in.</p>
                            <?php
                        } else {
                            $_SESSION['auth_user'] = false;
                            ?>
                            <h2>Login failed</h2>
                            <p>Incorrect login or password. <a href="login.php">Try again</a></p>
                            <?php
                        }
                    }
                    break;
                case 'logout':
                    if ($_SESSION['auth_user']) {
                        $_SESSION['auth_user'] = false;
                        ?>
                        <h2>Success</h2>
                        <p>You're successfully log out!</p>
                        <?php
                    } else {
                        ?>
                        <h2>Information</h2>
                        <p>
                            It seems you're not logged in. Do you want to
                            <a href="login.php">log in</a> again?
                        </p>
                        <?php
                    }
                    break;
                default:
                    ?>
                    <h2>Error</h2>
                    <p>Invalid action <code><?php echo $_GET['action']; ?></code>.</p>
                <?php
            }
        }
        ?>
    </section>
<?php
require_once 'footer.php';
\$\endgroup\$
3
  • 1
    \$\begingroup\$ This makes me thankful Laravel exists. \$\endgroup\$ Jan 30 at 17:05
  • \$\begingroup\$ is that a plain text password and username/email/password fields limited to 255 characters? \$\endgroup\$
    – njzk2
    Jan 30 at 22:04
  • \$\begingroup\$ If you're looking for more general security feedback, you need rate-limiting on any endpoint that tests a password; every login system should support MFA; you should track sessions and allow a user to review active sessions and logged in devices, and selectively terminate unrecognized sessions; you should email the user when a new session is created for a device that hasn't previously been seen; you should rotate sessions when a password is reset; lots of features to add here beyond just checking the username/password for correctness. \$\endgroup\$
    – user229044
    Mar 19 at 23:59

3 Answers 3

6
\$\begingroup\$

General review

Consider restricting access to init script

At first I saw lib/app_init.php and wondered why a login script would need to alter database tables. Presumably that script is only run one time when setting up the site, hence the name. It would be wise to ensure that script is only run by a site administrator - perhaps only via a command line environment and not via the web. For more information about command line usage of PHP see the PHP.net documentation for Using PHP from the command line - especially the Usage section with examples included.

Consider hashing passwords

I also noticed passwords appear to be stored in plain text. That may be fine for a personal project where nobody else may need a user but for most projects it would be wise to not store the passwords in plain text. One could use a password hashing algorithm to store the passwords - e.g. password_hash() to create the hash and then password_verify() to ensure the user input matches what is stored in the database.

Other suggestions

Use strict equality comparisons

It is good that PDO appears to be used with constants for the database credentials stored in a central file, and parameters are bound to queries in an effort to avoid SQL injection.

In index.php is this condition:

if ($posts->rowCount() == 0) {

PDOStatement::rowCount() returns an integer:

public PDOStatement::rowCount(): int

Because of this the condition can use strict equality:

if ($posts->rowCount() === 0) {

It is a good habit to use strict equality operators. Consider the case where code might check to see if a string contains a substring at the start using strpos().

$string = 'abcdefg';
$substring = 'abc';
if (strpos($string, $substring) == 0) {

That would work fine but be aware stropos may return an integer or boolean false:

screenshot of return value section of documentation for strpos function

Note the text with the red background:

Warning This function may return Boolean false, but may also return a non-Boolean value which evaluates to false. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.

As this demo illustrates, using a non-strict comparison could lead to an issue if the sub-string was not contained in the string:

$string = 'abcdefg';
$substring = 'notehere';
if (strpos($string, $substring) == 0) {
    //this would get executed
}

Short echo tags can be used

The short echo tag <?= can be used in place of <?php echo.

\$\endgroup\$
5
  • \$\begingroup\$ How can lib/app_init.php be rewritten in a way to be able to run it only through the command line? Should this file be placed in /var/www/html? \$\endgroup\$ Jan 30 at 8:00
  • \$\begingroup\$ Maybe you didn't read your strict equality link carefully, because the question outlines very good reasons not to use it in PHP, and none of the current answers sufficiently rebut that. \$\endgroup\$ Jan 30 at 17:11
  • \$\begingroup\$ @HashimAziz Thank you for the constructive criticism - I have removed the link and added an explanation instead. \$\endgroup\$ Jan 30 at 18:10
  • \$\begingroup\$ @lester I added some links to the documentation about command line usage. Where you put the files is up to you and depending on execution permissions it may be wise to store such files outside of the publicly available web directories. \$\endgroup\$ Jan 30 at 18:19
  • \$\begingroup\$ @HashimAziz I agree on the "didn't read" part but "very good reasons" is sadly a fallacy. For the modern PHP at least, which made a huge leap towards strict typing since that ancient post has been written. There are almost no reasons left to use loose comparisons. And even switch got a strict typed alternative. \$\endgroup\$ Jan 31 at 6:10
5
\$\begingroup\$

As whole, it's a very robust code. Only some inconsistences can be improved.

Error reporting

I would start from the infamous piece of code, which fallacy is only matched by its enormous popularity:

die('An error occurred at auth_user function: ' . $ex->getMessage());

You are making here a very popular mistake, considering yourself the only user of the site. But if you think of it, when the time comes and the site goes live, a regular user would hardly make any sense from that message. At the same time, being unable to stand behind each users's back, you'd never see it.

In reality, error messages are never blurted out at site users like this. Error messages are meant for the programmer. And on a live site they must be logged instead of being printed.

Moreover, even if your goal is to display the error message, there is too much code for this. If you remove try, catch and echo, the result will be pretty much the same: surprisingly, PHP is pretty capable of reporting errors ;-)

Not to mention that PHP can log errors as well. Therefore, all this try and catch stuff is just overkill to report errors. PHP will inform you about all problems without a single line of code like this, so you should remove all that elaborate error reporting from your scripts. You may read more from my article on the most important things every web-programmer should know.

Security

Although you are using parameters in SQL, thus preventing SQL injections, there are two security concerns still:

  1. Passwords must be hashed. Period. There is absolutely no reason to keep them plain.
  2. All output into HTML context must be html-escaped, thus preventing accidental design breaks and XSS attacks.

Database connection

Connecting in the every function makes your code bloated and may lead to problems in the future, when your code will have to run more than one SQL query per request. Besides, there are some useful options that you are missing.

Connect only once, create a single PDO instance and then use it in all your functions (hence you'd have to add a parameter for it). Here you may read more in my other article dedicated to PDO connection.

Usability

Although for the login form the current approach is viable, for the more complex forms you's better implement a more user friendly approach, showing the form back in case of errors, along with error messages and already entered data.

Besides, current transact_user.php creates a minor inconvenience for a user, always asking to resend the data when the page is reloaded. There always must be a redirect after the POST request.

Can't think of any good example at the moment but you can take a look at this.

Separation of concerns / code structure

In the index.php there is a bit too much different things mixed. Error reporting, SQL, HTML... Try to separate these things a bit. At least move the database operation above output. Better yet, compile your pages from four distinct parts:

  • a bootstrap file, where database connection and session_start(), as well as some configuration options might go.
  • the "logic" part that could be called a controller. It does all the data manipulations
  • the page's template
  • the site-wide design template

It will make your code easily maintainable. You can see the full example from the above link.

Other stuff

  • once in require_once is really overkill. You aren't going to include the same script twice, are you? Oh, surely you do, in transact_user.php, including config/db.php first by itself and then inside lib/user.php. So here is one too many and you can safely remove the extra one. Using require instead of require_once will help you to spot such mistakes.
  • htmlspecialchars(nl2br(...)) won't do any good, though you will surely find out it yourself, after adding at least one post with a line break ;-)
  • use absolute paths. One fine day your code may not find such a file as config/db.php. Use at least the __DIR__ palliative, though it doesn't make your path truly independent, rather binds to the current script, making it essentially relative.
\$\endgroup\$
2
\$\begingroup\$

Your code is very dense, but very well indented and easy to read.

However, there are still some easy improvements you can implement to make your code a whole lot better.

Variable names

You have the variable $stmt which stores a prepared statement query string.
Why don't you call it what it actually is?

The one that you could call $stmt is the $posts variable.
You could also call it $posts_stmt, as $posts may be misinterpreted as being an array.

If you want to see how confusing your naming can be, try to read this without any context:

$query = $db_conn->prepare($stmt);

What is that? A database connection (which one of the 20-or-so database drivers?) is preparing a prepared statement and returning a query???

Now, try to read something like this:

$stmt = $pdo->prepare($query);

Holy string!

You have this pattern all over your code:

$stmt = 'CREATE TABLE IF NOT EXISTS post ( ' .
  'id int(11) NOT NULL PRIMARY KEY AUTO_INCREMENT, ' .
  'title varchar(255) UNIQUE NOT NULL, ' .
  'content text NOT NULL, ' .
  'author_id int(11) NOT NULL, ' .
  'created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, ' .
  'updated_at DATETIME NOT NULL ON UPDATE CURRENT_TIMESTAMP, ' .
  'FOREIGN KEY fk_post_author_id (author_id) REFERENCES author (id))';

Instead, consider using a nowdoc:

$stmt = <<<'SQL'
  CREATE TABLE IF NOT EXISTS post (
    id int(11) NOT NULL PRIMARY KEY AUTO_INCREMENT,
    title varchar(255) UNIQUE NOT NULL,
    content text NOT NULL,
    author_id int(11) NOT NULL,
    created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
    updated_at DATETIME NOT NULL ON UPDATE CURRENT_TIMESTAMP,
    FOREIGN KEY fk_post_author_id (author_id) REFERENCES author (id)
  )
  SQL;

Assuming you are doing this elaborate concatenations to avoid extra spacing in the resulting SQL, PHP's heredoc can do it for you! Just align the closing SQL token with your code, and PHP will strip all extra spacing from the text!

Besides being actually readable, and the SQL being properly formatted for humans to read, some editors may also take a hint from the heredoc name (SQL) and do a proper syntax highlighting for the SQL in the string (though all modern editors such as VSCode are pretty capable of sniffing out the format right from the contents.

Multiple database connections

Instead of doing that, consider having a class that handles it all for you.
This class could manage the connection and keep the variable in a static variable.

Something like this (PSEUDOCODE WARNING!!!):


namespace Lib;

/**
 * @extends \PDO
 */
class DB {
  private static $pdo = null;
  
  public static function connect(string $dsn = PDO_DSN, string $user = DB_USER, string $pass = DB_PASS) {
    self::$pdo = new PDO($dns, $user, $pass);
    
    return self::$pdo !== false;
  }

  public static function __callStatic($name, $arguments) {
    return call_user_func_array([self::$pdo, $name], $arguments);
  }
}

This is an example of something you can do.

Your views are messy

Your code for your views is pretty messy, and it can be improved massively.

You can create a class that handles the views, and makes it easier for you to just present something.

Or, you can use a language like twig or Smarty, to keep everything organized.

Manual includes are soooo 2008...

Instead, consider using composer or write your own simple code to do all the including.

If you want to do it yourself, you can do something like this (PSEUDOCODE!):

spl_autoload_register(function ($class_name) {
    include __DIR__ . '/lib/' . $class_name . '.php';
});

Then, just throw everything into the lib directory and create a file with the exact casing as the class name (E.g.: class Session => lib/Session.php).

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Upvoted for heredoc and downvoted for suggesting that static magic jiggery-pokery. It does no harm to learn proper dependency injection from the beginning. Also took a liberty to fix a bit the heredoc part, hope you don't mind. \$\endgroup\$ Jan 31 at 5:53
  • \$\begingroup\$ Also, I disagree with the naming part. I wouldn't call "posts" what is currently $stmt. It's an SQL statement, so it's traditionally called $query or $sql. Calling it $posts_query is a waste: this is a one time variable, used and discarded immediately below. Besides, $posts_query doesn't convey any additional meaning. It's just an SQL query that would be executed, and the code below doesn't depend on its contents. What I would call "posts" is an array/collection that contains actual posts, the result of PDO::fetchAll() call. \$\endgroup\$ Jan 31 at 6:02
  • \$\begingroup\$ Thank you for the edit. I firmly stand with what I said regarding the variable naming. I also think you've misread it, since I'm not calling it "posts" (which can be perceived as an array), but calling it "posts_stmt" (posts statement). Calling it "posts_query" is wrong, in my opinion, because it isn't a query. I do agree with you about proper dependency injection, but, O.P. seems to be a beginner, and dependency injection is something that may be hard to grasp when you're beginning to write PHP. \$\endgroup\$ Jan 31 at 18:50
  • \$\begingroup\$ Dependency injection is the simplest thing in the world and surely can be grasped by anyone. If you pass the PDO object as a function parameter, instead of introducing a global state, it's already a dependency injection! userExists($pdo, $username, $password) - we injected a dependency, a service on which the function depends. As simple as that. \$\endgroup\$ Jan 31 at 19:31
  • \$\begingroup\$ @YourCommonSense In a very short way, yes. However, that kinda "forces" you to keep a global variable. A decent dependency injection system will remove that need, which is where the complexity balloons (at least to me). \$\endgroup\$ Jan 31 at 19:49

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.