From cd09ba6108f98973b6649a6149790c3d4502b4cc Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Mon, 12 Feb 2024 21:24:55 +0200 Subject: [PATCH] fix: backport xss and rce fixes to v7.1 (#834) * fix: backport xss and rce fixes to v7.1 This was fixed in #829 and #833 * chore(lint): lint'em real good * chore: remove reek --- .github/workflows/lint.yml | 15 --------- .rubocop.yml | 7 +++++ .../sidekiq_unique_jobs.rb | 2 +- lib/sidekiq_unique_jobs/web.rb | 31 ++++++++++--------- spec/performance/lock_digest_spec.rb | 2 +- spec/performance/locksmith_spec.rb | 2 +- spec/sidekiq_unique_jobs/changelog_spec.rb | 26 ++++++++-------- spec/sidekiq_unique_jobs/cli_spec.rb | 10 +++--- .../lua/delete_by_digest_spec.rb | 12 +++---- spec/sidekiq_unique_jobs/lua/lock_spec.rb | 2 +- spec/sidekiq_unique_jobs/lua/unlock_spec.rb | 12 +++---- spec/sidekiq_unique_jobs/redis/hash_spec.rb | 4 +-- spec/sidekiq_unique_jobs/redis/set_spec.rb | 4 +-- .../redis/sorted_set_spec.rb | 14 ++++----- spec/sidekiq_unique_jobs/redis/string_spec.rb | 8 ++--- spec/sidekiq_unique_jobs/web_spec.rb | 4 +-- .../shared_contexts/with_sidekiq_options.rb | 6 ++-- spec/support/sidekiq_meta.rb | 2 +- 18 files changed, 78 insertions(+), 85 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index ee28fe1f8..739d5576b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -18,18 +18,3 @@ jobs: bundler-cache: true - run: bin/bundle --jobs=$(nproc) --retry=$(nproc) - run: bin/rubocop -P - - reek: - runs-on: ubuntu-latest - strategy: - fail-fast: true - - steps: - - uses: actions/checkout@v3 - - uses: ruby/setup-ruby@v1 - with: - ruby-version: 3.1 - bundler: 2.4.12 - bundler-cache: true - - run: bin/bundle --jobs=$(nproc) --retry=$(nproc) - - run: bin/reek . diff --git a/.rubocop.yml b/.rubocop.yml index bc9209e0d..aa97834f3 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -135,6 +135,13 @@ RSpec/RepeatedExample: Exclude: - spec/sidekiq_unique_jobs/unique_args_spec.rb +RSpec/SpecFilePathFormat: + Exclude: + - spec/performance/lock_digest_spec.rb + - spec/performance/locksmith_spec.rb + - spec/sidekiq_unique_jobs/configuration_spec.rb + - spec/sidekiq_unique_jobs/middleware/server/until_and_while_executing_spec.rb + Style/Documentation: Enabled: true Exclude: diff --git a/lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb b/lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb index a931966d5..5798ee2b1 100644 --- a/lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb +++ b/lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb @@ -186,7 +186,7 @@ def configure(options = {}) yield config else options.each do |key, val| - config.send("#{key}=", val) + config.send(:"#{key}=", val) end end end diff --git a/lib/sidekiq_unique_jobs/web.rb b/lib/sidekiq_unique_jobs/web.rb index 06a2d21a3..be14cbb5c 100644 --- a/lib/sidekiq_unique_jobs/web.rb +++ b/lib/sidekiq_unique_jobs/web.rb @@ -14,11 +14,11 @@ def self.registered(app) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize end app.get "/changelogs" do - @filter = params[:filter] || "*" + @filter = h(params[:filter] || "*") @filter = "*" if @filter == "" - @count = (params[:count] || 100).to_i - @current_cursor = params[:cursor] - @prev_cursor = params[:prev_cursor] + @count = h(params[:count] || 100).to_i + @current_cursor = h(params[:cursor]) + @prev_cursor = h(params[:prev_cursor]) @total_size, @next_cursor, @changelogs = changelog.page( cursor: @current_cursor, pattern: @filter, @@ -34,11 +34,11 @@ def self.registered(app) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize end app.get "/locks" do - @filter = params[:filter] || "*" + @filter = h(params[:filter] || "*") @filter = "*" if @filter == "" - @count = (params[:count] || 100).to_i - @current_cursor = params[:cursor] - @prev_cursor = params[:prev_cursor] + @count = h(params[:count] || 100).to_i + @current_cursor = h(params[:cursor]) + @prev_cursor = h(params[:prev_cursor]) @total_size, @next_cursor, @locks = digests.page( cursor: @current_cursor, @@ -50,11 +50,11 @@ def self.registered(app) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize end app.get "/expiring_locks" do - @filter = params[:filter] || "*" + @filter = h(params[:filter] || "*") @filter = "*" if @filter == "" - @count = (params[:count] || 100).to_i - @current_cursor = params[:cursor] - @prev_cursor = params[:prev_cursor] + @count = h(params[:count] || 100).to_i + @current_cursor = h(params[:cursor]) + @prev_cursor = h(params[:prev_cursor]) @total_size, @next_cursor, @locks = expiring_digests.page( cursor: @current_cursor, @@ -72,7 +72,7 @@ def self.registered(app) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize end app.get "/locks/:digest" do - @digest = params[:digest] + @digest = h(params[:digest]) @lock = SidekiqUniqueJobs::Lock.new(@digest) erb(unique_template(:lock)) @@ -85,9 +85,10 @@ def self.registered(app) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize end app.get "/locks/:digest/jobs/:job_id/delete" do - @digest = params[:digest] + @digest = h(params[:digest]) + @job_id = h(params[:job_id]) @lock = SidekiqUniqueJobs::Lock.new(@digest) - @lock.unlock(params[:job_id]) + @lock.unlock(@job_id) redirect_to "locks/#{@lock.key}" end diff --git a/spec/performance/lock_digest_spec.rb b/spec/performance/lock_digest_spec.rb index 49f8bf8f5..a562619de 100644 --- a/spec/performance/lock_digest_spec.rb +++ b/spec/performance/lock_digest_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe SidekiqUniqueJobs::LockDigest, perf: true do +RSpec.describe SidekiqUniqueJobs::LockDigest, :perf do let(:lock_digest) { described_class.new(item) } let(:job_class) { UntilExecutedJob } let(:class_name) { worker_class.to_s } diff --git a/spec/performance/locksmith_spec.rb b/spec/performance/locksmith_spec.rb index 90b57737b..0a14e809f 100644 --- a/spec/performance/locksmith_spec.rb +++ b/spec/performance/locksmith_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe SidekiqUniqueJobs::Locksmith, perf: true do +RSpec.describe SidekiqUniqueJobs::Locksmith, :perf do let(:locksmith_one) { described_class.new(item_one) } let(:locksmith_two) { described_class.new(item_two) } diff --git a/spec/sidekiq_unique_jobs/changelog_spec.rb b/spec/sidekiq_unique_jobs/changelog_spec.rb index c6e57f811..577a17e8c 100644 --- a/spec/sidekiq_unique_jobs/changelog_spec.rb +++ b/spec/sidekiq_unique_jobs/changelog_spec.rb @@ -39,14 +39,14 @@ it "clears out all entries" do expect { clear }.to change { entity.entries.size }.by(-1) - expect(clear).to be == 1 + expect(clear).to eq 1 end end context "without entries" do it "returns 0 (zero)" do expect { clear }.not_to change { entity.entries.size } - expect(clear).to be == 0 + expect(clear).to eq 0 end end end @@ -55,13 +55,13 @@ subject(:exist?) { entity.exist? } context "when no entries exist" do - it { is_expected.to be == false } + it { is_expected.to be false } end context "when entries exist" do before { simulate_lock(key, job_id) } - it { is_expected.to be == true } + it { is_expected.to be true } end end @@ -69,13 +69,13 @@ subject(:pttl) { entity.pttl } context "when no entries exist" do - it { is_expected.to be == -2 } + it { is_expected.to eq(-2) } end context "when entries exist without expiration" do before { simulate_lock(key, job_id) } - it { is_expected.to be == -1 } + it { is_expected.to eq(-1) } end context "when entries exist with expiration" do @@ -92,13 +92,13 @@ subject(:ttl) { entity.ttl } context "when no entries exist" do - it { is_expected.to be == -2 } + it { is_expected.to eq(-2) } end context "when entries exist without expiration" do before { simulate_lock(key, job_id) } - it { is_expected.to be == -1 } + it { is_expected.to eq(-1) } end context "when entries exist with expiration" do @@ -107,7 +107,7 @@ expire(key.changelog, 600) end - it { is_expected.to be == 600 } + it { is_expected.to eq 600 } end end @@ -115,7 +115,7 @@ subject(:expires?) { entity.expires? } context "when no entries exist" do - it { is_expected.to be == false } + it { is_expected.to be false } end context "when entries exist" do @@ -124,7 +124,7 @@ expire(key.changelog, 600) end - it { is_expected.to be == true } + it { is_expected.to be true } end end @@ -132,13 +132,13 @@ subject(:count) { entity.count } context "when no entries exist" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "when entries exist" do before { simulate_lock(key, job_id) } - it { is_expected.to be == 2 } + it { is_expected.to eq 2 } end end diff --git a/spec/sidekiq_unique_jobs/cli_spec.rb b/spec/sidekiq_unique_jobs/cli_spec.rb index c1113031d..7c983b771 100644 --- a/spec/sidekiq_unique_jobs/cli_spec.rb +++ b/spec/sidekiq_unique_jobs/cli_spec.rb @@ -37,9 +37,9 @@ jobs del PATTERN Options: - d, [--dry-run], [--no-dry-run] # set to false to perform deletion - c, [--count=N] # The max number of digests to return - # Default: 1000 + -d, [--dry-run], [--no-dry-run] # set to false to perform deletion + -c, [--count=N] # The max number of digests to return + # Default: 1000 deletes unique digests from redis by pattern HEADER @@ -55,8 +55,8 @@ jobs list PATTERN Options: - c, [--count=N] # The max number of digests to return - # Default: 1000 + -c, [--count=N] # The max number of digests to return + # Default: 1000 list all unique digests and their expiry time HEADER diff --git a/spec/sidekiq_unique_jobs/lua/delete_by_digest_spec.rb b/spec/sidekiq_unique_jobs/lua/delete_by_digest_spec.rb index dfdd68775..fb004b7c9 100644 --- a/spec/sidekiq_unique_jobs/lua/delete_by_digest_spec.rb +++ b/spec/sidekiq_unique_jobs/lua/delete_by_digest_spec.rb @@ -40,12 +40,12 @@ it "deletes the expected keys from redis" do expect(delete_by_digest).to eq(8) - expect(queued.count).to be == 0 - expect(primed.count).to be == 0 - expect(locked.count).to be == 0 + expect(queued.count).to eq 0 + expect(primed.count).to eq 0 + expect(locked.count).to eq 0 - expect(run_queued.count).to be == 0 - expect(run_primed.count).to be == 0 - expect(run_locked.count).to be == 0 + expect(run_queued.count).to eq 0 + expect(run_primed.count).to eq 0 + expect(run_locked.count).to eq 0 end end diff --git a/spec/sidekiq_unique_jobs/lua/lock_spec.rb b/spec/sidekiq_unique_jobs/lua/lock_spec.rb index 98bb92de0..672bbd89b 100644 --- a/spec/sidekiq_unique_jobs/lua/lock_spec.rb +++ b/spec/sidekiq_unique_jobs/lua/lock_spec.rb @@ -20,7 +20,7 @@ let(:now_f) { SidekiqUniqueJobs.now_f } let(:lock_limit) { 1 } - shared_context "with a primed key", with_primed_key: true do + shared_context "with a primed key", :with_primed_key do before do call_script(:queue, key.to_a, argv_one) rpoplpush(key.queued, key.primed) diff --git a/spec/sidekiq_unique_jobs/lua/unlock_spec.rb b/spec/sidekiq_unique_jobs/lua/unlock_spec.rb index 3319d427d..528b35d10 100644 --- a/spec/sidekiq_unique_jobs/lua/unlock_spec.rb +++ b/spec/sidekiq_unique_jobs/lua/unlock_spec.rb @@ -18,7 +18,7 @@ let(:locked_jid) { job_id_one } let(:lock_limit) { 1 } - shared_context "with a lock", with_a_lock: true do + shared_context "with a lock", :with_a_lock do before do call_script(:queue, key.to_a, argv_one) rpoplpush(key.queued, key.primed) @@ -172,10 +172,10 @@ it "does not unlock" do expect { unlock }.to change { changelogs.count }.by(1) - expect(queued.count).to be == 0 - expect(primed.count).to be == 0 + expect(queued.count).to eq 0 + expect(primed.count).to eq 0 - expect(locked.count).to be == 1 + expect(locked.count).to eq 1 expect(locked.entries).to contain_exactly(job_id_two) expect(locked[job_id_two].to_f).to be_within(0.5).of(now_f) end @@ -189,9 +189,9 @@ expect(queued.count).to eq(1) expect(queued.entries).to contain_exactly("1") - expect(primed.count).to be == 0 + expect(primed.count).to eq 0 - expect(locked.count).to be == 0 + expect(locked.count).to eq 0 expect(locked.entries).to be_empty expect(locked[job_id_one]).to be_nil end diff --git a/spec/sidekiq_unique_jobs/redis/hash_spec.rb b/spec/sidekiq_unique_jobs/redis/hash_spec.rb index c8ac4fa17..bd3cc88cb 100644 --- a/spec/sidekiq_unique_jobs/redis/hash_spec.rb +++ b/spec/sidekiq_unique_jobs/redis/hash_spec.rb @@ -38,13 +38,13 @@ subject(:count) { entity.count } context "without entries" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "with entries" do before { hset(digest, job_id, now_f) } - it { is_expected.to be == 1 } + it { is_expected.to eq 1 } end end end diff --git a/spec/sidekiq_unique_jobs/redis/set_spec.rb b/spec/sidekiq_unique_jobs/redis/set_spec.rb index decdd270d..847c62014 100644 --- a/spec/sidekiq_unique_jobs/redis/set_spec.rb +++ b/spec/sidekiq_unique_jobs/redis/set_spec.rb @@ -24,13 +24,13 @@ subject(:count) { entity.count } context "without entries" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "with entries" do before { sadd(digest, job_id) } - it { is_expected.to be == 1 } + it { is_expected.to eq 1 } end end end diff --git a/spec/sidekiq_unique_jobs/redis/sorted_set_spec.rb b/spec/sidekiq_unique_jobs/redis/sorted_set_spec.rb index eb79ed6f4..b44f5ae45 100644 --- a/spec/sidekiq_unique_jobs/redis/sorted_set_spec.rb +++ b/spec/sidekiq_unique_jobs/redis/sorted_set_spec.rb @@ -36,13 +36,13 @@ context "when given an array of arrays" do let(:values) { [[1.0, "string"], [2.0, "other"]] } - it { is_expected.to be == 2 } + it { is_expected.to eq 2 } end context "when given a string entries" do let(:values) { "abcdef" } - it { is_expected.to be == true } + it { is_expected.to be true } end end @@ -50,13 +50,13 @@ subject(:count) { entity.count } context "without entries" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "with entries" do before { zadd(digest, now_f, job_id) } - it { is_expected.to be == 1 } + it { is_expected.to eq 1 } end end @@ -64,7 +64,7 @@ subject(:clear) { entity.clear } context "without entries" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "with entries" do @@ -77,7 +77,7 @@ zcard(digest) end - it { is_expected.to be == 100 } + it { is_expected.to eq 100 } end end @@ -105,7 +105,7 @@ context "with entries" do before { zadd(digest, now_f, job_id) } - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end end end diff --git a/spec/sidekiq_unique_jobs/redis/string_spec.rb b/spec/sidekiq_unique_jobs/redis/string_spec.rb index 2fac8e3de..1fdfccadf 100644 --- a/spec/sidekiq_unique_jobs/redis/string_spec.rb +++ b/spec/sidekiq_unique_jobs/redis/string_spec.rb @@ -10,13 +10,13 @@ subject(:count) { entity.count } context "without entries" do - it { is_expected.to be == 0 } + it { is_expected.to eq 0 } end context "with entries" do before { set(digest, job_id) } - it { is_expected.to be == 1 } + it { is_expected.to eq 1 } end end @@ -30,7 +30,7 @@ context "with entries" do before { set(digest, job_id) } - it { is_expected.to be == job_id } + it { is_expected.to eq job_id } end end @@ -44,7 +44,7 @@ context "with entries" do before { set(digest, job_id) } - it { is_expected.to be == 1 } + it { is_expected.to eq 1 } end end end diff --git a/spec/sidekiq_unique_jobs/web_spec.rb b/spec/sidekiq_unique_jobs/web_spec.rb index f0801806a..6d5cb4a97 100644 --- a/spec/sidekiq_unique_jobs/web_spec.rb +++ b/spec/sidekiq_unique_jobs/web_spec.rb @@ -15,8 +15,8 @@ def app domain: "foo.com", path: "/", expire_after: 2_592_000, - secret: "change_me", - old_secret: "also_change_me" + secret: "change_me" * 16, + old_secret: "also_change_me" * 16 run Sidekiq::Web end diff --git a/spec/support/shared_contexts/with_sidekiq_options.rb b/spec/support/shared_contexts/with_sidekiq_options.rb index 223b02712..b6e1e05c9 100644 --- a/spec/support/shared_contexts/with_sidekiq_options.rb +++ b/spec/support/shared_contexts/with_sidekiq_options.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.shared_context "with global config", with_global_config: true do +RSpec.shared_context "with global config", :with_global_config do let(:global_config) { {} } around do |example| @@ -8,7 +8,7 @@ end end -RSpec.shared_context "with job options", with_job_options: true do +RSpec.shared_context "with job options", :with_job_options do let(:job_options) { {} } around do |example| @@ -16,7 +16,7 @@ end end -RSpec.shared_context "with sidekiq options", with_sidekiq_options: true do |**_options| +RSpec.shared_context "with sidekiq options", :with_sidekiq_options do |**_options| let(:sidekiq_options) { {} } around do |example| diff --git a/spec/support/sidekiq_meta.rb b/spec/support/sidekiq_meta.rb index 881c20c9e..a28400272 100644 --- a/spec/support/sidekiq_meta.rb +++ b/spec/support/sidekiq_meta.rb @@ -27,7 +27,7 @@ def sidekiq_redis_driver if (sidekiq = example.metadata.fetch(:sidekiq, :disable)) sidekiq = :fake if sidekiq == true - Sidekiq::Testing.send("#{sidekiq}!") + Sidekiq::Testing.send(:"#{sidekiq}!") end if (sidekiq_ver = example.metadata[:sidekiq_ver])