World's most pointless dynamic SQL
-
I thought I'd let you all in on the delights of this PHP website I've inherited from an astoundingly incompetent coder. Here's a representative snippet, I may post more as I find them.
function getExhibitorId($id) { $the_sql="SELECT id FROM exhibitornew WHERE id=".$id." LIMIT 1"; // snip... }
-
I suppose the point could be to check whether that ID exists in the database?
-
Woops. I should have pasted a bit more of the function in to make it obvious that that isn't the intention. But for the record, no, he doesn't explicitly check whether or not the function returned any rows.
In fact, it's worse than I thought. It's not just a pointless function, it's dangerous. If the function DOES return a row, the "id" attribute of that object (it's an instance method) gets changed to whatever id was put into the function. But the other attributes stay the same. So essentially, it gives one object the same id as another object, which is utterly pointless and stupid. Thankfully I don't think this method actually gets called anywhere.
Also, the function has no return value, which to me is counter-intuitive for a function name starting with 'get'.
Another example of his m@d function naming sk1llz:
function doIt() { // snip... }
-
@gherkin said:
I'm currently working with a codebase with this method;function doIt() { // snip... }private void doAllTheWork(String f, Vector<String> r, int v, String channel, boolean a) {To be fair, the code wasn't created for work. It was created by a student back in college.
-
@belgariontheking said:
To be fair, the code wasn't created for work. It was created by a student back in college.
Oh, come on, btk, you can give out his name. What are the odds that the idiot who wrote that reads this site?
-
@bstorer said:
@belgariontheking said:
To be fair, the code wasn't created for work. It was created by a student back in college.
Oh, come on, btk, you can give out his name. What are the odds that the idiot who wrote that reads this site?
quite good
-
@Steeldragon said:
@bstorer said:
@belgariontheking said:
To be fair, the code wasn't created for work. It was created by a student back in college.
Oh, come on, btk, you can give out his name. What are the odds that the idiot who wrote that reads this site?
quite good
Indeed, in the past 2 years I've seen the site referenced 4 times in 3 three different computer magazine's ([url=http://www.linuxmag.nl/]Linux magazine[/url]2x, the name says it all, [url=http://en.wikipedia.org/wiki/C%27t]C't[/url], computer technology (the dutch version), the currently most proffesional computer magazine and another one which I don't remember)
-
@dtech said:
@Steeldragon said:
@bstorer said:
@belgariontheking said:
To be fair, the code wasn't created for work. It was created by a student back in college.
Oh, come on, btk, you can give out his name. What are the odds that the idiot who wrote that reads this site?
quite good
Indeed, in the past 2 years I've seen the site referenced 4 times in 3 three different computer magazine's (Linux magazine2x, the name says it all, C't, computer technology (the dutch version), the currently most proffesional computer magazine and another one which I don't remember)
You guys both missed the joke. It's Welpog's code.
-
@tdb said:
I suppose the point could be to check whether that ID exists in the database?
Even if it is, the code is still stupid. SQL injection and the variable is named $the_sql, which is completely pointless.
-
@savar said:
Maybe i'm missing something, but there isn't necessarily any danger of SQL injection here. It's a function, so the $id variable might be getting validated before the function gets called, or it might be coming from a source that would never give anything other than valid data.@tdb said:
I suppose the point could be to check whether that ID exists in the database?
Even if it is, the code is still stupid. SQL injection and the variable is named $the_sql, which is completely pointless.
-
@morbiuswilters said:
@dtech said:
@Steeldragon said:
@bstorer said:
@belgariontheking said:To be fair, the code wasn't created for work. It was created by a student back in college.
Oh, come on, btk, you can give out his name. What are the odds that the idiot who wrote that reads this site?
quite good
Indeed, in the past 2 years I've seen the site referenced 4 times in 3 three different computer magazine's (Linux magazine2x, the name says it all, C't, computer technology (the dutch version), the currently most proffesional computer magazine and another one which I don't remember)
You guys both missed the joke. It's Welpog's code.
Well, two people do seem to have missed a joke. I'm not sure that anyone missed out on any actual humour, though.
-
It took me a while to spot the real WTF. And I call myself a database engineer :-(
B
-
I'm still not sure whether or not it's a WTF. It is silly to return the same thing you are searching for but if the code is simply trying to work out if an id exists then it could be much worse. “Select ” for instance. That "LIMIT 1" seems like a bad idea, though.
Personally, I'd use something like
"select count() from table where id="...
and check the number returned. If the query returns 0 then the id does not exist. If the query returns 1 then it does exist. If the query returns >1 then something hinky is going on and the program should raise some sort of error.
I’d also do some checking for a SQL injection attack, of course.
B
-
@Loonacy said:
Maybe i'm missing something, but there isn't necessarily any danger of SQL injection here. It's a function, so the $id variable might be getting validated before the function gets called, or it might be coming from a source that would never give anything other than valid data.
Ah, yes, assuming that the input is validated somewhere else is always a good plan. Security Not In Layers, I beleive its called.
You know how they say "never trust input from the user"? Well, never trust input from other developers, APIs or third party code, either.
-
@Loonacy said:
Maybe
i'm missing something, but there isn't necessarily any danger of SQL
injection here. It's a function, so the $id variable might be getting
validated before the function gets called, or it might be coming from a
source that would never give anything other than valid data.Why do you all expect $id to be an id value? The variable could have changed its meaning from that to something that would be better called $id_criterion or $id_filter and would be able to contain a quoted id or something like
"(SELECT exhibitorid FROM stuff WHERE color = 'blue')" or "'3' AND deleted = 'F'" or "'7' OR id IS NULL" or a session variable or something like that.
-
@halcyon1234 said:
@Loonacy said:
Maybe i'm missing something, but there isn't necessarily any danger of SQL injection here. It's a function, so the $id variable might be getting validated before the function gets called, or it might be coming from a source that would never give anything other than valid data.
Ah, yes, assuming that the input is validated somewhere else is always a good plan. Security Not In Layers, I beleive its called.
You know how they say "never trust input from the user"? Well, never trust input from other developers, APIs or third party code, either.
This isn't always desirable, sometimes you need to work around a limited system, with something like getExhibitorId(" 0 OR col2 = 'hotdog' "); and forcing redundant validation would hamper agile development. When the client keeps asking for more features or changing the specs without increasing the deadline, you need to keep code adaptable.
-
with something like getExhibitorId(" 0 OR col2 = 'hotdog' ")
I read the reply via email, and not via the forum where I could see your tags, so Congratulations: you nearly made me physically ill. But after seeing the tags, I have to agree-- yes, that is a perfectly cromulant coding practice. Almost as good as building the database in Access-- because then you can just put up a public-facing Web version of Query Builder, and let web site users build their own queries. It's fully customizable!
-
@halcyon1234 said:
@Loonacy said:
Maybe i'm missing something, but there isn't necessarily any danger of SQL injection here. It's a function, so the $id variable might be getting validated before the function gets called, or it might be coming from a source that would never give anything other than valid data.
Ah, yes, assuming that the input is validated somewhere else is always a good plan. Security Not In Layers, I beleive its called.
You know how they say "never trust input from the user"? Well, never trust input from other developers, APIs or third party code, either.
*sigh* Clearly this is the case, but I really didn't feel like getting into an argument about how stupid this is. However, you said the right thing and now I must support you. Database escaping should be done at the level of the query, no higher. In fact, you should not be building queries like this at all, but be using some kind of DB API that sanitizes all dynamic data before it goes into the query. I can't fathom why people think it's a good idea to escape data at a higher level, because it frequently means you can't escape it at the lower level and you end up just concatenating a variable into a query without knowing if it was properly escaped beforehand. I suppose if you want to get really hackish, you could check to see if it was escaped by doing something like if (substr_count($id, "'") && !substr_count($id, "''")) { $id = escape($id); } but that's pretty much a sign that someone has failed along the way. Seriously, people, stop doing this shit. Do not escape data before it is actually used somewhere. Use some kind of DB API that always escapes variables for you so you never have to think about it. Use some kind of template system that always escapes HTML for you so you don't have to think about it.
-
We had one \'Senior\' developer who liked to escape data at a high level (probably with ancient functions like php\'s \"addslashes()\"), then pass it to our database API functions (which escape automatically). The result was that the data in the database was full of escape characters. He didn\'t stay with us for long.
It's a PITA maintaining code like that in the first post, because I'm so used to letting the API do the escaping for me. I have to follow the code path every time to see if the original developer bothered to escape the variables I'm putting into his.
-
@havokk said:
I'm still not sure whether or not it's a WTF. It is silly to return the same thing you are searching for but if the code is simply trying to work out if an id exists then it could be much worse. “Select *” for instance. That "LIMIT 1" seems like a bad idea, though.
Personally, I'd use something like
"select count(*) from table where id="...
and check the number returned. If the query returns 0 then the id does not exist. If the query returns 1 then it does exist. If the query returns >1 then something hinky is going on and the program should raise some sort of error.
BWhile in this situation that's fine, isn't it considered extremely bad form to use count(*) to check for the existance of a record/set of records? That's what EXISTS is there for. In some other cases, count(*) will cause a complete table scan.
-
@morbiuswilters said:
Use some kind of DB API that always escapes variables for you so you never have to think about it.
No - use a database API that supports parameters. Never escape data into a SQL statement if parameters are available.
-
@Jeff S said:
PreparedStatements? Those are very good for this stuff. It basically offloads a lot of SQL sanitation, so no "Bobby Tables" attacks will actually go through.@morbiuswilters said:
Use some kind of DB API that always escapes variables for you so you never have to think about it.
No - use a database API that supports parameters. Never escape data into a SQL statement if parameters are available.
-
@danixdefcon5 said:
PreparedStatements? Those are very good for this stuff. It basically offloads a lot of SQL sanitation, so no "Bobby Tables" attacks will actually go through.
No, parameters (e.g. [url=http://sqlite.org/c3ref/bind_blob.html]sqlite3_bind_*[/url]). "Prepared statements" are SQL statements ([url=http://www.google.com/search?q=SQL+PREPARE+EXEC]google[/url]). Parameters require using the API, rather than using raw SQL. Parameters can be more useful, as they can be used (well, at least in SQLite anyway) pretty much anywhere, whereas SQL PREPARE is more limited.
