This project is archived and is in readonly mode.

#555 ✓ resolved
Robin Lu

protect_from_forgery is not quite class-wise

Reported by Robin Lu | July 6th, 2008 @ 06:03 PM | in 2.x

protect_from_forgery is not quite class-wise currently. When one controller declares protect_from_forgery, protect_against_forgery? of every controller returns true even for controllers without protect_from_forgery. This causes the inconsistency of forgery verification and code generation of helpers. The controller without verify_authenticity_token filter still needs a :secret to generate the token for helpers like link_to_remote.

If I want controller without protect_from_forgery not being affected, I have to explicitly set allow_forgery_protection to false foe each of them or set allow_forgery_protection to false in ApplicationController but still explicitly set allow_forgery_protection to true whenerver protect_from_forgery is called.

This patch exchange the usage of allow_forgery_protection and request_forgery_protection_token which seems a bit misplaced for me. It means to make protect_from_forgery more class-wise without breaking default behavior of the protection. You can still declare protect_from_forgery in ApplicationController to enable all or you can make it controller by controller without affecting other controllers.

Comments and changes to this ticket

  • José Valim

    José Valim July 7th, 2008 @ 03:38 PM

    I've run unto this problem myself and I like Your solution.

    +1

  • Robin Lu

    Robin Lu July 7th, 2008 @ 05:59 PM

    Maybe I should give an example here.

    For two controllers, one has protect_from_forgery and not.

    Class A < ApplicationController

    protect_from_forgery

    ...

    end

    Class B < ApplicationController

    session :off

    ...

    end

    If I did not use cookie session or declare controller B as session off, when I use link_to_remote in the views for B, I get a crash for no :secret is given in B.

    By default allow_forgery_protection is true and request_forgery_protection_token is a cattr_accessor. So no matter where protect_from_forgery is called once, protect_against_forgery? will return true everywhere.

  • Michael Koziarski

    Michael Koziarski July 8th, 2008 @ 07:35 PM

    • Assigned user set to “Michael Koziarski”

    This looks ok but needs tests.

  • Florian Aßmann

    Florian Aßmann July 26th, 2008 @ 09:35 AM

    I think the controller should modify the router tree and mark paths as protected.

    When the path for the xhr is marked as forgery protected it should request a token.

    Just an idea though...

    Since I'm not using action specific FP I only:

    -- a/actionpack/lib/action_controller/base.rb

    +++ b/actionpack/lib/action_controller/base.rb

    @@ -340,7 +340,7 @@ module ActionController #:nodoc:

    1. Sets the token parameter name for RequestForgery. Calling +protect_from_forgery+
    2. sets it to :authenticity_token by default.
    • cattr_accessor :request_forgery_protection_token
    • class_inheritable_accessor :request_forgery_protection_token
    1. Indicates whether or not optimise the generated named
    2. route helper methods

    Regards

    Florian Aßmann

  • Joshua Peek

    Joshua Peek October 28th, 2008 @ 04:33 PM

    • State changed from “new” to “open”
  • Pratik

    Pratik March 6th, 2009 @ 05:37 PM

    • State changed from “open” to “incomplete”
  • CancelProfileIsBroken

    CancelProfileIsBroken August 5th, 2009 @ 02:32 PM

    • Tag changed from actionpack, patch, request-forgery-protection to actionpack, bugmash, patch, request-forgery-protection
  • Elise Huard

    Elise Huard August 9th, 2009 @ 11:14 AM

    -1, tested this and it's not a problem on master

  • CancelProfileIsBroken

    CancelProfileIsBroken August 10th, 2009 @ 02:49 AM

    • State changed from “incomplete” to “resolved”

    I think we can call this resolved now.

  • Jeremy Kemper

    Jeremy Kemper August 10th, 2009 @ 07:10 AM

    • Tag changed from actionpack, bugmash, patch, request-forgery-protection to actionpack, patch, request-forgery-protection
  • klkk

    klkk May 23rd, 2011 @ 03:14 AM

    • Importance changed from “” to “”

Create your profile

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

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Attachments

Pages