Skip to content

Commit

Permalink
Fix account takeover through CSRF attack
Browse files Browse the repository at this point in the history
This commit fixes an account takeover vulnerability when [Rails
`protect_from_forgery`](https://api.rubyonrails.org/classes/ActionController/RequestForgeryProtection/ClassMethods.html)
method is both:

- Executed whether as:
  - A `before_action` callback (the default)
  - A `prepend_before_action` (option `prepend: true`) before the
  `:load_object` hook in `Spree::UsersController` (most likely order to
  find).

- Configured to use `:null_session` or `:reset_session`
  strategies (`:null_session` is the default in case the no strategy is
  given, but `rails --new` generated skeleton use `:exception`).

Before this commit, the user was fetched in a `prepend_before_action`
hook named `:load_object`. I.e., the user was loaded into an instance
variable before touching the session as a safety countermeasure. As the
request went forward, `#update` was called on that instance variable.

The `:exception` strategy prevented the issue as, even if the user was
still fetched, the request was aborted before the update phase. On the
other hand, prepending `:protect_from_forgery` after the `:load_object`
hook (not very likely, as `ApplicationController` is loaded in
the first place and it's the most likely place to have that definition)
wiped the session before trying to fetch the user from it.

We could have fixed the most likely issue by just using a
`before_action` for `:load_object`, but it's safer not to rely on the
order of callbacks at all.
  • Loading branch information
waiting-for-dev committed Nov 12, 2021
1 parent c03ca9a commit ec4fbe6
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/controllers/frontend/spree/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

class Spree::UsersController < Spree::StoreController
skip_before_action :set_current_order, only: :show, raise: false
prepend_before_action :load_object, only: [:show, :edit, :update]
prepend_before_action :authorize_actions, only: :new

include Spree::Core::ControllerHelpers

def show
load_object
@orders = @user.orders.complete.order('completed_at desc')
end

Expand All @@ -25,7 +25,12 @@ def create
end
end

def edit
load_object
end

def update
load_object
if @user.update(user_params)
spree_current_user.reload
redirect_url = spree.account_url
Expand Down
42 changes: 42 additions & 0 deletions spec/requests/spree/frontend/user_update_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# frozen_string_literal: true

RSpec.feature 'User update', type: :request do
context 'CSRF protection' do
%i[exception reset_session null_session].each do |strategy|
# Completely clean the configuration of forgery protection for the
# controller and reset it after the expectations. However, besides `:with`,
# the options given to `protect_from_forgery` are processed on the fly.
# I.e., there's no way to retain them. The initial setup corresponds to the
# dummy application, which uses the default Rails skeleton in that regard.
# So, if at some point Rails changed the given options, we should update it
# here.
around do |example|
controller = Spree::UsersController
old_allow_forgery_protection_value = controller.allow_forgery_protection
old_forgery_protection_strategy = controller.forgery_protection_strategy
controller.skip_forgery_protection
controller.allow_forgery_protection = true
controller.protect_from_forgery with: strategy

example.run

controller.allow_forgery_protection = old_allow_forgery_protection_value
controller.forgery_protection_strategy = old_forgery_protection_strategy
end

it "is not possible to take account over with the #{strategy} forgery protection strategy" do
user = create(:user, email: '[email protected]', password: 'password')

post '/login', params: "spree_user[email][email protected]&spree_user[password]=password"
begin
put '/users/123456', params: 'user[email][email protected]'
rescue
# testing that the account is not compromised regardless of any raised
# exception
end

expect(user.reload.email).to eq('[email protected]')
end
end
end
end

0 comments on commit ec4fbe6

Please sign in to comment.