Phishing application God class

This project is ultimately my senior thesis to graduate from college. It will end up being very long with several code reviews and redesigns. The next rubberduck perhaps?

I've been working on a project that tests awareness of phishing scams. It generates emails based on templates and sends them out to a database of emails if they haven't received a "project" of that nature recently.

This utilizes a very in-depth GUI to send out emails based on inputs provided by a user, an interface to view and track the results generated off of webbugs placed in the emails and on associated webpages, a design view to create new email templates (currently, you must input HTML and eventually plain text is inputted and parsed as HTML), and links to other relevant pages that are associated with the company using the product and information about the project.

The entire project can be found here on GitHub.

Ultimately, I'm looking to address these four major points:

• Security

• Encapsulation

• Cleanliness

• Readability

This means that I ultimately need to take my God Class that I used to create a demoable product to show people, and separate it out into several working classes while keeping the root controller intact to function as the logic class of the application.

To make this more readable, I've separated it down to the methods within the class and made each it's own code segment. Something also worth mentioning is that I may end up rewriting this into another language to be used as a learning experience. With that said, what would be some strong languages to use that are relevant in today's world that this application would be effective in? With all of that said, here we go:

PhishingController

Webbug execution - email and website:

```
public function webbugExecutionWebsite($urlid) {
$db = $this->openDatabaseDefault();
if(!empty($_SERVER['REMOTE_ADDR'])) {
$ip = $_SERVER['REMOTE_ADDR'];
$host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
$req

Security

Try to follow current best practices. If you are very new to web security, take a quick look at the OWASP Top 10.

SQL Injection

You are open to SQL Injection via the $urlid parameter as well as $browseragent, $reqpath, and possibly $ip and $host. As well as do second order injection via $username and $projectName. And that's just in your first two functions, your other functions are vulnerable as well.

It should also be said that casting to int or escaping input is not the best approach to SQL injections.

Current best-practice is to use prepared statements. It's safer, it's more readable, it's easy to use. There is really no excuse to not use them.

XSS

You echo variables without encoding, which can lead to XSS. Examples are $toEmail, $user['USR_Username'], etc.

Ideally, you should use a templating engine, which encodes variables by default. This is more secure, and also results in better code.

Structure / Encapsulation / Readability

Try to read up on MVC, and try to make more use of Laravel functionality. For example, Laravel has a query builder, templates, etc.

Globals

From a quick first look at your code, there is a lot of work being done on global variables, either via superglobals or via setenv.

Duplication

webbugExecutionWebsite and webbugExecutionEmail contain a lot of duplicate code (they are essentially the same function). Try to extract that duplication to functions which you then reuse.

Your functions do too much

Your functions do way too much, which is why you have so much duplication. And why your code is quite difficult to read, or change, or test.

A good example is sendEmail which indeed sends mail, but also proccesses a request, does some work on projects and templates, logs stuff, selects users from the database, and updates users in the database, exists, and prints information to the user.

None of your functions should ever exit, and only your functions that are explicitly meant to print something should print something.

Passing God Objects

Functions should accept the specific arguments they need, not some god request object. So instead of this:

public function updateDefaultEmailSettings(Request $request) {
    $username = $request['usernameText'];
    $company = $request['companyText'];
    $host = $request['mailServerText'];
    $port = $request['mailPortText'];
    $userId = \Session::get('authUserId');

write this:

public function updateDefaultEmailSettings($username, $company, $host, $port, $userId) {

Now it's explicit what this function needs, and you can even use it outside of a specific request (eg when testing).

It does accept quite a lot of arguments, but if you want to reduce the size, you need to do it right: Combine variables that logically belong together. Eg host and port could be part of a URL class.

Formatting / Cleanliness

Your formatting and code cleanliness is pretty good. Your code is mostly consistent and well formatted, your variable names are understandable, etc. A couple of small points:

• You mix between snake_case and camelCase (and sometimes just nocase, eg urlid). Choose one and stick with it.

• Your variable names are sometimes not consistent. For example the request usernameText vs username variable vs USR_Username database field. Here it's just the pre-/post-fix that is different, but another example is authUserId vs USR_UserId vs authCheck for the same value. Try to name the same things the same to avoid confusion.

StackExchange Code Review Q#133504, answer score: 5