I’ve an allergic reaction to SQL Injections
A few days ago I posted about looking at GitHub projects for junior developer candidates. One of the things that is very common in such scenario is to see them use string concatenation for queries, I hate that. I just reached to a random candidate GitHub profile right now and found this gem:
The number of issues that I have with this code is legion.
- Not closing the connection or disposing the command.
- The if can be dropped entirely.
- And, of course, the actual SQL INJECTION vulnerability in the code.
There is a reason that I have such a reaction for this type of code, even when looking at junior developer candidates. For them, this is acceptable, I guess. They are learning and focusing mostly on what is going on, not the myriad of taxes that you have to pay in order to get something to production. This is never meant to be production code (I hope, at least). I’m not judging this on that level. But I have to very consciously remind myself of this fact whenever I run into code like this (and as I said, this is all too common).
The reason I have such a visceral reaction to this type of code is that I see it in production systems all too often. And that leads to nasty stuff like this:
And this code led to a 70GB data leak on Gab. The killer for me that this code was written by someone with 23 years of experience.
I actually had to triple check what I was seeing when I read the code the first time, because I wasn’t sure that this is actually possible. I thought maybe this is some fancy processing done to avoid SQL injection, not that this is basically string interpolation.
Some bugs are things that you can excuse. A memory leak or a double free are things that will happen to anyone who is writing in C, regardless of experience and how careful they write. They are often subtle and easy to miss, happening in corner cases of error handling.
This sort of bug is a big box of red flags. It is also on fire.
Comments
when I read this, the first thing I thought of was what your opinion would be:
https://nee.lv/2021/02/28/How-I-cut-GTA-Online-loading-times-by-70/
How they were ok with loading times in minutes I still cannot fathom though.
Reviewed a pull request just yesterday where a developer who should have known better was concatenating an integer ID into a SQL string rather than using a parameter. He figured it was OK because it's an integer, right? Can't inject arbitrary content via an integer!
I had to explain that this did also mean that every single request was going to be a different SQL query and thus compiled separately, execution plans not cached, would show up separately on our performance insights, etc..
Peter,
That is actually the sort of issue that I can see slipping by.
The fact that they didn't profile that themselves is a crime, though. Loading time in minutes requires at least a short look at the profiler. But it is likely that this actually got worse over time.
When they wrote it, the json was very small, and got big over time, no one actually paid attention to it.
DC,
Yes, there are so many reasons why you want to avoid it...
The Select * hurts my eyes
Slapout,
Why? Unless you are reading more columns than actually needed, this doesn't actually really cost anything
Comment preview