From 20420be1a670d4d3c33bfd0c0689d8fa4b5d35e2 Mon Sep 17 00:00:00 2001
From: Chris Morris <chris.morris@livingsocial.com>
Date: Wed, 25 Sep 2013 16:56:34 -0500
Subject: [PATCH] Fixed logic to strip out user params.

Disclaimer: changes like these in this sort of app are tricky because
it's harder to presume the intention of the code in question.

The prior line:

```
user.update_attributes(params[:user].reject { |k| k == ("password" || "password_confirmation") || "user_id" })
```

returns an empty hash, because of the way the block evaluates:

```
irb(main):002:0> k = 'foo'
=> "foo"
irb(main):003:0> k == ("password" || "password_confirmation") || "user_id"
=> "user_id"
```

Before the last change to that line, without 'user_id' outside the
params, it didn't evaluate properly either:

```
irb(main):007:0> k = 'password_confirmation'
=> "password_confirmation"
irb(main):008:0> k == ("password" || "password_confirmation")
=> false
irb(main):009:0> ("password" || "password_confirmation")
=> "password"
```

So, in the normal use case for this form, you can't update any other
attribute of the User. To me, that's probably the best argument for
making this change, but it does simplify the SQL Injection attack
(although perhaps the prior complication was intended).

Before this change, injecting conditional SQL into the user_id param in
the account_settings update call would allow the password of whatever
account is found (e.g. the first one if injecting 'OR 1=1') to be reset,
but without additional attacks, the email address of that account is not
known.

After this change, the email address of that account now is also updated
in addition to the password, making it simpler to get in as an admin --
though you're still presuming the first account to be an admin.
---
 .gitignore                          | 18 ++----------------
 Gemfile                             |  9 +++++----
 Gemfile.lock                        |  1 +
 app/controllers/users_controller.rb |  2 +-
 4 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/.gitignore b/.gitignore
index 6e10082..c87e5f5 100755
--- a/.gitignore
+++ b/.gitignore
@@ -1,22 +1,8 @@
-# See http://help.github.com/ignore-files/ for more about ignoring files.
-#
-# If you find yourself ignoring temporary files generated by your text editor
-# or operating system, you probably want to add a global ignore instead:
-#   git config --global core.excludesfile ~/.gitignore_global
-
-# Ignore bundler config
 /.bundle
-
-# Ignore the default SQLite database.
+/bin
 /db/*.sqlite3
-
-# Ignore all logfiles and tempfiles.
 /log/*.log
 /tmp
 .elasticbeanstalk/
-
-# Ignore Mac folder settings
 .DS_Store
-
-# Ignore data directory
-/public/data
\ No newline at end of file
+/public/data
diff --git a/Gemfile b/Gemfile
index 72031de..1f1f2d9 100755
--- a/Gemfile
+++ b/Gemfile
@@ -11,13 +11,14 @@ gem 'foreman'
 
 group :development do
   gem 'brakeman'
-  gem 'guard-brakeman'
-	gem 'guard-rspec'
-  gem 'rb-fsevent'
-  gem 'guard-shell'
   gem 'bundler-audit'
+  gem 'guard-brakeman'
   gem 'guard-livereload'
+  gem 'guard-rspec'
+  gem 'guard-shell'
+  gem 'pry'
   gem 'rack-livereload'
+  gem 'rb-fsevent'
 	gem 'travis-lint'
 end
 
diff --git a/Gemfile.lock b/Gemfile.lock
index e82bd83..27bae65 100755
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -237,6 +237,7 @@ DEPENDENCIES
   jquery-rails
   minitest (~> 4.0)
   powder
+  pry
   rack-livereload
   rails (= 3.2.13)
   rb-fsevent
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 09701a3..b52a43b 100755
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -36,7 +36,7 @@ class UsersController < ApplicationController
     
     user = User.find(:first, :conditions => "user_id = '#{params[:user][:user_id]}'")
     user.skip_user_id_assign = true
-    user.update_attributes(params[:user].reject { |k| k == ("password" || "password_confirmation") || "user_id" })
+    user.update_attributes(params[:user].reject { |k| %w(password password_confirmation user_id).include? k })
     pass = params[:user][:password]
     user.password = pass if !(pass.blank?)
     message = true if user.save!