Merge pull request #23649 from maclover7/fix-22675

Accept channel identifiers with no backslashes/escaping

actioncable/CHANGELOG.md

+* Allow channel identifiers with no backslahes/escaping to be accepted
+  by the subscription storer.
+
+   *Jon Moss*
+
 *   Safely support autoloading and class unloading, by preventing concurrent
     loads, and disconnecting all cables during reload.
 

actioncable/lib/action_cable/connection/subscriptions.rb

@@ -23,13 +23,13 @@ def execute_command(data)
       end
 
       def add(data)
-        id_key = data['identifier']
-        id_options = ActiveSupport::JSON.decode(id_key).with_indifferent_access
+        id_options = decode_hash(data['identifier'])
+        identifier = normalize_identifier(id_options)
 
         subscription_klass = connection.server.channel_classes[id_options[:channel]]
 
         if subscription_klass
-          subscriptions[id_key] ||= subscription_klass.new(connection, id_key, id_options)
+          subscriptions[identifier] ||= subscription_klass.new(connection, identifier, id_options)
         else
           logger.error "Subscription class not found (#{data.inspect})"
         end
@@ -37,7 +37,7 @@ def add(data)
 
       def remove(data)
         logger.info "Unsubscribing from channel: #{data['identifier']}"
-        remove_subscription subscriptions[data['identifier']]
+        remove_subscription subscriptions[normalize_identifier(data['identifier'])]
       end
 
       def remove_subscription(subscription)
@@ -46,7 +46,7 @@ def remove_subscription(subscription)
       end
 
       def perform_action(data)
-        find(data).perform_action ActiveSupport::JSON.decode(data['data'])
+        find(data).perform_action(decode_hash(data['data']))
       end
 
       def identifiers
@@ -63,8 +63,21 @@ def unsubscribe_from_all
       private
         delegate :logger, to: :connection
 
+        def normalize_identifier(identifier)
+          identifier = ActiveSupport::JSON.encode(identifier) if identifier.is_a?(Hash)
+          identifier
+        end
+
+        # If `data` is a Hash, this means that the original JSON
+        # sent by the client had no backslashes in it, and does
+        # not need to be decoded again.
+        def decode_hash(data)
+          data = ActiveSupport::JSON.decode(data) unless data.is_a?(Hash)
+          data.with_indifferent_access
+        end
+
         def find(data)
-          if subscription = subscriptions[data['identifier']]
+          if subscription = subscriptions[normalize_identifier(data['identifier'])]
             subscription
           else
             raise "Unable to find subscription with identifier: #{data['identifier']}"

actioncable/test/connection/subscriptions_test.rb

@@ -82,13 +82,13 @@ def speak(data)
     end
   end
 
-  test "unsubscrib from all" do
+  test "unsubscribe from all" do
     run_in_eventmachine do
       setup_connection
 
       channel1 = subscribe_to_chat_channel
 
-      channel2_id = ActiveSupport::JSON.encode(id: 2, channel: 'ActionCable::Connection::SubscriptionsTest::ChatChannel')
+      channel2_id = ActiveSupport::JSON.encode({ id: 2, channel: 'ActionCable::Connection::SubscriptionsTest::ChatChannel' })
       channel2 = subscribe_to_chat_channel(channel2_id)
 
       channel1.expects(:unsubscribe_from_channel)

actioncable/test/test_helper.rb

@@ -6,6 +6,7 @@
 require 'mocha/setup'
 
 require 'rack/mock'
+require 'active_support/core_ext/hash/indifferent_access'
 
 # Require all the stubs and models
 Dir[File.dirname(__FILE__) + '/stubs/*.rb'].each {|file| require file }