Add prepend option to protect_from_forgery. #18334

simi · 2015-01-05T00:42:17Z

This is first iteration. I try to refactor tests to keep it DRY'ed.

simi · 2015-01-05T00:43:35Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -87,6 +87,7 @@ module ClassMethods
      #
      # * <tt>:only/:except</tt> - Passed to the <tt>before_action</tt> call. Set which actions are verified.
      # * <tt>:if/:unless</tt> - Passed to the <tt>before_action</tt> call. Set when actions are verified.
+      # * <tt>:prepend</tt> - ?


I think Passed to the <tt>before_action</tt> call. should be skipped here since it is not commonly used option. Any idea how to describe it here in few words?

:prepend - By default, the verification of the authentication token is added to the front of the callback chain. If you need to make the verification depend on other callbacks, like authentication methods (say cookies vs oauth), this might not work for you. Pass prepend: false to just add the verification callback in the position of the protect_from_forgery call. This means any callbacks added before are run first.

The two first options should be described in terms of before_action. That’s all internal stuff. Maybe just:

only/except - Only apply forgery protection to a subset of actions. Like only: [ :create, :create_all ]
if/unless - Turn off the forgery protection entirely depending on the passed proc or method reference.

Also, needs to say “You can disable forgery protection” not just CRSF — since we also check same origin.

On Jan 4, 2015, at 5:46 PM, Josef Šimánek notifications@github.com wrote:

In actionpack/lib/action_controller/metal/request_forgery_protection.rb #18334 (diff):

@@ -87,6 +87,7 @@ module ClassMethods
#
# * :only/:except - Passed to the before_action call. Set which actions are verified.
# * :if/:unless - Passed to the before_action call. Set when actions are verified.

# \* <tt>:prepend</tt> - ?
Thanks.

https://cloud.githubusercontent.com/assets/193936/5608460/edecd69a-9484-11e4-8602-431ce1246175.png
—
Reply to this email directly or view it on GitHub https://github.com/rails/rails/pull/18334/files#r22445355.

egilburg · 2015-01-05T21:39:16Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -94,9 +99,11 @@ module ClassMethods
      # * <tt>:reset_session</tt> - Resets the session.
      # * <tt>:null_session</tt> - Provides an empty session during request but doesn't reset it completely. Used as default if <tt>:with</tt> option is not specified.
      def protect_from_forgery(options = {})
+        options.reverse_merge!(prepend: true)


Perhaps not mutate options here passed by user?

And where? The idea behind this is to make prepend: true default (to keep backwards compatibility), but be able to pass prepend: false to before_action when you need.

options = options.reverse_merge(prepend: true)

That makes sense. Thanks @egilburg.

dhh · 2015-01-06T18:23:01Z

Did you want to do some more work on this, or should I merge as-is?

simi · 2015-01-06T18:31:45Z

I'm thinking about some test improvements (only code style), but it can be done later. Dou you think changelog note is needed here?

dhh · 2015-01-06T18:40:33Z

I think this is fine. And yes, let's add a changelog entry describing the new option.

simi · 2015-01-06T19:21:25Z

I added some CHANGELOG note. Feel free to suggest me better text since I'm not english native speaker.

dhh · 2015-01-06T19:26:01Z

Here's a full-fledged example for it:

* Allow you to pass `prepend: false` to protect_from_forgery to have the verification callback appended instead of prepended to the chain. This allows you to let the verification step depend on prior callbacks. Example:

  class ApplicationController < ActionController::Base
    before_action :authenticate
    protect_from_forgery unless: -> { @authenticated_by.oauth? }

    private
      def authenticate
        if oauth_request?
          # authenticate with oauth
          @authenticated_by = 'oauth'.inquiry
        else
          # authenticate with cookies
          @authenticated_by = 'cookie'.inquiry
        end
      end
  end

simi · 2015-01-06T20:44:26Z

Maybe it is good idea to mention this in guides also. Do you agree?
Dne 6.1.2015 20:26 "David Heinemeier Hansson" notifications@github.com
napsal(a):

Here's a full-fledged example for it:

