Parse Error Help!

[dhelsdon]

I know this is probably a really obvious mistake but I just can't see where I went wrong on line 28, I get a parse error, expecting `'('' in <path>

if ($user && $pass){
			$sql = "SELECT id FROM 'users' WHERE 'username'='".$user."'";
			$res = mysql_query($sql) or die (mysql_error());
			if mysql_num_rows($res) > 0){
				$sql2 = "SELECT id FROM 'users' WHERE 'username'='".$user."' AND 'password'='".md5($pass)."'";
				$res2 = mysql_query($sql2) or die (mysql_error());
				if mysql_num_rows($res2) > 0){
/28/					$row = mysql_fetch_assoc($res2);
				    $_SESSION['uid'] = $row['id'];

			    echo "You have successfully logged in as" . $user;
			}

[dagon]

your missing an ( in the if stament

[bradgrafelman]

In fact, in the code posted above, two ('s are missing - one on line 27 and another on line 24.

Also note some other problems:

• Single quotes in query strings denote strings - not column/table/etc. names.

• The logic doesn't seem very optimized at all. First you ask SQL to find all the rows with a given username and select the id column. Then, you query it again, adding another clause to the WHERE statement specifying the password, again selecting the id. Nothing is ever done when id is selected the first time.

  Why not just query it one time (e.g. with the second query - with both username and password specified)?

• MD5 has been "cracked" so-to-speak and is thus no longer considered safe in terms of preventing unauthorized access. Perhaps you should consider using a safer alternative for hashing passwords such as SHA256?

• It looks like $user is never properly sanitized (can't assume what we're not shown), thus making your script vulnerable to SQL injection attacks. User-supplied data should never be placed directly into a SQL query. Instead, you should first sanitize it with a function such as [man]mysql_real_escape_string/man.

• In solving the above problem, don't forget that if you allow characters such as single/double quotes in usernames then you don't want to simply do $user = mysql_real_escape_string($user); before the SQL queries; this destroys data integrity and results in messages such as You have successfully logged in as Devil\'s Advocate .

• Perhaps this is simply for testing purposes, but don't forget to remove the "or die(mysql_error())" bits from your code; you should never display SQL error messages to the outside world (there's no need to - that information isn't meant for them).

[dhelsdon]

I scrapped that code, I think this is more secure..
Here's the code.

<?php
//session_start();
ob_start();

$username = protect($_POST['username']);
$password = $_POST['password'];

$errors[] = array();

if(!$username){
    $errors[] = "You did not supply a username!";
}

if(!$username){
    $errors[] = "You did not supply a password!";
}


if(count($errors) >= 1){
    echo "The following error(s) occured:<br>\n";
        foreach($errors AS $error){
            echo $error . "<br>\n";
        }
}else {

$sql = "SELECT * FROM `users` WHERE `username`='".$username."'";
$res = mysql_query($sql) or die(mysql_error());

if(mysql_num_rows($res) == 0){
    echo "The username you supplied does not exist!";
}else {
    $sql2 = "SELECT * FROM `users` WHERE `username`='".$username."' AND `password`='".md5($password)."'";
    $res2 = mysql_query($sql2) or die(mysql_error());

    if(mysql_num_rows($res2) == 0){
        echo "Username and password combination incorrect!";
    }else {
        $row = mysql_fetch_assoc($res2);

        // we're going to set the user id

        // for cookies
        setcookie('id',$row['uid'],time()+86400);

        // for sessions
        $_SESSION['uid'] = $row['id'];

        header("Location: my_account.php");
    }
}

}

ob_end_flush();

?> 

My error is

 Call to undefined function protect() in C:\wamp\www\forum\login.php on line 5

[bradgrafelman]

Well, as the error message states, there is no function called protect(). Where is this function supposed to be defined?

[dhelsdon]

Well could I use my global.php file to define protect()
also, what is the format for defining the protect string

[bradgrafelman]

Well, for the general case, you can do something like:

if(get_magic_quotes_gpc())
    $data = stripslashes($data);

$data = mysql_real_escape_string($data);