Gab Attack

I’m not going to pretend that I’m a fan of Gab on a political level, but code is always interesting to me. So seeing the screenshot from the Ars Technica article about the breach and download of Gab’s data was worth a read.

Yeah, that’s pretty bad, not just for the raw and unparameterised SQL, but, unless there’s something really hinky with their diffing tool, WTAF is up with the lack of indentation in the methods?

What surprised me the most, however, was the comment from former Facebook production engineer Dmitry Borodaenko quoted in the article:

Sadly Rails documentation doesn’t warn you about this [SQL injection] pitfall

I assume Dmitri is good at what he does, but this statement is just flat out wrong.

I mean, it’s pretty up-front in the section on Active Record querying :

Building your own conditions as pure strings can leave you vulnerable to SQL injection exploits. For example, Book.where("title LIKE '%#{params[:title]}%'") is not safe. See the next section for the preferred way to handle conditions using an array.

That’s an alert in bright red. That’s using Arel query syntax; we haven’t even got down as far as raw SQL queries yet…

And before you even get to the end of the section, you get this additional note:

For more information on the dangers of SQL injection, see the Ruby on Rails Security Guide .

That’s hardly a case of “doesn’t warn you about this pitfall”. It has big warnings against non-parameterised queries, and an entire section in the security guide on why it’s bad and how Rails specifically mitigates this risk.

Parameterising queries against SQL injection has always been at the forefront of Rails standards. I’ve been working with it since way back in version 1.0, and even then, there was always documentation on how and why to use parameterised queries, and big bright warning signs regarding raw SQL queries.

Yes, there are times when you need to do raw SQL queries. Sometimes Arel’s query generation is just not enough if it’s particularly complex. But this was not that case.

On top of that, if you’re making claims about your code being secure, you should be running brakeman as a pre-commit hook and/or as a step in your CI pipeline. Commits like this shouldn’t even be pushed, let alone approved (they are doing code reviews, right?), and never merged.

(A more cynical person might even have thoughts that this was deliberate, though I don’t see how you could get a job as CTO—nor maintain it afterwards—with any such attempt to introduce an easily recognisable flaw into the system…)