Allow you to pass prepend: false to protect_from_forgery to have the verification callback appended instead of prepended to the chain. This allows you to let the verification step depend on prior callbacks. Example:

class ApplicationController < ActionController::Base
before_action :authenticate
protect_from_forgery unless: -> { @authenticated_by.oauth? }

private
def authenticate
if oauth_request?
# authenticate with oauth
@authenticated_by = 'oauth'.inquiry
else
# authenticate with cookies
@authenticated_by = 'cookie'.inquiry
end
end
end

Reply to this email directly or view it on GitHub
#18334 (comment).

dhh · 2015-01-06T20:53:21Z

Yes, that'd be great.

On Jan 6, 2015, at 12:45, Josef Šimánek notifications@github.com wrote:

Maybe it is good idea to mention this in guides also. Do you agree?
Dne 6.1.2015 20:26 "David Heinemeier Hansson" notifications@github.com
napsal(a):

Here's a full-fledged example for it:

Allow you to pass prepend: false to protect_from_forgery to have the verification callback appended instead of prepended to the chain. This allows you to let the verification step depend on prior callbacks. Example:

class ApplicationController < ActionController::Base
before_action :authenticate
protect_from_forgery unless: -> { @authenticated_by.oauth? }

private
def authenticate
if oauth_request?

authenticate with oauth

@authenticated_by = 'oauth'.inquiry
else

authenticate with cookies

@authenticated_by = 'cookie'.inquiry
end
end
end

Reply to this email directly or view it on GitHub
#18334 (comment).

—
Reply to this email directly or view it on GitHub.

dhh · 2015-01-08T18:39:31Z

If you add the description I gave to the changelog and rebase, we can merge this. Then we can follow up with a doc PR for the guides.

simi · 2015-01-08T18:47:47Z

@dhh done.

…rgery Add prepend option to protect_from_forgery.

dhh · 2015-01-08T19:27:17Z

Actually, on second thoughts, @carlosantoniodasilva suggested that this should just be the default behavior, and I couldn't come up with a reason why that shouldn't be so. The protection just needs to run before the action is hit, so there shouldn't be any backwards incompatibility with changing from prepend to append.

So let's just do that. You can still get the old behavior by adding prepend: true, should you need it. But the new default will just be nothing, as to follow regular before_action behavior.

simi · 2015-01-08T19:28:10Z

OK, I'll edit this.

egilburg · 2015-01-08T19:33:37Z

@dhh could this introduce potential security vulnerability for users who happen to do some important (e.g. db destructive) step in a before filter? Arguably it's not great app design, but perhaps need make this a deprecated process rather than silent switch.

dhh · 2015-01-08T19:35:44Z

Eugune, do you have any examples of that? Sounds pretty crazy.

On Jan 8, 2015, at 11:34 AM, Eugene Gilburg notifications@github.com wrote:

@dhh https://github.com/dhh could this introduce potential security vulnerability for users who happen to do some important (e.g. db destructive) step in a before filter? Arguably it's not great app design, but perhaps need make this a deprecated process rather than silent switch.

—
Reply to this email directly or view it on GitHub #18334 (comment).

simi · 2015-01-08T19:39:39Z

As I said before you can use before_filter to update some DB column and it will be triggered before verify_authenticity_token is called.

class ApplicationController < ActionController::Base
  before_filter :update_something
  protect_from_forgery
end

I think that is the reason why it was prepended. I think it is ok to add option to skip prepending, but it should stay unchanged by default.

dhh · 2015-01-08T19:42:14Z

Right, it’s technically possible to do that. It’s technically possible to do a lot of crazy shit. At some point you have to say “we’re not going to take on every possible ounce of crazy”. I’d like to see just one real example from this before going down another migration path for a possibly-maybe-probably-not.

On Jan 8, 2015, at 11:40 AM, Josef Šimánek notifications@github.com wrote:

As I said before you can use before_filter to update some DB column and it will be triggered before verify_authenticity_token is called.

class ApplicationController < ActionController::Base
before_filter :update_something
protect_from_forgery
end
—
Reply to this email directly or view it on GitHub #18334 (comment).

egilburg · 2015-01-08T19:42:19Z

