PHP gaming community website

Question

I created a gaming community website about 13 years ago. It uses PHP, SQL, HTML, and CSS, and does not use a framework or template engine. It includes features such as a login system for members, different levels of access depending on your rank, dropping members that are inactive from the member list, and other features useful to a gaming community. At its height, the gaming community that this website supported had 300 active members, so this code has been in production for a while. This website's PHP code has about 97 versions.

I am behind on PHP best practices. I am thinking of upgrading the site, as practice to get me back up to speed, and from there I can use that knowledge and skill to create new websites. However, I would like some advice on what direction to take this website and my PHP programming in general.

Questions

1. Do I need to learn a framework like Laravel and convert everything to that? The learning curve for frameworks looks steep. Is it worth it? What kinds of features do they provide?

2. Do I need to start using classes and OOP? What are the benefits? Where would I start? As you can see, all of my code from this sample page is non-OOP.

3. I am already aware of a couple of things that I should upgrade. For example, I need to convert from MYSQL to PDO for security, a template engine like Smarty is better for code readability, I should sanitize my incoming $_POST data, I should use an ENUM for access levels instead of numbers, etc. There's a lot that needs to be upgraded.

Sample code

Here is a page in the member area that allows high ranking members to add, edit, and delete games, and pick which games that the community supports. Whatever games are supported are added to the join form, member profiles, etc. for members to choose whether or not they play them.

Screenshot

games_manage.php

<?php

