17
\$\begingroup\$

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

screenshot of manage games page

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>");
}
\$\endgroup\$
  • 13
    \$\begingroup\$ While you're considering major changes, also consider a rewrite in another language. \$\endgroup\$ – Michael Hampton Apr 16 '16 at 18:58
  • 3
    \$\begingroup\$ @MichaelHampton I was thinking about that too, although some quick googling indicated to me that PHP is still a very popular language for server-side coding. Do you have an alternative suggestion? \$\endgroup\$ – AdmiralAdama Apr 16 '16 at 20:03
  • 4
    \$\begingroup\$ It's popular, but that doesn't mean that it's good! I've been disillusioned with PHP for many years for a variety of reasons. After a brief affair with Ruby on Rails, I've mostly settled on Go for server-side web apps. \$\endgroup\$ – Michael Hampton Apr 16 '16 at 20:05
  • 8
    \$\begingroup\$ To offer another perspective, after 6 years of PHP followed by 2 years of Rails, I sometimes miss PHP. Like everything, Rails certainly has its downsides too, though they're less immediately obvious or as glaring. PHP can be a perfectly decent language if you use a good framework and know what blemishes of the language to avoid or minimize. It's certainly worth exploring other options (Rails really is great!), but PHP as a community has come a long way since the days of it being spaghetti insanity, and the language is continually improving. \$\endgroup\$ – Corbin Apr 17 '16 at 4:01
  • 4
    \$\begingroup\$ If you're interested in a solid, objective argument for why not PHP, I don't think any of us could do better than PHP: a fractal of bad design. Yes, PHP is still popular, but popularity isn't always the best measure of quality. If you don't find the kinds of issues raised their to be problems, then maybe you will be fine with PHP. @MichaelHampton suggested Go, but Ruby, Node.js, Python, and even .NET are quite widely used as well. You may also find some of their frameworks easier to get up and running with. \$\endgroup\$ – jpmc26 Apr 17 '16 at 7:36
18
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I bit the bullet and started learning Laravel. It took me 2 hours to get it installed on HostGator. I was getting tons of errors. A steep learning curve so far. I hope learning a framework is worth it. \$\endgroup\$ – AdmiralAdama Apr 17 '16 at 0:34
  • \$\begingroup\$ I edited my post to include my site's security features. \$\endgroup\$ – AdmiralAdama Apr 17 '16 at 13:27
  • \$\begingroup\$ In addition to the code duplication, I would advise against using print/echo to output HTML. It isn't necessary, adds (probably mostly theoretical) overhead, messes with (naive) syntax highlighters, etc. Either way, the main brain-melter I encountered was how to come up with a viable indentation scheme that covers both PHP and HTML realms... and if you get one that works in your source, it makes the output HTML look kinda awful, but of course that's less important. </digression> \$\endgroup\$ – underscore_d Apr 17 '16 at 13:28
  • 1
    \$\begingroup\$ @AdmiralAdama I added a small paragraph on those features. They are definitely a lot better than nothing, but I wouldn't rely on them. \$\endgroup\$ – tim Apr 17 '16 at 16:41
  • 1
    \$\begingroup\$ @AdmiralAdama Yes, at the beginning, the learning curve is somewhat steep (and especially the setup is often quite annoying). But once you are familiar with one web framework, it is relatively easy to adapt to a different one, as most are build somewhat similar. And using one will often safe you quite a bit of time building new websites and adding new functionalities (and you gain security, as the framework is maintained for you, and also provides you with the correct mechanisms). \$\endgroup\$ – tim Apr 17 '16 at 17:00
11
\$\begingroup\$

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.

\$\endgroup\$
  • 6
    \$\begingroup\$ My problem with this approach would be that 1) these are unlikely to be the only security issues. The code was written 13 years ago, with no concern for security. There are likely to be insecure includes and file uploads, and insecure use of potentially dangerous functions such as extract, eval, etc all over the place 2) patching it up will catch many of the insecure queries and prints, but it is extremely likely that some will be missed. 3) If you think that the website is so complex that a rewrite will take a year, I doubt it can be made even somewhat secure in a day. \$\endgroup\$ – tim Apr 17 '16 at 11:31
  • 1
    \$\begingroup\$ If the OP doesn't want to take the website offline, patching it up in a day may be a temporary solution, but I wouldn't feel good hosting anything like that on my server. Security really isn't something that you can just add on as an afterthought, you need proper mechanisms in place. I do understand that "just rewrite it" is suggested too often, but with such an old and broken website, it really seems like the best approach. \$\endgroup\$ – tim Apr 17 '16 at 11:31
  • \$\begingroup\$ I edited my post to include my site's security features. \$\endgroup\$ – AdmiralAdama Apr 17 '16 at 13:27
  • 1
    \$\begingroup\$ If you want to rewrite it and stay with PHP, then you can try to rewrite one page/module/function at a time. \$\endgroup\$ – immibis Apr 18 '16 at 0:43
  • \$\begingroup\$ I'd agree with this approach as well - I don't know where the OP is in his life, but personally I have a job and I don't have the time to undertake such a big project ... for less than 300 users? I'd fix the security holes, maybe check in with the top parts of the community to see if there's any easy features (on the scale of 15 minutes to 1 hour) to stick in. But that's that... There's just no time to spend weekend after weekend on something like this. \$\endgroup\$ – Pimgd Apr 18 '16 at 9:05
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\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.