From 04317173a02eb67c103364b5c8b5756ac61fac98 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 7 Oct 2015 11:24:20 -0500 Subject: First take at cross site forgery protection --- lib/action_cable.rb | 1 + lib/action_cable/connection/base.rb | 24 ++++++++++++- lib/action_cable/server/configuration.rb | 3 ++ test/connection/base_test.rb | 1 + test/connection/cross_site_forgery_test.rb | 55 ++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 test/connection/cross_site_forgery_test.rb diff --git a/lib/action_cable.rb b/lib/action_cable.rb index d2c5251634..49f14b51bc 100644 --- a/lib/action_cable.rb +++ b/lib/action_cable.rb @@ -8,6 +8,7 @@ require 'active_support' require 'active_support/json' require 'active_support/concern' require 'active_support/core_ext/hash/indifferent_access' +require 'active_support/core_ext/array/wrap' require 'active_support/core_ext/module/delegation' require 'active_support/callbacks' diff --git a/lib/action_cable/connection/base.rb b/lib/action_cable/connection/base.rb index 08a75156a3..a2ef99dff4 100644 --- a/lib/action_cable/connection/base.rb +++ b/lib/action_cable/connection/base.rb @@ -71,7 +71,7 @@ module ActionCable def process logger.info started_request_message - if websocket.possible? + if websocket.possible? && allow_request_origin? websocket.on(:open) { |event| send_async :on_open } websocket.on(:message) { |event| on_message event.data } websocket.on(:close) { |event| send_async :on_close } @@ -165,6 +165,28 @@ module ActionCable end + def allow_request_origin? + return true if server.config.disable_request_forgery_protection + + if env['HTTP_ORIGIN'].present? + origin_host = URI.parse(env['HTTP_ORIGIN']).host + + allowed = if server.config.allowed_request_origins.present? + Array.wrap(server.config.allowed_request_origins).include? origin_host + else + request.host == origin_host + end + + logger.error("Request origin not allowed: #{env['HTTP_ORIGIN']}") unless allowed + allowed + else + logger.error("Request origin missing.") + false + end + rescue URI::InvalidURIError + false + end + def respond_to_successful_request websocket.rack_response end diff --git a/lib/action_cable/server/configuration.rb b/lib/action_cable/server/configuration.rb index ac9fa7b085..315782ec3e 100644 --- a/lib/action_cable/server/configuration.rb +++ b/lib/action_cable/server/configuration.rb @@ -6,6 +6,7 @@ module ActionCable attr_accessor :logger, :log_tags attr_accessor :connection_class, :worker_pool_size attr_accessor :redis_path, :channels_path + attr_accessor :disable_request_forgery_protection, :allowed_request_origins def initialize @logger = Rails.logger @@ -16,6 +17,8 @@ module ActionCable @redis_path = Rails.root.join('config/redis/cable.yml') @channels_path = Rails.root.join('app/channels') + + @disable_request_forgery_protection = false end def channel_paths diff --git a/test/connection/base_test.rb b/test/connection/base_test.rb index 2f008652ee..a4bd7f4a03 100644 --- a/test/connection/base_test.rb +++ b/test/connection/base_test.rb @@ -16,6 +16,7 @@ class ActionCable::Connection::BaseTest < ActiveSupport::TestCase setup do @server = TestServer.new + @server.config.disable_request_forgery_protection = true env = Rack::MockRequest.env_for "/test", 'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket' @connection = Connection.new(@server, env) diff --git a/test/connection/cross_site_forgery_test.rb b/test/connection/cross_site_forgery_test.rb new file mode 100644 index 0000000000..b904dbd8b6 --- /dev/null +++ b/test/connection/cross_site_forgery_test.rb @@ -0,0 +1,55 @@ +require 'test_helper' +require 'stubs/test_server' + +class ActionCable::Connection::CrossSiteForgeryTest < ActiveSupport::TestCase + HOST = 'rubyonrails.com' + + setup do + @server = TestServer.new + end + + test "default cross site forgery protection only allows origin same as the server host" do + assert_origin_allowed 'http://rubyonrails.com' + assert_origin_not_allowed 'http://hax.com' + end + + test "disable forgery protection" do + @server.config.disable_request_forgery_protection = true + assert_origin_allowed 'http://rubyonrails.com' + assert_origin_allowed 'http://hax.com' + end + + test "explicitly specified a single allowed origin" do + @server.config.allowed_request_origins = 'hax.com' + assert_origin_not_allowed 'http://rubyonrails.com' + assert_origin_allowed 'http://hax.com' + end + + test "explicitly specified multiple allowed origins" do + @server.config.allowed_request_origins = %w( rubyonrails.com www.rubyonrails.com ) + assert_origin_allowed 'http://rubyonrails.com' + assert_origin_allowed 'http://www.rubyonrails.com' + assert_origin_allowed 'https://www.rubyonrails.com' + assert_origin_not_allowed 'http://hax.com' + end + + private + def assert_origin_allowed(origin) + response = connect_with_origin origin + assert_equal -1, response[0] + end + + def assert_origin_not_allowed(origin) + response = connect_with_origin origin + assert_equal 404, response[0] + end + + def connect_with_origin(origin) + ActionCable::Connection::Base.new(@server, env_for_origin(origin)).process + end + + def env_for_origin(origin) + Rack::MockRequest.env_for "/test", 'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket', 'SERVER_NAME' => HOST, + 'HTTP_ORIGIN' => origin + end +end -- cgit v1.2.3 From 8275cfb20fda9c89c5b8d7fc17f9f88822fc34d2 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 7 Oct 2015 14:43:47 -0500 Subject: Use Array() instead of Array.wrap --- lib/action_cable.rb | 1 - lib/action_cable/connection/base.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/action_cable.rb b/lib/action_cable.rb index 49f14b51bc..d2c5251634 100644 --- a/lib/action_cable.rb +++ b/lib/action_cable.rb @@ -8,7 +8,6 @@ require 'active_support' require 'active_support/json' require 'active_support/concern' require 'active_support/core_ext/hash/indifferent_access' -require 'active_support/core_ext/array/wrap' require 'active_support/core_ext/module/delegation' require 'active_support/callbacks' diff --git a/lib/action_cable/connection/base.rb b/lib/action_cable/connection/base.rb index a2ef99dff4..5bf7086b60 100644 --- a/lib/action_cable/connection/base.rb +++ b/lib/action_cable/connection/base.rb @@ -172,7 +172,7 @@ module ActionCable origin_host = URI.parse(env['HTTP_ORIGIN']).host allowed = if server.config.allowed_request_origins.present? - Array.wrap(server.config.allowed_request_origins).include? origin_host + Array(server.config.allowed_request_origins).include? origin_host else request.host == origin_host end -- cgit v1.2.3 From d621ae41c11398992647c600b484446ecc76a11b Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 12 Oct 2015 17:33:07 -0500 Subject: Set appropriate origin and host in the tests --- test/connection/base_test.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/connection/base_test.rb b/test/connection/base_test.rb index a4bd7f4a03..6c8bacde9a 100644 --- a/test/connection/base_test.rb +++ b/test/connection/base_test.rb @@ -16,9 +16,10 @@ class ActionCable::Connection::BaseTest < ActiveSupport::TestCase setup do @server = TestServer.new - @server.config.disable_request_forgery_protection = true - env = Rack::MockRequest.env_for "/test", 'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket' + env = Rack::MockRequest.env_for "/test", 'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket', + 'SERVER_NAME' => 'rubyonrails.com', 'HTTP_ORIGIN' => 'http://rubyonrails.com' + @connection = Connection.new(@server, env) @response = @connection.process end -- cgit v1.2.3 From ecab8314eba8519bd593cbc097ef60ee0c285cf2 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Mon, 12 Oct 2015 18:14:14 -0500 Subject: Treat ORIGIN as an opaque identifier and do equality comparison with the specified whitelist --- lib/action_cable/connection/base.rb | 17 +++-------------- test/connection/base_test.rb | 3 ++- test/connection/cross_site_forgery_test.rb | 12 ++++++------ 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/lib/action_cable/connection/base.rb b/lib/action_cable/connection/base.rb index 5bf7086b60..f7c5f050d8 100644 --- a/lib/action_cable/connection/base.rb +++ b/lib/action_cable/connection/base.rb @@ -168,23 +168,12 @@ module ActionCable def allow_request_origin? return true if server.config.disable_request_forgery_protection - if env['HTTP_ORIGIN'].present? - origin_host = URI.parse(env['HTTP_ORIGIN']).host - - allowed = if server.config.allowed_request_origins.present? - Array(server.config.allowed_request_origins).include? origin_host - else - request.host == origin_host - end - - logger.error("Request origin not allowed: #{env['HTTP_ORIGIN']}") unless allowed - allowed + if Array(server.config.allowed_request_origins).include? env['HTTP_ORIGIN'] + true else - logger.error("Request origin missing.") + logger.error("Request origin not allowed: #{env['HTTP_ORIGIN']}") false end - rescue URI::InvalidURIError - false end def respond_to_successful_request diff --git a/test/connection/base_test.rb b/test/connection/base_test.rb index 6c8bacde9a..bc8b5ba568 100644 --- a/test/connection/base_test.rb +++ b/test/connection/base_test.rb @@ -16,9 +16,10 @@ class ActionCable::Connection::BaseTest < ActiveSupport::TestCase setup do @server = TestServer.new + @server.config.allowed_request_origins = %w( http://rubyonrails.com ) env = Rack::MockRequest.env_for "/test", 'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket', - 'SERVER_NAME' => 'rubyonrails.com', 'HTTP_ORIGIN' => 'http://rubyonrails.com' + 'HTTP_ORIGIN' => 'http://rubyonrails.com' @connection = Connection.new(@server, env) @response = @connection.process diff --git a/test/connection/cross_site_forgery_test.rb b/test/connection/cross_site_forgery_test.rb index b904dbd8b6..6073f89287 100644 --- a/test/connection/cross_site_forgery_test.rb +++ b/test/connection/cross_site_forgery_test.rb @@ -6,11 +6,12 @@ class ActionCable::Connection::CrossSiteForgeryTest < ActiveSupport::TestCase setup do @server = TestServer.new + @server.config.allowed_request_origins = %w( http://rubyonrails.com ) end - test "default cross site forgery protection only allows origin same as the server host" do - assert_origin_allowed 'http://rubyonrails.com' - assert_origin_not_allowed 'http://hax.com' + teardown do + @server.config.disable_request_forgery_protection = false + @server.config.allowed_request_origins = [] end test "disable forgery protection" do @@ -20,16 +21,15 @@ class ActionCable::Connection::CrossSiteForgeryTest < ActiveSupport::TestCase end test "explicitly specified a single allowed origin" do - @server.config.allowed_request_origins = 'hax.com' + @server.config.allowed_request_origins = 'http://hax.com' assert_origin_not_allowed 'http://rubyonrails.com' assert_origin_allowed 'http://hax.com' end test "explicitly specified multiple allowed origins" do - @server.config.allowed_request_origins = %w( rubyonrails.com www.rubyonrails.com ) + @server.config.allowed_request_origins = %w( http://rubyonrails.com http://www.rubyonrails.com ) assert_origin_allowed 'http://rubyonrails.com' assert_origin_allowed 'http://www.rubyonrails.com' - assert_origin_allowed 'https://www.rubyonrails.com' assert_origin_not_allowed 'http://hax.com' end -- cgit v1.2.3