Security Tips for this Tiny App of mine

PHP
1

Hi,

I’ve recently built an uber basic Guestbook App in PHP and wondered if any kind soldier/soldieress would be able to take a quick scan over it?

I’m basically just looking for feedback on the security of the forms (namely the add_entry and getEntries functions) and maybe even general tips on how to code it a bit better.

Many thanks for taking a look and any pointers you can give.

2

I haven’t looked through all of your code, but this stood out to me:

  • For the delete action it is not checked if $_GET[‘id’] exists. I would change line 106 of index.php to


    $id = isset($_GET['id']) ? intval($_GET['id']) : false;
  • In the both the add_entry and delete_entry function you have $mysqli->close(); that will be never reached because the function will definitely have returned before it reaches that statement (if you know Java, the compiler would give you the “Unreachable code” error for this one).
  • The function getEntries doesn’t close the MySQL connection at all
  • Normally you’d open a DB connection once at the start of the script and close it at the end of the script (or not, and let PHP handle it while it’s garbage cleaning), instead of connecting and disconnecting all the time.
  • Mind how you name stuff
    [LIST]
  • Don’t name variables $s - sure you know what it means now, but will you still know it in half a year? My personal rule of thumb is that variables in counters (used in for loops, while loops, etc) are OK to have one character, all other variables should have at least three characters
  • Don’t give functions names like “processtext”, but rather names that explain how they processes the text so you still get it when you see that function in half a year
  • Pick either underscores or camel case and stick to it. Mixing it like this will get you in trouble one day
    [/LIST]

HTH :slight_smile:

3

Many, many thanks for such a detailed look at my code. Some really great points you’ve raised here and I plan to take these on when I run ‘Phase 2’ :slight_smile:

So do you reckon I should just keep $mysqli->close(); for the footer.php file and remove all instances of it in the functions.php file?

4

I would open and close it in index.php, not spread it out over multiple files.
Do take into consideration that you either need to pass the mysql connection to the functions that need it as a parameter, or let the functions get them from the global scope.

5

Solid advice. Thanks again Scallio :smiley:

6

Hey Scallio,

Interestingly when I remove all instances of db connections in my functions.php file and move it to index.php:


$mysqli = new mysqli('localhost', 'root', '', 'guestbook');
include('inc/functions.php');

I get notice messages saying Notice: Undefined variable: mysqli - is this because it isn’t being “passed” to the functions file?

7

Correct, that’s why I said:

:slight_smile:

In other words, you need to do:


// functions.php
function delete_entry($id, $mysqli)
{
   // ....
}

// index.php
delete_entry($id, $mysqli);

OR


// functions.php
function delete_entry($id)
{
   global $mysqli;
   // ....
}

The first method is a bit cleaner, because you easily swap out mysqli instances if you want/need to, but of course the second method is a bit simpler.

8

D’oh. It was too close to lunchtime. It now makes sense. runs off in shame Thank you