if (authenticate ("9", "0", "1") == "verified")
{
    // *** ADD FORM PROCESSOR ****
    if ( $_GET['action'] == "add" && $_POST['submit'] == "Add Game" )
    {
        $game_name = $_POST['game_name'];
        $game_image_url = $_POST['game_image_url'];
        $game_active = $_POST['game_active'];

        $query = mysql_query("INSERT INTO game_list SET game_name = '$game_name', game_image_url = '$game_image_url', game_active = '$game_active';");

        addtolog("$username_l added $game_name to the list of games played");
        feedback ("Add Game", "Successful", "Manage Games", "console.php?id=games_manage");
    }
    // *** ADD ****
    else if ( $_GET['action'] == "add" )
    {
        print("

<form action=\"console.php?id=games_manage&amp;action=add\" method=\"post\">
    <h2><center>
    Add Game
    </center></h2>

    <p align=\"center\">
    <b>Game Name:</b><br />
    <input name=\"game_name\" size=\"20\" />
    </p>

    <p align=\"center\">
    <b>Image URL:</b><br />
    <input name=\"game_image_url\" size=\"50\" />
    </p>

    <p align=\"center\">
    <b>Visible?</b><br />
    Does our community play this game? Do you<br />
    want this game to show up on the website?<br />
    <input type=\"radio\" name=\"game_active\" value=\"1\" checked /> Yes<br>
    <input type=\"radio\" name=\"game_active\" value=\"0\" /> No<br>
    </p>

    <p align=\"center\">
    <input type=\"submit\" value=\"Add Game\" name=\"submit\" size=\"10\">
    <input type=\"reset\" value=\"Reset\" name=\"reset\" size=\"10\">
    </p>
</form>

        ");
    }
    // *** DELETE FORM PROCESSOR ****
    else if ( $_GET['action'] == "delete" )
    {
        $game_id = $_GET['game'];

        $query = mysql_query("SELECT game_name FROM game_list WHERE game_id = '$game_id';");
        $myrow = mysql_fetch_array($query);
        extract($myrow);

        $query = mysql_query("DELETE FROM game_list WHERE game_id = '$game_id';");
        $query = mysql_query("DELETE FROM game_players WHERE game_id = '$game_id';");

        addtolog("$username_l deleted $game_name from the list of games played");
        feedback ("Delete Game", "Successful", "Manage Games", "console.php?id=games_manage");
    }
    // *** EDIT FORM PROCESSOR ****
    else if ( $_GET['action'] == "edit" && $_POST['submit'] == "Edit Game" )
    {
        $game_id = $_GET['game'];
        $game_name = $_POST['game_name'];
        $game_image_url = $_POST['game_image_url'];
        $game_active = $_POST['game_active'];

        $query = mysql_query("UPDATE game_list SET game_name = '$game_name', game_image_url = '$game_image_url', game_active = '$game_active' WHERE game_id = '$game_id';");

        addtolog("$username_l edited the details of the game $game_name");
        feedback ("Edit Game", "Successful", "Manage Games", "console.php?id=games_manage");

    }
    // *** EDIT ****
    else if ( $_GET['action'] == "edit" )
    {
        $game_id = $_GET['game'];

        $query = mysql_query("SELECT * FROM game_list WHERE game_id = '$game_id';");
        $myrow = mysql_fetch_array($query);
        extract($myrow);

        if ($game_active == "0")
            {$checked0 = "checked";}
        elseif ($game_active == "1")
            {$checked1 = "checked";}

        print("

<form action=\"console.php?id=games_manage&amp;action=edit&amp;game=$game_id\" method=\"post\">
    <h2><center>
    Edit Game
    </center></h2>

    <p align=\"center\">
    <b>Game Name:</b><br />
    <input name=\"game_name\" size=\"20\" value=\"$game_name\" />
    </p>

    <p align=\"center\">
    <b>Image URL:</b><br />
    <input name=\"game_image_url\" size=\"50\" value=\"$game_image_url\"  />
    </p>

    <p align=\"center\">
    <b>Image:</b><br />
    <img src=\"$game_image_url\" width=\"$game_icon_width_in_px" . "px\" alt=\"Error!\" title=\"$game_name\" />
    </p>

    <p align=\"center\">
    <b>Active?</b><br />
    Does our community play this game? Do you<br />
    want this game to show up on the website?<br />
    <input type=\"radio\" name=\"game_active\" value=\"1\" $checked1 /> Yes<br>
    <input type=\"radio\" name=\"game_active\" value=\"0\" $checked0 /> No<br>
    </p>

    <p align=\"center\">
    <input type=\"submit\" value=\"Edit Game\" name=\"submit\" size=\"10\">
    <input type=\"reset\" value=\"Reset\" name=\"reset\" size=\"10\">
    </p>
</form>

        ");
    }
    // *** LIST ****
    else
    {
        print("

<h2><center>
Manage Games
</center></h2>

<p>
Use this page to manage the games that your users can mark that they play. Useful for communities that support multiple games. You can even add games not listed here.
</p>

<p align=\"center\">
<input type=\"submit\" value=\"Add Game\" onclick=\"window.location='console.php?id=games_manage&amp;action=add';\" /> 
</p>

<center>
<table width=\"500px\" class=\"valign_middle\">
    <tr class=\"valign_middle\">
        <th width=\"110px\" class=\"valign_middle\">Options</th>
        <th class=\"valign_middle\">Visible?</th>
        <th class=\"valign_middle\">Game</th>
        <th class=\"valign_middle\" align=\"center\">Image</th>
        <!-- <th class=\"valign_middle\">Image URL</th> -->
    </tr>

        ");

        $query = mysql_query("SELECT * FROM game_list ORDER BY game_active DESC, game_name ASC;");
        while ( $myrow = mysql_fetch_array($query) )
        {
            extract($myrow);
            if ( $game_active == 1)
            {
                $game_active_text = "<b>Yes</b>";
            }
            else
            {
                $game_active_text = "No";
            }

            print("

    <tr class=\"valign_middle\">
        <td class=\"valign_middle\">
            <input type=\"submit\" value=\"Edit\" onclick=\"window.location='console.php?id=games_manage&amp;action=edit&amp;game=$game_id';\" />
            <input type=\"submit\" value=\"Delete\" onclick=\"window.location='console.php?id=games_manage&amp;action=delete&amp;game=$game_id';\" />
        </td>
        <td class=\"valign_middle\">
            $game_active_text
        </td>
        <td class=\"valign_middle\">
            $game_name
        </td>
        <td class=\"valign_middle\" align=\"center\">
            <img src=\"$game_image_url\" width=\"$game_icon_width_in_px" . "px\" alt=\"Error!\" title=\"$game_name\" />
        </td>
        <!--
        <td class=\"valign_middle\">
            $game_image_url
        </td>
        -->
    </tr>

            ");
        }

        print("

</table>
</center>

        ");
    }
}

?>

Security

My website's lack of security has been mentioned in several posts. I do have some security, since this is production code. I just didn't post the gigantic function and navigation files that get loaded before the code for this specific page.

SQL injection defense code

This code runs mysql_real_escape_string on all incoming GET, POST, and COOKIE data.

if ( get_magic_quotes_gpc() != 1 )
{
    foreach ($_POST as $key=>$val)
    {
        $_POST[$key]=mysql_real_escape_string($val);
    }
    foreach ($_GET as $key=>$val)
    {
        $_GET[$key]=mysql_real_escape_string($val);
    }
    foreach ($_COOKIE as $key=>$val)
    {
        $_COOKIE[$key]=mysql_real_escape_string($val);
    }
}

XSS defense code

This code runs strip_tags on all incoming GET, POST, and COOKIE data if the user is not a high rank in the clan. High ranking users are assumed to be trusted to include HTML tags in their incoming form data, so that they can do things like add HTML to their internal news posts.

Yes, this is vulnerable. Adding this, at the time, was easier than sanitizing all incoming data and all outgoing data on a case-by-case basis.

if (authenticate ("6", "0", "0") != "verified")
{
    if (is_array ($_GET))
    {
        while (list($k, $v) = each($_GET))
        {
            if (is_array ($_GET[$k]))
            {
                while (list ($k2, $v2) = each ($_GET[$k]))
                {$_GET[$k][$k2] = strip_tags ($v2);}
                @reset($_GET[$k]);
            }
            else
            {$_GET[$k] = strip_tags($v);}
        }
        @reset($_GET);
    }

    if (is_array ($_POST))
    {
        while (list($k, $v) = each($_POST))
        {
            if (is_array ($_POST[$k]))
            {
                while (list ($k2, $v2) = each ($_POST[$k]))
                {$_POST[$k][$k2] = strip_tags ($v2);}
                @reset($_POST[$k]);
            }
            else
            {$_POST[$k] = strip_tags($v);}
        }
        @reset($_POST);
    }

    if (is_array ($_COOKIE))
    {
        while (list($k, $v) = each($_COOKIE))
        {
            if (is_array ($_COOKIE[$k]))
            {
                while (list ($k2, $v2) = each ($_COOKIE[$k]))
                {$_COOKIE[$k][$k2] = strip_tags ($v2);}
                @reset($_COOKIE[$k]);
            }
            else
            {$_COOKIE[$k] = strip_tags($v);}
    }
        @reset($_COOKIE);
    }
}

CSRF

I do not have a defense against CSRF, since I only read about that the other day, shortly before making this code review.

Expiring cookies

Cookies (and therefore user logins) expire after 30 days.

setcookie ($cookie_prefix . "_name", $username, time () + 2592000, "/", $domain_name);
setcookie ($cookie_prefix . "_pass", $password, time () + 2592000, "/", $domain_name);
setcookie ($cookie_prefix . "_name", $username, time () + 2592000, "/", 'www.' . $domain_name);
setcookie ($cookie_prefix . "_pass", $password, time () + 2592000, "/", 'www.' . $domain_name);

Sessions

I chose to not use sessions. The cookies handle everything related to user logins.

IP ban for too many login attempts

More than 10 failed login attempts results in a 1 day IP ban from the entire site. The IP ban looks at the first 3 digits of the IP address, which helps to block some types of dynamic IP where the 4th digit will change but the rest of the digits will stay the same. This is a less effective technique now, with the prevalence of static IP's at home, less AOL users (often dynamic IP following the aforementioned pattern), and mobile phones with constantly changing 3G/4G IP's, but it was a decent idea back in the day.

I have not added support for IPv6. If somebody connects to my website using an IPv6, it may throw some kind of error when my code tries to add the overly large IPv6 IP to the IP log. I haven't tested it yet.

// unsuccessful login
else
{
    $query = mysql_query ("SELECT * FROM logins WHERE ip1 = '$ip1' AND ip2 = '$ip2' AND ip3 = '$ip3';");
    $myrow = mysql_fetch_array ($query);
    if ($myrow == NULL)
    {
        $query = mysql_query ("INSERT INTO logins SET ip1 = '$ip1', ip2 = '$ip2', ip3 = '$ip3', logins = '1', lastseen = '$datetime';");
    }
    else
    {
        extract ($myrow);
        $period = date ("Y-m-d H:i:s", mktime (date ("H"), date ("i"), date ("s"), date ("m"), date ("d") - 1, date ("Y")));
        if ($lastseen >= $period)
        {
            $logins = $logins + 1;
            $query = mysql_query ("UPDATE logins SET logins = '$logins', lastseen = '$datetime' WHERE ip1 = '$ip1' AND ip2 = '$ip2' AND ip3 = '$ip3';");
        }
        else
        {
            $query = mysql_query ("UPDATE logins SET logins = '1', lastseen = '$datetime' WHERE ip1 = '$ip1' AND ip2 = '$ip2' AND ip3 = '$ip3';");
        }
        if ($logins == "10")
        {
            $bantime = date ("Y-m-d H:i:s", mktime (date ("H"), date ("i"), date ("s"), date ("m"), date ("d") + 1, date ("Y")));
            $query = mysql_query ("INSERT INTO ipbans SET ip1 = '$ip1', ip2 = '$ip2', ip3 = '$ip3', expiration = '$bantime', description = '10 Failed Logins (Auto Ban)';");
        }
    }

    print ("<html><head><meta http-equiv=\"refresh\" content=\"3;url=console.php\"></head><body><h2>Login Incorrect!</h2><p><a href=\"javascript:history.go(-1)\">Go Back</a></p></body></html>");
}

Answer 1

If this code is representative of all your code, then yes, I would suggest a complete rewrite, using more modern concepts (OOP, MVC, template engine, some framework, etc).

Until then, I would probably take the website offline, or at the very least add a web application firewall to the server.

Security

Your code is currently vulnerable to SQL injection, XSS, and CSRF.

You need to look into prepared statements to be secure against SQL injection. Some frameworks also provide ORMs or query builders, which may make it easier for you to securely access your database.

You then need to HTML encode user input when echoing it. Most templating engines also provide this functionality, and some encode per default (which I would recommend).

Most frameworks also offer some mechanism to protect against CSRF.

Structure

Your code is too long and not well structured, as you don't have any functions.

Because of this, it also contains quite a bit of duplication (eg the edit/add forms which are quite similar, duplicated select queries, etc).

Most frameworks would make it easier to structure your code well, as they for example use MVC, or expect you to program things a certain way.

Misc

• extract is a function that you mostly shouldn't use. It can have security implications, and it leads to less readable code. Just assign things to variables explicitly yourself.
• You can use ' and " for strings. You can use this to avoid having to escape " in all your forms.
• you can use guard clauses to decrease the nesting of your code and make it more readable. Eg instead of if (authenticate ("9", "0", "1") == "verified") {long block of code} you would write if (authenticate ("9", "0", "1") !== "verified") {return or die} long block of code.

Regarding your security code

I'm glad to see that you have some defenses in place so that you are not completely vulnerable.

That being said, having one (or more) input filter functions in place is not really a good approach to security. It will catch some attacks (maybe even most), but there are always cases where it will not work.

It will also lead to usability problems and bugs, which isn't good, but not necessarily your main concern. Still, these bugs can have security implications. For example, my super secure password i<3super_secure_passwords becomes the way less secure i for your website (because of strip_tags).

You are also still vulnerable to XSS. strip_tags doesn't work when you are already in a tag, eg here: <input name=\"game_name\" size=\"20\" value=\"$game_name\" /> ($game_name is user supplied, and an attacker can add additional attributes to get into a JavaScript context).

Allowing your mods to post any tags is also not a good idea, as they can relatively easily escalate their privileges to admin. You don't need to write some kind of filter yourself (and you shouldn't, as it's quite difficult), just use an existing library, eg HTML Purifier.

Regarding SQL injection, the code you posted is probably safe (although strongly discuraged; there is a reason that magic quotes isn't enabled by default anymore), but if you have queries where user input is not surrounded by quotes, it will not work (eg when passing ids, when using limit, etc).

Also, don't store passwords in cookies, and make cookies httpOnly.

Answer 2

I'm going to provide a different type of answer: do almost nothing.

If you ask a programmer what to do, then they will almost always say "rewrite". This is often a catastrophic decision.

If you rewrite using a new-to-you templating engine, then it will take perhaps a year, and you will have a website that has no new features, but new bugs. Your website will fail.

If you rewrite using a new language or technological method, then it will take a year, and you will have a website that has no new features, but new bugs. Your website will fail.

Looking at your code sample, the website is so old that you should instead be doing maintenance only. In this case you need to fix the security problems. You must do this, and I strongly advise you do nothing else.

In the first instance you must use htmlspecialchars() when displaying text, and you must use mysqli_real_escape_string() when inserting text into your database. Others will tell you to rewrite, use PDO, refactor, AJAX etc. Resist these calls to rewrite everything. Take the simple approach and you can fix the severe security issues in a day. If you try anything bigger, then the project will fail, and your website will fail.

Answer 3

1) It is not necessary for you to start using Laravel or other PHP framework, the learning curve will depend on your understanding of PHP. Down below I leave you a link to a post which includes the most important features of Laravel: enter link description here

2) You should start implementing OOP in your project, it gives it a better modularity and less coupling. I think that you should start, first, taking a look to AngularJS, your code looks very coupled with the html. You should try to take out all your PHP code of the html and use Angular/jQuery to make AJAX calls to your PHP server asking for the data. So at least, the refactor to OOP and Laravel/other framework you only make it in the back end.

3)As I think that using php code with html just does not look right, at least for me. It's very hard to read and to maintain.

Answer 4

Too many things in one page

You rely primarily on query string parameters to distinguish between the different actions the user is requesting. The modern trend is to instead set up different locations for different operations. For example, it appears that in your code, you rely on the query string parameter named action to determine what to do. So your URLs might look something like this:

• Add game: https://mysite.com/?action=add
• Edit game: https://mysite.com/?action=edit
• Delete game: https://mysite.com/?action=delete

Instead, it would be better to have completely separate paths for each:

• Add game: https://mysite.com/addgame
• View/Edit game: https://mysite.com/game/mycoolgame (where mycoolgame is the game's ID)
• Delete game: https://mysite.com/game/mycoolgame/delete (alternatively, you could use a DELETE HTTP request, but this is, imo, a matter of preference.)

You may reuse some paths and distinguish between operations by the HTTP method, but you could also split certain operations out into their own locations as well. The bottom line is really let the URL path do some of this logic for you, instead of gigantic if-else-if blocks. This will simplify your code and reduce the impact of programming mistakes.

OOP vs. Procedural

Other users have strongly recommended that you switch to the Object Oriented Paradigm, but opinions on its virtues and drawbacks vary widely. For example, there is quite a push for all the major languages to support programming in the functional paradigm. I will not advocate for or against using the OO paradigm, but I do want to make you aware that OOP has by no means clearly beaten out the opposing procedural and functional paradigms. Furthermore, OOP isn't magic. You will not immediately realize the promised benefits of looser coupling and better organization just by using objects; it takes skill to identify the proper abstractions to achieve those benefits (true of any paradigm). My advice would be to use objects where you can see uses for them. If that's your whole program, then follow an OOP style; if not, procedural code and functional code are not automatically bad anymore than OOP code is.