Explicitly fail on attempts to write into disabled sessions #42231

byroot · 2021-05-15T09:30:37Z

Until now config.session_store :disabled simply silently
discard the session hash at the end of the request.

By explicitly failing on writes, it can help discovering bugs
earlier.

Reads are still permitted.

I'm opening this as a draft because there are still quite a lot of unknowns.

byroot · 2021-05-15T09:31:50Z

actionview/lib/action_view/helpers/csrf_helper.rb

+      rescue ActionDispatch::Request::Session::DisabledSessionError
+        # TODO: proper deprecation message
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          Calling `csrf_meta_tags` when session are disabled is deprecated. Either configure session or disable CSRF protection
+        MSG
+        nil


I think this is a good example of why failing loudly would be preferable. Right now if you disable sessions but forget to disable CSRF protection, the tokens would still be generated but would always fail to validate.

Another possibility is to not output any tags and log a debug warning.

This has echos of when we disabled autoload for the lib folder. Some people prefer hard failure and some people prefer soft warnings so I suspect it'll need to be configurable - hard failure for new applications and soft warnings for upgrading apps.

Another possibility is to not output any tags and log a debug warning.

Yes that's what I'm doing here as to preserve the current behavior (except it uses a deprecation warning rather than a regular one). I think that make sense when changing existing things.

hard failure for new applications and soft warnings for upgrading apps.

Agreed.

byroot · 2021-05-15T10:14:28Z

The Active Record test failure is also interesting:

