Skip to content

Conversation

@chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented Jul 6, 2025

Fixes #295 by replicating logic below from Rails abstract_store which propagates the CSRF token from the request environment only during commit_session.

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

rails/rails@f2c66ce#diff-5f81b06a0e3051b576daee16c960b21e715a6cc26d97d020c546d2fa697c9bc6R345-R348

I couldn't really find an alternate approach that seemed better, e.g by re-using the Rails code directly (either Request.commit_csrf_token or AbstractStore.commit_session). This is because in the jruby-rack case, the request object that the store sees appears to be a Rack::Request whereas the Rails abstract_store code expects an ActionDispatch::Request so req.commit_csrf_token is not defined. I couldn't find a way to cleanly access the ActionDIspatch::Request from the Rack::Request either.

How this all works earlier is a bit confusing for me, and the current approach is obviously somewhat coupled to the current Rails 7.1/7.2/8.0 behaviour, but it seemed possibly OK given the otherwise deep integration we are dealing with here? The way they have implemented this in Rails doesn't seem great to me due to the need for AbstractStore to now understand when to invoke a special CSRF hook, so this has created a bit more coupling for us.

Additional minor technical changes

  • Changes sessions to inherit from non-deprecated ::Rack::Session::Abstract::Persisted to fix warnings
  • Avoid unnecessarily overriding context(env) and align signatures for overridden functions with Rack 2.2.x for easy of understanding (the only supported Rack version for jruby-rack 1.2)
  • Prefer newer impl names from ::Persisted rather than legacy ::ID (delete_session rather than destroy_session, write_session rather than set_session)
  • corrects appraisal lock files for running on older jruby versions

@chadlwilson chadlwilson force-pushed the fix-rails-71-session-usage branch 6 times, most recently from f654991 to 31ad419 Compare July 6, 2025 10:21
@chadlwilson chadlwilson force-pushed the fix-rails-71-session-usage branch 5 times, most recently from 5bf7143 to fc527fa Compare July 8, 2025 09:04
@chadlwilson chadlwilson marked this pull request as ready for review July 8, 2025 09:04
@chadlwilson chadlwilson changed the title Add Rails 7.1 CSRF token support Fix Rails 7.1 CSRF token support Jul 8, 2025
@chadlwilson
Copy link
Contributor Author

@kares if you could take a look, it'd be much appreciated!

@kares kares self-assigned this Jul 9, 2025
@kares kares self-requested a review July 9, 2025 07:35
@kares kares removed their assignment Jul 9, 2025
Previous lock files included a zeitwerk version requiring Ruby 3.2, i.e JRuby 10 causing unnecessary variation at install time.
@chadlwilson chadlwilson force-pushed the fix-rails-71-session-usage branch from b149e13 to ef22fbc Compare July 18, 2025 02:16
Copy link
Member

@kares kares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 haven't tried or investigated all the changes in Rack but this looks reasonable to me.

also invited you to the jruby-rack team, you should be able to merge these on your own now.

@chadlwilson chadlwilson merged commit d904dbd into jruby:master Jul 22, 2025
61 checks passed
@chadlwilson chadlwilson deleted the fix-rails-71-session-usage branch July 22, 2025 11:56
@chadlwilson chadlwilson added this to the 1.3.0 milestone Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants