Building session array from post to build search query

Question

So I am making post requests to a page for search queries. I am turning them into a session array so I can paginate the results. The process kind of feels ugly and long winded. I need code review for two things.

1. Shortening / making this code more elegant if possible
2. Security - any glaringly obvious vulnerabilities I have missed. I am running the posts through mysqli_real_escape_string but is the way I am building the query vulnerable?

input appreciated

<?php
if ($_SERVER['REQUEST_METHOD'] == "POST") {
 //only accept posts from our domain
 if (strpos($_SERVER['HTTP_REFERER'], DOMAIN)) {
 if (isset($_POST['csrf_token']) && $_POST['csrf_token'] === $_SESSION['csrf_token']) {
 // some arrays to check against
 $stock_type = array(
 "beers",
 "wines",
 "spirits",
 "apops"
 );
 $trade_type = array(
 "buying",
 "selling"
 );
 // create session array from post variables
 $_SESSION['search'] = array(
 'stock_type' => mysqli_real_escape_string($connection, $_POST['stock_type']),
 'trade_type' => mysqli_real_escape_string($connection, $_POST['trade_type']),
 'brand' => mysqli_real_escape_string($connection, $_POST['brand']),
 'country' => mysqli_real_escape_string($connection, $_POST['country']),
 'min' => mysqli_real_escape_string($connection, $_POST['min']),
 'max' => mysqli_real_escape_string($connection, $_POST['max']),
 'user' => mysqli_real_escape_string($connection, $_POST['user'])
 );
 // the start of the search variable
 $where = "WHERE t.published = 1";
 // buld up the search variable based on whats been posted
 if (!empty($_SESSION['search']['stock_type']) && in_array($_SESSION['search']['stock_type'], $stock_type)) {
 $where .= " and t.stock_type = '" . $_SESSION['search']['stock_type'] . "'";
 }
 if (!empty($_SESSION['search']['trade_type']) && in_array($_SESSION['search']['trade_type'], $trade_type)) {
 $where .= " and t.buying_selling= '" . $_SESSION['search']['trade_type'] . "'";
 }
 if (!empty($_SESSION['search']['brand'])) {
 $where .= " and t.brand LIKE '%" .$_SESSION['search']['brand']. "%'";
 }
 if (!empty($_SESSION['search']['country']) && in_array($_SESSION['search']['country'], $country_list)) {
 $where .= " and u.country= '" . $_SESSION['search']['country'] . "'";
 }
 if (!empty($_SESSION['search']['min']) && ctype_digit($_SESSION['search']['min'])) {
 $where .= " and t.price >= '" . $_SESSION['search']['min'] . "'";
 }
 if (!empty($_SESSION['search']['max']) && ctype_digit($_SESSION['search']['max'])) {
 $where .= " and t.price <= '" . $_SESSION['search']['max'] . "'";
 }
 if (!empty($_SESSION['search']['user']) && ctype_digit($_SESSION['search']['user'])) {
 $where .= " and u.subscription = '" . $_SESSION['search']['user'] . "'";
 }
 $_SESSION['where'] = $where;
 }
 }
} else {
 $where = $_SESSION['where'];
}
if (isset($_SESSION['where'])) {
 mysqli_select_db($connection, $database_connection);
 $query = "SELECT SQL_CALC_FOUND_ROWS t.*,
 u.subType,
 Count(l.trade_id) AS leads 
 FROM trading t 
 LEFT JOIN leads l 
 ON l.trade_id = t.trade_id
 LEFT JOIN users u 
 ON u.user_id = t.user_id 
 $where
 GROUP BY t.trade_id 
 ORDER BY timestamp DESC 
 LIMIT $start, $perpage";
 $something = mysqli_query($connection, $query) or die(mysqli_error($connection));
 $totalRows = mysqli_num_rows($something);
}
?>

Answer 1

Security - any glaringly obvious vulnerabilities I have missed. I am running the posts through mysqli_real_escape_string but is the way I am building the query vulnerable?

Maybe. You didn't post where $start and $perpage come from, if they are user-supplied you may be vulnerable.

Also, you should switch to using prepared statements. Escaping is a weaker form of protection against SQL injections.

Returning early

You can avoid nested if statements by reversing it and returning early:

if ($_SERVER['REQUEST_METHOD'] != "POST") {
 // handle this case
}
if (!strpos($_SERVER['HTTP_REFERER'], DOMAIN)) {
 // return / die with CSRF error
}
if (!isset($_POST['csrf_token']) || $_POST['csrf_token'] !== $_SESSION['csrf_token']) {
 // return / die with CSRF error
}