Error:
ActiveRecord::DatabaseSelectorTest#test_the_middleware_chooses_writing_role_with_POST_request:
ActionDispatch::Request::Session::DisabledSessionError: Your application has sessions disabled. To write to the session you must first configure a session store
    /rails/actionpack/lib/action_dispatch/request/session.rb:250:in `load_for_write!'
    /rails/actionpack/lib/action_dispatch/request/session.rb:148:in `[]='
    /rails/activerecord/lib/active_record/middleware/database_selector/resolver/session.rb:39:in `update_last_write_timestamp'
    /rails/activerecord/lib/active_record/middleware/database_selector/resolver.rb:73:in `block (2 levels) in write_to_primary'
    /rails/activesupport/lib/active_support/notifications/instrumenter.rb:44:in `instrument'
    /rails/activerecord/lib/active_record/middleware/database_selector/resolver.rb:70:in `block in write_to_primary'
    /rails/activerecord/lib/active_record/connection_handling.rb:370:in `with_role_and_shard'
    /rails/activerecord/lib/active_record/connection_handling.rb:160:in `connected_to'
    /rails/activerecord/lib/active_record/middleware/database_selector/resolver.rb:69:in `write_to_primary'
    /rails/activerecord/lib/active_record/middleware/database_selector/resolver.rb:44:in `write'
    /rails/activerecord/lib/active_record/middleware/database_selector.rb:70:in `select_database'
    /rails/activerecord/lib/active_record/middleware/database_selector.rb:57:in `call'
    /rails/activerecord/test/cases/database_selector_test.rb:285:in `test_the_middleware_chooses_writing_role_with_POST_request'

I think it makes sense. That's another footgun where you could setup the DB selection middleware in an API only app, and not realize that it's silently not working.

pixeltrix · 2021-05-15T10:31:07Z

I think it makes sense. That's another footgun where you could setup the DB selection middleware in an API only app, and not realize that it's silently not working.

Ugh, seems that's a current issue - was surprising to me that the connection switching relies on the session.

Also we write the config out to production.rb even if the app is generated with --api - possibly should skip that though you can write your own custom resolver.

byroot · 2021-05-15T12:26:07Z

Ok, so I got the main CI green. I'd appreciate some more feedback on the current approach, and if it's positive I can try to get this PR to mergeable quality.

pixeltrix · 2021-05-15T14:03:27Z

Ok, so I got the main CI green. I'd appreciate some more feedback on the current approach, and if it's positive I can try to get this PR to mergeable quality.

General approach seems fine but not keen on using exceptions as flow control - is this a temporary thing or is there issues in doing something else?

byroot · 2021-05-15T14:06:20Z

using exceptions as flow control

I'm not sure what you are referring to. If you are talking about the two rescue DisabledSessionError they are just to "specialize" the error and provide a more helpful one (or to just issue a warning for legacy apps).

byroot · 2021-05-15T14:14:30Z

they are just to "specialize" the error and provide a more helpful one

I realized I was planing on doing that but didn't yet -_-.

But ok, I think I now see what you mean. You'd expect a more explicit session.enabled? precondition check?

pixeltrix · 2021-05-15T16:00:04Z

But ok, I think I now see what you mean. You'd expect a more explicit session.enabled? precondition check?

Yes, exceptions are fine where we're terminating the render but where we're continuing to process the page it's generally slower (unless things have changed in recent ruby versions?).

byroot · 2021-05-15T16:11:17Z

Yes, exceptions are fine where we're terminating the render

Right, in that specific case we do terminate (unless you have the legacy option enabled). I chose this approach as IMHO it makes the code more straightforward, but I don't mind changing it for a call.

it's generally slower (unless things have changed in recent ruby versions?).

No, no changes, exception cost are still directly related to how deep the stack is. In this case it didn't concern me because we're in an error path, so it's not expected that you hit this path at all in production.

casperisfine · 2021-05-17T09:50:34Z

actionview/lib/action_view/helpers/csrf_helper.rb

-            tag("meta", name: "csrf-token", content: form_authenticity_token)
-          ].join("\n").html_safe
+          if session.respond_to?(:enabled?) && !session.enabled?
+            if Rails.application.config.action_dispatch.silence_disabled_session_errors


I don't like this Rails.application.config reach, but I'm unsure what a better alternative would be.

Maybe the config would be better defined on action_controller so we can add it to CONTROLLER_DELEGATES like how we handle request_forgery_protection_token? We could also add a session_enabled? or session_disabled? helper method similar to protect_from_forgery?.

Oh, actually that makes me think we could do this check in protect_against_forgery?. no?

Yes, that probably makes sense.

casperisfine · 2021-05-17T09:52:15Z

Ok, I changed the rescue for precondition checks.

byroot · 2021-05-17T12:53:14Z

I think it might be good to go now.

pixeltrix · 2021-05-17T14:08:50Z

actionpack/lib/action_controller/base.rb

+      attr_accessor :silence_disabled_session_errors
+    end
+    @silence_disabled_session_errors = true
+


Why here and not a config_accessor in RequestForgeryProtection?

Because that's where configs are stored by default. It also avoid eager loading RequestForgeryProtection if it's not used (at least I think, I admit I haven't double checked)

Sorry, I wasn't being clear - we have an included block in RequestForgeryProtection here:

https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L66-L67

Since we're accessing the config in that module I think it's safe to assume that it's loaded at that point is it not?

Do we also need to have a block in railties configuration for setting the default in new apps vs. upgrading apps?

Since we're accessing the config in that module I think it's safe to assume that it's loaded at that point is it not?

Hum, I don't think so. Because if you have load_defaults 7.0, then we set config.action_controller.silence..., which will cause the action controller Railtie to try to set ActionController::Base.silence....

Do we also need to have a block in railties configuration for setting the default in new apps vs. upgrading apps?

Right here: https://github.com/rails/rails/pull/42231/files#diff-9f26e965e7e2a6842bd6ea755924a621038bc9dd8d63cfa6b34092aa32ede02cR204-R206 no?

Hum, I don't think so. Because if you have load_defaults 7.0, then we set config.action_controller.silence..., which will cause the action controller Railtie to try to set ActionController::Base.silence....

My concern is that it's not a pattern that's used elsewhere in Action Pack and it's not in the module where it's used. There's a similar scenario with default_protect_from_forgery is there not? That's set in the load_defaults 5.2 block so something prevents that from blowing up. The default_protect_from_forgery config is applied using the :action_controller_base load hook so could we not use the same approach?

Right here: https://github.com/rails/rails/pull/42231/files#diff-9f26e965e7e2a6842bd6ea755924a621038bc9dd8d63cfa6b34092aa32ede02cR204-R206 no?

Sorry, obviously my 👀 are deteriorating with my advancing years 👴🏻

could we not use the same approach?

You are totally right, I missed this. I updated the PR.

obviously my 👀 are deteriorating with my advancing years 👴🏻

🤣

actionpack/lib/action_controller/metal/request_forgery_protection.rb

pixeltrix · 2021-05-17T16:20:49Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -90,6 +101,11 @@ module RequestForgeryProtection
      config_accessor :default_protect_from_forgery
      self.default_protect_from_forgery = false

+      # Controls wether trying to use forgery protection without a working session store


Typo here - should be whether

🤦 .

Fixed.

pixeltrix · 2021-05-17T16:28:46Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

+        if !session.respond_to?(:enabled?) || session.enabled?
+          true
+        else
+          if Base.silence_disabled_session_errors


Just one more - this doesn't need to use Base as there's an instance accessor.

Until now `config.session_store :disabled` simply silently discard the session hash at the end of the request. By explictly failing on writes, it can help discovering bugs earlier. Reads are still permitted.

casperisfine · 2021-05-17T18:03:27Z

actionpack/CHANGELOG.md

+*   Writing into a disabled session will now raise an error.
+
+    Previously when no session store was set, writing into the session would silently fail.
+
+    *Jean Boussier*


I added a changelog entry, since it's quite a big change.

casperisfine · 2021-05-17T18:04:16Z

Thanks for the reviews @pixeltrix!

I'll likely merge tomorrow unless someone else chime in.

Rails 7.0 deprecated writing to a session when the session is disabled and defaults a session to a disabled session. rails/rails#42231 In the past, ActionDispatch::TestRequest.create would set the TestSession explicitly but this is not the case anymore and the default session is used. With the PR mentioned above, this raises an exception when testsing view components that implement sessions.

Tonkpils · 2021-05-19T17:41:17Z

actionpack/lib/action_dispatch/http/request.rb

@@ -447,6 +441,10 @@ def check_method(name)
        HTTP_METHOD_LOOKUP[name] || raise(ActionController::UnknownHttpMethod, "#{name}, accepted HTTP methods are #{HTTP_METHODS[0...-1].join(', ')}, and #{HTTP_METHODS[-1]}")
        name
      end
+
+      def default_session
+        Session.disabled(self)


Setting this default_session as disabled without checking the configuration causes any requests built without a session to have this by default and can be surprising.

If my application has a config.session_store set as :cookie_store and I create an ActionDispatch::Request object its default session will be disabled. Would it be worth adding this information to the changelog as it is likely to break applications using ActionDispatch::Request manually

/cc @byroot

We happened to run into this in our test suite from both ActionDispatch::TestRequest and when using ActionController::Renderer from a job. I do like the new behaviour, it could catch bugs where one tries to set a session which has no effect, but I think we should call out this change more explicitly as others are likely to run into it.

any requests built without a session to have this by default and can be surprising.

But that's the idea no. Previously if you were to write into that request's session it would be silently not be persisted, right?

Also I'm curious to hear more about the use case of using ActionDispatch::Request manually.

Would it be worth adding this information to the changelog

I'm totally open to improve the changelog, if you have specific suggestions of things to call out, please let me know.

We happened to run into this in our test suite from both ActionDispatch::TestRequest

Hum, TestRequest should probably assume a working session no? That might be an oversight.

It appears setting the `rack.session` to a simple hash doesn't work anymore as it now has a few additional methods Rails is relying on to determine whether it's enabled or not: rails/rails#42231 Failure: NoMethodError: undefined method `enabled?' for {}:Hash rails (f55cdafe4b82) actionpack/lib/action_dispatch/middleware/flash.rb:62:in `commit_flash' Turns we we don't seem to need to set `rack.session` for the tests here.

