Mysidia Adoptables Support Forum  

Home Community Mys-Script Creative Off-Topic
Go Back   Mysidia Adoptables Support Forum > Community Board > Feedback and Suggestions

Notices

Reply
 
Thread Tools Display Modes
  #1  
Old 02-03-2011, 03:05 PM
nobackseat nobackseat is offline
Member
 
Join Date: Feb 2011
Posts: 13
Gender: Male
Credits: 1,133
nobackseat is on a distinguished road
Default Opinions and Suggestions (Security issue inside)

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

Last edited by nobackseat; 02-03-2011 at 03:08 PM.
Reply With Quote
  #2  
Old 02-03-2011, 03:20 PM
Tony's Avatar
Tony Tony is offline
I program.
 
Join Date: Jan 2011
Posts: 75
Gender: Male
Credits: 7,807
Tony is on a distinguished road
Default

recommendation is to make the backend object-oriented

I agree.
Reply With Quote
  #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: 31,890
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
  #4  
Old 02-05-2011, 01:12 PM
nobackseat nobackseat is offline
Member
 
Join Date: Feb 2011
Posts: 13
Gender: Male
Credits: 1,133
nobackseat is on a distinguished road
Default

Kaeliah,

Please, tell me what made you treat my suggestion post differently than others?

I don't recall mentioning that I was impatient. Or that this script should charge money as well as the staff?

Quote:
Many of these things have already been corrected for the next release
Then there was clearly no reason to get defensive, other than that of you feeling talked down to. Which I did not do intentionally.

I was just sharing my honest thoughts. It was for your benefit, as I am not anticipating the next release of the script...

I was not directly blaming you or the 'development team', for the obvious faults of the original developer.

NBS
Reply With Quote
  #5  
Old 02-05-2011, 02:39 PM
Hall of Famer's Avatar
Hall of Famer Hall of Famer is offline
Administrator, Lead Coder
 
Join Date: Dec 2008
Location: South Brunswick
Posts: 4,448
Gender: Male
Credits: 327,820
Hall of Famer is on a distinguished road
Default

Umm this may just be my interpretation. Most of those programming flaws were made a long time ago by BMR, the script's creator. It is fine if you bring it up to me in a private message, but I do think its unfair for my dev staff to have to take this blame that is supposed to be on BMR(In fact, no one blames BMR for this, we pretty much realize that there are mistakes made before and hope to correct them one by one). Dont you think so?

But yeah, we are indeed working on it and trying our best to improve it over the next a few weeks and months. Fixing all those stuff will take some time, and I hope you understand and give us more time before making a judgment on the direction this script goes. I appreciate your interest in Mys, have fun!

Hall of Famer
__________________


Mysidia Adoptables, a free and ever-improving script for aspiring adoptables/pets site.
Reply With Quote
  #6  
Old 02-05-2011, 02:58 PM
nobackseat nobackseat is offline
Member
 
Join Date: Feb 2011
Posts: 13
Gender: Male
Credits: 1,133
nobackseat is on a distinguished road
Default

You mustn't of taken the time to read my post.

Quote:
I was not directly blaming you or the 'development team', for the obvious faults of the original developer.
She assumed that it was directed at her/her team.

Quote:
Fixing all those stuff will take some time, and I hope you understand and give us more time before making a judgment on the direction this script goes.
Again, what is with this time restraint that I am apparently implying?

Obviously, in aiming to assist the direction of the script, I created this thread.

NBS
Reply With Quote
  #7  
Old 02-05-2011, 03:22 PM
Kaeliah's Avatar
Kaeliah Kaeliah is offline
Premium Member
 
Join Date: Sep 2010
Location: Pennsylvania, United States
Posts: 485
Gender: Female
Credits: 31,890
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
Kaeliah,

Please, tell me what made you treat my suggestion post differently than others?

I don't recall mentioning that I was impatient. Or that this script should charge money as well as the staff?



Then there was clearly no reason to get defensive, other than that of you feeling talked down to. Which I did not do intentionally.

I was just sharing my honest thoughts. It was for your benefit, as I am not anticipating the next release of the script...

I was not directly blaming you or the 'development team', for the obvious faults of the original developer.

NBS
I apologize, I took the post kind of personally and felt as though I(and the rest of the Dev Staff) was being blamed for the serious issues with the original script, or at least being blamed for not having fixed them yet... My apologies, I didn't mean to sound rude or anything, although I apologize for being defensive.
__________________
[My Shop] ♥ [My Blog] ♥ [Subscribe] ♥ [My Mods] ♥ [Mod TOS]
Reply With Quote
  #8  
