InvalidAuthenticityToken is not thrown anymore #1917

Closed
jaroslawr opened this Issue Jun 30, 2011 · 22 comments

Projects

None yet

5 participants

@jaroslawr

It seems that at some point in time the code of the request forgery protection was modified in such a way, that the InvalidAuthenticityToken exception is not thrown anymore. This change was made in a very sloppy way, take a look at:

https://github.com/rails/rails/blob/v3.0.9/actionpack/lib/action_controller/metal/request_forgery_protection.rb

The exception class is still there, but the exception is not being thrown anymore, instead the handle_unverified_request method is being called, which by default just quietly resets the session (!?). This is a pain to debug if you happen to have a problem with token somewhere in the application, you just get logged out of the sudden without any apparent reason. What's worse is that the handle_unverified_request method is not at all documented and in fact the documentation still talks about throwing an exception:

http://api.rubyonrails.org/classes/ActionController/RequestForgeryProtection.html

@vijaydev
Ruby on Rails member

From 3.1, Rails shows a warning in the logs when the session is reset. You are right about the api doc being outdated. You can change it in https://github.com/lifo/docrails, if you wish. Or I'm sure someone will take it up and fix :)

@pixeltrix
Ruby on Rails member

This is because of a security issue - read this for more information.

Yes, the method could be documented better - you can contribute some via the docrails project if you'd like to. The docrails repository is a public access repo where anyone can submit documentation patches.

@pixeltrix pixeltrix closed this Jun 30, 2011
@jaroslawr

Why are you closing this issue? This is a serious problem and it was not fixed. Since when is it OK for Rails to have docs that are misleading and completely out of date?

@vijaydev
Ruby on Rails member

I've made the change in docrails: rails/docrails@5fe67fa

@jaroslawr

vijaydev: Thanks!

I think this change was made in a rather mindless way and it should also be fixed in the code. The InvalidAuthenticityToken class is still present in request_forgery_protection.rb, which makes controllers using old invalid token handling code like this:

rescue_from ActionController::InvalidAuthenticityToken, :with => :handle_potential_forgery

def handle_potential_forgery(exception)
  ...
end

