Depixelate

I few weeks ago I was given access to a beta test of a Rails application (let’s call it foobar app to keep things civilized) that was built by an established rails consulting shop. The app used subdomains as account keys and my account came setup as depixelate.foobar.com.

I started poking around and immediately discovered an extremely serious bug.

Basically the app helps companies track projects (I’m anonymizing again). So I created a new project and was redirected to depixelate.foobar.com/projects/81. Of course behind the schemes I knew the app was rendering the ‘show’ view of the ProjectsController and loading the project with an id of 81.

Just for kicks I thought I’d snoop around. Expecting an error message I changed the id to 80, 79, 78, 77, etc. WOW! I shook my head as I looked through a dozen account’s projects before getting bored.

To be fair this was beta software. I submitted the bug report and it was quickly fixed.

Don’t let this happen in your app. Below are 3 ways to silo data securely to prevent these types of situations.

1) Always scope finders

The bug described above probably resulted from code like this:

def show
  @project = Project.find(params[:id])
end

The correct scope would have taken into account the project id and the account id.

I always use before_filters to simplify scoping in my apps. If I were designing the described app I might do something like this:

application.rb

class ApplicationController < ActionController::Base

  def current_account
    @current_account ||= (session[:account_id] ? Account.find(session[:account_id]) : nil)
  end

  def halt_and_respond_with(message)
    flash[:error] = message
    render :template => 'shared/message'
    false
  end
	
end

projects_controller.rb

class ProjectsController < ApplicationController
  before_filter :find_project, :except => [ :index, :new, :create ]
  ..

  def show
  end

  ...
	
  private
	
    def find_project
      @project = current_account.projects.find_by_id(params[:id])
      return halt_and_respond_with('Invalid project.') if @project.nil?
    end

end

2) Always test scoping

I know that you know that you’re suppose to test those edge cases. But remember that testing edge cases often means testing scope. Incorrect scoping often results in a security issue so make this a top priority when setting up your test suite.

Here is an example of testing scope from our described app:

accounts.yml

foo:
  id: 1
  name: foo
bar:
  id: 2
  name: bar

projects.yml

p1:
  id: 1
  account_id: 1
  name: foo project
p2:
  id: 2
  account_id: 2
  name: bar project

test_helper.rb

class Test::Unit::TestCase
  ...

  def login_as(a)
    @request.session[:account_id] = accounts(a).id
    @request.host = "#{account.subdomain}.localhost.com"  
  end
	
end

projects_controller_test.rb

class ProjectsControllerTest < Test::Unit::TestCase
  fixtures :accounts, :projects
  ...
  
  def test_show_scope
    login_as :foo
    get :show, :id => accounts(:bar).projects.first.id
    assert_template 'shared/message'
    assert flash[:error]
  end
	
end

3) Use scoped sequences rather than ids (dependent on app domain)

Sometimes sequential data per scope makes more sense than simple database ids. Consider an invoicing app with two accounts. One account creates the first invoice and it is assigned invoice #1. Meanwhile another account creates their first invoice and it is assigned invoice #2. This instantly confuses the user as they are wondering why their first invoice says #2. In cases like this sequences that are scope-specific work very well. I ripped apart the acts_as_paranoid plugin back in the day to produce acts_as_sequenced. If anyone’s interested I’ll package it up as a plugin. Otherwise toss it in lib/ and include in environment.rb.

Conclusion

Just scope it damnit! And test!