How Homakov hacked GitHub & the line of code that could have prevented it
I agree that the default for Rails should be more secure, but this was a really basic mistake by the GitHub team.  Yes, mistakes do happen, but there's a very simple way to avoid the exploit that is postulated in this article.<p>The 'schematic' of what the public key update looks like from the original post:<p><pre><code>    class PublicKeyController < ApplicationController
      before_filter :authorize_user
      ...
      def update
        @current_key = PublicKey.find_by_id params[:key]['id']
        @current_key.update_attributes(params[:key])
      end
    end
</code></pre>
The correct way to code this is as follows:<p><pre><code>    class PublicKeyController < ApplicationController
      before_filter :authorize_user
      ...
      def update
        @current_key = current_user.public_keys.find params[:key]['id']
        @current_key.update_attributes(params[:key])
      end
    end
</code></pre>
This has two updates to it that protect against this exploit:<p>1. Rather than calling "find_by_id" on the PublicKey model, which searches all public keys, you call it on the current user's list of public keys.  This scopes the search down to the public keys that they own.  Thus, if you pass in the id of a key they do not own, it will not be found, leading us to:<p>2. Using "find" instead of "find_by_id" will trigger an ActiveRecord::RecordNotFound error (404) if the resource is not found.  Of course, find_by_id will just return nil in this instance, so the update_attributes part would still fail, but triggering a 404 is an easier, cleaner way of dealing with this, I think.<p>It's really very simple: you don't let people access stuff they don't own.<p>Now, this does not protect you against faked timestamps, or against privilege escalation by passing in a faked "role" parameter, and so on.  You still need to use attr_accessible to protect yourself from that stuff, but scoping resources down to the user who owns them is a simple technique that should be standard practice for applications with authentication.