That's an example of hazardously bad programming practices. You're one mistake away from complete disaster. You should be sure that it takes more than one mistake to expose you to that sort of risk.

Casting to int is not a general purpose escaping system, and further, if you miss even one of these your entire application can be trashed.

Using mysql_query at all is a sign there's something severely wrong with your application.

It's a sign that the developer has had a fairly informal introduction to SQL. Personally I see no reason not to use the PDO extension in any situation where you would use mysql.

Depends on the purpose of the code. If it's one-page script then it's fine. If it's the context of the bigger app, mysql_query is probably a problem.

There is an opposing viewpoint, i.e. if you actually need an int, casting to int is one of the most reasonable ways of getting it.

Yes, and given that (IIRC) $_GET always returns strings anyway, casting to int before adding to a database makes sense. Even so, you should still attempt to validate the string with ctype_digit() and make sure it accurately represents the integer you expect. If you just cast directly to an int, you can't really predict the results.

And also, doing the cast inside the sql statement makes it difficult to see. It should be done, if at all, outside the query where it's obvious to anyone looking at the code that this is something that should be paid attention to.

In this case, you don't "need an int", you need a value that's safe to put in a database query.

If you're inserting a value in a database, you always, always, always use the proper escaping mechanism. No exceptions.

That's why using a library with a reliable, well-defined, easy to use escaping system is absolutely imperative.

In any case the real problem with this line of code is not the int cast, it's that it's using a GET request to delete a record.

Not necessarily - there can still be data in the _GET array, even if the request method is POST.

Yes, fair point, but I do think it's a red flag. On looking a bit further I see that yes, in fact, he is using a GET link to delete records: https://github.com/Paton/Saaave/blob/master/_views/browse.ph...

This is an actual, serious problem which I would note in a code review, as opposed to the int thing which cannot, as far as I can can tell, ever lead to an exploit or malfunction which would have been avoided by using a named escape function in this code. If anyone can think of a specific example to prove this wrong, please say so.

It still doesn't check if a particular id belongs to the user, so you can delete all the items in the table. But I agree that's different kind of problem :)

Your assuming it's a system where data belongs to set users - maybe any user is allowed to delete any piece of data.

The search obviously doesn't find all cases, but is a good start.

While there's nothing technically wrong with the example given, I might argue that since that won't work in all cases, it might be better to enforce a more rigorous policy of SQL query cleansing, or using bound params. Although this example is so simple I might not.

Then again, the fact that $_GET is even available at the location the query is taking place means this is most likely a type of design that I abhor, that PHP makes easy. Put actions in functions or methods, and then call them.

Pretty much every language will make getting direct user input then passing it to a database easy. What generally makes this less easy (or at least less intuitive) is a framework. Don't compare the likes of Rails or Django to PHP. Compare Laravel4 or Symfony2.

That doesn't mean PHP doesn't deserve some stick, it does, but most of it's current reported problems spawn from backwards compatibility. Nobody should be using mysql_*, they should be using prepared statements via PDO.

They could solve this by deleteing all the functions you're not supposed to use, but a whole bunch of legacy PHP would stop working. I'm fairly sure this would illicit more hate than the current method of slowly deprecating.

What I meant is that $_GET, $_POST and $_REQUEST aren't available in functions unless you declare them global. The fact that $_GET is being used in the query means either that the query is being performed in the main of that script (probably lots of small php files meant to be called by the browser), or that they are in a function/method and then pull in the global $_GET array.

IMHO, neither are good design choices, as one couples the program to HTTP too tightly (and not in a sane way), the other leads to crazy spaghetti PHP as the control flow can be affected by global variables set (or received from the user) far from where they are used.