Login Form Coding

Agreaves

Can somebody help me fix this code it's the code for a log in form😕

<?php
//What to do when the login button has been pressed
if(@$_POST['login']){
//Strip slashes
require_once("stripslashes.inc.php");
//Connect to the database
require_once("db_con.php");
//Check the database for the username
$result = mysql_query("SELECT username FROM members WHERE username='$_POST[username]'") or die(mysql_error());
//Obtain the results
$num = mysql_num_rows($result);
}
//If the username exist, check if the password matches.
if ($num > 0){
$result2 = mysql_query("SELECT username FROM members WHERE username='$_POST[username]' AND password=SHA1('$_POST[password]'") or die(mysql_error());
$num2 = mysql_num_rows($result2);
}
//If the password exists,authorize the session and log in the user.
if ($num2 > 0){
$_SESSION['auth']="yes";
$_SESSION['logname'] = $_POST['username'];
$today = time();
$result = mysql_query("INSERT INTO login (username,logintime) VALUES ('$_SESSION[logname]',NOW())") or ("UPDATE login SET logintime=$today WHERE username='$_POST[username]");
}
?>

Bjom

The code lags [php ] [/php ] tags to make it readable.

Agreaves

Im new to php, I have only been doing it a week now. Im just working on this small website. So you're saying I need to split the code up with some php tags right?, and what about the coding itself, will it work the way I want it, because I have been testing it and it wont work. I want it to validate the user and check their username against the database and log them in, nothing too complicated. Also I use this code as a include on the login page using the require function.

bradgrafelman

Agreaves wrote:
So you're saying I need to split the code up with some php tags right?

No, Bjom was suggesting you utilize the board's [noparse]

..

[/noparse] (as I have done for you) when posting PHP code as it makes your code much easier to read and analyze.

Agreaves wrote:
I have been testing it and it wont work.

Diagnostically speaking, "it wont work" doesn't tell us anything (of course it isn't working - that's why you're here, after all). What is it doing? What have you done to debug it? Are you either logging or displaying all errors (e.g. error_reporting is set to E_ALL and either display_errors or log_errors is enabled properly)?

As far as the code goes, there are several issues with it. Here are a few things I noticed:

This line:
```
if(@$_POST['login']){
```
uses the '@' error suppressor to hide errors. Don't use this operator at all. Instead, you should be using something like [man]isset/man or [man]empty/man to check for the existence of that external data.
Can you show us what "stripslashes.inc.php" does?
Why are you running two separate queries to first verify the username and then the password? Why not just include both in a single query and, if no rows are found, reject the username/password combination as invalid.
Speaking of those queries, user-supplied data should never be placed directly into a SQL query, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. Instead, you must first sanitize it with a function such as [man]mysql_real_escape_string/man (for string data).
These lines:
```
$_SESSION['auth']="yes"; 
$_SESSION['logname'] = $_POST['username']; 
```
suggest you are attempting to use PHP's sessions. However, you don't appear to be calling [man]session_start/man first (meaning that you actually aren't using sessions as far as PHP is aware).
This line:
```
$result = mysql_query("INSERT INTO login (username,logintime) VALUES ('$_SESSION[logname]',NOW())") or ("UPDATE login SET logintime=$today WHERE username='$_POST[username]");
```
as a couple of problems.

First and foremost, the right-hand side of the 'or' operator is just a string. It doesn't actually do anything (such as execute a MySQL query, which is what the left-hand side is doing).

Second, there's no need to fix the above mistake because there's no need for two separate queries. Instead, use MySQL's INSERT ... ON DUPLICATE KEY UPDATE syntax.

Agreaves

Im new to php on a whole, I dont know if all the error reporting options are on. With this code I took them from different books. I need some proper resources to help me with the coding. I will post the codes from the include files as well and you can guide me from there. Below is the code for the

<?php
function stripFormSlashes($arr){
	if(!is_array($arr)){
		return stripslashes($arr);
	} else {
		return array_map('stripFormSlashes', $arr);
	}
}
$_POST = stripFormSlashes($_POST);
?>

Bjom

Welcome to the forums Agreaves. Now it's easier to read.

Be careful with code you find. Even in books there seems to be an enormous amount of terrible code around. Actually reading around in the internet and looking at different tutorials for the same issue is much better than a book - you'll become more flexible and you will see both: good and bad code.

That being said, I dislike a few things about that code:

a) Everything what Brad pointed out. Especially the use of the @ is bad practice and unnecessary. The error_reporting setting in the php.ini can be used to show all errors while developping and hiding them from users when your code goes live.

