6 ms·
> “Sadly Rails documentation doesn't warn you about this pitfall, [...]" said Dmitry Borodaenko, a former production engineer at Facebook who brought the commit
by tomc1985 6y ago
> “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.
This is completely and utterly untrue.
https://guides.rubyonrails.org/security.html#sql-injection 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.
- kstrauser 6y agoMaybe 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.
- rsynnott 6y ago> Maybe they were new 15 years ago in Rails That's when Rails came out :) I'm fairly sure ActiveRecord has always supported them.
- dragonwriter 6y ago> Maybe they were new 15 years ago in Rails, Well, Rails was < 2 years old then, so everything was new in Rails.
- ratsmack 6y agoI would think a Ruby code fuzzer would have caught something like that... if they have used one.
- btilly 6y agoBound parameters were a new thing like 15 years ago. When they were new depends on what language and database library you use. Perl's DBI had them 25 years ago.
- AdamN 6y agoDear lord I'm getting old ... :-)
- kstrauser 6y agoThough my creaking bones are weary, Yet I shall prepare this query.
- tomc1985 6y agoProbably; I vaguely remember them being discussed at my very first programming job around mid 2000's, which was writing Perl
- tharkun__ 6y agoI definitely fixed up some very SQL injectiony code somewhere between 2006-2008 in Perl.
- banana_giraffe 6y agoAnd 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.
- FanaHOVA 6y agoIt's like Twitter calling Rails LEGO because they weren't able to scale it :)
- GongOfFour 6y agoThe other funny thing is this bit: > 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.
- lilyball 6y agoI 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.
- theptip 6y agoUnfamiliar 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.)
- tinco 6y agoYeah, 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.
- xref 6y agoCome on man, don’t just comment “the article is wrong” then walk away. Put in some effort.
- avgDev 6y agoYikes. I'm working on an internal web app which can only be accessed on our network and even I am using parametrized queries to prevent injection. What a joke, and then blaming the documentation...just wow.
- amatecha 6y agoIt looks like the Gab devs followed the documentation precisely then! Just... apparently they copied the wrong part?! heheh ;)