This project is archived and is in readonly mode.

#3913 ✓resolved
bshelton229

protect_from_forgery :except override in individual controllers isn't working in rails 3pre

Reported by bshelton229 | February 9th, 2010 @ 06:58 PM | in 3.0.2

When protect_from_forgery is defined in application_controller.rb (ApplicationController), protect_from_forgery :except => :method within individual controllers doesn't skip forgery protection for those methods as it should.

If protect_from_forgery is set in each controller explicitly, one of them containing an :except clause, the exception is honored. Seems to be that the problem is only when protect_from_forgery is defined in ApplicationController, the exceptions in individual controllers don't take.

Comments and changes to this ticket

  • José Valim

    José Valim February 10th, 2010 @ 06:48 AM

    • Milestone cleared.
    • Assigned user set to “Yehuda Katz (wycats)”
  • Dan Pickett

    Dan Pickett May 9th, 2010 @ 06:22 PM

    • Tag set to bugmash
  • Rohit Arondekar

    Rohit Arondekar May 15th, 2010 @ 11:37 AM

    Confirmed on Rails 3 beta 3 on ruby 1.9.2dev (2010-05-08 trunk 27665)

    Took the following steps (or skip to the attached rails 3 beta 3 app)

    rails forgery_3913
    rails generate scaffold Post title:string content:text
    rails generate controller Imposter new

    Added protect_from_forgery :except => :create to posts_controller

    Edited app/views/imposter/new.html.erb to have => http://pastie.org/961450

    rails server

    Go to http://localhost:3000/imposter/new and try submitting the form. It throws a ActionController::InvalidAuthenticityToken in PostsController#create

    P.S Find attached a rails 3 b3 app. Do rails server and try submitting from the above url. Should not work.

  • Anil Wadghule

    Anil Wadghule May 15th, 2010 @ 01:12 PM

    Verified this issue on Rails head. But looks like it's an issue with before_filter. Attached is an rails app showing issue with before filter.

    in ApplicationController,

    I have added

    before_filter :set_message

    def set_message
    @message = "I am set" end

    and in UsersController,

    before_filter :set_message, :except => :index

    http://localhost:3000/users/ should not print "I am set". But it is printing.

  • Anil Wadghule
  • Ryan Bigg

    Ryan Bigg May 17th, 2010 @ 10:36 AM

    I can confirm this issue exists too.

  • Ryan Bigg

    Ryan Bigg May 17th, 2010 @ 10:37 AM

    • State changed from “new” to “open”
  • Rohit Arondekar

    Rohit Arondekar May 18th, 2010 @ 06:31 AM

    I have a failing test for this issue. I've attached it as a patch. I've tested it on ruby 1.9.2dev (2010-05-08 trunk 27665) [x86_64-linux]

    P.S This is my first addition of a test so if I've not followed any conventions or need to make changes feel free to let me know and I'll do them.

  • Rohit Arondekar

    Rohit Arondekar May 18th, 2010 @ 06:33 AM

    I forgot to mention that it's not for the original issue, but rather for the issue Anil reported regarding the before_filter. Since it looks like this is cause the original issue.

  • Anil Wadghule

    Anil Wadghule May 18th, 2010 @ 12:21 PM

    Rohit, your test patch looks good. But you can write it in filters_test.rb instead of callbacks_test.rb.

  • Rohit Arondekar

    Rohit Arondekar May 18th, 2010 @ 02:19 PM

    I have a few questions.

    (1) Given that ApplicationController has a before_filter :foo and BooController < ApplicationController has a before_filter :foo, how does Rails know that the second before_filter shouldn't be executed?

    (2) If I tell you that the before_filter of ApplicationController is being run before the before_filter of a specific controller has the chance to specify :except, would I be talking sense or am I talking crazy?

    I have reason to believe that (2) is happening. But I can't explain why exactly because I've used rudimentary echoing of vars in callbacks.rb to arrive at that conclusion. If what I've said is of any help to arrive at a solution, then great! But I could be talking total nonsense and if I am please ignore me :P

  • Rohit Arondekar

    Rohit Arondekar May 20th, 2010 @ 01:39 AM

    I have a little more vague information.

    I think the "appending" that's happening is to the filter chain. Or is there such a thing as a callback chain too?

    I think before the specific before_filter gets a chance to provide the :except option, the before_filter of the ApplicationController is being executed. But it doesn't execute the before_filter twice, so there is a check for this?

    Can somebody point to the code where the filter chain is processed/executed?

  • Rohit Arondekar

    Rohit Arondekar May 20th, 2010 @ 10:00 AM

    Some more info:

    I think I'm certain now that filters are defined over callbacks. See api comment for set_callback in activesupport/lib/active_support/callbacks.rb


      # When creating or skipping callbacks, you can specify conditions that
      # are always the same for a given key. For instance, in ActionPack,
      # we convert :only and :except conditions into per-key conditions.
      #
      #   before_filter :authenticate, :except => "index"
      # becomes
      #   dispatch_callback :before, :authenticate, :per_key => {:unless => proc {|c| c.action_name == "index"}}
    

    Also I think this is the method that is somehow not removing the before_filter of the ApplicationController even though there is a before_filter with :except option in a specific controller.


      def skip_callback(name, *filter_list, &block)
        __update_callbacks(name, filter_list, block) do |chain, type, filters, options|
          filters.each do |filter|
            filter = chain.find {|c| c.matches?(type, filter) }
    
            if filter && options.any?
              new_filter = filter.clone(chain, self)
              chain.insert(chain.index(filter), new_filter)
              new_filter.recompile!(options, options[:per_key] || {})
            end
    
            chain.delete(filter)
            send("_removed_#{name}_callbacks") << filter
          end
        end
      end
    

    I don't know how long it will take to me to understand the whole flow completely, so I thought I would just put it in here for anyone else who might know how to fix it.

  • Rohit Arondekar

    Rohit Arondekar June 9th, 2010 @ 08:32 AM

    I don't think there is any code that will remove the before_filter set by ApplicationController even though it's subclasses have one defined, with exceptions. Can anybody point me to the code that does this?

  • José Valim

    José Valim June 9th, 2010 @ 08:37 AM

    • Milestone cleared.
    • Assigned user changed from “Yehuda Katz (wycats)” to “José Valim”

    The code does not remove the previous before filter, but should update it. This is a serious bug, I should work on it with Yehuda.

  • Bruno Michel

    Bruno Michel June 11th, 2010 @ 05:05 PM

    FWIW, I'd always used:

    skip_before_filter :verify_authenticity_token, :only => [:method]
    

    for skiping this filter. I'm not convinced that declaring the same filter two time should merge their :only|:except, skip_before_filter is here for that, no?

  • Rohit Arondekar

    Rohit Arondekar June 22nd, 2010 @ 06:44 AM

    Was this fixed in master? I can't reproduce this anymore.

  • Repository

    Repository June 22nd, 2010 @ 06:58 AM

    • State changed from “open” to “resolved”

    (from [02399a1184d0f87b14af461fab731120d92c5ad8]) Ensure overwritten callbacks conditions in controllers work [#4761 state:resolved] [#3913 state:resolved] http://github.com/rails/rails/commit/02399a1184d0f87b14af461fab7311...

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “High”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Tags

Referenced by

Pages