From c1c96a014049b2660ce3a89b3c1b7aef072ae922 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sat, 15 May 2021 10:50:37 +0200 Subject: [PATCH] Explicitly fail on attempts to write into disabled sessions 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. --- actionpack/CHANGELOG.md | 6 +++ .../metal/request_forgery_protection.rb | 31 ++++++++++- .../lib/action_dispatch/http/request.rb | 12 ++--- .../lib/action_dispatch/request/session.rb | 52 ++++++++++++++----- .../test/dispatch/session/cache_store_test.rb | 1 + .../dispatch/session/cookie_store_test.rb | 1 + .../test/cases/database_selector_test.rb | 5 ++ .../lib/rails/application/configuration.rb | 4 ++ .../application/middleware/session_test.rb | 43 +++++++++++++++ 9 files changed, 135 insertions(+), 20 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index a44390e0c4..ba8f29bab0 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,9 @@ +* 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* + * Add support for 'require-trusted-types-for' and 'trusted-types' headers. Fixes #42034 diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index 68253d3d19..1e505a9576 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -57,6 +57,17 @@ class InvalidCrossOriginRequest < ActionControllerError #:nodoc: module RequestForgeryProtection extend ActiveSupport::Concern + class DisabledSessionError < StandardError + MESSAGE = <<~EOS.squish + Request forgery protection requires a working session store but your application has sessions disabled. + You need to either disable request forgery protection, or configure a working session store. + EOS + + def initialize(message = MESSAGE) + super + end + end + include AbstractController::Helpers include AbstractController::Callbacks @@ -90,6 +101,11 @@ module RequestForgeryProtection config_accessor :default_protect_from_forgery self.default_protect_from_forgery = false + # Controls whether trying to use forgery protection without a working session store + # issues a warning or raises an error. + config_accessor :silence_disabled_session_errors + self.silence_disabled_session_errors = true + # Controls whether URL-safe CSRF tokens are generated. config_accessor :urlsafe_csrf_tokens, instance_writer: false self.urlsafe_csrf_tokens = false @@ -438,7 +454,20 @@ def form_authenticity_param # :doc: # Checks if the controller allows forgery protection. def protect_against_forgery? # :doc: - allow_forgery_protection + allow_forgery_protection && ensure_session_is_enabled! + end + + def ensure_session_is_enabled! + if !session.respond_to?(:enabled?) || session.enabled? + true + else + if silence_disabled_session_errors + ActiveSupport::Deprecation.warn(DisabledSessionError::MESSAGE) + false + else + raise DisabledSessionError + end + end end NULL_ORIGIN_MESSAGE = <<~MSG diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index faa83c583b..082fc50cc7 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -359,14 +359,8 @@ def body_stream #:nodoc: get_header("rack.input") end - # TODO This should be broken apart into AD::Request::Session and probably - # be included by the session middleware. def reset_session - if session && session.respond_to?(:destroy) - session.destroy - else - self.session = {} - end + session.destroy end def session=(session) #:nodoc: @@ -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) + end end end diff --git a/actionpack/lib/action_dispatch/request/session.rb b/actionpack/lib/action_dispatch/request/session.rb index bf9d26fda1..ea00d77fab 100644 --- a/actionpack/lib/action_dispatch/request/session.rb +++ b/actionpack/lib/action_dispatch/request/session.rb @@ -6,6 +6,7 @@ module ActionDispatch class Request # Session is responsible for lazily loading the session from store. class Session # :nodoc: + DisabledSessionError = Class.new(StandardError) ENV_SESSION_KEY = Rack::RACK_SESSION # :nodoc: ENV_SESSION_OPTIONS_KEY = Rack::RACK_SESSION_OPTIONS # :nodoc: @@ -23,6 +24,10 @@ def self.create(store, req, default_options) session end + def self.disabled(req) + new(nil, req, enabled: false) + end + def self.find(req) req.get_header ENV_SESSION_KEY end @@ -31,6 +36,10 @@ def self.set(req, session) req.set_header ENV_SESSION_KEY, session end + def self.delete(req) + req.delete_header ENV_SESSION_KEY + end + class Options #:nodoc: def self.set(req, options) req.set_header ENV_SESSION_OPTIONS_KEY, options @@ -60,30 +69,38 @@ def to_hash; @delegate.dup; end def values_at(*args); @delegate.values_at(*args); end end - def initialize(by, req) + def initialize(by, req, enabled: true) @by = by @req = req @delegate = {} @loaded = false @exists = nil # We haven't checked yet. + @enabled = enabled end def id options.id(@req) end + def enabled? + @enabled + end + def options Options.find @req end def destroy clear - options = self.options || {} - @by.send(:delete_session, @req, options.id(@req), options) - # Load the new sid to be written with the response. - @loaded = false - load_for_write! + if enabled? + options = self.options || {} + @by.send(:delete_session, @req, options.id(@req), options) + + # Load the new sid to be written with the response. + @loaded = false + load_for_write! + end end # Returns value of the key stored in the session or @@ -135,7 +152,7 @@ def []=(key, value) # Clears the session. def clear - load_for_write! + load_for_delete! @delegate.clear end @@ -163,7 +180,7 @@ def update(hash) # Deletes given key from the session. def delete(key) - load_for_write! + load_for_delete! @delegate.delete key.to_s end @@ -199,6 +216,7 @@ def inspect end def exists? + return false unless enabled? return @exists unless @exists.nil? @exists = @by.send(:session_exists?, @req) end @@ -227,13 +245,23 @@ def load_for_read! end def load_for_write! - load! unless loaded? + if enabled? + load! unless loaded? + else + raise DisabledSessionError, "Your application has sessions disabled. To write to the session you must first configure a session store" + end + end + + def load_for_delete! + load! if enabled? && !loaded? end def load! - id, session = @by.load_session @req - options[:id] = id - @delegate.replace(session.stringify_keys) + if enabled? + id, session = @by.load_session @req + options[:id] = id + @delegate.replace(session.stringify_keys) + end @loaded = true end end diff --git a/actionpack/test/dispatch/session/cache_store_test.rb b/actionpack/test/dispatch/session/cache_store_test.rb index 7bee7dee79..205a1ae75d 100644 --- a/actionpack/test/dispatch/session/cache_store_test.rb +++ b/actionpack/test/dispatch/session/cache_store_test.rb @@ -93,6 +93,7 @@ def test_setting_session_value_after_session_reset get "/call_reset_session" assert_response :success assert_not_equal [], headers["Set-Cookie"] + assert_not_nil headers["Set-Cookie"] get "/get_session_value" assert_response :success diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index 2a90428471..c8fb9eabe4 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/actionpack/test/dispatch/session/cookie_store_test.rb @@ -221,6 +221,7 @@ def test_setting_session_value_after_session_reset get "/call_reset_session" assert_response :success assert_not_equal [], headers["Set-Cookie"] + assert_not_nil headers["Set-Cookie"] assert_not_nil session_payload assert_not_equal session_payload, cookies[SessionKey] diff --git a/activerecord/test/cases/database_selector_test.rb b/activerecord/test/cases/database_selector_test.rb index 0438adf7a3..13b8681cd6 100644 --- a/activerecord/test/cases/database_selector_test.rb +++ b/activerecord/test/cases/database_selector_test.rb @@ -282,6 +282,8 @@ def test_the_middleware_chooses_writing_role_with_POST_request assert ActiveRecord::Base.connected_to?(role: :writing) [200, {}, ["body"]] }) + cache = ActiveSupport::Cache::MemoryStore.new + middleware = ActionDispatch::Session::CacheStore.new(middleware, cache: cache, key: "_session_id") assert_equal [200, {}, ["body"]], middleware.call("REQUEST_METHOD" => "POST") end @@ -290,6 +292,9 @@ def test_the_middleware_chooses_reading_role_with_GET_request assert ActiveRecord::Base.connected_to?(role: :reading) [200, {}, ["body"]] }) + cache = ActiveSupport::Cache::MemoryStore.new + middleware = ActionDispatch::Session::CacheStore.new(middleware, cache: cache, key: "_session_id") + assert_equal [200, {}, ["body"]], middleware.call("REQUEST_METHOD" => "GET") end end diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 75cff916cc..3678325e02 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -201,6 +201,10 @@ def load_defaults(target_version) action_dispatch.return_only_request_media_type_on_content_type = false end + if respond_to?(:action_controller) + action_controller.silence_disabled_session_errors = false + end + if respond_to?(:action_view) action_view.button_to_generates_button_tag = true action_view.apply_stylesheet_media_default = false diff --git a/railties/test/application/middleware/session_test.rb b/railties/test/application/middleware/session_test.rb index 58d5deb278..bcfda062f4 100644 --- a/railties/test/application/middleware/session_test.rb +++ b/railties/test/application/middleware/session_test.rb @@ -337,6 +337,49 @@ def dump_flash assert_not_includes Rails.application.middleware, ActionDispatch::Flash end + test "disabled session allows reads and delete but fail on writes" do + add_to_config "config.session_store :disabled" + + controller :test, <<-RUBY + class TestController < ApplicationController + def write_session + request.session[:foo] = "bar" + render plain: "This shouldn't work" + end + + def read_session + render plain: request.session[:foo].inspect + end + + def reset_session + request.reset_session + render plain: "It worked!" + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + get "/write_session" => "test#write_session" + get "/read_session" => "test#read_session" + get "/reset_session" => "test#reset_session" + end + RUBY + + require "#{app_path}/config/environment" + + get "/write_session" + assert_equal 500, last_response.status + + get "/read_session" + assert_equal 200, last_response.status + assert_equal nil.inspect, last_response.body + + get "/reset_session" + assert_equal 200, last_response.status + assert_equal "It worked!", last_response.body + end + test "cookie_only is set to true even if user tries to overwrite it" do add_to_config "config.session_store :cookie_store, key: '_myapp_session', cookie_only: false" require "#{app_path}/config/environment"