Storing data in session

$_SESSION isn't really the right place to store temporary data like this, as it is global and only meant to store data across requests.

I think that you are doing this because it makes it easier to see which values are safe, and which are not. If you don't want to switch to prepared statements (which you should), you should use some other means of doing this, such as storing the values in your own array or appending "safe" to the variable names.

Answer 2

I recently switched from mysqli_ to PDO for the SQL injection reason. I'll give you a very basic example of connecting and searching a row:

<?php
 $host = "localhost";
 $name = "database name";
 $user = "root";
 $pass = "root";
 $variable1 = $_POST['variable1'];
 $variable2 = $_POST['variable2'];
 $db_connection = new PDO('mysql:host='. $host .';dbname=' . $name .';charset=utf8', $user, $pass);
 $database_search = $db_connection->prepare('SELECT * FROM table WHERE column1=:variable1 AND column2=:variable2');
 $database_search->bindValue(':variable1', $variable1, PDO::PARAM_STR);
 $database_search->bindValue(':variable2', $variable2, PDO::PARAM_STR);
 $database_search->execute();
 $database_search_results = $database_search->fetch();
 echo $database_search_results['column1'];

Note that when I use bindValue() on $variable1 and $variable2, the database already knows what the query is, which makes the query invulnerable to SQL injection.

Answer 3

I like the idea of returning early - hadn't thought of this. Below is the full snippet after a tidy up including the $start & $perpage get values. Ended up going with a foreach loop and switch statements.

<?php
if (!isset($_SESSION)) {
 session_start();
}
$perpage = 10;
if (isset($_GET["page"])) {
 if (!ctype_digit($_GET['page'])) {
 exit();
 } else {
 $page = intval($_GET["page"]);
 }
} else {
 $page = 1;
}
$calc = $perpage * $page;
$start = $calc - $perpage;
if ($_SERVER['REQUEST_METHOD'] == "POST") {
 //only accept posts from our domain
 if (strpos($_SERVER['HTTP_REFERER'], DOMAIN)) {
 if (isset($_POST['csrf_token']) && $_POST['csrf_token'] === $_SESSION['csrf_token']) {
 $ok_values = array(
 "beers",
 "wines",
 "spirits",
 "apops",
 "buying",
 "selling",
 "1"
 );
 $_SESSION['search'] = array(
 't.stock_type' => $_POST['stock_type'],
 't.buying_selling' => $_POST['trade_type'],
 'brand' => $_POST['brand'],
 'u.country' => $_POST['country'],
 'min' => $_POST['min'],
 'max' => $_POST['max'],
 'u.subscription' => $_POST['user']
 );
 $where = "WHERE t.published = 1";
 foreach ($_SESSION['search'] as $key => $value) {
 if (!empty($value)) {
 $value = mysqli_real_escape_string($connection, $value);
 switch ($key) {
 case "brand":
 $where .= " and t.brand LIKE '%" . $value . "%'";
 break;
 case "min":
 if (ctype_digit($value)) {
 $where .= " and t.price >= '" . $value . "'";
 }
 break;
 case "max":
 if (ctype_digit($value)) {
 $where .= " and t.price <= '" . $value . "'";
 }
 break;
 default:
 if (in_array($value, $ok_values)) {
 $where .= " and $key = '" . $value . "'";
 }
 }
 }
 }
 if (isset($where)) {
 $_SESSION['where'] = $where;
 }
 }
 }
} else {
 if (isset($_SESSION['where'])) {
 $where = $_SESSION['where'];
 }
}
mysqli_select_db($connection, $database_connection);
$query = "SELECT SQL_CALC_FOUND_ROWS t.*, 
 u.subtype, 
 count(l.trade_id) AS leads 
 FROM trading t 
 LEFT JOIN leads l 
 ON l.trade_id = t.trade_id 
 LEFT JOIN users u 
 ON u.user_id = t.user_id $where 
 GROUP BY t.trade_id 
 ORDER BY timestamp DESC 
 LIMIT $start, $perpage";
$something = mysqli_query($connection, $query) or die(mysqli_error($connection));
$totalRows = mysqli_num_rows($something);
// total rows before limit clause
$rows = sprintf("SELECT FOUND_ROWS() AS `found_rows`");
$total = mysqli_query($connection, $rows) or die(mysqli_error($connection));
$rows_total = mysqli_fetch_assoc($total);
?>