Hacker News Reader

Comment by abvdasker | Hacker News Reader

This is bad code and it's a little surprising to me that a former Facebook engineer wrote it (and subsequently became the CTO).

It's been a while since I wrote any Rails but the offenses that jump out just from a cursory inspection:

- large raw SQL query which could almost certainly be accomplished in a more idiomatic way with AREL or ActiveRecord

- no user input sanitizing

- using a regular string literal for a large text block instead of a Ruby here document

- leaving a mess of commented-out code at the bottom of the method

Apparently Gab isn't exactly hiring the best and brightest.

View on HN

I'm not terribly surprised on their hiring practices. I got a Gab account a few years ago for kicks, and I was banned within a week. I never got any justification for my ban, and after scouring my locked account for any offending posts I can only conclude that their moderators are horribly corrupt.

I love decentralized software, but don't let Gab fool you. They're just out to grab power right now.

Idk when this guy worked there, but today a Facebook product dev likely doesn't get a chance to make a mistake like this. Which is good. But it also means people who've only worked there can have some really surprising gaps

It's always worth remembering that there's a difference between employment and a university degree. Universities don't grant degrees until someone has proved they have whatever competency the degree indicates. Employment means you got paid by the company, but what you did to get paid could be lots of things... And it's pretty easy to lie on a résumé. Universities have a reputation to lose if they grant a degree to someone who goes on to mess up badly; corporations have no such ties to a former employee, and Mr. Marotto making a major error when he's no longer employed by Facebook doesn't reflect poorly on Facebook.

"Former Facebook engineer" means Facebook paid him... No more and no less.

Not to be that guy but some of the worst security vulnerabilities I've seen has been from senior devs with college degrees. Having a degree =/= you know your shit or being "immune" to absurdly stupid mistakes like this.

You'd be surprised how often I've found leaked API tokens, passwords in plaintext etc on github for instance.

> Universities don't grant degrees until someone has proved they have whatever competency the degree indicates.

Universities grant degrees based on the successful completion of course requirements. I don't know if it's fair to conflate that with what you said. As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent?

It's an even further stretch if you think that at University cares all that much if one of their Computer Science graduates makes a mistake.

> As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent?

It means that the university granted you a degree (and, indeed, the med student who got a D average in every class is still called "Doctor" when they finish the process). The point is: universities have a vested interest in the question "Does that degree indicate you have competence?" and incentives to change if the answer, on average, trends toward "no" for their graduates. Corporations have no particular vested interest in the question "Does the corporation's name on an ex-employee's résumé mean that employee has competence?" That's some competitor's problem, not theirs.

Software Engineer and former SRE-SE here; also a two year drop out.

> Universities don't grant degrees until someone has proved they have whatever competency the degree indicates

That is not a rule. I have interviewed countless folks with CS and CE degrees that can't get past basic scalability questions in abstract systems design. I've started plenty of hour long interviews only to learn five minutes in that the candidate has a four year degree and cannot correctly identify a use case for binary search nor implement it in plain code.

> Universities have a reputation to lose if they grant a degree to someone who goes on to mess up badly

They absolutely do not lose reputation. We don't publish on a website where the degrees came from that allowed Cambridge Analytica to exploit the American people. Everyone will easily write off the offending programmers lineage as a fluke and move on with their lives. Nobody really cares where you came from in this world, just what you're up to now.

> Mr. Marotto making a major error when he's no longer employed by Facebook doesn't reflect poorly on Facebook.

I agree that this would be a pretty trash take and a poor precedent to establish long term.

> "Former Facebook engineer" means Facebook paid him... No more and no less.

Simultaneously, this is not true. Working for Facebook carries some prestige, as we can see from all the folks who proudly sport "x-Facebook", "x-Google", "x-Netflix" show us. Obviously some people make use of this market and obviously someone is foolish enough to believe the sheen. The value in that perception rests with the viewer; if they hired him because of his work at Facebook then it was significant, if they would've hired him regardless then it matters very little.

>Universities don't grant degrees until someone has proved they have whatever competency the degree indicates.

haha

you'd be really shocked if you knew about the great lengths people go to get their degree and learning stuff ain't one of them :-)

Have you ever worked on any really large platforms or databases? IME it's pretty common that the idiomatic Arel/ActiveRecord code you might normally write can become hopelessly slow when the query gets complex and the datasets get large. I'm not trying to pick on ActiveRecord here, pretty much all ORMs I've ever used will do this sometimes. The only way to get the speed up to something acceptable may be for someone who knows SQL and the DB Schema well to write a custom SQL query that makes the most of the database indexes and optimizations.

When a large product in Production is having performance issues, it's very possible that you'll write something like that in a time crunch to get the site back up. The code might not be the cleanest possible because you aren't sure how well it will work yet. It might be checked into Git because any remotely sane deployment procedure requires it. And maybe somebody years later will pore through the Git logs and come back and drop snark on you without knowing anything about what was involved in writing and releasing that code and they've never had to deal with something like that.

At a glance my first though was "there's probably SQLi here", but it's not obvious to me that attacker controlled parameters could actually be used to exploit this. The ID and pagination parameters all seem to be ints, and it's not obvious to me that an attacker could turn them into arbitrary strings. Is custom_filters.phrase the attacker controlled string?

There's nothing in that code that says that any of those parameters have to be ints. They're certainly expecting ints, but there's no casts in that code that would enforce that. Presumably somewhere upstream they're just passing directly from the POST/querystring into these API functions.

Remember: Ruby is a dynamically typed language.

Sounds like a place where static types would have helped...

Not that one should rely entirely on them for these sorts of situations.

Static types don't help with string interpolation.

They help with ensuring you're not interpolating things that can be strings where you're only expecting numbers.

You'd expect the ID parameter to be typed (at a minimum from the database). It's not clear from the code in the article whether the pagination parameters are type checked (or whether they'd raise a type error at any point if set to strings).

It's definitely bad code, but if you're going to write an article which so heavily implies "this idiot's bad commit is why a breach occurred", then whether the code is actually exploitable seems worth investigating first imo.

Again, it's Ruby. Unless you do unusual things, there's no type checking beyond duck typing.

We can't tell from just that commit whether that code is definitely exploitable, but they'd have to be doing non-default things to make it not exploitable.

so once again using sane typing systems would save the day?

It'd have helped this specific case where we can know that all the arguments must be numbers, but you're going to hit the same injection issue as soon as you need to allow a string argument.

The general fix is bound parameters, not typed arguments.

porque-no-los-dos.gif

I guess that there's a principled argument against strong typing, but I can't think of one against bound parameters. :D

Not an argument per-se, but there were cases where I had to manually build raw query parameter-values when using the `IN(v1, v2, v3)` operation. Many libraries still do not support passing in a list parameter as of 2021, even newer ones. For example, the Rust MySQL library.

I felt much safer doing that knowing the parameters I'm concatenating are integers.

You could probably use a static type system to ensure you're not allowing string arguments from untrusted input and possibly even allow string constants to be used as arguments. Not that I think this provides any sort of advantage (or that I've even seen this), but it seems like it would be possible and work.

Why is it surprising to you that Facebook could hire someone inept?

I mean I did say it's only a little surprising.

Reply on news.ycombinator.com