work without a glitch, even through the exception is not thrown any more and this code simply stops working when upgrading to a Rails version which introduced this change (A minor revision breaks compatibility and it's not even mentioned neither in the release notes nor in the documentation!). Either the handle_unverified_request method should be removed and the exception should be thrown again (after resetting the session?), or the exception class should be removed from the code and from the tests. In fact the commit that introduced this patched the test controllers to throw the exception as it was in the past, but it seems not enough thought was given to everybody else applications and tests:

ae19e41

Finding the cause of my application randomly logging people out costed me a few days of work and it seems to me a part of a larger issue of a very careless attitude towards maintaining compatibility and not breaking existing functionality with the introduction of the newest kool-aid. Until recently I seldom had to dig into Rails code to find a problem occurring with my application, but when migrating to Rails 3.0 I find problem after problem of this kind, previously I have spent a week trying to spot a performance regression, which turned out to be caused by changes in PostgreSQLAdapter and made the application in concern work three to four times slower. Perhaps changes should be made and accepted with greater care...

@pixeltrix
Ruby on Rails member

@vijaydev thanks for that - any chance you can document the handle_unverified_request method?

@sztywny I closed it because I knew that someone like @vijaydev would fix it and if I left it open then before too long we'll be back to where we were with the LH tracker and we'd have HN articles screaming "OMG! Rails has a thousand bugs". As for the hyperbole in your reply - no it's not a serious problem, it's not the first time that docs have been incorrect (it won't be the last either) and I think ‘completely’ out of date is a little bit of exaggeration don't you think.

I'm sorry you lost some time due to this but this kind of thing happens to all of us.

@jaroslawr

You knew that someone would fix it and that's why you have closed it? I thought it works a bit differently, first someone fixes it, then you close the issue ^^ I think it is a serious problem if people are loosing days of work because of it, and if you look at the links posted above by paneq I'm not the only one affected by this. I also pointed out a specific, technical defect with the changes that were made which you have chosen to completely ignore.

@vijaydev
Ruby on Rails member

@sztywny: I understand the frustration. I myself had suffered silent session resets and spent time on debugging. But let's not get carried away. Calling people's hard work mindless is not going to help anyone. And if I remember right, 3.0.4 was released primarily as a security fix release and this change was well advertised (on Google groups, rails blog etc).

As for the code changes you mention, @nzkoz or @tenderlove might have a better understanding and they can take this discussion forward.

@vijaydev
Ruby on Rails member

@pixeltrix: Sure. Will doc that.

@pixeltrix
Ruby on Rails member

@sztywny the reason the exception is still there is so that you can override handle_unverified_request and raise it yourself. The reason that the code defaults to resetting the session is that the main class of requests affected by the change were API requests which generally weren't using sessions so it was better than starting to generate errors. Since it was a security flaw there was no choice other than to break backwards compatibility.

@pixeltrix
Ruby on Rails member

@vijaydev thanks

@jaroslawr

@pixeltrix: I don't really understand the logic here. I think the session is being reset because of the security issue itself (see http://groups.google.com/group/rubyonrails-security/browse_thread/thread/2d95a3cc23e03665) and that's fine. But there always was a way to customize the handling of the situation in which the token was invalid, and the way to do it was to use rescue_from as I have shown above and I'm pretty sure a lot of applications already do it this way. So, if you don't want a server error you can customize the method handling the exception and you're all fine. If the exception would be thrown after the session was reset it would be still OK security-wise, it would not break compatibility in this aspect and people would more easily understand why the sudden logging-out occurs. If you absolutely want to introduce a method for handling an invalid token, that's fine, but it should not be done in a bugfix release, should be documented in release notes and in this situation what exactly would be the point of raising this exception from your handle_unverified_request? Sincerly, it looks to me like this was done just to avoid heavier modifications in the Rails test suite. If the exception would be removed, then at least this rescue_from code that is just dead now, would at least cause a runtime error.

@jaroslawr

@vijaydev: Have a look at the release notes: http://weblog.rubyonrails.org/2011/2/8/new-releases-2-3-11-and-3-0-4 It just says it's a security fix and there isn't a word about an upgrade procedure. The only place this was documented is a blog post: http://weblog.rubyonrails.org/2011/2/8/csrf-protection-bypass-in-ruby-on-rails

@vijaydev
Ruby on Rails member

To nitpick, what you call "release notes" is also a blog post. The links @paneq provided also talks a lot more about this.

@NZKoz
Ruby on Rails member

As mentioned by others this change was deliberate. We cannot raise exceptions in the unverified request scenario any more because we cannot whitelist API and Ajax requests like we did previously. Because of this the default handler resets the session which prevents the bulk of the relevant attack vectors.

However you do still need to be careful and we're clearly lacking documentation on the things you need to be careful with.

@sztywny: While I appreciate your frustration you seem to have missed that the entire reason that the releases were done was to address a serious, exploitable security flaw with the previous method. We had to change it, and change it in a way that's backwards compatible. Making the exception be raised by default would reject every single api request, which is clearly unacceptable.

Unfortunately you couldn't even work around this using rescue_from because an API without a csrf_token is valid you don't want to abort processing, but for security reasons you still needed to fire the unverified request handler.

@jaroslawr

@NZKoz: Ok, now I finally understand the rationale for this, perhaps parts of what you just said should also be part of documentation, so that it's harder for other people to miss the point as well.

@NZKoz
Ruby on Rails member

@sztywny: If you wanted to have a go at documenting it, I'd appreciate it. One of the problems is that as part of addressing the finer points of the flash vulnerability I no longer have any idea what is and isn't confusing to users who didn't spend a month brainstorming the problems and solutions

@jaroslawr

@NZKoz: One thing I still don't understand is why the InvalidAuthenticityToken exception class was and still is kept in the code. If it were removed then people migrating to newer Rails versions would get an error from their rescue_from clause, like it is right now that clause just silently stops working.

@NZKoz
Ruby on Rails member
@jaroslawr

@NZKoz: And this is the part that I still think should be changed. Seldom do libraries (frameworks) provide exception classes that the API user "might be willing to throw". If Rails itself doesn't raise the exception, it should not be part of Rails. From what I understand there is anyhow no point in raising InvalidAuthenticityToken from handle_unverified_request, then adding a rescue_from and finally doing the actual handling of the invalid token situation in some additional method, because you can do all the handling directly in handle_unverified_request, returning false in case one wants to stop the processing (this should maybe be documented). The same with rendering a 422 or a 500 http code. And removing this exception class prevents old code from failing silently after a migration to a newer version of Rails, which I guess will keep hitting migrating people if it won't be fixed.

@pixeltrix
Ruby on Rails member

@sztywny there are perfectly valid reasons for throwing the InvalidAuthenticityToken. For example you may be using the [Hoptoad] notification service and want to capture these errors. Now you could call the Hoptoad notifier yourself in handle_unverified_request but why not just raise the error and let the built-in handler deal with it.

And as @NZKoz points out, if you're upgrading and you have a bunch of code elsewhere handling this then just raising this error is quicker and simpler as an upgrade than having to rewrite and move the code into handle_unverified_request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment