FIX: Protect Redis config from being manipulated

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.

diff --git a/lib/message_bus/backends/redis.rb b/lib/message_bus/backends/redis.rb
index a4751b0..df54493 100644
--- a/lib/message_bus/backends/redis.rb
+++ b/lib/message_bus/backends/redis.rb
@@ -327,7 +327,7 @@ LUA
       private
 
       def new_redis_connection
-        ::Redis.new(@redis_config)
+        ::Redis.new(@redis_config.dup)
       end
 
       # redis connection used for publishing messages
diff --git a/spec/lib/message_bus/backend_spec.rb b/spec/lib/message_bus/backend_spec.rb
index ec88e2b..989e5fb 100644
--- a/spec/lib/message_bus/backend_spec.rb
+++ b/spec/lib/message_bus/backend_spec.rb
@@ -406,4 +406,12 @@ describe PUB_SUB_CLASS do
 
     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

GitHub sha: 983134d8

1 Like

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

2 Likes

Hey Daniel, thank you for review, I know it is confusing, I spent half of 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?

4 Likes

You’re right. Good catch.

4 Likes

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