There are several minor things, so what say I do a small code review right here for you.
If Statements
if ([color="blue"]!isset($_SESSION['username'][/color])) {
header('Location: /website/login.php');
}
if ([color="blue"]isset($_SESSION['username'])[/color] && isset($_SESSION['userid'])) {
$username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
}
The first significant thing with these two here is that you have the same term that is inverted in one if statement, and not inverted in the other.
These two if statement can be combined together, so that you donât have to think so hard to realise that they are actually related.
if (!isset($_SESSION['username'])) {
header('Location: /website/login.php');
} [color="green"]else if ([/color]isset($_SESSION['userid'])) {
$username = [COLOR="Blue"]filter_var($_SESSION['username'], FILTER_SANITIZE_STRING)[/COLOR];
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
}
Even better though is to realise that filter_var gives you an empty value if the variable doesnât exist, so you could do this instead:
$username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
if (empty($username)) {
header('Location: /website/login.php');
}
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
Database Variables
Next up is the start of the database condition:
if (isset($_POST['item_submit'])) {
[COLOR="Blue"]$user_id[/COLOR] = mysqli_real_escape_string($link, $_SESSION['user_id']);
$item_name = mysqli_real_escape_string($link, $_POST['item_name']);
The first problem here is that $user_id already exists, having retrieved the value via filter_var. Now itâs replaced with a completely different value.
Move those variable assignments up top (and remove the mysqli_real_escape_string for now) and that solves several other problems too.
$item_submit = isset($_POST['item_submit']);
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
$item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
...
if ($item_submit) {
What are those other problems if they had been left where they were?
One of those other problem is that superglobals such as $_SESSION and $_POST are used within that database section. Those should have been handled earlier on, so that you donât have superglobals scattered all throughout your code.
Getting that done before performing the database work is one of those invisible demarcation barriers. Itâs a âseparation of dutiesâ that helps to simplify your code, and make it easier to modify and understand later on.
SQL Statement
Next we get to the $sql statement.
A fundamental flaw in how the mysqli_real_escape_string was used before is that whenever you assign the escaped value to variables, those variables have a flow-on effect long after the database part of the code has been done.
While in this particular case it doesnât have a direct impact, if you had also escaped $username to send to your database, you would then find when outputting it in the HTML section that the $username variable is already escaped.
The solution to this is to ensure that escaped variables do not persist beyond the point where they are used.
One common way to do this is to encapsulate things within a function, so that the escaped values do not persist once the function is complete.
Another way that you could immediately use here, is to escape only when creating the sql string.
But first, letâs look at the SQL string.
$sql = mysqli_query($link, "INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')");
if (!$sql) {
Instead of $sql being the SQL string, it is instead the result of the database call. Most commonly the $result variable is used instead to store the result of the database call, so letâs separate out the SQL statement from the result.
$sql = "INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')";
$result = mysqli_query($link, $sql);
if (!$result) {
Now we just need to ensure that the variables being entered in to the SQL statement are properly escaped. Commonly sprintf() is used to help reduce the complexities of that.
$sql = sprintf('INSERT INTO items (user_id, item_name) VALUES (%d, "%s")',
intval($userid),
mysql_real_escape_string($item_name)
);
Notice how $userid is converted to an integer? In the SQL statement, %d is always a number (a decimal value) so you donât need to look any further to know that the code is correct. Further on, we use intval to ensure that whatever $userid happens to be, that it can now be nothing but an integer number. Even though we sanitised earlier on, other things can happen to that variable throughout the code, so itâs a good practice to always escape (or cast) in some way the SQL variables. Itâs a final safety net.
Itâs either that or use prepared statements, which many people donât want to deal with.
The End Result
So what are we left with after all that?
There are some other minor issues remaining such as variable name consistency, but I donât want to go tweaking things too much until you can see a clearly defined reason to do so.
For the sake of succinctness I have left off the HTML code below the PHP script as thereâs no change to that part. Even though the script still looks mostly similar to what you had, hopefully you can see the benefits now of the improvements that have been made.
<?php
session_start();
$username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
if (empty($username)) {
header('Location: /website/login.php');
}
$item_submit = isset($_POST['item_submit']);
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
$item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
if ($item_submit) {
$sql = sprintf('INSERT INTO items (user_id, item_name) VALUES (%d, "%s")',
intval($userid),
mysql_real_escape_string($item_name)
);
$result = mysqli_query($link, $sql);
if (!$result) {
$error = 'DATABASE ERROR.';
include 'error.php';
exit();
} else {
header('Location: /website/items.php?user=' . urlencode($username));
}
}
}
?>
Or, if you take the function path:
<?php
session_start();
function insertItem($link, $userid, $item_name)
{
$userid = intval($userid);
$item_name = mysql_real_escape_string($item_name);
$sql = 'INSERT INTO items (user_id, item_name) VALUES (' . $userid . ', "' . $item_name . '")';
return mysqli_query($link, $sql);
}
$username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
if (empty($username)) {
header('Location: /website/login.php');
}
$item_submit = isset($_POST['item_submit']);
$userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
$item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
if ($item_submit) {
if (insertItem($link, $userid, $item_name)) {
header('Location: /website/items.php?user=' . urlencode($username));
} else {
$error = 'DATABASE ERROR.';
include 'error.php';
exit();
}
}
?>