rails-bot bot added actionpack actionview railties labels May 15, 2021

byroot commented May 15, 2021

View reviewed changes

byroot mentioned this pull request May 15, 2021

Use a regular flash object if session store is disabled #42230

Merged

casperisfine force-pushed the disabled-session branch 3 times, most recently from 1045c6e to bef3e60 Compare May 15, 2021 12:07

rails-bot bot added the activerecord label May 15, 2021

byroot requested a review from pixeltrix May 15, 2021 12:24

casperisfine force-pushed the disabled-session branch 2 times, most recently from 6258313 to 7802575 Compare May 17, 2021 09:31

casperisfine reviewed May 17, 2021

View reviewed changes

casperisfine force-pushed the disabled-session branch 2 times, most recently from 5bcf936 to 1885f30 Compare May 17, 2021 12:17

byroot marked this pull request as ready for review May 17, 2021 12:53

pixeltrix reviewed May 17, 2021

View reviewed changes

actionpack/lib/action_controller/metal/request_forgery_protection.rb Show resolved Hide resolved

casperisfine force-pushed the disabled-session branch from 1885f30 to e9a9533 Compare May 17, 2021 16:11

pixeltrix reviewed May 17, 2021

View reviewed changes

casperisfine force-pushed the disabled-session branch from e9a9533 to 343d602 Compare May 17, 2021 16:22

