[georgemcbay]

Speaking as someone who isn't a Rails developer (but does use GitHub Enterprise for work projects), when this first broke I was on the side of github and thought homakov was acting irresponsibly.

Now that more background is coming out, I think he probably did the Rails community at large a huge favor here. Had this just been fixed quietly on GitHub, that would certainly be better for GitHub's PR but the wider community might never have realized the lurking horror that the Rails team appears to have been unlikely to do anything about other than point people to the existing docs.

This situation shows that pointing people to those docs was clearly an inadequate solution. If GitHub (arguably the poster child for Rails apps outside of 37signal's own apps) could fuck this up, anyone using Rails could. All of this exposure to the problem is net positive for everyone using Rails other than GitHub and the core Rails team, IMO.

[prophetjohn]

It's not like this is a little known pitfall with Rails. Anyone who has read Hartl's Rails tutorial knows about it[1]. It's very commonly mentioned in basics for Rails security.

And I say all this someone who has never professionally developed for Rails. My experience with Rails consists of a couple half-done toy projects. I find it pretty surprising that Github makes this mistake. But I don't think they should be burned at the stake for this. The bigger problem was how they were initially handling the issue, which they're trying to rectify now.

[1] However, Hartl recommends using attr_accessible at the model level and DHH says this preventative measure should be implemented in the controller, ie:

    class PostsController < ActionController::Base
        def create
            Post.create(post_params)
        end
  
        def update
            Post.find(params[:id]).update_attributes!(post_params)
        end

    private
        def post_params
          params[:post].slice(:title, :content)
        end
    end

[nknight]

I'm frankly amazed at how optimistic HN seems to be about "professional" coding practices. To this day I find "professional" developers writing fresh SQL injection vulnerabilities with some frequency.

[celer]

People who read HN tend to be better coders than those who don't. Fizzbuzz exists because it is needed.

[javascriptlol]

I don't find it in any way surprising that this has happened any more than I would find it surprising that if you put a big hole in a footpath that someone would fall into it.

[antris]

I see this whole story as "Irresponsible kid accidentally teaches the whole town a lesson"

[acqq]

More "an innocent kid was the only one not afraid to say that emperor is actually naked."

Please read this post written by meric, then give the counterarguments if you have them:

http://news.ycombinator.com/item?id=3665083

"I don't believe it is the stunt that costed github five to six figures. The loss of wealth was already there from day 1 when Github developers did not read Rails documentation and/or when Rails decided to make attributes publicly accessible by default. Today it is merely a "correction" where instead of Github's customer losing confidential company information without knowing it is now Github bearing the costs upfront, as it should be. In the "emperor has no clothes" story would you say it was the kid who pointed out the emperor had no clothes caused the emperor's embarrassment?"

You can also compare the whole context with the misfeature of PHP:

http://www.php.net/manual/en/security.globals.php

[dandelany]

Irresponsible, perhaps, but somehow I doubt it was accidental :)

[reledi]

I think everyone won in the end. Homakov got the credit he deserved, Rails' security flaw got a lot of attention, and GitHub had the chance to prove themselves, which in my opinion they did.

[davesims]

It's an easy mistake to make, but arguably no easier than, for instance, not escaping input strings to guard against SQL injection. IMO it falls to the developer to set protected on vulnerable attributes. This is pretty basic Rails security practice.

EDIT: not 'escaping', but using hashes or formatted strings, etc., you get the idea.

[einhverfr]

I am glad you fixed this in your post btw, but I still think the initial thought of using escaping to solve issues like SQL injection shows the way we think about security is flawed.

In general we tend to see security flaws as programming flaws. In other words, the programmer makes a mistake and therefore there is a security hole. The problem with this approach is that programmers make mistakes all the time.

Certainly it is impossible to take all the weight off the programmer's shoulders. Mistakes will always allow a program to be misused, misdirected, and so forth. However, most security issues are best solved as architecture problems, not as developer problems.

For example input sanitation is generally a bad idea* beyond certain things we should never see in inputs. It's far better to find ways of making the input safe to the database and to the web interface that doesn't depend on it being sanitized on input.

* This is because you can only sanitize based on how you want things to go on output, whether you are outputting from your program to the db or to a user interface of some sort. If you sanitize for HTML, you can't use the same info reasonably well for LaTeX, etc.....

So I am of the considered opinion that data should be checked for basic sanity on input (no termination sequences in the middle of input strings, etc), and escaped on use or output. If you have a framework to do this, then you centralize that logic so you don't have to think about it every time. This drastically cuts down on things like XSS, SQL Injection, and the like.

This sort of thing again strikes me as something the framework should prevent. That's not necessarily a flaw of Rails if you use Rails as a toolkit for your own application frameworks. However, it is an architectural flaw, not a programming mistake.

[zzzeek]

I'm not a Rails/Ruby user but any decent database abstraction layer or ORM should be using bound parameters for all literal values. "Escaping" of SQL strings is best left to the database driver.

[michaelbuckbee]

You're correct and Rails does do this (handle parameters in such a way as to prevent SQL injection attacks), however it is always possible to circumvent these protections and code things up in such a way (concatenate your own raw SQL string and push it through) as to shoot yourself in the foot.

[javascriptlol]

Or you could disallow raw SQL strings and always construct programmatically (e.g. building ASTs). All of these recurring holes are due to bad design, period. Imagine if your microwave manufacturer said "ultimately it's up to the consumer to avoid irradiating himself". Nobody expects you to be saved from sticking a drill into your face, but nor should it electrocute you by forgetting to do something.

[jarin]

This kind of sums up the point of contention.

Rails, by default, does things like escaping input and output strings, CSRF protection, masking password fields in the logs, etc. So why doesn't it do the same with attribute assignment?

[capsule_toy]

I'll take a guess and say it's because it's not possible to magically generate that code. If I wrote a code generator, there's no way that program could know which attributes should be accessible. The only way to get a scaffold to work out of the box is to require some user configuration or allow all attributes to be modifiable.

It's a case of ease-of-use trumping security.

[speleding]

As homakov suggested, you could at least define *_id attributes as "protected" by default. Only being able to change attributes on your own records probably causes a lot less grieve.