Skip to content

Commit

Permalink
Backport 7785ac1 (#5157)
Browse files Browse the repository at this point in the history
This was implemented because of the discussion in #5151
Original commit message:

Validate days parameter to avoid possible DoS in Web UI

Thank you to Sergey Shpakov of http://tutum.space for reporting.

Signed-off-by: Sven Marquardt <[email protected]>
  • Loading branch information
MaSven authored Jan 30, 2022
1 parent f814f4b commit 8ab6d61
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 2 deletions.
2 changes: 2 additions & 0 deletions lib/sidekiq/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ def lengths

class History
def initialize(days_previous, start_date = nil)
#we only store five years of data in Redis
raise ArgumentError if days_previous < 1 || days_previous > (5 * 365)
@days_previous = days_previous
@start_date = start_date || Time.now.utc.to_date
end
Expand Down
2 changes: 1 addition & 1 deletion lib/sidekiq/web/action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def request
end

def halt(res)
throw :halt, res
throw :halt, [res, {"Content-Type" => "text/plain"}, [res.to_s]]
end

def redirect(location)
Expand Down
5 changes: 4 additions & 1 deletion lib/sidekiq/web/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ def self.set(key, val)

get "/" do
@redis_info = redis_info.select{ |k, v| REDIS_KEYS.include? k }
stats_history = Sidekiq::Stats::History.new((params['days'] || 30).to_i)
days = (params["days"] || 30).to_i
return halt(401) if days < 1 || days > 180

stats_history = Sidekiq::Stats::History.new(days)
@processed_history = stats_history.processed
@failed_history = stats_history.failed

Expand Down
9 changes: 9 additions & 0 deletions test/test_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@
Time::DATE_FORMATS[:default] = @before
end

describe "histoy" do
it "does not allow invalid input" do
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(-1) }
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(0) }
assert_raises(ArgumentError) { Sidekiq::Stats::History.new(2000) }
assert Sidekiq::Stats::History.new(200)
end
end

describe "processed" do
it 'retrieves hash of dates' do
Sidekiq.redis do |c|
Expand Down
2 changes: 2 additions & 0 deletions test/test_web.rb
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ def app
get '/'

assert_equal 200, last_response.status
get '/?days=100000000'
assert_equal 401, last_response.status
end
end

Expand Down

1 comment on commit 8ab6d61

@taylorkearns
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Is this ready for pull-request?

Please sign in to comment.