I have this query below:
// They have a token and estimate id
if (isset($_GET['estimate_token']) && isset($_GET['estimate_id']))
{
if ($select = $db -> prepare("SELECT estimate_id FROM estimates WHERE estimate_token =?"))
{
$select -> bind_param('s', $_GET['estimate_token']);
$select -> execute();
$select -> store_result();
$select -> bind_result($estimate_id);
$select -> fetch();
if ($select -> num_rows == '0')
{
header ("Location: ./login.php");
}else{
}
$select -> close();
}
}
Customers are given a link via email with the token and an estimate id from the database. When they click the link it brings them to the correct estimate. The problem I am having is that if the customers manually replaces the estimate_id or estimate_token with any number in the url it still keeps you on the website where it should kicking you to the login.php. This is bad because it allows customers to view other estimates in the system.
I think the problem lies in the $select -> num_rows
throwing a false positive.
You logic is invalid - you must find a record where both token
and id
equals to $_GET
values, so you need to use query like:
$SELECT estimate_id FROM estimates WHERE estimate_token =? AND estimate_id = ?
This will select only one certain record.
Full code is something like:
if ($select = $db -> prepare("SELECT estimate_id FROM estimates WHERE estimate_token = ? and estimate_id = ?"))
{
// supposing id is `int`
$select -> bind_param('si', $_GET['estimate_token'], $_GET['estimate_id']);
$select -> execute();
$select -> store_result();
// if you need to know just if row exists
// there's no need for this two lines
//$select -> bind_result($estimate_id);
//$select -> fetch();
if ($select -> num_rows == 0)
{
header ("Location: ./login.php");
} else {
}
$select -> close();
}
Shouldn't you kick them out immediately if they don't have a token? I think that might be the problem. Or , with the way you've written it, you don't have an ELSE for what happens if they don't have a token and estimate ID. Your only ELSE statement is inside your DB select code.
This seems like much more sane logic, which will ensure they're thrown to login when they submit no, or blank tokens/IDs. It will also cover you if you needed to do any further validation as you just have 1 bool to set if you need to throw them out.
Since you're not using the result from the database either, changing it to a database-side count may be slightly quicker.
$needsLogin = true;
if (!empty($_GET['estimate_token']) && !empty($_GET['estimate_id']))
{
$select = $db->prepare('SELECT COUNT(1) FROM estimates WHERE estimate_token = ? AND estimate_id = ?');
$select->bind_params('si', $_GET['estimate_token'], $_GET['estimate_id']);
$select->execute();
$select->bind_result($cnt);
$select->fetch();
if ($cnt > 0) {
$needsLogin = false;
}
$select->close();
}
if ($needsLogin) {
header('Location: ./login.php');
die();
}
PS. PDO will make your DB code nicer to deal with:
$select = $db->prepare('SELECT COUNT(1) FROM estimates WHERE estimate_token = ? AND estimate_id = ?');
$select->execute(array($_GET['estimate_token'], $_GET['estimate_id']));
$cnt = $select->fetchColumn();
if ($cnt > 0) {
$needsLogin = false;
}