PHP gaming community website

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

-
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?

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

-
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

```


Add Game



Game Name:




Image URL:




Visible?
Does our community play this game? Do y

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: ` ($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.

StackExchange Code Review Q#125858, answer score: 18