From 73f29f67e12dcf8c193ae73f008fcd79aae4a24b Mon Sep 17 00:00:00 2001 From: Mike Perham Date: Sun, 31 Jan 2021 12:28:24 -0800 Subject: [PATCH] Simplify Web UI sessions Remove all of the hacks and support infrastructure around Rack sessions. Rails provides this by default so we don't need it for 90% of users. The other 10% should know and provide a Rack session. This is a big change and has the potential to break many installs. It will be part of the 7.0 major version bump and require a lengthy beta period to ensure we document as many edge cases and solutions as possible. See also #4671, #4728 and many others. --- 7.0-Upgrade.md | 49 ++++++++++++++++++++++ lib/sidekiq/web.rb | 60 ++++++++++----------------- lib/sidekiq/web/csrf_protection.rb | 16 +++++++- myapp/simple.ru | 7 ++++ test/test_web.rb | 65 ------------------------------ 5 files changed, 93 insertions(+), 104 deletions(-) create mode 100644 7.0-Upgrade.md diff --git a/7.0-Upgrade.md b/7.0-Upgrade.md new file mode 100644 index 000000000..75845f08c --- /dev/null +++ b/7.0-Upgrade.md @@ -0,0 +1,49 @@ +# Welcome to Sidekiq 7.0 + + +## Web UI Sessions + +One focus of this major version upgrade is a refactoring of the Web UI +in order to simplify and integrate better with Rails and Rack sessions. +The most frustrating support and user pain point has been debugging problems with +the Web UI's use of Rack sessions. + +Previously Sidekiq::Web would provide a basic cookie session if not +configured but this often collided with the Rails session. Starting in +7.0, Sidekiq will not provide a session. The application developer must +provide a session somehow. Here's the main ways: + +### Rails + +Mount Sidekiq::Web within the application's routes in `config/routes.rb`. +Rails already provides a session to anything mounted within it. + +```ruby +Rails.application.routes.draw do + mount Sidekiq::Web => "/sidekiq" + .... +end +``` + +### Rack + +If you are not running Rails but mounting Sidekiq::Web as a basic Rack +app, first create a shared secret key in IRB: + +```ruby +require 'securerandom' +secret_key = SecureRandom.hex(32) +File.open(".secret.key", "w") {|f| f.write(secret_key) } +``` + +And then configure your session middleware to use that secret: + +```ruby +use Rack::Session::Cookie, secret: File.read(".secret.key") +run Sidekiq::Web +``` + +This is similar to how Rails puts its secret_key in `config/initializers/secret_token.rb` +so all Rails processes can share the same key. The session cookies will +be encrypted with that secret so no one can read them but your app code. +See the [Rack::Session::Cookie RDoc](https://www.rubydoc.info/gems/rack/Rack/Session/Cookie) for more options. diff --git a/lib/sidekiq/web.rb b/lib/sidekiq/web.rb index 864aca78b..4644ea902 100644 --- a/lib/sidekiq/web.rb +++ b/lib/sidekiq/web.rb @@ -78,15 +78,27 @@ def set(attribute, value) send(:"#{attribute}=", value) end - attr_accessor :app_url, :session_secret, :redis_pool, :sessions + def sessions=(val) + raise <<~EOM + Warning: disabling sessions opens your Sidekiq UI up to CSRF attacks. You may + disable them by setting `SIDEKIQ_DISABLE_SESSIONS_THIS_IS_A_BAD_IDEA=true`. + EOM + end + + def session_secret=(val) + raise <<~EOM + Sidekiq no longer allows setting the session secret directly. Either configure + your session middleware directly or allow Sidekiq to reuse the Rails session. + EOM + end + + attr_accessor :app_url, :redis_pool attr_writer :locales, :views end def self.inherited(child) child.app_url = app_url - child.session_secret = session_secret child.redis_pool = redis_pool - child.sessions = sessions end def settings @@ -126,18 +138,11 @@ def set(attribute, value) send(:"#{attribute}=", value) end - # Default values - set :sessions, true - - attr_writer :sessions - - def sessions - unless instance_variable_defined?("@sessions") - @sessions = self.class.sessions - @sessions = @sessions.to_hash.dup if @sessions.respond_to?(:to_hash) - end - - @sessions + def sessions=(val) + raise <<~EOM + Sidekiq no longer allows configuring the session via Sidekiq::Web. Either configure + your session middleware directly or allow Sidekiq to reuse the Rails session. + EOM end def self.register(extension) @@ -153,33 +158,12 @@ def using?(middleware) end def build_sessions - middlewares = self.middlewares - - s = sessions + disable_sessions = ENV["SIDEKIQ_DISABLE_SESSIONS_THIS_IS_A_BAD_IDEA"] == "true" || ENV["RACK_ENV"] == "test" # turn on CSRF protection if sessions are enabled and this is not the test env - if s && !using?(CsrfProtection) && ENV["RACK_ENV"] != "test" + unless disable_sessions || using?(CsrfProtection) middlewares.unshift [[CsrfProtection], nil] end - - if s && !using?(::Rack::Session::Cookie) - unless (secret = Web.session_secret) - require "securerandom" - secret = SecureRandom.hex(64) - end - - options = {secret: secret} - options = options.merge(s.to_hash) if s.respond_to? :to_hash - - middlewares.unshift [[::Rack::Session::Cookie, options], nil] - end - - # Since Sidekiq::WebApplication no longer calculates its own - # Content-Length response header, we must ensure that the Rack middleware - # that does this is loaded - unless using? ::Rack::ContentLength - middlewares.unshift [[::Rack::ContentLength], nil] - end end def build diff --git a/lib/sidekiq/web/csrf_protection.rb b/lib/sidekiq/web/csrf_protection.rb index f847abc11..ab8f32b17 100644 --- a/lib/sidekiq/web/csrf_protection.rb +++ b/lib/sidekiq/web/csrf_protection.rb @@ -66,7 +66,21 @@ def deny(env) end def session(env) - env["rack.session"] || fail("you need to set up a session middleware *before* #{self.class}") + env["rack.session"] || fail(<<~EOM) + Sidekiq::Web needs a valid Rack session to work. If this is a Rails app, + make sure you mount Sidekiq::Web *inside* your application routes: + + Rails.application.routes.draw do + mount Sidekiq::Web => "/sidekiq" + .... + end + + If this is a bare Rack app, use a session middleware before Sidekiq::Web: + + secret_key = SecureRandom.hex(32) + use Rack::Session::Cookie, secret: secret_key + run Sidekiq::Web + EOM end def accept?(env) diff --git a/myapp/simple.ru b/myapp/simple.ru index 9bae9a0c5..3a00f1c59 100644 --- a/myapp/simple.ru +++ b/myapp/simple.ru @@ -11,4 +11,11 @@ end Sidekiq::Client.push('class' => "HardWorker", 'args' => []) require 'sidekiq/web' + +# In a multi-process deployment, all Web UI instances should share +# this secret key so they can all decode the encrypted browser cookies +# and provide a working session. +# Rails does this in /config/initializers/secret_token.rb +secret_key = SecureRandom.hex(32) +use Rack::Session::Cookie, secret: secret_key run Sidekiq::Web diff --git a/test/test_web.rb b/test/test_web.rb index 8d3c90b51..e5280480d 100644 --- a/test/test_web.rb +++ b/test/test_web.rb @@ -30,11 +30,6 @@ def perform(a, b) end end - it 'can configure via set() syntax' do - app.set(:session_secret, "foo") - assert_equal "foo", app.session_secret - end - it 'can show text with any locales' do rackenv = {'HTTP_ACCEPT_LANGUAGE' => 'ru,en'} get '/', {}, rackenv @@ -780,66 +775,6 @@ def app assert_equal 'v3rys3cr31', session_options[:secret] assert_equal 'nicehost.org', session_options[:host] end - - describe 'sessions options' do - include Rack::Test::Methods - - describe 'using #disable' do - def app - app = Sidekiq::Web.new - app.disable(:sessions) - app - end - - it "doesn't create sessions" do - get '/' - assert_nil last_request.env['rack.session'] - end - end - - describe 'using #set with false argument' do - def app - app = Sidekiq::Web.new - app.set(:sessions, false) - app - end - - it "doesn't create sessions" do - get '/' - assert_nil last_request.env['rack.session'] - end - end - - describe 'using #set with an hash' do - def app - app = Sidekiq::Web.new - app.set(:sessions, { domain: :all }) - app - end - - it "creates sessions" do - get '/' - refute_nil last_request.env['rack.session'] - refute_empty last_request.env['rack.session'].options - assert_equal :all, last_request.env['rack.session'].options[:domain] - end - end - - describe 'using #enable' do - def app - app = Sidekiq::Web.new - app.enable(:sessions) - app - end - - it "creates sessions" do - get '/' - refute_nil last_request.env['rack.session'] - refute_empty last_request.env['rack.session'].options - refute_nil last_request.env['rack.session'].options[:secret] - end - end - end end describe "redirecting in before" do