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.
This commit is contained in:
parent
61fb58f6a7
commit
c1c96a0140
@ -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
|
||||
|
@ -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
|
||||
|
@ -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
|
||||
|
||||
|
@ -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
|
||||
|
@ -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
|
||||
|
@ -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]
|
||||
|
||||
|
@ -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
|
||||
|
@ -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
|
||||
|
@ -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"
|
||||
|
Loading…
Reference in New Issue
Block a user