...

View Full Version : Coding Issues



CoolAsCarlito
10-29-2009, 12:25 AM
I was told that these are issues I am having with how my coding looks. Any help as to fixing my problems with these.

1) Calling die() is not an appropriate way of handling errors.
2) You should use some sort of salting along with the password hashing.
3) Your usage of stripslashes() seems to suggest that you are running with magic quotes on; turn it off.
4) You should separate your presentational logic from your business logic (separations of concerns).
5) You are assuming that $_POST['username'] and $_POST['password'] are both set. If they are not you will get an E_NOTICE. You should check that an index exists in an array before you use it (unless you already know it does).
6) You don't need to MySQL escape $_POST['password'] seeing as you aren't using it in any query.
7) You are selecting the same user from the database twice, which is obviously redundant.
8) Assuming your variable upholds the invariant that usernames are unique, you needn't use a while loop when fetching the info because there will only every be at most one row returned. Seeing as you've also verified that a row was actually returned, you don't even need to check that mysql_fetch_array() returns the result. If it doesn't it would be a bug in PHP.


<?php

require "backstageconfig.php";
require "backstagefunctions.php";

ob_start();
//if the login form is submitted
if(isset($_POST['submit']))
{
// makes sure they filled it in
if(!$_POST['username'] || !$_POST['password'])
{
die('You did not fill in a required field.');
}
$username = mysql_real_escape_string($_POST['username']);
$pass = mysql_real_escape_string($_POST['password']);

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

//Gives error if user dosen't exist
$check2 = mysql_num_rows($check);
if ($check2 == 0)
{
die('That user does not exist in our database.');
}
while($info = mysql_fetch_array( $check ))
{
$pass = md5(stripslashes($_POST['password']));
$info['password'] = stripslashes($info['password']);
//$_POST['pass'] = md5($_POST['pass']); THIS IS DONE IN THE ABOVE STATEMENT
//gives error if the password is wrong
if ($pass != $info['password'])
{
die('Incorrect password, please try again.');
}
else

// if login is ok then we add a cookie and send them to the correct page
{
$username = stripslashes($username);
$_SESSION['username'] = $username;
$_SESSION['loggedin'] = time();

// Finds out the user type
$query = "SELECT `admin` FROM `users` WHERE `username` = '" . $username . "'";
$result = mysql_query($query) or die(mysql_error());
$row = mysql_fetch_array($result);
$admin = $row['admin'];
$_SESSION['admin'] = $admin;

#########################################
######## ADMIN SCRIPT CAN BE ADDED BELOW
#########################################
if(isset($_SESSION['admin'])) { ?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta http-equiv="Content-Style-Type" content="text/css">
<meta http-equiv="Content-Language" content="en-us">
<meta name="language" content="en-us">
<title>Backstage V1 Administration Console</title>
<link rel="stylesheet" href="backstage.css" type="text/css" media="screen">
</head>
<body>
<div id=container>
<div class=header>
<table cellpadding="0" cellspacing="0" border="0" width="95%">
<tr>
<td width=110 align=center></td>
<td></td>
<td width=40 valign=bottom align=right>
<a href="#" onclick="">Home</a> | <a href="#" onclick="">Logout</a> | <a target="_blank" href="http://kansasoutlawwrestling.com/phpBB3">Forums</a></td>
</tr>
</table>
</div>
<div id=container2>
<div id=nav>
<?php if(isset($_SESSION['loggedin'])) { ?>
<h1>Character</h1>
<ul>
<li><a href="#" onclick="">Biography</a></li>
<li><a href="#" onclick="">Allies</a></li>
<li><a href="#" onclick="">Rivals</a></li>
<li><a href="#" onclick="">Quotes</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin'])) { ?>
<h1>Submit</h1>
<ul>
<li><a href="#" onclick="">Roleplay</a></li>
<li><a href="#" onclick="">News</a></li>
<li><a href="#" onclick="">Match</a></li>
<li><a href="#" onclick="">Seg</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>
<h1>Handler</h1>
<ul>
<li><a href="#" onclick="">Directory</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>
<h1>Booking</h1>
<ul>
<li><a href="#" onclick="">Champions</a></li>
<li><a href="#" onclick="">Booker</a></li>
<li><a href="#" onclick="">Compiler</a></li>
<li><a href="#" onclick="">Archives</a></li>
</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>
<h1>Fed Admin</h1>
<ul>
<li><a href="#" onclick="">Handlers</a></li>
<li><a href="#" onclick="">Characters</a></li>
<li><a href="#" onclick="">Applications</a></li>
<li><a href="#" onclick="">Event Names</a></li>
<li><a href="#" onclick="">Title Names</a></li>
<li><a href="#" onclick="">Match Types</a></li>
<li><a href="#" onclick="">Divisions</a></li>
<li><a href="#" onclick="">Arenas</a></li>

</ul>
<?php } ?>
<?php if(isset($_SESSION['loggedin']) && $_SESSION['admin'] == 1) { ?>
<h1>Site Admin</h1>
<ul>
<li><a href="#" onclick="">Templates</a></li>
<li><a href="#" onclick="">Content</a></li>
<li><a href="#" onclick="">Bio Configuration</a></li>
<li><a href="#" onclick="">News Categories</a></li>
<li><a href="#" onclick="">Menus</a></li>
</ul>
<?php } ?>
</div>
<div id=content>

</div>
<div id="footer">Backstage 1 &copy; 2009
</div>
</div>
</div>
</body>
</html>
<?php
#########################################
######## ADMIN SCRIPT HAS TO END ABOVE
#########################################
}
}
}
}
else
{
// if they have not submitted the form
?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta http-equiv="Content-Style-Type" content="text/css">
<meta http-equiv="Content-Language" content="en-us">
<meta name="language" content="en-us">
<title>Backstage V1 Administration Console</title>
<link rel="stylesheet" href="backstage.css" type="text/css" media="screen">
</head>
<body>
<div id=login>
<form method="POST" action="/mybackstage/backstage.php">
<h1>KOW Backstage</h1>
<p><label>Username:<br><input type="text" name="username" id="log" tabindex="1"></label></p>
<p><label>Password:<br><input type="password" name="password" id="pwd" tabindex="2"></label></p>
<p style="text-align: center;"><input type="submit" class="button" name="submit" id="submit" value="Login &raquo;" tabindex="4"></p>
</form>
</div>
</body>
</html>
<?php
}
?>