@dhh no I don't, was just being a bit cautious since this is a security feature.

dhh · 2015-01-08T19:45:20Z

Yup, it’s worth considering, and no fault for bringing it up. But I’d like more than a “I could possibly imagine” before we drive it that way. As with everything, we’ll spell out these changes in the upgrade document.

On Jan 8, 2015, at 11:43 AM, Eugene Gilburg notifications@github.com wrote:

@dhh https://github.com/dhh no I don't, was just being a bit cautious since this is a security feature.

—
Reply to this email directly or view it on GitHub #18334 (comment).

Per this comment rails#18334 (comment) we want `protect_from_forgery` to default to `prepend: false`. `protect_from_forgery` will now be insterted into the callback chain at the point it is called in your application. This is useful for cases where you want to `protect_from_forgery` after you perform required authentication callbacks or other callbacks that are required to run after forgery protection. If you want `protect_from_forgery` callbacks to always run first, regardless of position they are called in your application, then you can add `prepend: true` to your `protect_from_forgery` call. Example: ```ruby protect_from_forgery prepend: true ```

Per this comment #18334 (comment) we want `protect_from_forgery` to default to `prepend: false`. `protect_from_forgery` will now be insterted into the callback chain at the point it is called in your application. This is useful for cases where you want to `protect_from_forgery` after you perform required authentication callbacks or other callbacks that are required to run after forgery protection. If you want `protect_from_forgery` callbacks to always run first, regardless of position they are called in your application, then you can add `prepend: true` to your `protect_from_forgery` call. Example: ```ruby protect_from_forgery prepend: true ```

simi reviewed Jan 5, 2015
View reviewed changes

simi force-pushed the prepend-false-for-protect-from-forgery branch from 40fbd51 to 6790897 Compare January 5, 2015 01:45

egilburg reviewed Jan 5, 2015
View reviewed changes

simi force-pushed the prepend-false-for-protect-from-forgery branch from 6790897 to d44164c Compare January 5, 2015 22:45

simi mentioned this pull request Jan 5, 2015

Improve protect_from_forgery documentation. #18354

Merged

simi force-pushed the prepend-false-for-protect-from-forgery branch from d44164c to 7d62d53 Compare January 6, 2015 09:42

simi force-pushed the prepend-false-for-protect-from-forgery branch from 7d62d53 to 2fce31f Compare January 6, 2015 19:20

simi force-pushed the prepend-false-for-protect-from-forgery branch from 2fce31f to f54a4a8 Compare January 8, 2015 18:46

Add prepend option to protect_from_forgery.

0074bbb

simi force-pushed the prepend-false-for-protect-from-forgery branch from f54a4a8 to 0074bbb Compare January 8, 2015 18:47

dhh added a commit that referenced this pull request Jan 8, 2015

Merge pull request #18334 from simi/prepend-false-for-protect-from-fo…

4dcfc1f

…rgery Add prepend option to protect_from_forgery.

dhh merged commit 4dcfc1f into rails:master Jan 8, 2015

simi deleted the prepend-false-for-protect-from-forgery branch January 8, 2015 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prepend option to protect_from_forgery. #18334

Add prepend option to protect_from_forgery. #18334

simi commented Jan 5, 2015

simi Jan 5, 2015

dhh Jan 5, 2015

simi Jan 5, 2015

dhh Jan 5, 2015

egilburg Jan 5, 2015

simi Jan 5, 2015

egilburg Jan 5, 2015

simi Jan 5, 2015

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

authenticate with oauth

authenticate with cookies

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

egilburg commented Jan 8, 2015

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

dhh commented Jan 8, 2015

egilburg commented Jan 8, 2015

dhh commented Jan 8, 2015

Add prepend option to protect_from_forgery. #18334

Add prepend option to protect_from_forgery. #18334

Conversation

simi commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

simi commented Jan 6, 2015

dhh commented Jan 6, 2015

authenticate with oauth

authenticate with cookies

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

egilburg commented Jan 8, 2015

dhh commented Jan 8, 2015

simi commented Jan 8, 2015

dhh commented Jan 8, 2015

egilburg commented Jan 8, 2015

dhh commented Jan 8, 2015