SitePoint Sponsor

User Tag List

Results 1 to 10 of 10
  1. #1
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)

    sanitize $_GET and set to variable

    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?

    Code php:
    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?

    Code php:
    $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.

  2. #2
    SitePoint Zealot Zurev's Avatar
    Join Date
    Feb 2009
    Posts
    171
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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. #3
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,705
    Mentioned
    102 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Zurev View Post
    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?
    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.

    Code php:
    $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:

    Code php:
    $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:

    Code php:
    $user = mysqli_real_escape_string($link, $user);
     
    $sql = 'SELECT username' .
        ' FROM users' .
        ' WHERE username = "' . $user . '"';
    $find_user = mysqli_query($link, $sql);
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  4. #4
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    68
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    You are using mysqli, why don't you also use one of its most important possibilities : prepared statements ?

  5. #5
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,705
    Mentioned
    102 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Arkh View Post
    You are using mysqli, why don't you also use one of its most important possibilities : prepared statements ?
    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.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #6
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    68
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    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.
    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. #7
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Thanks all.

    Firstly, I'm glad I got something right!

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

    Code php:
    $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:

    Code php:
    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.

  8. #8
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    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?

    Code php:
    $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. #9
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,705
    Mentioned
    102 Post(s)
    Tagged
    4 Thread(s)
    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.
    Code php:
    $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();
    }
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  10. #10
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    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.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •