Avoiding Scoping Pitfalls
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!
