diff --git a/CHANGELOG.md b/CHANGELOG.md index a1bbb24d..78627b86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master +**Bug fixes**: + +- [#168](https://github.com/castle/castle-ruby/pull/168) do not apply whitelisting by default + ## 3.6.0 (2020-01-07) **BREAKING CHANGES:** diff --git a/README.md b/README.md index dd6ac95b..38b58bde 100644 --- a/README.md +++ b/README.md @@ -67,15 +67,24 @@ Castle.configure do |config| # Whitelisted and Blacklisted headers are case insensitive and allow to use _ and - as a separator, http prefixes are removed # Whitelisted headers - # @note In case of the whitelist, we won't send the values of other headers but we will send their names - config.whitelisted = ['X_HEADER'] - # or append to default - config.whitelisted += ['http-x-header'] - - # Blacklisted headers take advantage over whitelisted elements + # By default, the SDK sends all HTTP headers, except for Cookie and Authorization. + # If you decide to use a whitelist, the SDK will: + # - always send the User-Agent header + # - send scrubbed values of non-whitelisted headers + # - send proper values of whitelisted headers. + # @example + # config.whitelisted = ['X_HEADER'] + # # will send { 'User-Agent' => 'Chrome', 'X_HEADER' => 'proper value', 'Any-Other-Header' => true } + # + # We highly suggest using blacklist instead of whitelist, so that Castle can use as many data points + # as possible to secure your users. If you want to use the whitelist, this is the minimal + # amount of headers we recommend: + config.whitelisted = Castle::Configuration::DEFAULT_WHITELIST + + # Blacklisted headers take precedence over whitelisted elements + # We always blacklist Cookie and Authentication headers. If you use any other headers that + # might contain sensitive information, you should blacklist them. config.blacklisted = ['HTTP-X-header'] - # or append to default - config.blacklisted += ['X_HEADER'] end ``` diff --git a/lib/castle/configuration.rb b/lib/castle/configuration.rb index ab45118f..012104ec 100644 --- a/lib/castle/configuration.rb +++ b/lib/castle/configuration.rb @@ -9,22 +9,27 @@ class Configuration FAILOVER_STRATEGY = :allow REQUEST_TIMEOUT = 500 # in milliseconds FAILOVER_STRATEGIES = %i[allow deny challenge throw].freeze - WHITELISTED = [ - 'User-Agent', - 'Accept-Language', - 'Accept-Encoding', - 'Accept-Charset', - 'Accept', - 'Accept-Datetime', - 'X-Forwarded-For', - 'Forwarded', - 'X-Forwarded', - 'X-Real-IP', - 'REMOTE_ADDR', - 'X-Forwarded-For', - 'CF_CONNECTING_IP' + + # @note this value is not assigned as we don't recommend using a whitelist. If you need to use + # one, this constant is provided as a good default. + DEFAULT_WHITELIST = %w[ + Accept + Accept-Charset + Accept-Datetime + Accept-Encoding + Accept-Language + Cache-Control + Connection + Content-Length + Content-Type + Host + Origin + Pragma + Referer + TE + Upgrade-Insecure-Requests + X-Castle-Client-Id ].freeze - BLACKLISTED = ['HTTP_COOKIE'].freeze attr_accessor :host, :port, :request_timeout, :url_prefix attr_reader :api_secret, :whitelisted, :blacklisted, :failover_strategy @@ -36,8 +41,8 @@ def initialize self.host = HOST self.port = PORT self.url_prefix = URL_PREFIX - self.whitelisted = WHITELISTED - self.blacklisted = BLACKLISTED + self.whitelisted = [].freeze + self.blacklisted = [].freeze self.api_secret = '' end diff --git a/lib/castle/extractors/headers.rb b/lib/castle/extractors/headers.rb index 84017134..edfb0810 100644 --- a/lib/castle/extractors/headers.rb +++ b/lib/castle/extractors/headers.rb @@ -4,23 +4,45 @@ module Castle module Extractors # used for extraction of cookies and headers from the request class Headers + # Headers that we will never scrub, even if they land on the configuration blacklist. + ALWAYS_INCLUDED_HEADERS = %w[User-Agent].freeze + + # Headers that will always be scrubbed, even if whitelisted. + ALWAYS_SCRUBBED_HEADERS = %w[Cookie Authorization].freeze + + # Rack does not add the HTTP_ prefix to Content-Length for some reason + CONTENT_LENGTH = 'CONTENT_LENGTH' + + # Prefix that Rack adds for HTTP headers + HTTP_HEADER_PREFIX = 'HTTP_' + + private_constant :ALWAYS_INCLUDED_HEADERS, :ALWAYS_SCRUBBED_HEADERS, + :CONTENT_LENGTH, :HTTP_HEADER_PREFIX + + # @param request [Rack::Request] def initialize(request) - @request = request - @request_env = @request.env + @request_env = request.env @formatter = HeaderFormatter.new end # Serialize HTTP headers + # @return [Hash] def call - @request_env.keys.each_with_object({}) do |header, acc| - name = @formatter.call(header) + @request_env.keys.each_with_object({}) do |env_header, acc| + next unless env_header.start_with?(HTTP_HEADER_PREFIX) || env_header == CONTENT_LENGTH + + header = @formatter.call(env_header) - if Castle.config.whitelisted.include?(name) && !Castle.config.blacklisted.include?(name) - acc[name] = @request_env[header] + if ALWAYS_SCRUBBED_HEADERS.include?(header) + acc[header] = true + elsif ALWAYS_INCLUDED_HEADERS.include?(header) + acc[header] = @request_env[env_header] + elsif Castle.config.blacklisted.include?(header) + acc[header] = true + elsif Castle.config.whitelisted.empty? || Castle.config.whitelisted.include?(header) + acc[header] = @request_env[env_header] else - # When a header is not whitelisted or blacklisted, we're not suppose to send - # it's value but we should send it's name to indicate it's presence - acc[name] = true + acc[header] = true end end end diff --git a/spec/lib/castle/client_spec.rb b/spec/lib/castle/client_spec.rb index d362c123..393fc1d4 100644 --- a/spec/lib/castle/client_spec.rb +++ b/spec/lib/castle/client_spec.rb @@ -22,13 +22,7 @@ let(:headers) do { - 'Rack.version': true, 'Rack.input': true, 'Rack.errors': true, - 'Rack.multithread': true, 'Rack.multiprocess': true, 'Rack.run-Once': true, - 'Request-Method': true, 'Server-Name': true, 'Server-Port': true, - 'Query-String': true, 'Path-Info': true, 'Rack.url-Scheme': true, - 'Https': true, 'Script-Name': true, 'Content-Length': true, - 'User-Agent': ua, 'X-Forwarded-For': ip.to_s, 'Rack.request.cookie-Hash': true, - 'Rack.request.cookie-String': true, 'Cookie': true + 'Content-Length': '0', 'User-Agent': ua, 'X-Forwarded-For': ip.to_s, 'Cookie': true } end let(:context) do diff --git a/spec/lib/castle/configuration_spec.rb b/spec/lib/castle/configuration_spec.rb index a625c7e6..db9a2ab3 100644 --- a/spec/lib/castle/configuration_spec.rb +++ b/spec/lib/castle/configuration_spec.rb @@ -77,7 +77,7 @@ describe 'whitelisted' do it do - expect(config.whitelisted.size).to be_eql(13) + expect(config.whitelisted.size).to be_eql(0) end context 'with setter' do @@ -88,19 +88,11 @@ expect(config.whitelisted).to be_eql(['Header']) end end - - context 'when appending' do - before do - config.whitelisted += ['header'] - end - it { expect(config.whitelisted).to be_include('Header') } - it { expect(config.whitelisted.size).to be_eql(14) } - end end describe 'blacklisted' do it do - expect(config.blacklisted.size).to be_eql(1) + expect(config.blacklisted.size).to be_eql(0) end context 'with setter' do @@ -111,14 +103,6 @@ expect(config.blacklisted).to be_eql(['Header']) end end - - context 'when appending' do - before do - config.blacklisted += ['header'] - end - it { expect(config.blacklisted).to be_include('Header') } - it { expect(config.blacklisted.size).to be_eql(2) } - end end describe 'failover_strategy' do diff --git a/spec/lib/castle/context/default_spec.rb b/spec/lib/castle/context/default_spec.rb index cfe090dd..fd183a09 100644 --- a/spec/lib/castle/context/default_spec.rb +++ b/spec/lib/castle/context/default_spec.rb @@ -9,8 +9,8 @@ let(:env) do Rack::MockRequest.env_for('/', 'HTTP_X_FORWARDED_FOR' => ip, - 'HTTP-Accept-Language' => 'en', - 'HTTP-User-Agent' => 'test', + 'HTTP_ACCEPT_LANGUAGE' => 'en', + 'HTTP_USER_AGENT' => 'test', 'HTTP_COOKIE' => "__cid=#{cookie_id};other=efgh") end let(:request) { Rack::Request.new(env) } @@ -23,18 +23,17 @@ it { expect(default_context[:active]).to be_eql(true) } it { expect(default_context[:origin]).to be_eql('web') } - it { + + it do expect(default_context[:headers]).to be_eql( - 'Rack.version' => true, 'Rack.input' => true, 'Rack.errors' => true, - 'Rack.multithread' => true, 'Rack.multiprocess' => true, 'Rack.run-Once' => true, - 'Request-Method' => true, 'Server-Name' => true, 'Server-Port' => true, - 'Query-String' => true, 'Path-Info' => true, 'Rack.url-Scheme' => true, - 'Https' => true, 'Script-Name' => true, 'Content-Length' => true, - 'X-Forwarded-For' => '1.2.3.4', 'Accept-Language' => 'en', 'User-Agent' => 'test', - 'Rack.request.cookie-Hash' => true, 'Rack.request.cookie-String' => true, + 'X-Forwarded-For' => '1.2.3.4', + 'Accept-Language' => 'en', + 'User-Agent' => 'test', + 'Content-Length' => '0', 'Cookie' => true ) - } + end + it { expect(default_context[:ip]).to be_eql(ip) } it { expect(default_context[:library][:name]).to be_eql('castle-rb') } it { expect(default_context[:library][:version]).to be_eql(version) } diff --git a/spec/lib/castle/extractors/headers_spec.rb b/spec/lib/castle/extractors/headers_spec.rb index bf97f15c..c67adeb6 100644 --- a/spec/lib/castle/extractors/headers_spec.rb +++ b/spec/lib/castle/extractors/headers_spec.rb @@ -1,32 +1,88 @@ # frozen_string_literal: true describe Castle::Extractors::Headers do - subject(:extractor) { described_class.new(request) } + subject(:headers) { described_class.new(request).call } let(:client_id) { 'abcd' } let(:env) do - Rack::MockRequest.env_for('/', - 'HTTP_X_FORWARDED_FOR' => '1.2.3.4', - 'HTTP_OK' => 'OK', - 'TEST' => '1', - 'HTTP_COOKIE' => "__cid=#{client_id};other=efgh") + Rack::MockRequest.env_for( + '/', + 'Action-Dispatch.request.content-Type' => 'application/json', + 'HTTP_AUTHORIZATION' => 'Basic 123456', + 'HTTP_COOKIE' => "__cid=#{client_id};other=efgh", + 'HTTP_OK' => 'OK', + 'HTTP_ACCEPT' => 'application/json', + 'HTTP_X_FORWARDED_FOR' => '1.2.3.4', + 'HTTP_USER_AGENT' => 'Mozilla 1234', + 'TEST' => '1' + ) end let(:request) { Rack::Request.new(env) } - describe 'extract http headers with whitelisted and blacklisted support' do + context 'when whitelist is not set in the configuration' do + it do + is_expected.to eq('Accept' => 'application/json', + 'Authorization' => true, + 'Cookie' => true, + 'Content-Length' => '0', + 'Ok' => 'OK', + 'User-Agent' => 'Mozilla 1234', + 'X-Forwarded-For' => '1.2.3.4') + end + end + + context 'when whitelist is set in the configuration' do + before { Castle.config.whitelisted = %w[Accept OK] } + + it do + is_expected.to eq('Accept' => 'application/json', + 'Authorization' => true, + 'Cookie' => true, + 'Content-Length' => true, + 'Ok' => 'OK', + 'User-Agent' => 'Mozilla 1234', + 'X-Forwarded-For' => true) + end + end + + context 'when blacklist is set in the configuration' do + context 'with a User-Agent' do + before { Castle.config.blacklisted = %w[User-Agent] } + + it do + is_expected.to eq('Accept' => 'application/json', + 'Authorization' => true, + 'Cookie' => true, + 'Content-Length' => '0', + 'Ok' => 'OK', + 'User-Agent' => 'Mozilla 1234', + 'X-Forwarded-For' => '1.2.3.4') + end + end + + context 'with a different header' do + before { Castle.config.blacklisted = %w[Accept] } + + it do + is_expected.to eq('Accept' => true, + 'Authorization' => true, + 'Cookie' => true, + 'Content-Length' => '0', + 'Ok' => 'OK', + 'User-Agent' => 'Mozilla 1234', + 'X-Forwarded-For' => '1.2.3.4') + end + end + end + + context 'when a header is both whitelisted and blacklisted' do before do - Castle.config.whitelisted += ['TEST'] + Castle.config.whitelisted = %w[Accept] + Castle.config.blacklisted = %w[Accept] end + it do - expect(extractor.call).to eql( - 'Test' => '1', 'Ok' => true, 'Rack.version' => true, - 'Rack.input' => true, 'Rack.errors' => true, 'Rack.multithread' => true, - 'Rack.multiprocess' => true, 'Rack.run-Once' => true, 'Request-Method' => true, - 'Server-Name' => true, 'Server-Port' => true, 'Query-String' => true, - 'Path-Info' => true, 'Rack.url-Scheme' => true, 'Https' => true, - 'Script-Name' => true, 'Content-Length' => true, 'X-Forwarded-For' => '1.2.3.4', - 'Cookie' => true - ) + expect(headers['Accept']).to eq(true) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a539a209..cc2d71b8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,11 +12,14 @@ require 'castle' -Castle.configure do |config| - config.api_secret = 'secret' -end - WebMock.disable_net_connect!(allow_localhost: true) RSpec.configure do |config| + config.before do + Castle.instance_variable_set(:@configuration, Castle::Configuration.new) + + Castle.configure do |cfg| + cfg.api_secret = 'secret' + end + end end