Skip to content
Permalink
Browse files

FIX: Protect Redis config from being manipulated

Redis gem is allowing to pass custom connector as an option. Later, code is removing that option and initialize custom connector:
```
options.delete(:connector).new(@options) # https://github.com/redis/redis-rb/blob/master/lib/redis/client.rb#L95
```

It works for MessageBus when Redis server is running. When it fails and MessageBus is trying to reconnect, because it is a passing reference to Redis, customer connector is not available anymore.

Therefore, I think it would be better to pass a copy of the original object.
  • Loading branch information
lis2 authored and SamSaffron committed Oct 18, 2019
1 parent 138d74e commit 983134d898c1e1a2363cbc4d517e389f12c85973
Showing with 9 additions and 1 deletion.
  1. +1 −1 lib/message_bus/backends/redis.rb
  2. +8 −0 spec/lib/message_bus/backend_spec.rb
@@ -327,7 +327,7 @@ def global_subscribe(last_id = nil, &blk)
private

def new_redis_connection
::Redis.new(@redis_config)
::Redis.new(@redis_config.dup)
end

# redis connection used for publishing messages
@@ -406,4 +406,12 @@ def new_test_bus

got.map { |m| m.data }.must_equal ["12"]
end

it 'should not lose redis config' do
test_only :redis
redis_config = { connector: Redis::Client::Connector }
@bus.instance_variable_set(:@redis_config, redis_config)
@bus.send(:new_redis_connection)
expect(@bus.instance_variable_get(:@redis_config)[:connector]).must_equal Redis::Client::Connector
end
end

4 comments on commit 983134d

@discoursereviewbot

This comment has been minimized.

Copy link

discoursereviewbot replied Oct 18, 2019

Daniel Waterworth posted:

Redis can be quite confusing with all its layers of clients, but it does already dup the config:

https://github.com/redis/redis-rb/blob/c1d05615f4de09dc2eff127a083c4c678d36fd83/lib/redis.rb#L46

@discoursereviewbot

This comment has been minimized.

Copy link

discoursereviewbot replied Oct 18, 2019

lis2 posted:

Hey Daniel,
thank you for review, I know it is confusing, I spent half a day searching for that bug :slight_smile:

In my opinion, we still need that fix.

So here @options = options.dup after that step we got 2 objects. One is @options (dup of options) and second is original options.
Then we are passing original options to client @original_client = @client = client.new(options)

Then later in a client again they are making a copy with parse_options but original options are still there

 def initialize(options = {})
      @options = _parse_options(options)

And they are trying to remove attribute from orignal options not @options

elsif options.include?(:connector) && options[:connector].respond_to?(:new)
          options.delete(:connector).new(@options)

Does it make sense?

@discoursereviewbot

This comment has been minimized.

Copy link

discoursereviewbot replied Oct 18, 2019

Daniel Waterworth posted:

You're right. Good catch.

@discoursereviewbot

This comment has been minimized.

Copy link

discoursereviewbot replied Feb 14, 2020

system posted:

This commit appears in #214 which was merged by @SamSaffron.

Please sign in to comment.
You can’t perform that action at this time.