Skip to content

Commit 64eff7f

Browse files
committed
Security: Attempt to block auth of nil tokens, etc.
Some Rails authentication systems have suffered from a vulnerability involving nil or blank login tokens: http://www.rorsecurity.info/2007/10/28/restful_authentication-login-security/ This patch includes a bunch of test cases testing for possible attacks along these lines, and some sanity-checking code in our authentication methods. Note that the tests and the code don't really "line up" here--most of the test methods passed already, and most of the sanity-checking code is probably unnecessary. But again, better safe than sorry.
1 parent c500bf8 commit 64eff7f

File tree

2 files changed

+47
-0
lines changed

2 files changed

+47
-0
lines changed

app/models/user.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ def self.find_admins(*args)
3232

3333
# Authenticates a user by their login name and unencrypted password. Returns the user or nil.
3434
def self.authenticate_for(site, login, password)
35+
return nil if site.nil? || login.nil? || login.blank? || password.nil? || password.blank?
3536
u = find(:first, @@membership_options.merge(
3637
:conditions => ['users.login = ? and (memberships.site_id = ? or users.admin = ?)', login, site.id, true]))
3738
u && u.authenticated?(password) ? u : nil
@@ -55,6 +56,7 @@ def self.find_all_by_site_with_deleted(site, options = {})
5556
end
5657

5758
def self.find_by_token(site, token)
59+
return nil if site.nil? || token.nil? || token.blank?
5860
find(:first, @@membership_options.merge(:conditions => ['token = ? and token_expires_at > ? and (memberships.site_id = ? or users.admin = ?)', token, Time.now.utc, site.id, true]))
5961
end
6062

spec/models/user_spec.rb

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
require File.dirname(__FILE__) + '/../spec_helper'
2+
3+
describe User do
4+
before :each do
5+
@site = Site.make
6+
end
7+
8+
def make_admin_with_token token
9+
user = User.make(:token_expires_at => 1.day.from_now, :admin => true)
10+
user.token = token # May be nil, so we can't pass to User.make.
11+
user.save!
12+
end
13+
14+
it "should not find users with nil token" do
15+
# This test always passed, before we did anything specific to fix it.
16+
make_admin_with_token nil
17+
User.find_by_token(@site, nil).should be_nil
18+
end
19+
20+
it "should not find users with empty token" do
21+
make_admin_with_token ''
22+
User.find_by_token(@site, '').should be_nil
23+
end
24+
25+
def make_admin_with_login_and_password login, password
26+
User.make(:login => login, :password => password, :admin => true)
27+
end
28+
29+
it "should not find users with empty login" do
30+
begin
31+
make_admin_with_login_and_password '', 'foo'
32+
User.authenticate_for(@site, '', 'foo').should be_nil
33+
rescue ActiveRecord::RecordInvalid # This is OK, too.
34+
end
35+
end
36+
37+
it "should not find users with empty password" do
38+
begin
39+
make_admin_with_login_and_password 'joe', ''
40+
User.authenticate_for(@site, 'joe', '').should be_nil
41+
rescue ActiveRecord::RecordInvalid # This is OK, too.
42+
end
43+
end
44+
end
45+

0 commit comments

Comments
 (0)