Go Back   CodingForums.com > :: Server side development > PHP

Before you post, read our: Rules & Posting Guidelines

Reply
 
Thread Tools Rate Thread
Enjoy an ad free experience by logging in. Not a member yet? Register.
Old 10-28-2009, 11:25 PM   PM User | #1
CoolAsCarlito
Regular Coder

 
Join Date: Jun 2008
Posts: 679
Thanks: 114
Thanked 2 Times in 2 Posts
CoolAsCarlito can only hope to improve
Coding Issues

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 Code:
<?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
}
?>
CoolAsCarlito is offline   Reply With Quote
Old 10-28-2009, 11:58 PM   PM User | #2
Lamped
Super Moderator


 
Join Date: Feb 2009
Location: England
Posts: 539
Thanks: 8
Thanked 63 Times in 54 Posts
Lamped will become famous soon enough
Quote:
Originally Posted by CoolAsCarlito View Post
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.
__________________
lamped.co.uk :: Design, Development & Hosting
marcgray.co.uk :: Technical blog
Lamped is offline   Reply With Quote
Old 10-29-2009, 01:16 AM   PM User | #3
CoolAsCarlito
Regular Coder

 
Join Date: Jun 2008
Posts: 679
Thanks: 114
Thanked 2 Times in 2 Posts
CoolAsCarlito can only hope to improve
So do you see anything that I should change?
CoolAsCarlito is offline   Reply With Quote
Old 10-29-2009, 06:19 AM   PM User | #4
it career
Banned

 
Join Date: Jun 2007
Location: Web Designer
Posts: 321
Thanks: 0
Thanked 6 Times in 6 Posts
it career can only hope to improve
Quote:
Originally Posted by CoolAsCarlito View Post
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.
it career is offline   Reply With Quote
Reply

Bookmarks

Jump To Top of Thread


Thread Tools
Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump


All times are GMT +1. The time now is 10:08 AM.


Advertisement
Log in to turn off these ads.