> “Sadly Rails documentation doesn't warn you about this pitfall, [...]" said Dmitry Borodaenko, a former production engineer at Facebook who brought the commit to my attention wrote in an email.
Maybe they were new 15 years ago in Rails, but I was using them in Perl in the mid-90s.
There is no excuse for writing an SQL injection in 2021. Zero. None. And if you're in the position to write code that'll be deployed to production, you darn well better have it reviewed by peers before it's merged.
If the CTO did this and worked around the developers, he's a freaking idiot. If the engineers saw this and signed off because he's the CTO, they're freaking idiots. I wouldn't ordinarily be this harsh about it, but come on. SQL injections in 2021? That's astoundingly bad.
And Visual Basic itself had them 23 years ago as well.
Though, the inflection point, at least in my circles, was in 1998, when rain.forest.puppy talked about "piggy backing" SQL commands.
Even there they mentioned using parameters with stored procedures to protect yourself.
It's just clear no one in IIS land did at the time, so there was a wide open exploit playground for everyone that saw that release of Phrack, once they got done playing with directory traversal exploits.
> Specifically, line 23 strips the code of “reject” and “filter,” which are API functions that implement a programming idiom that protects against SQL injection attacks.
That's not true at all. If you are going to do technical analysis that calls out mistakes, the publication should have multiple technical people review the article.
Unfamiliar with Ruby/Rails, why's that untrue? Is it because `reject` / `filter` are just array mapping functions rather than ActiveRecord methods? (Best explanation I could come up with on a quick skim.)
Yeah, at a glance it seems they had a performance problem, possibly from their use of reject and filter which you correctly guess operate over the results instead of becoming part of the query.
I'm guessing he was not familiar with the codebase and maybe not even ruby/rails in general and just wrote the whole sql query by hand, not bothering to figure out the correct way of parametrization , using string interpolation instead.
I had the same reaction, but upon reflection, the reject/filter actions themselves aren’t what’s safe, it’s the usage of functions that accept unsanitized input and safely wrap the SQL that’s going on here.
This is completely and utterly untrue.
https://guides.rubyonrails.org/security.html#sql-injection shows examples that are exactly like the code in question in that commit
Bound parameters were a new thing like 15 years ago.