Use parameter to filter results in a database without directly allowing users to access database

Question

So background context:

I wrote this script for a personal project that holds a large amount of items, I already display and am able to filter the results based on its country/location. I am a student PHP coder and would like constructive criticism on my thought process/code.

Thought Process:

I decided to code my country filter script as the countries are from a pre-defined list in my database (table: countries) and for extra safety precautions I wanted to ensure the end-user never gains direct access to the database queries - as from my understanding this is where security issues can become apparent. Essentially, the idea is that I grab a list of the countries from the database and store it in an array, then I compare that list to the users input. If there is a match then filter the results as such, if not change their input to NULL which produces a "No Result" message further on.

Table: countries

+----+-------------+-------------+ | id | name | slug | +----+-------------+-------------+ | 1 | Australia | australia | +----+-------------+-------------+ | 2 | New Zealand | new-zealand | +----+-------------+-------------+

Table: items

+----+----------+-----------------------------------+-----------+---------+ | id | name | description | countryid | suspend | +----+----------+-----------------------------------+-----------+---------+ | 1 | Item One | description of Item One | 1 | 0 | +----+----------+-----------------------------------+-----------+---------+ | 2 | Item Two | description of suspended Item Two | 2 | 1 | +----+----------+-----------------------------------+-----------+---------+

PHP Code:

if(isset($_GET['l'])) {
$location = strip_tags($_GET['l']);
$sql = "SELECT slug FROM countries";
$countries = $db->prepare($sql);
$countries->execute();
$getCountryList = $countries->fetchAll(PDO::FETCH_COLUMN); // Get all country slugs
if(in_array($location, $getCountryList)) { // Match country slugs to inputted country - this ensures the user input never touches the database >:)
  $sql = "SELECT countryid, name FROM countries WHERE slug = :slug LIMIT 1";
  $matchCountry = $db->prepare($sql);
  $matchCountry->execute(array(':slug' => $location));
  $countryDetails = $matchCountry->fetch(PDO::FETCH_ASSOC);
  $location = $countryDetails['countryid']; // The ID so I can search the items
  $locationName = $countryDetails['name']; // This now equals the nice to read name e.g "Australia"
} else {
  $location = '';
  $_GET['l'] = '';
}
}

The snippet that filters the items:

if(isset($_GET['l']) && $_GET['l'] != '') {
                // Location
                $sql = "SELECT * FROM items WHERE items.suspend = 0 AND items.countryid = :countryid GROUP BY items.id";
                $results = $db->prepare($sql);
                $results->execute(array(':countryid' => $location));
}

I hope I have given all the information needed! I'd like to know specifically if this extreme measure is over the top and if there is a better way to achieve such functionality.

Answer 1

It's completely unnecessary to execute the request SELECT slug FROM countries only to verify the user input before running SELECT countryid, name FROM countries WHERE slug = :slug LIMIT 1. You can safely use the user input in the second query directly. This is thanks to using prepared statements. Even if the user input is malicious, PDO prevents SQL injection.

In other words, the first part of your snippet could be simply this:

if (isset($_GET['l'])) {
    $location = strip_tags($_GET['l']);
    $sql = "SELECT countryid, name FROM countries WHERE slug = :slug LIMIT 1";
    $matchCountry = $db->prepare($sql);
    $matchCountry->execute(array(':slug' => $location));
    $countryDetails = $matchCountry->fetch(PDO::FETCH_ASSOC);
}

If your main goal is to do SELECT * FROM items, then if items.countryid is indexed, and countries.countryid is indexed, then it would be best to write a single JOIN:

SELECT items.* FROM items JOIN countries USING (countryid)
WHERE items.suspend = 0 AND countries.slug = :slug GROUP BY items.id

This optimizes performance with the fewest necessary queries, without compromising safety.

However, it's recommended to avoid SELECT * queries, and always specify the fields you want to fetch explicitly.