Skip to content
Permalink
Browse files Browse the repository at this point in the history
Protect Spree::OrdersController#populate against CSRF attacks
See
GHSA-h3fg-h5v3-vf8m
for all the details.

Some time ago, all order actions were left out of CSRF protection (see
95ea570). The reason given was that the
authentication token got stale after the second rendering because the
product page is cached. That was limited to `#populate` in
cb79754 (see also
spree/spree#5601).

However, those assumptions are not correct. Although the authenticity
token changes at every request, that doesn't mean that the old ones are
no longer valid. The variation comes from a one-time pad added to a
session-dependant token (and meant to avoid timing attacks). However,
before validation, that one-time pad is removed. That means the token
remains valid as long as the session has not been reset. Think about
submitting a form from one browser tab after opening another with the
same URL. Even if both tokens differ, the submission from the first tab
will still be valid. You can read
https://medium.com/rubyinside/a-deep-dive-into-csrf-protection-in-rails-19fa0a42c0ef
for an in-deep understanding.

The initial confusion could come because of
rails/rails#21948. Due to browser-side cache,
a form can be re-rendered and sent without any attached request cookie.
That will cause an authentication error, as the sent token won't match
with the one in the session (none in this case). There's no perfect
solution for that, and all partial fixes should be seen at the
application level. From our side, we must provide a safe default. For an
excellent survey of all the available options, take a look at
https://github.com/betagouv/demarches-simplifiees.fr/blob/5b4f7f9ae9eaf0ac94008b62f7047e4714626cf9/doc/adr-csrf-forgery.md.
The information given in that link is third-party but it's very
relevant here. For that reason we've copied it in the security advisory
(see link above), but all the credit goes to @kemenaran.
  • Loading branch information
waiting-for-dev committed Dec 14, 2021
1 parent f4b6de0 commit 4d17cac
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion frontend/app/controllers/spree/orders_controller.rb
Expand Up @@ -10,7 +10,6 @@ class OrdersController < Spree::StoreController
before_action :assign_order, only: :update
# note: do not lock the #edit action because that's where we redirect when we fail to acquire a lock
around_action :lock_order, only: :update
skip_before_action :verify_authenticity_token, only: [:populate]

def show
@order = Spree::Order.find_by!(number: params[:id])
Expand Down

0 comments on commit 4d17cac

Please sign in to comment.