Sanitize $_GET and set to variable

PHP
1

I am trying to sanitize $_GET[‘user’], check if the user exists in the database, and then set it to a variable ($g_user) for use in the rest of the page.

I came up with this and am wondering if it is ok?


if (isset($_GET['user'])) {
	$user = filter_input(INPUT_GET, 'user', FILTER_SANITIZE_STRING);
	$find_user = mysqli_query($link, sprintf("SELECT username FROM users WHERE username = '%s'",
		mysqli_real_escape_string($link, $user)
	));
		$result = mysqli_num_rows($find_user);
			if ($result != 1) {
				$error = 'User not found.';
				include 'error.php';
				exit();
			}
			else {
			$g_user = $user;
		}
}

or even this?


$user = filter_input(INPUT_GET, 'user', FILTER_SANITIZE_STRING);
if (empty($user)) {
	header('Location: /');
	exit();
}
else {
$find_user = mysqli_query($link, sprintf("SELECT username FROM users WHERE username = '%s'",
		mysqli_real_escape_string($link, $user)
	));
		$result = mysqli_num_rows($find_user);
			if ($result != 1) {
                $error = 'User not found.';
                include 'error.php';
                exit();
			}
			else {
			$g_user = $user;
		}
}

edit: the reason I ask is because I only just recently learned about filtering and sprintf, so I want to make sure I am doing this the right way. :slight_smile:

2

At quick glance the second one looks fine, you pretty much want to perform the same sanitizing you do when a username is initially registered, then check for the row count, which you’ve clearly done.

However, why the need for sprintf?

3

Because when appropriately formatted, it can provide a useful and readable understanding of the code? Although, when only one variable is used the benefit isn’t as easy to determine.


$sql = sprintf('SELECT username FROM users WHERE username = "%s"',
        mysqli_real_escape_string($link, $user)
);
$find_user = mysqli_query($link, $sql);

It might also be usefully formatted like this:


$sql = 'SELECT username' .
    ' FROM users' .
    ' WHERE username = "' . mysqli_real_escape_string($link, $user) . '"';
$find_user = mysqli_query($link, $sql);

Or if it were in its own function, perhaps something like this:


$user = mysqli_real_escape_string($link, $user);

$sql = 'SELECT username' .
    ' FROM users' .
    ' WHERE username = "' . $user . '"';
$find_user = mysqli_query($link, $sql);
4

You are using mysqli, why don’t you also use one of its most important possibilities : prepared statements ?

5

You’ll find that prepared statements were suggested, complete with sample code, in this post

I suspect that the OP might find them too unusual to warrant with, but prepared statements are the safest way to pass variables to the database.

6

As he is using sprintf to format his sql strings, I think prepared statements are one step away as those are almost the same thing.

7

Thanks all.

Firstly, I’m glad I got something right! :smiley:

re sprintf: in the past, before I knew better, I would have done it like this:


$user = filter_input(INPUT_GET, 'user', FILTER_SANITIZE_STRING);
if (empty($user)) {
    header('Location: /');
    exit();
}
else {
$user = mysqli_real_escape_string($link, $user)
$find_user = mysqli_query($link, sprintf("SELECT username FROM users WHERE username = '$user'");
        $result = mysqli_num_rows($find_user);
            if ($result != 1) {
                $error = 'User not found.';
                include 'error.php';
                exit();
            }
            else {
            $g_user = $user;
        }
}

But I was under the impression that it was not correct as you are replacing the sanitized variable straight away and then letting it persist after the query, and that is fixed by the sprintf method I chose to use, or by:


WHERE username = "' . mysqli_real_escape_string($link, $user) . '"';

If I understood correctly, as long as you escape variables WITHIN the query only, it is the right way.

re prepared statements: Paul has helped me out a lot recently and I have learned a great deal, and as he said he provided some examples… but it just looks completely foreign to me right now.

It has taken me some months to get where I am (from not knowing how to even include a file, and I can now do quite a bit) with procedural style.

However, once I finish this site that I am working on, I fully intend to make another site and learn prepared statements. :slight_smile:

8

Changed it around a little. I forgot about separating the sql query and the result. However it is more clear (to me) how to properly do that when it is a ‘simple’ query like an INSERT…

Seeing as though I have to do a mysqli_num_rows check… it just doesn’t seem like I have the layout right around $result/$find_user. Seems kinda messy now… or is this fine?


$sql = sprintf("SELECT username FROM users WHERE username = '%s'",
        mysqli_real_escape_string($link, $user)
    );
	$result = mysqli_query($link, $sql);
        $find_user = mysqli_num_rows($result);
            if ($find_user != 1) {
                $error = 'User not found';
                include 'error.php';
                exit();
            }
9

The indenting is kind of messy, but there are also some minor improvements that can be made too.

The mysqli_num_rows function is simple enough that the code should become even more easily understandable when it’s moved inside of the if condition.
Using != is also a sloppy practice. With !== there tends to be less confusion and better accuracy.

And if you like, we can also put in a visual break between getting the result and doing something with the result.


$sql = sprintf("SELECT username FROM users WHERE username = '%s'",
    mysqli_real_escape_string($link, $user)
);
$result = mysqli_query($link, $sql);

if (mysqli_num_rows($result) !== 1) {
    $error = 'User not found';
    include 'error.php';
    exit();
}
10

Thank you Paul, that looks better. And thanks for the != tip too, looked that up and I understand why - have replaced it throughout my code.