Rails 7.1 CSRF / request_forgery_protection broken on jruby-rack session storage

[chadlwilson]

Rails 7.1 changed its CSRF protection approach in rails/rails#44283 to allow storage outside the session. As a side effect of this approach, Rails now stores the token temporarily inside the request.env and then hijacks commit_session within the Rails abstract_store to actually store the CSRF token inside the session just prior to commit. FWIW, this also broke Devise: rails/rails#52244

rails/rails@f2c66ce#diff-5207bc67aa19cddd5a4997dec3c69fa4ba541750fbd881c6f7e30b27df9ea9ddR70-R73

rails/rails@f2c66ce#diff-60b77e427ea7ba142faa477fac10b8d0134cede4e35a3b1953c425200fadf1acR433-R435

This hijacking seems to not work with jruby-rack and the CSRF token is lost, causing all subsequent POSTs etc to fail. I believe essentially we have lost "abstract_store compatibility" with this change in Rails.

I have a hack which fixes it, but need to find the "right" way to do this, potentially that is slightly easier to maintain and gets things a bit readier for Rack 3.x.....

def commit_session(req, status, headers, body)
  session = req.env[::Rack::RACK_SESSION]
  options = req.env[::Rack::RACK_SESSION_OPTIONS]
  session[:_csrf_token] = req.env["action_controller.csrf_token"] if req.env["action_controller.csrf_token"]

@kares are you perhaps able to help make a 1.2-stable branch I can target for a PR as at ff790fb ?

[kares]

Hey, here you go: https://github.com/jruby/jruby-rack/tree/1.2-stable
Thought some of the CI + deps-bumps would be useful to have as well, so branches a few commits after ff790fb

[kares]

And nice find regarding session-store changes, not sure how well jruby-rack could support multiple Rails versions with that...

[chadlwilson]

Hey, here you go: https://github.com/jruby/jruby-rack/tree/1.2-stable Thought some of the CI + deps-bumps would be useful to have as well, so branches a few commits after ff790fb

Thanks @kares - I agree. I'll see if a cherry pick of any other changes is needed.

And nice find regarding session-store changes, not sure how well jruby-rack could support multiple Rails versions with that...

I'm not actually sure how jruby-rack interoperated with abstract_store earlier (I am not a Rails expert, and relatively new to it yet there is a jruby-rack reference to it being "legacy"), or even how Rack::Request relates to ActionDispatch::Request in real usage so mainly I am diffing code and doing trial and error without really understanding the relationship with Rails. :-)

The main challenge is that jruby-rack overrides so much of Rack's session/store abstraction and is thus highly coupled; while also deliberately breaking some of Rack's features (which might not make much sense). This change by the Rails folks is also clearly making assumptions about use of abstract_store which jruby-rack (at least) seems to break.

I do have a hack at #296 which is working OK for this particular issue, however I'd ideally like to reinstate some "real" Rails tests if possible. If you have a local branch somewhere with your experiment reinstating a Rails stub application test, feel free to push it and I'll take a look?

[kares]

or even how Rack::Request relates to ActionDispatch::Request in real usage

Not sure if I am up-to-date with latest Rack but Rack::Request is mostly an utility class (isn't enforced by Rack).
You can assume ActionDispatch::Request as the request object in Rails.

Internally the AD::Request is using Rack::Request.new(env) (mostly for parsing request query/params).
Rack middleware should only rely on env object which AD::Request "implements" (include Rack::Request::Env).

The main challenge is that jruby-rack overrides so much of Rack's session/store abstraction and is thus highly coupled; while also deliberately breaking some of Rack's features (which might not make much sense). This change by the Rails folks is also clearly making assumptions about use of abstract_store which jruby-rack (at least) seems to break.

That part wasn't updated in a while, maybe there's better options now with Rails to implement a session-store.
At least the abstract_store sounds like it...