pixeltrix reviewed May 17, 2021

View reviewed changes

casperisfine force-pushed the disabled-session branch from 343d602 to d12f836 Compare May 17, 2021 16:30

pixeltrix approved these changes May 17, 2021

View reviewed changes

Explicitly fail on attempts to write into disabled sessions

c1c96a0

Until now `config.session_store :disabled` simply silently discard the session hash at the end of the request. By explictly failing on writes, it can help discovering bugs earlier. Reads are still permitted.

casperisfine force-pushed the disabled-session branch from d12f836 to c1c96a0 Compare May 17, 2021 18:03

casperisfine reviewed May 17, 2021

View reviewed changes

casperisfine mentioned this pull request May 17, 2021

Session options are nil. Unable to add sessions and automatic connection switching to an api_only app. #41932

Closed

byroot merged commit 9dc032a into rails:main May 18, 2021

byroot deleted the disabled-session branch May 18, 2021 06:59

Tonkpils mentioned this pull request May 18, 2021

Ensure session is set on request ViewComponent/view_component#925

Merged

Tonkpils reviewed May 19, 2021

View reviewed changes

DavidColby mentioned this pull request Oct 2, 2021

Turbo stream partial with button_to tag throws ActionView::Template::Error hotwired/turbo-rails#243

Closed

tvdeyen mentioned this pull request Sep 5, 2022

Add a session store to dummy app AlchemyCMS/alchemy-json_api#60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly fail on attempts to write into disabled sessions #42231

Explicitly fail on attempts to write into disabled sessions #42231

byroot commented May 15, 2021

byroot May 15, 2021

pixeltrix May 15, 2021

byroot May 15, 2021

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

casperisfine May 17, 2021

pixeltrix May 17, 2021

casperisfine May 17, 2021

pixeltrix May 17, 2021

casperisfine commented May 17, 2021

byroot commented May 17, 2021

pixeltrix May 17, 2021

byroot May 17, 2021

pixeltrix May 17, 2021

byroot May 17, 2021

pixeltrix May 17, 2021

casperisfine May 17, 2021

pixeltrix May 17, 2021

casperisfine May 17, 2021

pixeltrix May 17, 2021

casperisfine May 17, 2021

casperisfine May 17, 2021

casperisfine commented May 17, 2021

Tonkpils May 19, 2021 •

edited

jhawthorn May 19, 2021

byroot May 19, 2021

Explicitly fail on attempts to write into disabled sessions #42231

Explicitly fail on attempts to write into disabled sessions #42231

Conversation

byroot commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

byroot commented May 15, 2021

pixeltrix commented May 15, 2021

byroot commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperisfine commented May 17, 2021

byroot commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperisfine commented May 17, 2021

Tonkpils May 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tonkpils May 19, 2021 •

edited