2

Kind of an unclear question but I'm trying to check if a username has been taken or not. The code I have now isn't erroring but it's also not working, when echoing the $username variable I get nothing.

$sql="SELECT people_username FROM people WHERE people_username='{$_POST['username']}'";

    //Set the result of the query as $username and if the select fails echo an error message
    if ($username = !mysql_query($sql,$con)) {
        die('Error: ' . mysql_error());
    }

    else if ($_POST['username'] == $username){
        $errors[  ] = 'This username is already in use, please try again...sorry';
    }

Is it a syntax error or is my logic wrong?

CNJ
  • 50
  • 4
  • 1
    SQL injection attack risk, pulling direction from `$_POST` - use mysql_escape_real_string. – OMG Ponies Feb 08 '10 at 21:32
  • I have it filtered earlier with $_POST['username'] = filter_var($_POST['username'],FILTER_SANITIZE_STRING); – CNJ Feb 08 '10 at 21:34
  • Try hard coding the $_POST['username'] with a real username to check db connection is up first. – Yada Feb 08 '10 at 21:36
  • @Charlie: I wouldn't modify `$_POST['username']` because you'd be 'tainting' a php-supplied variable. Once you do that, you will never be sure if that variable was posted or supplied through the script. – Daniel Vassallo Feb 08 '10 at 21:40
  • This code is in a function, so if I set the post username to another variable I can't get it later when I'm inserting from another function. so I've kept the sanitized input in the global variable so I can get to it. Is there a better way to do that? – CNJ Feb 08 '10 at 21:44
  • I think I answered my own question, I'm going to use the $GLOBALS array – CNJ Feb 08 '10 at 22:06

4 Answers4

2

i would just do

$resource = mysql_query("SELECT people_username FROM people WHERE people_username='".mysql_escape_string($_POST['username'])."'");
if(!$resource) {
    die('Error: ' . mysql_error());
} else if(mysql_num_rows($resource) > 0) {
    $errors[  ] = 'This username is already in use, please try again...sorry';
} else {
    //username is not in use... do whatever else you need to do.
}
seventeen
  • 1,531
  • 3
  • 14
  • 18
1

If some cheeky user happens to try: '; DROP people; -- as a username, you'd be in big trouble.

You may want to check the following Stack Overflow post for further reading on this topic:

As for the other problem, the other answers already addressed valid solutions. However, make sure to fix the SQL injection vulnerability first. It is never too early for this.

Community
  • 1
  • 1
Daniel Vassallo
  • 337,827
  • 72
  • 505
  • 443
  • i think you cant do that anymore. you can extend the select but im prety sure mysql_query just executes 1 query. – useless Feb 09 '10 at 01:10
0

Your code is wrong.

It should be something like this:

$sql="SELECT people_username FROM people WHERE people_username='".mysql_escape_string($_POST['username'])."'";

//If the select fails echo an error message
if (!($result = mysql_query($sql,$con))) {
    die('Error: ' . mysql_error());
}

$data = mysql_fetch_assoc($result);

if ($data == null){
    $errors[  ] = 'This username is already in use, please try again...sorry';
}

Notice that for security reasons you need to escape the strings you use in SQL queries.

Johnco
  • 4,037
  • 4
  • 34
  • 43
0
  1. mysql_query($sql,$con) returns a resultset (which may be empty)
  2. you are not testing any condition with if($var = !'value'), you are just assigning a negated resultset to the variable $username (what beast that is, I am not sure)

My suggestion: Simplify the code, do not overload lines of code with multiple tasks. 3. List item

Majid Fouladpour
  • 29,356
  • 21
  • 76
  • 127
  • With ifs we frequently forget to use `==`: so this is wrong `if($a = 3)` because it is an assignment which changes the value of `$a` to `3` and is always true. To check if `$a` is equal to `3` we need `if($a == 3)` – Majid Fouladpour Feb 08 '10 at 21:45