![]() |
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:
Also, why is $comment assigned to itself in there? Anyways, next... PHP Code:
PHP Code:
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 |
recommendation is to make the backend object-oriented
I agree. |
Quote:
|
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:
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 |
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 |
You mustn't of taken the time to read my post.
Quote:
Quote:
Obviously, in aiming to assist the direction of the script, I created this thread. NBS |
Quote:
|
Quote:
|
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 Code:
function secure($data) { 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. :) |
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 |
Thanks for bringing those up, NBS. As far as I can see,
1) Well, yeah, but in previous things where I've used this, there isn't any escaping in the first place. 2) Ooops. :/ Might as well add that. 3) Hmm, I don't see why. Assuming the script always uses all of $_POST and $_GET (which is usually does), it needs to secure everything in them. 4) Well, that's a very valid point, only currently, there isn't anything in the script which requires this. I do get that this could be an issue in the future, but for now it's okay. xD It mostly makes sense, though. I use this approach because I hate having to secure variables from forms before using them, because I inevitably end up forgetting about them. xP |
Quote:
Which is why I use a database class... :P Quote:
NBS |
Sorry to double post, but just adding to my original post...
I have noticed the excessive use of mysql_result. Perhaps it is because that is the only function that one may know, or one copies and pastes from the current script. In either case, it is pretty bad. mysql_result is not the best option in most cases, simply because its operation is resource intensive. In fact its use is only recommended for SELECTing ONE column (from what I've gathered with colleagues). I highly recommend mysql_fetch_array. Look into it if you are interested. Last thing, and I think this is a huge issue, is so many people are suppressing errors, well, eveywhere. This is extremely bad practice. Seriously, if you are that paranoid about an error showing, then create an error handler...? It is bad practice, besides the obvious, because sometimes if you change the code that the suppressed line is dependent on, there is a good chance the interpreter will output a different error (and line number) than the one you were trying to ignore. Then how do you find out what is wrong? Just my two cents. NBS |
We're already doing our best to switch to mysql_fetch_array - the problem with this kind of feedback (it's just a minor problem, but still) is that we're working on a really updated version behind the scenes and so it's hard to know what we have and haven't done. :/
Anyway, an error handler does sound like a good idea. I'll look into that soon (or if anyone else on the dev team feels like it, then they can). |
All times are GMT -5. The time now is 01:00 AM. |
Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2025, vBulletin Solutions Inc.