Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/controllers/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -716,15 +716,17 @@ def authorized_request?
# format_tokens_defined? confirms whether a format token is defined in both the environment and query params.
# Returns a boolean.
def format_tokens_defined?
return true if ENV.fetch('FORMAT_TOKEN', '').present? && params.key?(:format_token)
return false unless ENV.fetch('FORMAT_TOKEN', '').present?

false
request.headers['HTTP_FORMAT_TOKEN'].presence || params[:format_token].presence
end

# valid_token? confirms whether the token received from the user matches the one defined in the environment. Returns
# a boolean.
def valid_token?
return true if ActiveSupport::SecurityUtils.secure_compare(params[:format_token], ENV.fetch('FORMAT_TOKEN', ''))
candidate = request.headers['HTTP_FORMAT_TOKEN'].presence || params[:format_token]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the search controller in a different way, using .presence within the format_tokens_defined? method as well as within the validity check. This prevents the need for a guard clause on candidate.blank?, as it stops valid_token? from being called in the first place. I've added a test on lines 2001-2013 to confirm the intended behavior (red-green refactor FTW, I think)


return true if ActiveSupport::SecurityUtils.secure_compare(candidate, ENV.fetch('FORMAT_TOKEN', ''))

false
end
Expand Down
5 changes: 3 additions & 2 deletions config/initializers/rack_attack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,13 @@ def self.req_ip_free_path?(path)
end

# Returns true when the request includes a valid format token that matches the FORMAT_TOKEN environment variable.
# This allows programmatic API requests (with valid tokens) to bypass throttling.
# This supports tokens supplied via request header or via query parameter. This allows programmatic API requests (with
# valid tokens) to bypass throttling.
def self.valid_format_token?(req)
format_token_env = ENV.fetch('FORMAT_TOKEN', '')
return false if format_token_env.blank?

format_token_param = req.params['format_token']
format_token_param = req.env['HTTP_FORMAT_TOKEN'].presence || req.params['format_token']
return false if format_token_param.blank?

ActiveSupport::SecurityUtils.secure_compare(format_token_param, format_token_env)
Expand Down
76 changes: 74 additions & 2 deletions test/controllers/search_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1888,7 +1888,7 @@ def source_filter_count(controller)
'Expected tuning parameters to affect results, but counts and title orders are identical.')
end

test 'results can be returned in JSON format when env is set and valid token is provided' do
test 'results can be returned in JSON format when env is set and valid token is provided via param' do
secret_value = 'sooper_sekret'
quepid_ua = 'Quepid/1.0 (Web Scraper)'
ClimateControl.modify FORMAT_TOKEN: secret_value do
Expand All @@ -1903,6 +1903,21 @@ def source_filter_count(controller)
end
end

test 'results can be returned in JSON format when env is set and valid token is provided via header' do
secret_value = 'sooper_sekret'
quepid_ua = 'Quepid/1.0 (Web Scraper)'
ClimateControl.modify FORMAT_TOKEN: secret_value do
mock_timdex_search_with_hits(10)
get "/results?q=test&format=json", headers: { 'HTTP_USER_AGENT' => quepid_ua, 'HTTP_FORMAT_TOKEN' => secret_value }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 2 issues:

1. Line is too long. [121/120] [rubocop:Layout/LineLength]


2. Prefer single-quoted strings when you don't need string interpolation or special symbols. [rubocop:Style/StringLiterals]

assert_response :success
assert_equal 'application/json; charset=utf-8', response.content_type
response_json = JSON.parse(response.body)
assert_equal %w[results hits errors], response_json.keys
assert_instance_of Array, response_json['results']
assert_instance_of Integer, response_json['hits']
end
end

test 'results in JSON format will report zero hit count in the face of missing data' do
SearchController.any_instance.stubs(:load_timdex_results).returns(nil)
secret_value = 'sooper_sekret'
Expand All @@ -1927,7 +1942,7 @@ def source_filter_count(controller)
end
end

test 'results can be returned in JSON format when env is set and valid token is provided even with bot challenge' do
test 'results returned in JSON format when valid token is provided via querystring even with bot challenge' do
secret_value = 'sooper_sekret'
quepid_ua = 'Quepid/1.0 (Web Scraper)'

Expand All @@ -1943,6 +1958,22 @@ def source_filter_count(controller)
end
end

test 'results returned in JSON format when valid token is provided via headers even with bot challenge' do
secret_value = 'sooper_sekret'
quepid_ua = 'Quepid/1.0 (Web Scraper)'

ClimateControl.modify(FORMAT_TOKEN: secret_value, FEATURE_BOT_DETECTION: 'true') do
mock_timdex_search_with_hits(10)
get "/results?q=test&format=json", headers: { 'HTTP_FORMAT_TOKEN' => secret_value, 'HTTP_USER_AGENT' => quepid_ua }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 2 issues:

1. Line is too long. [121/120] [rubocop:Layout/LineLength]


2. Prefer single-quoted strings when you don't need string interpolation or special symbols. [rubocop:Style/StringLiterals]

assert_response :success
assert_equal 'application/json; charset=utf-8', response.content_type
response_json = JSON.parse(response.body)
assert_equal %w[results hits errors], response_json.keys
assert_instance_of Array, response_json['results']
assert_instance_of Integer, response_json['hits']
end
end

