4 ms·

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.

krapp 13y ago
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.
astrodust 13y ago
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.
- ratsbane 13y ago
  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.
  - oneeyedpigeon 13y ago
    Not necessarily - there can still be data in the _GET array, even if the request method is POST.
    - ratsbane 13y ago
      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.php 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.

krapp 13y ago

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.

astrodust 13y ago

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.

ratsbane 13y ago
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.
- oneeyedpigeon 13y ago
  Not necessarily - there can still be data in the _GET array, even if the request method is POST.
  - ratsbane 13y ago
    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.php 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.