Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 4 of 4

Thread: Coding Issues

  1. #1
    Regular Coder
    Join Date
    Jun 2008
    Posts
    682
    Thanks
    114
    Thanked 2 Times in 2 Posts

    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
    }
    ?>

  • #2
    Super Moderator
    Join Date
    Feb 2009
    Location
    England
    Posts
    539
    Thanks
    8
    Thanked 63 Times in 54 Posts
    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

  • #3
    Regular Coder
    Join Date
    Jun 2008
    Posts
    682
    Thanks
    114
    Thanked 2 Times in 2 Posts
    So do you see anything that I should change?

  • #4
    Banned
    Join Date
    Jun 2007
    Location
    Web Designer
    Posts
    321
    Thanks
    0
    Thanked 6 Times in 6 Posts
    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.


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •