From 968bc810433c9e9f571a3d9e025969da98fd82a4 Mon Sep 17 00:00:00 2001 From: Mike Perham Date: Fri, 12 Feb 2021 14:50:51 -0800 Subject: [PATCH] Improve Web UI session experience (#4804) * 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 95% of users. The other 5% need to provide a Rack session. This is a big change and has the potential to break installs so it deserves at least a minor version bump. See also #4671, #4728 and many others. --- Changes.md | 30 +++++- Gemfile.lock | 2 +- lib/sidekiq/version.rb | 2 +- lib/sidekiq/web.rb | 92 +++++-------------- lib/sidekiq/web/application.rb | 8 +- lib/sidekiq/web/csrf_protection.rb | 23 ++++- lib/sidekiq/web/router.rb | 5 +- .../db/migrate/20120123214055_create_posts.rb | 2 +- myapp/simple.ru | 9 +- test/test_web.rb | 75 ++------------- 10 files changed, 101 insertions(+), 147 deletions(-) diff --git a/Changes.md b/Changes.md index 8ad2923d1..e46be3a5d 100644 --- a/Changes.md +++ b/Changes.md @@ -2,7 +2,35 @@ [Sidekiq Changes](https://github.com/mperham/sidekiq/blob/master/Changes.md) | [Sidekiq Pro Changes](https://github.com/mperham/sidekiq/blob/master/Pro-Changes.md) | [Sidekiq Enterprise Changes](https://github.com/mperham/sidekiq/blob/master/Ent-Changes.md) -6.1.3 +HEAD +--------- + +- Refactor Web UI session usage. [#4804] + Numerous people have hit "Forbidden" errors and struggled with Sidekiq's + Web UI session requirement. If you have code in your initializer for + Web sessions, it's quite possible it will need to be removed. Here's + an overview: +``` +Sidekiq::Web needs a valid Rack session for CSRF protection. If this is a Rails app, +make sure you mount Sidekiq::Web *inside* your routes in `config/routes.rb` so +Sidekiq can reuse the Rails session: + + Rails.application.routes.draw do + mount Sidekiq::Web => "/sidekiq" + .... + end + +If this is a bare Rack app, use a session middleware before Sidekiq::Web: + + # first, use IRB to create a shared secret key for sessions and commit it + require 'securerandom'; File.open(".session.key", "w") {|f| f.write(SecureRandom.hex(32)) } + + # now, update your Rack app to include the secret with a session cookie middleware + use Rack::Session::Cookie, secret: File.read(".session.key"), same_site: true, max_age: 86400 + run Sidekiq::Web +``` + + 6.1.3 --------- - Warn if Redis is configured to evict data under memory pressure [#4752] diff --git a/Gemfile.lock b/Gemfile.lock index 4fc5b655d..b868df523 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: . specs: - sidekiq (6.1.3) + sidekiq (6.2.0) connection_pool (>= 2.2.2) rack (~> 2.0) redis (>= 4.2.0) diff --git a/lib/sidekiq/version.rb b/lib/sidekiq/version.rb index 78505e7e1..e0f4bc42b 100644 --- a/lib/sidekiq/version.rb +++ b/lib/sidekiq/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Sidekiq - VERSION = "6.1.3" + VERSION = "6.2.0" end diff --git a/lib/sidekiq/web.rb b/lib/sidekiq/web.rb index 864aca78b..fff3fa8ef 100644 --- a/lib/sidekiq/web.rb +++ b/lib/sidekiq/web.rb @@ -13,10 +13,8 @@ require "sidekiq/web/csrf_protection" require "rack/content_length" - require "rack/builder" require "rack/file" -require "rack/session/cookie" module Sidekiq class Web @@ -40,14 +38,6 @@ def settings self end - def middlewares - @middlewares ||= [] - end - - def use(*middleware_args, &block) - middlewares << [middleware_args, block] - end - def default_tabs DEFAULT_TABS end @@ -73,32 +63,37 @@ def disable(*opts) opts.each { |key| set(key, false) } end - # Helper for the Sinatra syntax: Sidekiq::Web.set(:session_secret, Rails.application.secrets...) def set(attribute, value) send(:"#{attribute}=", value) end - attr_accessor :app_url, :session_secret, :redis_pool, :sessions + def sessions=(val) + puts "WARNING: Sidekiq::Web.sessions= is no longer relevant and will be removed in Sidekiq 7.0. #{caller(1..1).first}" + end + + def session_secret=(val) + puts "WARNING: Sidekiq::Web.session_secret= is no longer relevant and will be removed in Sidekiq 7.0. #{caller(1..1).first}" + 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 self.class.settings end - def use(*middleware_args, &block) - middlewares << [middleware_args, block] + def middlewares + @middlewares ||= [] end - def middlewares - @middlewares ||= Web.middlewares.dup + def use(*args, &block) + middlewares << [args, block] end def call(env) @@ -126,67 +121,26 @@ 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) + puts "Sidekiq::Web#sessions= is no longer relevant and will be removed in Sidekiq 7.0. #{caller[2..2].first}" end def self.register(extension) extension.registered(WebApplication) end - private - - def using?(middleware) - middlewares.any? do |(m, _)| - m.is_a?(Array) && (m[0] == middleware || m[0].is_a?(middleware)) - end + def default_middlewares + @default ||= [ + [[Sidekiq::Web::CsrfProtection]], + [[Rack::ContentLength]] + ] end - def build_sessions - middlewares = self.middlewares - - s = sessions - - # turn on CSRF protection if sessions are enabled and this is not the test env - if s && !using?(CsrfProtection) && ENV["RACK_ENV"] != "test" - 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 + private def build - build_sessions - - middlewares = self.middlewares klass = self.class + m = middlewares + default_middlewares ::Rack::Builder.new do %w[stylesheets javascripts images].each do |asset_dir| @@ -195,7 +149,7 @@ def build end end - middlewares.each { |middleware, block| use(*middleware, &block) } + m.each { |middleware, block| use(*middleware, &block) } run WebApplication.new(klass) end diff --git a/lib/sidekiq/web/application.rb b/lib/sidekiq/web/application.rb index 251b4d265..37b610f6b 100644 --- a/lib/sidekiq/web/application.rb +++ b/lib/sidekiq/web/application.rb @@ -4,7 +4,6 @@ module Sidekiq class WebApplication extend WebRouter - CONTENT_LENGTH = "Content-Length" REDIS_KEYS = %w[redis_version uptime_in_days connected_clients used_memory_human used_memory_peak_human] CSP_HEADER = [ "default-src 'self' https: http:", @@ -42,6 +41,13 @@ def self.set(key, val) # nothing, backwards compatibility end + head "/" do + # HEAD / is the cheapest heartbeat possible, + # it hits Redis to ensure connectivity + Sidekiq.redis { |c| c.llen("queue:default") } + "" + end + get "/" do @redis_info = redis_info.select { |k, v| REDIS_KEYS.include? k } stats_history = Sidekiq::Stats::History.new((params["days"] || 30).to_i) diff --git a/lib/sidekiq/web/csrf_protection.rb b/lib/sidekiq/web/csrf_protection.rb index f847abc11..59a52ce71 100644 --- a/lib/sidekiq/web/csrf_protection.rb +++ b/lib/sidekiq/web/csrf_protection.rb @@ -66,7 +66,28 @@ 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 for CSRF protection. 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: + + + # first, use IRB to create a shared secret key for sessions and commit it + require 'securerandom'; File.open(".session.key", "w") {|f| f.write(SecureRandom.hex(32)) } + + + # now use the secret with a session cookie middleware + use Rack::Session::Cookie, secret: File.read(".session.key"), same_site: true, max_age: 86400 + run Sidekiq::Web + EOM end def accept?(env) diff --git a/lib/sidekiq/web/router.rb b/lib/sidekiq/web/router.rb index efd9ff1ab..34d586ead 100644 --- a/lib/sidekiq/web/router.rb +++ b/lib/sidekiq/web/router.rb @@ -15,6 +15,10 @@ module WebRouter REQUEST_METHOD = "REQUEST_METHOD" PATH_INFO = "PATH_INFO" + def head(path, &block) + route(HEAD, path, &block) + end + def get(path, &block) route(GET, path, &block) end @@ -39,7 +43,6 @@ def route(method, path, &block) @routes ||= {GET => [], POST => [], PUT => [], PATCH => [], DELETE => [], HEAD => []} @routes[method] << WebRoute.new(method, path, block) - @routes[HEAD] << WebRoute.new(method, path, block) if method == GET end def match(env) diff --git a/myapp/db/migrate/20120123214055_create_posts.rb b/myapp/db/migrate/20120123214055_create_posts.rb index e97a6e03b..ca4238157 100644 --- a/myapp/db/migrate/20120123214055_create_posts.rb +++ b/myapp/db/migrate/20120123214055_create_posts.rb @@ -1,4 +1,4 @@ -class CreatePosts < ActiveRecord::Migration +class CreatePosts < ActiveRecord::Migration[6.0] def change create_table :posts do |t| t.string :title diff --git a/myapp/simple.ru b/myapp/simple.ru index 9bae9a0c5..8e0074711 100644 --- a/myapp/simple.ru +++ b/myapp/simple.ru @@ -1,7 +1,7 @@ # Easiest way to run Sidekiq::Web. # Run with "bundle exec rackup simple.ru" -require 'sidekiq' +require 'sidekiq/web' # A Web process always runs as client, no need to configure server Sidekiq.configure_client do |config| @@ -10,5 +10,10 @@ 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, same_site: true, max_age: 86400 run Sidekiq::Web diff --git a/test/test_web.rb b/test/test_web.rb index 8d3c90b51..69f7c58a6 100644 --- a/test/test_web.rb +++ b/test/test_web.rb @@ -9,7 +9,7 @@ include Rack::Test::Methods def app - Sidekiq::Web + @app ||= Sidekiq::Web.new end def job_params(job, score) @@ -17,9 +17,8 @@ def job_params(job, score) end before do - ENV["RACK_ENV"] = "test" Sidekiq.redis {|c| c.flushdb } - Sidekiq::Web.middlewares.clear + app.default_middlewares.clear end class WebWorker @@ -30,11 +29,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 @@ -743,6 +737,7 @@ def add_worker def app app = Sidekiq::Web.new app.use(Rack::Auth::Basic) { |user, pass| user == "a" && pass == "b" } + app.use(Rack::Session::Cookie, secret: SecureRandom.hex(32)) app end @@ -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 @@ -857,7 +792,9 @@ def app end def app - Sidekiq::Web.new + app = Sidekiq::Web.new + app.use Rack::Session::Cookie, secret: 'v3rys3cr31', host: 'nicehost.org' + app end it "allows afters to run" do