Old 02-05-2011, 03:51 PM
Hall of Famer's Avatar
Hall of Famer Hall of Famer is offline
Administrator, Lead Coder
 
Join Date: Dec 2008
Location: South Brunswick
Posts: 4,448
Gender: Male
Credits: 327,820
Hall of Famer is on a distinguished road
Default

Quote:
Originally Posted by nobackseat View Post
You mustn't of taken the time to read my post.



She assumed that it was directed at her/her team.



Again, what is with this time restraint that I am apparently implying?

Obviously, in aiming to assist the direction of the script, I created this thread.

NBS
I did read your post, but that statement was not in your original post(yeah, you wrote a reply later to clear this confusion, thanks). I was just stating a possibility why Kaeliah found your thread offensive, she replied to it before you explained you were not blaming this current dev staff team for what BMR did years ago.
__________________


Mysidia Adoptables, a free and ever-improving script for aspiring adoptables/pets site.
Reply With Quote
  #9  
Old 02-06-2011, 04:54 AM
Arianna's Avatar
Arianna Arianna is offline
Dev Staff
 
Join Date: Sep 2009
Posts: 334
Gender: Female
Credits: 21,055
Arianna will become famous soon enough
Default

The script in general is pretty bad, and we're trying to improve it. :) The shoutbox script, if I'm not mistaken, is made by me - and I have to admit, it's a pretty bad error. However, in future versions, I have the following code in functions.php:

Code:
// clean all our data
$_POST = array_map('secure',$_POST);
$_GET = array_map('secure',$_GET);
And then later, the function:
Code:
function secure($data) {
	//This function performs security checks on all incoming form data
	if(is_array($data)) {
		die("Hacking Attempt!");
	}
	$data = htmlentities($data);
	$data = mysql_real_escape_string($data);
	$data = strip_tags($data, '');
	return $data;
}
So rather than sanitizing the $_POST data every time we get a variable, it just does it once.

As for object-oriented-ness, I actually previously recoded the whole script in OOP for my own site. The thing is that people have been working on the current version, so either the edits made to this would need to be scrapped, or it would need to be programmed from scratch. :/

Anyway, you make very good suggestions, especially because the script is really flawed in the first place. As Kaeliah and HoF have already said, a lot of these have been/are being corrected for 1.2.x, but some issues you brought up haven't been thought over in detail yet. Thank you. :)

Last edited by Arianna; 02-06-2011 at 05:14 AM.
Reply With Quote
  #10  
Old 02-06-2011, 11:40 AM
nobackseat nobackseat is offline
Member
 
Join Date: Feb 2011
Posts: 13
Gender: Male
Credits: 1,133
nobackseat is on a distinguished road
Default

Hello Arianna!

Thanks for the reply.

Your approach is "bad" for a few reasons...

1) The site already manually protects variables, so the variables that are used, would be escaped twice.
2) You left out $_COOKIE
3) More information would be escaped than would be needed, so it is much slower, as are arrays in general.
4) What if, say for user profile input, you wanted to permit certain HTML tags or something (not BBCode)? Since the input is stripped already, there isn't much you can do.
5) Encourages bad practices; user who learned PHP from Mysidia, may leave the part out of the code, following their habit of simply putting it directly in queries.

Make sense?

NBS

Last edited by nobackseat; 04-27-2011 at 01:43 PM.
Reply With Quote
Reply

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Opinions Please Tequila Webmasters Area 10 12-13-2012 04:00 PM
Opinions on Progress RoconzaArt Art Gallery 9 01-20-2011 10:38 PM
Rename adoptables (Security issue fixed!) kisazeky Addons/Mods Graveyard 23 10-15-2009 01:14 AM
What should I code next? Opinions Please! BMR777 Other Chat 29 07-14-2008 04:15 AM


All times are GMT -5. The time now is 06:13 AM.

Currently Active Users: 675 (0 members and 675 guests)
Threads: 4,080, Posts: 32,024, Members: 2,016
Welcome to our newest members, jolob.
BETA





What's New?

What's Hot?

What's Popular?


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2024, vBulletin Solutions Inc.
vBCommerce I v2.0.0 Gold ©2010, PixelFX Studios
vBCredits I v2.0.0 Gold ©2010, PixelFX Studios
Emoticons by darkmoon3636