$_SESSION[everything]



  • I've been at my new job as a PHP/MySQL Web Developer for about a month now. My primary responsibility is to write new web apps, however, I'm also tasked with hacking at existing code. Existing code can be defined as web applications incepted anywhere from 2001-present; multiple authors over the years, no revision tracking, no documentation and radically different coding styles. I digress, however.

    Since I've started, I've seen some pretty "WTF"able examples, but today takes the cake. The code is meant to grab the entirety of a user information row out of a MySQL table on login and save it for use all over the site. 

     

    $Query = "SELECT clientname, clientemail, unitnumber,unitname,district_manager,dm_id,statement,client_statement,snack_statement,message from $TableName where password='$encpass'";
    $Result = mysql_db_query ($DBName, $Query, $Link) or die(mysql_error());
    $num = mysql_num_rows($Result);

    while ($Row = mysql_fetch_array ($Result)) {
    $_SESSION['clientname'] = $Row[clientname];
    $_SESSION['clientemail'] = $Row[clientemail];
    $_SESSION['unitnumber'] = $Row[unitnumber];
    // more $_SESSION sets here
    }
    }
    mysql_close ($Link);

    $browser = $_SESSION['browser'];
    $clientname = $_SESSION['clientname'];
    $clientemail = $_SESSION['clientemail'];
    $unitnumber = $_SESSION['unitnumber'];
    // more var sets here
    }

    After having a huge laugh with my boss, I replaced the code:

    $query = "SELECT * FROM `client_login` WHERE `username`='$user' AND `password`='$encpass' LIMIT 1";
    $result = mysql_query($query) or die('aack: Query "'.$query.'" caused error: '.$mysql_error());
    if($result) {
    $playerinfo = mysql_fetch_array($result);
    extract($playerinfo, EXTR_OVERWRITE);
    foreach ($playerinfo as $key => $value) {
    $_SESSION[$key] = $value;
    global $key;
    }
    }else{
    die('aack');
    }

    Anyway, back to work...

    PS: I have no idea what the hell would happen in the event more than one person had the same password. Thankfully, it never did.



  • @nion said:

    $playerinfo = mysql_fetch_array($result); extract($playerinfo, EXTR_OVERWRITE);

    You do know that by default, mysql_fetch_array fetches both assoc and numeric indexes which you then put into $_SESSION.

    e.g. $_SESSION['clientdata'] = 'foo', $_SESSION[0] = 'foo';

    Use mysql_fetch_assoc instead.

     

     

    @nion said:

    PS: I have no idea what the hell would happen in the event more than one person had the same password. Thankfully, it never did.

    Given that it's a while loop that processed the result, $session would end up filled with the lastmost row in the resultset.

     

    Also, you haven't escaped the inputted username or password. And the password is stored in plaintext.



  • @nion said:

    extract($playerinfo, EXTR_OVERWRITE);

    And why are you extracting the array? You don't reference the extracted vars and since $_SESSION now holds it all there's no need to have them. In case you don't know what extract does, it turns

    $array['foo'] = 'bar';
    $array['fee'] = 'bee';

    Into

    $foo = 'bar';
    $fee = 'bee';

    Sorry for the double post, I can't find an edit button. Every time I use this asinine forum software I end up wanting to punch things.



  • @bairy said:

    Use mysql_fetch_assoc instead.

    Good call, I missed that. 

     @bairy said:

    Also, you haven't escaped the inputted username or password. And the password is stored in plaintext.

     

    RE: that ^
    A) All-encompassing escapes are done elsewhere on the code

    B) The password isn't actually plaintext. $encpass contains the form submitted password in the matching encryption scheme to the db.



  • @bairy said:

    And why are you extracting the array? You don't reference the extracted vars and since $_SESSION now holds it all there's no need to have them.

     

    Actually, the extracted vars are referenced elsewhere in the site code. Parts that I'm NOT changing rely heavily on it, thus the extraction is needed. I have no idea why they are stored in $_SESSION as I've yet to come across any code that actually uses the session vars... but I kept it in my version just in case.



  • @nion said:

    $Query = "SELECT clientname, clientemail, unitnumber,unitname,district_manager,dm_id,statement,client_statement,snack_statement,message from $TableName where password='$encpass'";
    ... 
    After having a huge laugh with my boss, I replaced the code:
    
    $query = "SELECT * FROM `client_login` WHERE `username`='$user' AND `password`='$encpass' LIMIT 1";
    ... 

    Why oh why do people do that? It really doesn't help development, when you have to refer to the schema to know what fields to expect :(  (and stored procedures are a completely different story).



  • I wrote a rant, but decided not to post it. Hacking on old code can be a pain, so i'm going to give you the benefit of the doubt. However i would at the very least restructure the way you handle queries to a more maintainable form.

    Exposing queries to users is bad m'kay.

    define('DEBUG_EMAIL', 'myemail@example.org');

    $query = buildQuery("SELECT
    clientname, clientemail,unitnumber,
    unitname, district_manager, dm_id, statement,
    client_statement,snack_statement,message
    FROM `client_login` WHERE `username` = '%s' AND `password` = '%s' LIMIT 1"
    , array($user,$encpass)
    );

    $result = runQuery($query) or die('Sorry something went wrong. Please contact support');


    function buildQuery($sql, $params) {
    foreach ($params as &$param)
    $param = mysql_real_escape_string($param);

    return vsprinf($sql,$params);
    }


    function runQuery($query) {

    $result = mysql_query($query);

    if (!$result) {
    $subject = "query failure: ".$query;
    $body = "$query \n\n ".mysql_error();
    mail(DEBUG_EMAIL, $subject, $body);

    return false;
    }

    return $result;
    }


  • If you are going to get on the guy for his coding, the least you could do in your code is prevent sql injection the RIGHT way.  Bind vars.  Learn it, Love it, Use it.



  •  @stratos said:

    I wrote a rant, but decided not to post it. Hacking on old code can be a pain

    I found out from the sysadmin today that the $_SESSION variables weren't original to the code--they were added not long ago when we upgraded to PHP5 and lost registered globals. He also then scolded me for using globals myself, stating that sessions were the new wave of PHP or some nonsense---to each his own. He's a very knowledgable person, however, his demeanor sometimes reflects the "I'M THE SYSADMIN--IT'S MY WAY OR THE HIGHWAY RRRRRR" vibe. 



  •  Subsequently, here's a snippet of code a friend wrote that I run on $_POST, $_GET, etc.

    function mysql_escape_array(&$in_array, $strip = true)
    {
        // This takes an incoming array
        // ANY ARRAY
        // And mysql_real_escape_string s each member
        // Including member arrays
        $magic = $strip == "force" ? true : get_magic_quotes_gpc() && $strip;
    
        $keylist = array_keys($in_array); // No need to get values containing ARRAYS
        foreach($keylist as $key)
        {
            if(is_array($in_array[$key]))
            {
                // ARRAY! RECURSE!
                mysql_escape_array($in_array[$key], $strip);
            }
            else
            {
                // First, strip if needed!
                if($magic)
                    $in_array[$key] = stripslashes($in_array[$key]);
                // NOT ARRAY! ESCAPE!
                $in_array[$key] = mysql_real_escape_string($in_array[$key]);
            }
        }
        // Eh, good style
        return true;
    }
    


  • @nion said:

      extract($playerinfo, EXTR_OVERWRITE);
      foreach ($playerinfo as $key => $value) {
        $_SESSION[$key] = $value;
        global $key;
      }
    That can't be working, can it? The (redeclared) global variables aren't being set any value.



  • @Zecc said:

    That can't be working, can it? The (redeclared) global variables aren't being set any value.


    he must be thinking of $GLOBALS[$key] = $_SESSION[$key] = $value;



  •  @Zecc said:

    That can't be working, can it? The (redeclared) global variables aren't being set any value.

     I had questions about whether or not it would work... however, after print_r'ing $GLOBALS, they're all there.



  • @nion said:

    Actually, the extracted vars are referenced elsewhere in the site code. Parts that I'm NOT changing rely heavily on it, thus the extraction is needed. I have no idea why they are stored in $_SESSION as I've yet to come across any code that actually uses the session vars... but I kept it in my version just in case.

     

    That sucks... I HATE extract()... Its one of those things that some people think is a cool trick, but it really just leads to hard to maintain code when you try to do a search for that variable and can't figure out where it was defined. I've never seen a legitimate reason to use it.



  • @savar said:

    That sucks... I HATE extract()... Its one of those things that some people think is a cool trick, but it really just leads to hard to maintain code when you try to do a search for that variable and can't figure out where it was defined. I've never seen a legitimate reason to use it.

     

     When I first started developing PHP around 7 years ago, I was making/hacking at custom CMS/user system software. Example code introduced me to

    while($r = mysql_fetch_array($result)) { 
      extract($r, EXTR_OVERWRITE);
    }
    
    and I've been using that pretty much ever since. However, I don't rely as heavily on extract(); as I used to---especially now that I'm dealing with multiple tables/queries/etc at the same time. After the PHP4 register_globals switch took place, I had to learn about $superglobals the hard way: By all my code breaking.



  •  @PG4 said:

    If you are going to get on the guy for his coding, the least you could do in your code is prevent sql injection the RIGHT way.  Bind vars.  Learn it, Love it, Use it.

    In principle i agree with you, binding is a much neater solution then doing it manually. However on a practical level the code works fine and unless a very specific bug in php is found (which has happened in the past), the code i wrote has no known way of exploitation in the form of sql injection.

    The main point about the code however was not about that. I just put in the escaping function for neatness. It was to demonstrate how wrapping the builtin fuctions with functions of your own enables you to create a easier structure to make changes too and inject debugging when needed, and removing that debugging from being shown to the user.

     

    I found out from the sysadmin today that the $_SESSION variables weren't original to the code--they were added not long ago when we upgraded to PHP5 and lost registered globals. He also then scolded me for using globals myself, stating that sessions were the new wave of PHP or some nonsense---to each his own. He's a very knowledgable person, however, his demeanor sometimes reflects the "I'M THE SYSADMIN--IT'S MY WAY OR THE HIGHWAY RRRRRR" vibe.

    Well your sysadmin could be a prick, but he's right about the globals. However session is hardly the new global.  While session is global "accessable", letting stuff be globally accesable should be a carefully weighed choice, because it can make debuggin harder. 

     Also if he thinks sessions are the new wav, he's living in 2001 or something.  OO Frameworks are the new globals.



  • @savar said:

    That sucks... I HATE extract()... Its one of those things that some people think is a cool trick, but it really just leads to hard to maintain code when you try to do a search for that variable and can't figure out where it was defined. I've never seen a legitimate reason to use it.

    Oh... there is a good reason. It's the easiest way to define one or many variables with disallowed variable names:

    Without extract():

    $name = "impossible variable name lolz";
    $$name = "assignee";
    $name = "me, too...";
    $$name = "also"

    With ectract():

    extract(["impossible variable name lolz" => "assignee",
             "me, too..." => "also"]);

    See? There is a legitimate usage!!

    Edit: Or am I wrong? Haven't coded PHP in a while.



  • ${'impossible variable name lolz'} = "assignee";
    ${"me, too..."} = "also";
    Why, oh why, would anyone want this sort of variable names?


  • Really, the only thing I can say is this...

    <font color="#000000"> <font color="#0000bb"><?php

    </font><font color="#007700">function </font><font color="#0000bb">show_quotes</font><font color="#007700">(</font><font color="#0000bb">$string</font><font color="#007700">) {
      </font><font color="#0000bb">$string </font><font color="#007700">= </font><font color="#0000bb">urldecode</font><font color="#007700">(</font><font color="#0000bb">$string</font><font color="#007700">);
      </font><font color="#0000bb">$string </font><font color="#007700">= </font><font color="#0000bb">str_replace</font><font color="#007700">(</font><font color="#dd0000">'"'</font><font color="#007700">,</font><font color="#dd0000">""",$string);
      $string = str_replace('\'',"'",$string);
      $string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">','</font><font color="#0000bb">pizza</font><font color="#dd0000">', $string);
      //$string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">','"', $string);
      $string = str_replace('?','"', $string);
      $string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">',"'", $string);
      return $string;
     }
                    
    ?>
    </font> </font>



  •  Building on some earlier code, I've put together 2 functions that send email to you upon failure. 

     

    // Set debug email - place this here or somewhere else
    define('DEBUG_EMAIL','developer@yourdomain.com');

    /* Connects to a MySQL server and selects requested database, will die() upon either failure */
    function connectdb($database=NULL) {
    $connection = @mysql_connect("mysql","user","pw");
    if($connection) {
    $result=true;
    }else{
    $to = DEBUG_EMAIL;
    $headers = "From: MySQL Debug <noreply@yourdomain.com>";
    $subject = "MySQL Connect Failure";
    $body = "Date: ".date('M/d/Y @ hⓂs a')."\n";
    $body .= "File: ".$_SERVER['SCRIPT_FILENAME']."\n";
    $body .= "Query String: ".$_SERVER['QUERY_STRING']."\n";
    $body .= "IP: ".$_SERVER['REMOTE_ADDR'].' @ '.gethostbyaddr($_SERVER['REMOTE_ADDR'])."\n";
    $body .= "MySQL Said: ".mysql_error();
    mail($to,$subject,$body,$headers);
    $result=false;
    die($subject);
    }
    // set the default db name to use if not provided by the connectdb() call
    if(is_null($database)) { $database = "default_db_name"; }
    $db = @mysql_select_db($database);
    if($db) {
    $result=true;
    }else{
    $to = DEBUG_EMAIL;
    $headers = "From: MySQL Debug <noreply@yourdomain.com>";
    $subject = "MySQL Select DB Failure";
    $body = "Date: ".date('M/d/Y @ hⓂs a')."\n";
    $body .= "File: ".$_SERVER['SCRIPT_FILENAME']."\n";
    $body .= "DB: ".$database."\n";
    $body .= "IP: ".$_SERVER['REMOTE_ADDR'].' @ '.gethostbyaddr($_SERVER['REMOTE_ADDR'])."\n";
    $body .= "MySQL Said: ".mysql_error();
    mail($to,$subject,$body,$headers);
    $result=false;
    die($subject);
    }
    return $result;
    }

    /* Performs given query and returns true or false based on result
    function query($query) {
    $query = mysql_real_escape_string($query);
    $result = mysql_query($query);
    if (!$result) {
    $to = DEBUG_EMAIL;
    $headers = "From: MySQL Debug <noreply@yourdomain.com>";
    $subject = "MySQL Query Failure";
    $body = "Date: ".date('M/d/Y @ hⓂs a')."\n";
    $body .= "File: ".$_SERVER['SCRIPT_FILENAME']."\n";
    $body .= "Query String: ".$_SERVER['QUERY_STRING']."\n";
    $body .= "IP: ".$_SERVER['REMOTE_ADDR'].' @ '.gethostbyaddr($_SERVER['REMOTE_ADDR'])."\n";
    $body .= "MySQL Query: $query \n";
    $body .= "MySQL Said: ".mysql_error();
    mail($to, $subject, $body,$headers);
    return false;
    }
    return $result;
    }

    Not perfect and could probably be more effecient, but, I can't recall getting any errors unless I screw up when developing, so it shouldn't be sending too many e-mails.



  • @nion said:

    Really, the only thing I can say is this...

    <font color="#000000"> <font color="#0000bb"><?php

    </font><font color="#007700">function </font><font color="#0000bb">show_quotes</font><font color="#007700">(</font><font color="#0000bb">$string</font><font color="#007700">) {
      </font><font color="#0000bb">$string </font><font color="#007700">= </font><font color="#0000bb">urldecode</font><font color="#007700">(</font><font color="#0000bb">$string</font><font color="#007700">);
      </font><font color="#0000bb">$string </font><font color="#007700">= </font><font color="#0000bb">str_replace</font><font color="#007700">(</font><font color="#dd0000">'"'</font><font color="#007700">,</font><font color="#dd0000">""",$string);
      $string = str_replace('\'',"'",$string);
      $string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">','</font><font color="#0000bb">pizza</font><font color="#dd0000">', $string);
      //$string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">','"', $string);
      $string = str_replace('?','"', $string);
      $string = str_replace('</font><font color="#007700">?</font><font color="#dd0000">',"'", $string);
      return $string;
     }
                    
    ?>
    </font> </font>

     

     E_PARSE_ERROR_UNEXPECTED_"_EXPECTED_,_OR_;



  • @Zecc said:

    ${'impossible variable name lolz'} = "assignee";

    ${"me, too..."} = "also";
    Why, oh why, would anyone want this sort of variable names?
    Ask Navision developers - it's code is full of objectsvariables like "General Ledger Entry" etc.


  • Discourse touched me in a no-no place

    @nion said:

    define('DEBUG_EMAIL','developer@yourdomain.com');

    RFC2606?



  • @nion said:

     Building on some earlier code, I've put together 2 functions that send email to you upon failure. 

    [...]

    Not perfect and could probably be more effecient, but, I can't recall getting any errors unless I screw up when developing, so it shouldn't be sending too many e-mails.

     

    You should have written a third one too. No cookie for you



  • Wow, the cure is almost as bad as the disease.  Seriously, this is why people think PHP is a toy language.  Like, 90% of you in this thread need to stop writing PHP right now and go sit in the corner and think of how awful you are.

     

    I hate you all. 



  • @morbiuswilters said:

    Seriously, this is why people think PHP is a toy language.

    All languages are toys, once you decide you're going to play with them.

    If you can get your whole team to agree that you're going to play with a language, and you collectively establish a standard guideline on how you're going to play, it becomes a game.

    And if it ships, you win.

    You know, I was kidding at first, but now I think this is actually a potentially productive idea. If the project schedule is used as the rules, and you're playing in co-op mode... you might all actually have fun writing code again, even in the most WTF of jobs.

    I'm going to go play with this concept some more. And if this actually turns into some sort of industry-recognised methodology, we can laugh really hard about it. I can see the interview now: "How did you come up with this concept?" "Oh, it just came to me while posting on The Daily WTF. I suppose I must have stepped in it."



  • @morbiuswilters said:

    Wow, the cure is almost as bad as the disease.  Seriously, this is why people think PHP is a toy language.  Like, 90% of you in this thread need to stop writing PHP right now and go sit in the corner and think of how awful you are.

     

    I hate you all. 

     

    We hate you too,

    But hopefully you do understand that this was about a old code base, where the hours spent on it probebly couldn't be billed for much, so the quick, reasonably decent way is prefered to the proper way.

    Fix the issue and leave the crap to be crap, because nobody is paying to turn it into something else.



  • You're testing $result twice, once with the "or die", then once more with the "if" statement. Die('aack') could never be executed.


Log in to reply