How is my CMS design? Any security, design, or other issues?

I am trying to learn PHP in depth by creating this personal CMS. I try to utilize OOP concepts and do best I can...

The way I am trying to design this CMS is to keep including 'modules' within a page to minimize code. I am using extensively POST and GET variables to decide which module will be reached. The Index page will be the main page from which modules will be included and accessible.

My questions:

• Will this design be handy when code get more and more complicated? More POST requests etc.

• I could there be any security vulnerabilities?

• Any performance issues? (None noticed)

• Do I utilize code properly as this is my real PHP project?

• I would love some intensive criticism about this code.

Config File:

session();
            $this->getWebsite();
        }
        function connect() {
            $dbc = mysqli_connect($this->dbhost, $this->dbuser, $this->dbpass, $this->dbdb) or die("Error " . mysqli_error($dbc));
            return $dbc;
        }
        function session() {
            if(!isset($_SESSION)) {
                session_start();
            }
        }
        function getWebsite() {
            $dbc = $this->connect();
            $stmt = $dbc->prepare("SELECT * FROM website");
            $stmt->execute();
            $result = $stmt->get_result();
            $this->website = $result->fetch_assoc();
            $stmt->close();
            $this->theme = $this->website['theme'];
        }
    }
?>

Index.php File:

```
theme . "/head.php");
$moduleArray = array_diff(scandir('module/', 1), array('..', '.', 'admin'));

if(($_SERVER['REQUEST_METHOD'] === 'GET' || $_SERVER['REQUEST_METHOD'] === 'POST') && !empty($_GET['page'])) {
$p = $_GET['page'];
$ext = ".inc.php";

if (get_magic_quotes_gpc()) {
$p = stripslashes($p);
}

if(in_array($p . $ext, $moduleArray)) {
include("module/" . $p . $ext);
}
else {
header('Location: /');

Alright, I'll start off by answering the questions you've asked, and depending if it all get's covered, I might append more at the end. We'll see, I have yet to actually read your questions!

So, let's grab the first one...


Will this design be handy when code get more and more complicated? More POST requests etc.

No. Right now, I see no way of expanding this. As soon as it does "get more and more complicated", you will run into lots of problems, especially with the website architecture.

Part of the Object Oriented paradigm is a principle called SOLID. If we start off with the S, which stands for Single Responsibility, we immediately notice multiple flaws in your code.

For instance, your cms class is handling database connections, database querying, and session handling. So really, that tells us right off the bat that cms isn't even necessary! It tells you that you really need a database connection handler, a querying utility, and a separate session handling class. But even those could be broken down further to prevent having all-powerful "Manager" classes.

The second principle in SOLID is code be open and closed. That means open for extension and closed to modification. I see neither in your code right now. We cannot build onto cms or the index file or the demo page. And it's certainly going to be a pain to alter these classes, and since we even need to alter them, your code is not closed.

One possible solution would be follow KISS. Because your classes are responsible for so much, it's hard to keep things separated. This makes it difficult to change page-unique information. Give the functionality room to breathe, don't expect things to always fit in the same class, because that's not OOP.

Study the rest of SOLID, it's a large topic and I would be able to point out other mistakes, but it's best we move on!


could there be any security vulnerabilities?

Well, what are you afraid of? And under what circumstances are the code and the developer being placed in?

I say this because the threats will be different if you're the only one seeing this site, as opposed to the threats where the attacker knows your exact database scheme.

I'll point out some issues I see, but I cannot give a full security audit as I'm not a professional.

• Using .inc.php is a convention, but can be considered a bad one. There are threats associated to using it if you leave off the appended .php. Luckily, you look good! Be careful though.

• As of the PHP docs, using magic quote functions is deprecated. Avoid using any of these as support may fault and open you up to different types of attacks.

• Following that, you strip slash the page. However, that's only if the magic quotes are working. If they're not, the attacker can include any page he wants (given the right permissions). I'd avoid giving the user so much power.

• stripslashes($username) doesn't make sense. I'm not sure why you'd do this if you're using prepared statements. On top of this, I don't even know where you're getting $username from. It's dead code, which is also a security risk.

• All of the code you're injecting into your page via the database better be the the most clean code ever. If an attacker is able to insert page data into your database, they could potentially insert malicious HTML/JavaScript into the pages that every site user will see. Very bad.

This is what I could spot immediately, I'm sure I missed something though.


Any performance issues?

That's hard to say. Why don't you profile your code and see if it bottlenecks at any points? Then examine those parts and see about refactoring them.

From what I can tell, the database looks inefficient and you make a lot of unnecessary calls. Improper connection storage and caching also looks prevalent here. Every time you load a page, you start a new database connection, and then perform a whole new set of queries. Will the theme be changing on every single page load, or is that something you can store?


Do I utilize code properly as this is my real PHP project?

What does "properly" mean? I don't even know how to answer this! Are you asking if this is production worthy? If so, I'd automatically say no for all of the points I have and will point out.


I would love some intensive criticism about this code.

Well perfect, because I still have lots! :)

• First off, follow a PHP coding style standard. To start you off, I'll point you in the almost official PHP-FIG standard. It's important that every bit of code is consistent and readable. Follow the styles for naming things especially.

• Avoid shorthanding names such as "database" in names like $dbhost. It doesn't provide any benefit to shorten. Same with "password" in $dbpass.

• $dbdb is ridiculous. This obviously needs a new name.

• $dbc has no meaning or clarity what-so-ever. Give your variable context!

• Things like public $website should not be public. Encapsulate and give them getters and setters.

• Your con

StackExchange Code Review Q#57863, answer score: 29