Chris Morris
20420be1a6
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.
50 lines
1.5 KiB
Ruby
Executable File
50 lines
1.5 KiB
Ruby
Executable File
class UsersController < ApplicationController
|
|
|
|
skip_before_filter :has_info
|
|
skip_before_filter :authenticated, :only => [:new, :create]
|
|
|
|
def new
|
|
@user = User.new
|
|
end
|
|
|
|
def create
|
|
user = User.new(params[:user])
|
|
user.build_retirement(POPULATE_RETIREMENTS.shuffle.first)
|
|
user.build_paid_time_off(POPULATE_PAID_TIME_OFF.shuffle.first).schedule.build(POPULATE_SCHEDULE.shuffle.first)
|
|
user.build_work_info(POPULATE_WORK_INFO.shuffle.first)
|
|
user.performance.build(POPULATE_PERFORMANCE.shuffle.first)
|
|
if user.save
|
|
session[:user_id] = user.user_id
|
|
redirect_to home_dashboard_index_path
|
|
else
|
|
@user = user
|
|
render :new
|
|
end
|
|
end
|
|
|
|
def account_settings
|
|
@user = current_user
|
|
end
|
|
|
|
def update
|
|
message = false
|
|
#Safest
|
|
# user = current_user
|
|
|
|
# Still an Insecure DoR vulnerability
|
|
#user = User.find(:first, :conditions => ["user_id = ?", "#{params[:user][:user_id]}"])
|
|
|
|
user = User.find(:first, :conditions => "user_id = '#{params[:user][:user_id]}'")
|
|
user.skip_user_id_assign = true
|
|
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!
|
|
respond_to do |format|
|
|
format.html { redirect_to user_account_settings_path(:user_id => current_user.user_id) }
|
|
format.json { render :json => {:msg => message ? "success" : "false "} }
|
|
end
|
|
end
|
|
|
|
end
|