View Single Post
  #3  
Old 02-03-2011, 03:32 PM
Kaeliah's Avatar
Kaeliah Kaeliah is offline
Premium Member
 
Join Date: Sep 2010
Location: Pennsylvania, United States
Posts: 485
Gender: Female
Credits: 32,636
Kaeliah will become famous soon enough
Send a message via AIM to Kaeliah Send a message via MSN to Kaeliah
Default

Quote:
Originally Posted by nobackseat View Post
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
First off, we just got this script and have not yet had time to go through and fix all these errors. Many of these things have already been corrected for the next release(which has not been released yet as we're still working on it). While I appreciate you bringing these things to our attention, please understand that this is a FREE script being worked on completely by VOLUNTEERS, so progress may not be as quick as some may like. The next release, 1.2.* will happen when our Development team is satisfied with the changes, and the additions to the script. Until then, please be patient.
__________________
[My Shop] ♥ [My Blog] ♥ [Subscribe] ♥ [My Mods] ♥ [Mod TOS]
Reply With Quote