Lamped
10-29-2009, 12:58 AM
I was told that these are issues I am having with how my coding looks. Any help as to fixing my problems with these.

1) Calling die() is not an appropriate way of handling errors.
2) You should use some sort of salting along with the password hashing.
3) Your usage of stripslashes() seems to suggest that you are running with magic quotes on; turn it off.
4) You should separate your presentational logic from your business logic (separations of concerns).
5) You are assuming that $_POST['username'] and $_POST['password'] are both set. If they are not you will get an E_NOTICE. You should check that an index exists in an array before you use it (unless you already know it does).
6) You don't need to MySQL escape $_POST['password'] seeing as you aren't using it in any query.
7) You are selecting the same user from the database twice, which is obviously redundant.
8) Assuming your variable upholds the invariant that usernames are unique, you needn't use a while loop when fetching the info because there will only every be at most one row returned. Seeing as you've also verified that a row was actually returned, you don't even need to check that mysql_fetch_array() returns the result. If it doesn't it would be a bug in PHP.

1. It can be. Exceptions are better, but die('message') is acceptable.

2. If you're using MD5 or SHA-1, you're probably fine. There are utilities out there that can reverse common passwords, but you can go on forever with security concerns. I'd vote "not allowing stupid passwords" over this.

3. Magic quotes is the spawn of satan. You should either turn it off, or more importantly, check it's on before using stripslashes (or you can corrupt your data)

4. Yeah, you can use full templating systems too. Separation is something you'll learn in time.

5. Checking your inputs is really important. Knowing when it's necessary is good too. This is along the same lines as my pet hate on this forum, and peoples blatant disregard to sanitisation - it's not a swear word people... Check your inputs.

6. You don't escape anything, until you use it in something that might need escaping. Then you ALWAYS escape it. I don't care if you mysql_real_escape_string('Y'), as long as you remember to mysql_real_escape_string($_POST['somethingdangerous']. Use escaping/quoting/sanitisation as part of your calls to the important functions.

7 & 8: Reasonable points, but not world-breaking.

CoolAsCarlito
10-29-2009, 02:16 AM
So do you see anything that I should change?

it career
10-29-2009, 07:19 AM
So do you see anything that I should change?

Yes you need to modify whole of your code based on the review comments that you have got.



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum