What is meta? ×
Meta Stack Exchange is where users like you discuss bugs, features, and support issues that affect the software powering all 127 Stack Exchange communities.

It's not too uncommon to see poor code examples in questions where a user hasn't escaped their user inputs in one way or another.

For Example:

mysql_query("INSERT INTO foo VALUES ($_POST['bar'], $_POST['baz'])");

and this question: Update Mysql column field based on email address

This is often the first thing I notice about a post, and even though most of the time it's not part of the initial problem, I feel it needs to be mentioned - some might argue it's more important than answering the question.

What is the best thing to do in this situation? It's almost like we should have a short url to link to, or an FAQ page with these common pitfalls on.

share|improve this question
1  
@Siva basically it leaves the user open for SQL injection, but they do not know it/understand it - aside from their main question –  Dunhamzzz Jul 27 '11 at 19:46
add comment

5 Answers

up vote 11 down vote accepted

Right

Your approach leaves your code open to SQL injection. Consider using Parameters instead.
-- You 1 hour ago

Thanks, I'll keep that in mind. -- Me 1 hour ago

Wrong

Never use this kind of code! You're going to get haxorzd beyond belief. -- You 1 hour ago

Thanks, but this is an internal application, and it's never going to see the light of day beyond these walls, so I think I'll be all right. -- Me 1 hour ago

It doesn't matter. You're doing it wrong, I'm offended and can't answer your question. >.< -- You 1 hour ago.

Laugh if you want, but this has happened to me before (I've exaggerated only slightly for dramatic effect).

share|improve this answer
    
Dramatic effect has been appreciated as has the overall message. +tick –  Dunhamzzz Jul 27 '11 at 22:20
    
In general, I agree. In practice, there are far too many people who don't appreciate the dire consequences of such practices when you leave only mild comments like the first example. Obviously you need to keep a professional tone, but there are times when more emphasis might be needed. (Or I might just be a jerk--hard to say for sure.) –  Cody Gray Jul 28 '11 at 1:36
add comment

Always mention such risks. If you're answering the main question, mention it as an aside (“You need to foo the bar, like this: blah blah. By the way, another problem in your code is that it's vulnerable to an SQL injection; you should use this instead: <correct code>.”) If you're not answering the main question, write a comment.

share|improve this answer
    
+1 for always mention such risks. Even if you elide error-checking-, validation-, etc. specific code for purposes of clarity, always mention specifically that it should be included in production code. –  Cody Gray Jul 28 '11 at 1:37
add comment

I think it depends on whether the question is "help me fix a bug in this production code" or "here's a simplified example similar to a problem I have."

In the first case, a polite suggestion about sanitizing inputs and security practices is probably acceptable.

In the second case, it's probably irrelevant at best and obnoxious at worst.

share|improve this answer
add comment

I don't know php. So, I don't know what the above code is doing, except that I know it is inserting data in to a table named foo. Naming a table foo is bad, I guess. ;-)

This is my take:

  • If I find something that the OP can do better, I usually mention the available alternative solutions so the OP can learn to solve their problem better.

  • Keep in mind, that you are answering a question that might potentially help others who may come across a similar problem in future.

  • If you find something wrong in OP's approach, point it out so that OP as well as others too can learn from it.

  • If possible, provide links that might provide them a good read about the topic. In this case, point them out to links that might show case the down sides of SQL injection and the best practices to avoid that.

  • If OP is of the type who is willing to learn, they will pick it up from there. Even if not OP, others who come across the question might learn a thing or two.

share|improve this answer
    
Basically the user is leaving themself open to SQL injection, but they do not know it/understand it - this is aside from their main question –  Dunhamzzz Jul 27 '11 at 19:49
add comment

I think mentioning it is certainly worthwhile, but in a way that would not be taken as patronizing by someone who does know better but is trying to illustrate a problem in a clear fashion. Personally, I don't want to see several lines of anti-injection measures when they're not relevant to the problem at hand.

I certainly think it should only be a comment, unless the question specifically refers to increasing the security of the code.

share|improve this answer
add comment

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .