Opinions and Suggestions (Security issue inside)

[nobackseat]

First off, I wanted to say that I have worked with several clients using the Rusnak script, and the code in general is just really horrid.

With that said, I have a few things to point out, just from looking over the latest update for 5 minutes.

PHP Code:

//here the user posts a comment
$comment = $_POST["comment"];
if ($comment != "") {
    $date = date("Y-m-d H:i:s");
    // $date = "10-23-3 21:02:35";
    $user = $loggedinname;
    if ($isloggedin!="yes") {
        $user = "Guest";
        }
    $comment = $comment;
    mysql_query("INSERT INTO ".$prefix."shoutbox VALUES ('', '$user', '$date', '$comment')"); 


So, it appears $comment is directly inserted into the database, with no protection, with the exception of if it is empty...

Also, why is $comment assigned to itself in there?

Anyways, next...

PHP Code:

$num = mysql_numrows($result); 


I believe mysql_numrows() is deprecated. So why is it being used on a script that is freely available to public? Which, of course, which would be run on all different types of systems and PHP versions. You can't guarantee compatibility...

PHP Code:

$isloggedin = $loginstatus[loginstatus];
$loggedinname = $loginstatus[username];
$article_title = $pagecontent[title];
$article_content = $pagecontent[content];
$value[content] = $content;
$value[title] = $title; 


I haven't dug deep enough into the script, but I'm assuming that $loginstatus, $pagecontent and $value are arrays.

I see this mistake done everywhere, and it is quite a frustrating habit to see other people make. Put quotes in the brackets! Else, it has to check if it is DEFINE'd and what not. Generally bad practice, and it is tremendously slower.

Additionally, I believe seeing the encryption was MD5(). Seriously? Not even a salt? Yeah I suppose double MD5 is minorly safer, but that is just silly. MD5 was created in 1991... upgrade much?

Also, one last thing. For your major 2.0 release, I highly recommend that you re-do the template system if you can. In fact, a general recommendation is to make the backend object-oriented so it is easier for developers like us to build "mods" and what not.

Thanks,

NBS