test 'results can be returned in JSON format when env is set while load-more pagination is enabled' do
secret_value = 'sooper_sekret'
quepid_ua = 'Quepid/1.0 (Web Scraper)'
Expand All @@ -1967,12 +1998,31 @@ def source_filter_count(controller)
end
end

# We don't mock anything here because the error prevents any external lookups
test 'requests for JSON results with an empty token generate an unauthorized error' do
secret_value = 'sooper_sekret'
ClimateControl.modify FORMAT_TOKEN: secret_value do
get '/results?q=test&format=json', headers: { 'HTTP_FORMAT_TOKEN' => '' }

assert_response :unauthorized

get '/results?q=test&format=json&format_token='

assert_response :unauthorized
end
end

# We don't mock anything here because the error prevents any external lookups
test 'requests for JSON results when env var is not set generate an unauthorized error' do
secret_value = 'irrelevant'
ClimateControl.modify FORMAT_TOKEN: '' do
# via param
get "/results?q=test&format=json&format_token=#{secret_value}"
assert_response :unauthorized

# via headers
get '/results?q=test&format=json', headers: { 'HTTP_FORMAT_TOKEN' => secret_value }
assert_response :unauthorized
end
end

Expand All @@ -1983,8 +2033,30 @@ def source_filter_count(controller)
refute_equal secret_value, wrong_secret

ClimateControl.modify FORMAT_TOKEN: secret_value do
# via param
get "/results?q=test&format=json&format_token=#{wrong_secret}"
assert_response :unauthorized

# via headers
get '/results?q=test&format=json', headers: { 'HTTP_FORMAT_TOKEN' => wrong_secret }
assert_response :unauthorized
end
end

test 'requests for JSON results assign precedence to request header over query params if both present' do
secret_value = 'sooper_sekret'
wrong_secret = 'something_else'
refute_equal secret_value, wrong_secret

ClimateControl.modify FORMAT_TOKEN: secret_value do
# wrong header, correct query params - results in an unauthorized request
get "/results?q=test&format=json&format_token=#{secret_value}", headers: { 'HTTP_FORMAT_TOKEN' => wrong_secret }
assert_response :unauthorized

# correct header, wrong query params - results in success. Mocked response because we just test for success.
mock_timdex_search_with_hits(10)
get "/results?q=test&format=json&format_token=#{wrong_secret}", headers: { 'HTTP_FORMAT_TOKEN' => secret_value }
assert_response :success
end
end

Expand Down
40 changes: 38 additions & 2 deletions test/integration/rack_attack_format_token_bypass_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ def rack_request_with_format_token(token_value = nil)
Rack::Request.new('QUERY_STRING' => params)
end

def rack_request_with_format_token_via_header(token_value = nil)
Rack::Request.new('HTTP_FORMAT_TOKEN' => token_value)
end

test 'valid_format_token? returns false when FORMAT_TOKEN env var is blank' do
ClimateControl.modify(FORMAT_TOKEN: '') do
refute Rack::Attack.valid_format_token?(rack_request_with_format_token('some-token'))
Expand All @@ -44,19 +48,51 @@ def rack_request_with_format_token(token_value = nil)
end
end

test 'valid_format_token? returns false when token does not match' do
test 'valid_format_token? returns false when format_token header is missing' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
refute Rack::Attack.valid_format_token?(rack_request_with_format_token_via_header)
refute Rack::Attack.valid_format_token?(rack_request_with_format_token_via_header(''))
end
end

test 'valid_format_token? returns false when token param does not match' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
refute Rack::Attack.valid_format_token?(rack_request_with_format_token('wrong-token'))
refute Rack::Attack.valid_format_token?(rack_request_with_format_token('tampered-token'))
end
end

test 'valid_format_token? returns true when token matches FORMAT_TOKEN env var' do
test 'valid_format_token? returns false when token header does not match' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
refute Rack::Attack.valid_format_token?(rack_request_with_format_token_via_header('wrong-token'))
refute Rack::Attack.valid_format_token?(rack_request_with_format_token_via_header('tampered-token'))
end
end

test 'valid_format_token? returns true when querystring token matches FORMAT_TOKEN env var' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
assert Rack::Attack.valid_format_token?(rack_request_with_format_token('valid-token'))
end
end

test 'valid_format_token? returns true when header token matches FORMAT_TOKEN env var' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
assert Rack::Attack.valid_format_token?(rack_request_with_format_token_via_header('valid-token'))
end
end

test 'valid_format_token? assigns precedence to request header over query parameters' do
correct = 'valid-token'
incorrect = 'wrong-token'
ClimateControl.modify(FORMAT_TOKEN: correct) do
env = { 'HTTP_FORMAT_TOKEN' => correct, 'QUERY_STRING' => "format_token=#{incorrect}" }
assert Rack::Attack.valid_format_token?(Rack::Request.new(env))

env = { 'HTTP_FORMAT_TOKEN' => incorrect, 'QUERY_STRING' => "format_token=#{correct}" }
refute Rack::Attack.valid_format_token?(Rack::Request.new(env))
end
end

test 'valid_format_token? uses secure comparison to prevent timing attacks' do
ClimateControl.modify(FORMAT_TOKEN: 'valid-token') do
req = rack_request_with_format_token('valid-token')
Expand Down
Loading