Original body is not closed when `:warden` is thrown
Closed this issue · 11 comments
result = catch(:warden) do
@app.call(env)
endIn the case where throw :warden is used, a reference to the @app's original body component is lost and is unable to be closed.
It is the responsibility of middleware who replace the response body to close the original response body, according to the spec. Failing to do so can cause several issues, such as orphaned resources and ThreadErrors from deadlocks.
I found this via a ThreadError: deadlock; recursive locking in a Rails 3.1 application. When Rails' threadsafe mode is disabled (as it is by default in Rails ~> 3.1), the entire Rack request chain is wrapped by Rack::Lock. As one can see, in Rack::Lock, the @mutex is unlocked when the body (returned wrapped by Rack::BodyProxy) is closed.
This is usually hidden in warden, because it is typically added to the middleware after Rack::Lock. However, it is still not in spec, causes other issues, and arises when the warden is used before Rails (e.g. in config.ru to authenticate multiple Rack/Rails applications in the same process, as is the case in the application with which I am currently working).
Awesome catch! Could you please provide a pull request as well? :)
Before working on any changes I just wanted to start a conversation.
Really, I'm not sure how this can be fixed without changing the API. At the very least, you could make passing :response key in the options parameter to throw.
Can you think of a way to approach this with minimal API changes that preserves access to the response body? I tried to write a patch at first in manager.rb but quickly realized the throwers need to change, not (just) the catcher.
Hrm, you are right. I am not even sure we can push this responsibility to Warden. In theory, if you are the final application in a Rack stack (like a Rails controller), you can simply call throw, because there is no body yet. The issue is if a middleware or something that dispatches to another application calls throw, then it needs to take into account the previous response. It seems to be more of a property of the throwing code than warden.
Can you give more details about who was calling throw :warden in your case?
Unless some magic is in the way, it doesn't seem like anything outside of warden itself does:
$ bundle show --paths | xargs grep -nr ':warden' ./
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/History.rdoc:21:* Use the default scopes action when using a bare throw(:warden)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/History.rdoc:127: * 401 behaves exactly like throw :warden (staugaard)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/hooks.rb:42: # throw(:warden, :scope => scope, :reason => "Times Up")
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/hooks.rb:105: # params[:warden_failure] = opts
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/manager.rb:27: # Invoke the application guarding for throw :warden.
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/manager.rb:34: result = catch(:warden) do
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/proxy.rb:119: # The same as +authenticate+ except on failure it will throw an :warden symbol causing the request to be halted
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/proxy.rb:128: throw(:warden, opts) unless user
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/strategies/base.rb:126: # This causes the strategy to fail. It does not throw an :warden symbol to drop the request out to the failure application
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/strategies/base.rb:127: # You must throw an :warden symbol somewhere in the application to enforce this
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/strategies/base.rb:143: # Causes the authentication to redirect. An :warden symbol must be thrown to actually execute this redirect
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/lib/warden/strategies/base.rb:165: # Return a custom rack array. You must throw an :warden symbol to activate this
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:35: throw(:warden, :action => :unauthenticated)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:45: throw(:warden)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:53: throw(:warden, :action => :unauthenticated) unless e['warden'].authenticated?(:failz)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:66: throw(:warden)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:77: throw(:warden)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:113: throw(:warden)
/Users/bjeanes/.gem/ruby/1.9.3/gems/warden-1.2.3/spec/warden/manager_spec.rb:140: throw(:warden)Ah, so very likely it is whoever is calling env['warden'].authenticate! which muddies things up a bit. In my case, it's from a Rails controller.
It may be that Rack::Lock is simply designed with the assumption that the stack won't ever be jumped and there is no great solution. That would make my life sad right now! :(
@bjeanes you can always remove the Lock from inside Rails and place it on config.ru. A bunch of other stuff would fail though, like session, connection management. It would be better to identify who is calling authenticate! and if it is not the endpoint, properly close whatever is before.
Yeah I'll play around to fix my immediate problem, but I do think this issue in general is something that should probably be considered in any future major changes to warden.
Unfortunately, I think that stack jumping in any way (throw or raise, though at least the latter can be intercepted arbitrarily) is incompatible with correctly behaving Rack middleware. Anytime they do work after @app.call(env) it's liable to not happen correctly.
In theory, if you are the final application in a Rack stack (like a Rails controller), you can simply call throw, because there is no body yet. The issue is if a middleware or something that dispatches to another application calls throw, then it needs to take into account the previous response. It seems to be more of a property of the throwing code than warden.
To clarify, this is the situation I'm in (a Rails controller is calling env['warden'].authenticate!, which in turn calls throw :warden). Even though there is no response body yet, it is still a problem because a middleware between the controller and Warden (i.e. Rack::Lock) is relying on the stack being unrolled correctly so that it can perform correctly (i.e. releasing the Mutex at the end of the request, which is awkwardly signaled by having the body closed).
It seems like Warden is probably breaking the (possibly implicit) expectations of how Rack and Rack::Lock function by jumping the stack.
There probably isn't a perfect solution as long as Warden employs throw, but that would be a significant change in both the API and implementation. Does this just make it a caveat? Should there be a warning on the tin?
I'd love a PR to the README to alert people to this issue.
Closing this out given its gotten fairly stale. Still open to a PR though!