b) The way the stripslashes is incorporated makes the code hard to read. If the function is to be kept outside, then still at least the call to it should be in the main script so that you can see that something is happening to the $_POST array. Also the function is called "stripFormSlashes"...so call the script the same. (The author of that book should give you your money back.)

so either incorporate everything in the script or at least do this (last line of the .inc.php file needs to get deleted and moved to mainscript:

...
require_once("stripFormSlashes.inc.php");
$_POST = stripFormSlashes($_POST);
...

c) look at my sig to see what I think of "or die".

d) you could switch to using mysqli - read about it here

e) you can learn about sessions here and here

I hope we could give you some inspiration to improve that code. Just try and see how far you can get and when you get stuck come back here and we'll help you.

Bjom

Agreaves

Can somebody check this code and tell me whats wrong with it and also if I have used the session_destroy() function correctly.

<?php
//Start the session
session_start();
//Check if the person has already logged in
$_SESSION['username'] = $_POST['username'];
if(isset($_POST['login']))
{
//Display who is logged in
$message = 'Logged in as, ' .$_SESSION['username'];
} 
else 
{
//No message will be displayed if the user is already logged in 
$message = '';
}
session_destroy();
?>

bradgrafelman

Why are you destroying the session after you just stored data in it? Seems a bit counterproductive.

Agreaves

I figured I should because I included a file that restarts the session for the login form itself, I thought maybe it would have cause conflicts with the include. I took out the session_destroy function now. Im going to construct the code piece and post it here for help.

Agreaves

Ok so whats wrong with this code so far?

<?php
session_start();
$_SESSION['username'] = $_POST['username'];
if(isset($_POST['login']))
{
$message = 'Logged in as, ' .$_SESSION['username'];
} 
else 
{
$message = '';
}
?>
<?php
if(isset($_POST['login']))
{
$_POST = stripFormSlashes($_POST);
require_once("db_con.php");
mysql_db_query ('SELECT username+password FROM members WHERE username="$_POST[username]" AND password=SHA1("$_POST[password]")');
mysql_real_escape_string($_POST['username'],$_POST[password]);
}
?>

bradgrafelman

A comment about the function:

function stripFormSlashes($arr){ 
    if(!is_array($arr)){ 
        return stripslashes($arr); 
    } else { 
        return array_map('stripFormSlashes', $arr); 
    } 
}

You should never have to run [man]stripslashes/man on incoming data. The only reason you would need to do so is to reverse the effects of the highly deprecated magic_quotes_gpc directive. In other words, you should add an if() statement in that function so that the body of it will only execute if [man]get_magic_quotes_gpc/man returns true.

If you find that magic_quotes_gpc is enabled, however, note that you should disable it (via a php.ini file, .htaccess, etc.).

As for your code...

This line:
```
$_SESSION['username'] = $_POST['username']; 
```
will cause an 'undefined index' error message to be generated if $POST['username'] doesn't exist. Perhaps you should move this assignment inside of the if() statement (that way you at least know something was POST'ed)?
If you're going to use the stripFormSlashes() function, you might as well use it properly (once you fix it as per my suggestion above, of course). It should be called on any external data ($_POST, $GET, $COOKIE, etc.) before you ever use it.
Why are you selecting "username+password" in the SQL statement? Are username and password numeric columns? If not, why are you trying to perform addition on them?
You should never place user-supplied data directly into a SQL query, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. Instead, you must first sanitize it with a function such as [man]mysql_real_escape_string/man (for string data).
In SQL, strings are most commonly denoted by single quotes - not double quotes (which can instead be used to delimit identifiers - database/table/column/etc. names).
This line:
```
mysql_real_escape_string($_POST['username'],$_POST[password]);
```
Has several problems with it:
1. View the PHP manual page for [man]mysql_real_escape_string/man to learn what parameters it accepts. You can't simply make up whatever parameter list you want and expect PHP to know what your intent was.
2. $_POST[password] is missing quotes around the array index (since the index is a string, and all strings must be delimited - e.g. with quotes).
3. Once you fix the above two problems, note that the line still won't do anything. You're basically doing the same thing as this code:
```
"foobar";
```
  which will neither output anything nor modify any variables (nor do anything at all). See the PHP manual for examples of how to use this function (hint: it returns the new value).
Don't use [man]mysql_db_query/man. As the manual states (in a rather large display of colors/style), this function is deprecated.
You should always be checking to ensure that [man]mysql_query/man (once you switch to using it instead) executed successfully. If it didn't, you should either output or log debug information (such as the SQL query itself and, at the bare minimum, the error message returned by the MySQL server). [man]trigger_error/man is an easy way to do so.

Bjom

a) why switching out of and right back into php mode?

b) does the code work without errors? Can't imagine it does... calling the stripFormslashes function w/o defining it should cause an error.

c) what do you want to achieve with mysql_real_escape_string at the place where you put it?

Agreaves

I have not tested the code at all, im just writing it and trying to get help fixing it as I go along.

Agreaves

Im sure that magic quotes is on, on my host server, thats why I was trying to include the stripFormSlashes

bradgrafelman

Agreaves;10981428 wrote:
Im sure that magic quotes is on

Regardless, you should still include the if() statement as a matter of defensive programming... especially since it shouldn't be enabled and you should make every effort to get it disabled.

Agreaves

I dont think my hosting company will do that, I can ask them.

Agreaves

How should I write this code so as to not use direct user input in the query?

mysql_db_query ('SELECT username+password FROM members WHERE username="$_POST[username]" AND password=SHA1("$_POST[password]")');

bradgrafelman

Agreaves;10981431 wrote:
I dont think my hosting company will do that, I can ask them.

It shouldn't matter what they will or will not do - it's a simple matter of you setting the PHP directive to the value that you want. If that's not something they can accomodate, then I'd be wondering why you're stuck using an incompetent host when there are thousands upon thousands of other hosts out there willing to do business with you (which is a two way street).

Agreaves;10981432 wrote:
How should I write this code so as to not use direct user input in the query?
mysql_db_query ('SELECT username+password FROM members WHERE username="$_POST[username]" AND password=SHA1("$_POST[password]")');

One way is to use prepared statements. Another would be to use an appropriate method to sanitize the data before placing it into the string. For the latter, I like to utilize [man]sprintf/man to keep my code nice and neat, e.g.:

$sql = sprintf(
    "SELECT foo FROM bar WHERE id = %u AND username = '%s' AND password = '%s'",
    $_GET['id'],
    mysql_real_escape_string($_POST['username']),
    sha1($password)
);

Bjom

oops, double post - see below

Bjom

Get eclipse for PHP
Install mysql, apache, php on your local machine. (Either each individually or via a WAMP package like XAMPP)

Play around with your code local, then put on host.

Not only can you test everything beforehand w/o screwing with your hosted stuff, but also you will learn a lot in the process of installing and setting up. All useful stuff, that will help you understand what is actually going on and accordingly how you can get things done.

Another great extension on the eclipse side is X-Debug, btw.

Bjom