So, some of my feedback. Please note that all is meant constructively - I'm not looking to just bash, but most of these are pretty serious/big points that really do need to be addressed.
Also note that I've only grabbed some excerpts from the code - the same issues may exist in other places that I haven't looked. There are more issues, but I have a limited amount of time for writing this post, and these issues are a good place to start. I'll look at the code more once these points are dealt with.
Also, an important bit of advice:
Changing your code is okay. Code isn't write-once - if something gets messy, it's perfectly okay to rewrite it until it's clean and easy to understand. Even large changes in the internals of your codebase are completely okay - for example, switching to an ORM (this will be explained later in this post) is absolutely worth it in the long run, even if it means you will need to change significant parts of your code.
License
Well, BoxBilling iirc isn't completely opensourced,
Like mentioned before, KwiBill - according to the included license.txt, anyway - is not open-source, it just doesn't obfuscate the code (ie. the code is visible). Whether to distribute the 'Community Edition' as open-source or as freeware is, of course, completely up to you, but please make sure that you only call it "open-source" if it actually uses an
OSI-compliant license - preferably a pre-existing one rather than a custom one (more info about that
here). Otherwise, "freeware" is the correct term.
Database interaction
You're using PDO with parameterized queries - that's good. However, consider using an ORM such as
DataMapper or any of the
other options for CodeIgniter. It will make it much easier to write your database code, and make it much less error-prone.
There are a few snippets like this:
$this->ci->db->query('INSERT INTO `support_tickets`(`id`, `customerid`, `fullname`, `message`, `status`) VALUES (NULL,?,?,?,"1")',array($token,$fullname,$message));
$query = $this->ci->db->query('SELECT * FROM `support_tickets` WHERE `customerid` = ? AND `fullname` = ? AND `message` = ?',array($token,$fullname,$message));
foreach($query->result() as $row)
{
mail($email, 'Ticket Created', 'Ticket Created. To view it please visit: '.base_url().'main/support/viewticket/'.$row->id.'/'.$token);
redirect(base_url().'main/support/viewticket/'.$row->id.'/'.$token);
}
You seem to be trying to get the ID of the just-inserted row, but this is not the correct way of doing that (and it is extremely error-prone). Instead, use
lastInsertId.
Random generation
$token = rand(10,200);
$this->ci->db->query('INSERT INTO `support_tickets`(`id`, `customerid`, `fullname`, `message`, `status`) VALUES (NULL,?,?,?,"1")',array($token,$fullname,$message));
You're using `rand` for generating a 'secret' token - this is a very bad idea. `rand` and `mt_rand` are not truly random (ie. they're not "cryptographically secure random number generators"), and are relatively easy to guess (which is not something you want for a secret token). Unfortunately, CodeIgniter's random_string method is
also broken, so that's not a good choice either.
Instead, use the
RandomLib library. Make sure to read the README well - you need a medium strength generator.
Unnecessary code
if(!is_numeric($id))
{
redirect(base_url().'main/support/'); exit(); // just incase redirect fails
}
You don't need that `exit` statement there - the `redirect` method already does this for you. You should keep your code to the minimum required to make it work - unnecessary code makes it harder to spot real bugs, and harder to understand your code (even for yourself - which can lead to more bugs when you modify your code).
Poor comments
// process response
// Show ticket
// Show replies
Most of the comments are along these lines, but they're really not very useful like that.
The golden role of comments is this:
A comment shouldn't describe what you're doing, but why you're doing it.
What the purpose is of a block of code, should be obvious from the code itself - the function names, the arguments, the structure, and so on. If it isn't obvious from the code what it does, then that just means you need to refactor the code until it is. Dividing code into smaller, reusable chunks is usually a good idea.
Variable names
$this->ci->db->query($sql,array($fn,$ln,$ad,$st,$cn,$zp,$ph,$em,$pw));
These kind of variable names are very, very bad. They do not
at all describe what they contain, and it's completely unclear what they do. A variable name should always clearly describe what it contains.
Most importantly:
There is never a good reason to abbreviate variable names. Longer variable names do not make your code faster, or meaningfully smaller. Abbreviating variable names just makes your code harder to read. Don't go Java-style a la AbstractObjectFactoryMethodGenerator, but a name like $email_address is a much better name than $em or $emt.
Procedural code
Your code is very procedural - even if it
technically uses classes, it doesn't use them correctly, and you end up having a 'monster class' with all the functionality in it. This makes it hard to write understandable code. Again, an ORM should make this a lot easier.
Client data removal
if(password_verify($password, $row->password))
{
// valid, user information will be encrypted
$id = $row->id;
$firstname = sha1($row->firstname);
$lastname = sha1($row->lastname);
$address = sha1($row->address);
$city = sha1($row->city);
$country = sha1($row->country);
$zip = sha1($row->zip);
$phone = sha1($row->phone);
$email = sha1($row->email);
$sql = "UPDATE `customerdata` SET `firstname`=?,`lastname`=?,`address`=?,`city`=?,`country`=?,`zip`=?,`phone`=?,`email`=? WHERE `id` = ?";
$this->ci->db->query($sql, array($firstname,$lastname,$address,$city,$country,$zip,$phone,$email,$id));
// Erase user information
$sql = "DELETE FROM `customerdata` WHERE `id` = ?";
$this->ci->db->query($sql, array($id));
$this->ci->session->sess_destroy();
redirect(base_url().'filter/erased');
}
else
{
redirect(base_url().'filter/error');
}
I like the idea of this, but unfortunately the implementation is completely broken:
- SHA1 is hashing, not encryption.
- Encryption is not useful here (nor is hashing).
- You seem to be trying to implement a form of 'secure erase' (ie. overwrite) - but it doesn't work like that. If anything in your stack is journaled - some databases are, and most modern filesystems are - then it won't work at all as the place on disk where your modifications are written to, is an entirely different place from where the original data was.
Unless the database explicitly provides support for 'secure erase', and the filesystem also supports it, you
cannot securely erase customer data. Just make it a plain DELETE statement and call it a day - that's the best you can do.
Also keep in mind that it's often better to 'blank out' the data and set a 'deleted' column to '1' or something like that, rather than outright removing the records - that way, you don't get broken references from other tables. Also keep in mind that this functionality may not actually be legal in all jurisdictions, for tax reasons.
Templates
$content = 'Update your account settings.<br>';
$content .= '<form method="POST" action="account/update"><br>';
$content .= $this->ci->csrf->csrf_html();
$content .= '<input type="text" name="email" value="'.$email.'" class="form-control" name="email" placeholder="Email"><br>';
$content .= '<input type="password" name="password" value="" class="form-control" name="password" placeholder="Password"><
This is everywhere throughout the code, and it's a very bad idea - it's hard to maintain, hard to understand, and very easy to mess up your layout. You should be using a template engine of some sort.
I have no experience with CodeIgniter, so I can't recommend you any particular library, but look for any library that at the very least supports variables, loops and conditionals.
Email validation
$emt = $em;
$emt = str_replace('@', '', $emt);
$emt = str_replace('.', '', $emt);
if(ctype_alnum($emt))
{
$sql = "INSERT INTO `customerdata`(`id`, `firstname`, `lastname`, `address`, `city`, `country`, `zip`, `phone`, `email`, `password`) VALUES (NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
$this->ci->db->query($sql,array($fn,$ln,$ad,$st,$cn,$zp,$ph,$em,$pw));
redirect(base_url().'main/login');
} else {
redirect(base_url().'main/register?invalid_entry');
}
This is not the correct way to validate e-mail addresses. Use
filter_var instead.
