code critique: SESSION Logging

kpowning

Here is the following code that im including on all my pages that are restricted access. Please give me some feedback on how I have this coded and any other ideas that I should take into mind 🙂

/////////////////////
////CHECK SESSION////
/////////////////////
$time = time(); 
$result = mysql_query("SELECT * FROM session WHERE id = $meminfo[id]");
if (mysql_num_rows($result)>0) {
while($row=mysql_fetch_array($result)) {
/// Check session login time for expire ///
if ($time - strtotime($row['activity']) > 900){ //15 minute session timeout!
mysql_query("DELETE FROM session WHERE id =$meminfo[id]");
session_destroy();
header('Location: http://www.mysite.com/contractorquote.php');
exit;
} 
else 
{
//// Update session - if exists ////
mysql_query("UPDATE session SET sid = '".session_id()."', activity = now() WHERE id=$meminfo[id]");
} 
}
} else {
//// Insert new session - if non exists ////
mysql_query("INSERT INTO session (id, sid, activity) VALUES ($meminfo[id],'".session_id()."', now())");
}
///////////////////////
///END SESSION CHECK///
///////////////////////

shamly

SELECT * FROM session WHERE id = $meminfo[id]

This query should return only one result. So no need to use a while loop.
Close the mysql connection before redirecting to the other page if the session has timed out.

MarkR

You haven't made it clear where $meminfo[id] is coming from.

If you've got register_globals enabled, and it's coming from the request, then this is extremely vulnerable to attacks.

Why not use PHP's built in sessions functionality? It supports storing sessions in a database.

Generally speaking, if you want a session to expire, it's not necessary to do anything much other than let the cookies expire and occasionally clean out old sessions from the database. This doesn't need to be done very often.

Mark

Weedpacket

MarkR wrote:
...occasionally clean out old sessions from the database. This doesn't need to be done very often.

Which, if you use PHP's session interface as suggested, is done as automatically as it is for the default filesystem storage (which is nice, because if you're using two timeouts - one in the database and on the files, you could end up risking a synchronisation glitch, with the one timing out and being trashed before the other, or the other before the one).

shamly

Yes .. i also agree that the use of PHPs inbuilt session functions is the easiest way of handling sessions. Using database will be good for applications that are distributed amoungst several servers and all of them need to access data in the same session.