5 ms·
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 though
by georgemcbay 15y ago
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.
- antris 15y agoI see this whole story as "Irresponsible kid accidentally teaches the whole town a lesson"
- dandelany 15y agoIrresponsible, perhaps, but somehow I doubt it was accidental :)
- acqq 15y agoMore "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 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 http://www.php.net/manual/en/security.globals.php
- deleted 15y ago[deleted]
- davesims 15y agoIt'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.
- zzzeek 15y agoI'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 15y agoYou'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 15y agoOr 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 15y agoThis 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?
- 15y ago
- prophetjohn 15y agoIt'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 15y agoI'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 15y agoPeople who read HN tend to be better coders than those who don't. Fizzbuzz exists because it is needed.
- javascriptlol 15y agoI 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.
- reledi 15y agoI 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.