Commit 3322595f authored by Małgorzata Ksionek's avatar Małgorzata Ksionek

Stop logging location record with explicit access token

Changelog: security

Fix rubocop offence

Add specs for new method

Fix typo in spec title
parent aee6da52
......@@ -18,6 +18,7 @@ unless Gitlab::Runtime.sidekiq?
data[:db_duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:db)) if data[:db]
data[:view_duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:view)) if data[:view]
data[:duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:duration)) if data[:duration]
data[:location] = Gitlab::Utils.removes_sensitive_data_from_url(data[:location]) if data[:location]
# Remove empty hashes to prevent type mismatches
# These are set to empty hashes in Lograge's ActionCable subscriber
......
......@@ -197,6 +197,24 @@ module Gitlab
rescue Addressable::URI::InvalidURIError, TypeError
end
def removes_sensitive_data_from_url(uri_string)
uri = parse_url(uri_string)
return unless uri
return uri_string unless uri.fragment
stripped_params = CGI.parse(uri.fragment)
if stripped_params['access_token']
stripped_params['access_token'] = 'filtered'
filtered_query = Addressable::URI.new
filtered_query.query_values = stripped_params
uri.fragment = filtered_query.query
end
uri.to_s
end
# Invert a hash, collecting all keys that map to a given value in an array.
#
# Unlike `Hash#invert`, where the last encountered pair wins, and which has the
......
......@@ -173,6 +173,27 @@ RSpec.describe 'lograge', type: :request do
end
end
describe 'with access token in url' do
before do
event.payload[:location] = 'http://example.com/auth.html#access_token=secret_token&token_type=Bearer'
end
it 'strips location from sensitive information' do
subscriber.redirect_to(event)
subscriber.process_action(event)
expect(log_data['location']).not_to include('secret_token')
expect(log_data['location']).to include('filtered')
end
it 'leaves non-sensitive information from location' do
subscriber.redirect_to(event)
subscriber.process_action(event)
expect(log_data['location']).to include('&token_type=Bearer')
end
end
context 'with db payload' do
context 'when RequestStore is enabled', :request_store do
it 'includes db counters' do
......
......@@ -417,6 +417,29 @@ RSpec.describe Gitlab::Utils do
end
end
describe '.removes_sensitive_data_from_url' do
it 'returns string object' do
expect(described_class.removes_sensitive_data_from_url('http://gitlab.com')).to be_instance_of(String)
end
it 'returns nil when URI cannot be parsed' do
expect(described_class.removes_sensitive_data_from_url('://gitlab.com')).to be nil
end
it 'returns nil with invalid parameter' do
expect(described_class.removes_sensitive_data_from_url(1)).to be nil
end
it 'returns string with filtered access_token param' do
expect(described_class.removes_sensitive_data_from_url('http://gitlab.com/auth.html#access_token=secret_token')).to eq('http://gitlab.com/auth.html#access_token=filtered')
end
it 'returns string with filtered access_token param but other params preserved' do
expect(described_class.removes_sensitive_data_from_url('http://gitlab.com/auth.html#access_token=secret_token&token_type=Bearer&state=test'))
.to include('&token_type=Bearer', '&state=test')
end
end
describe 'multiple_key_invert' do
it 'invert keys with array values' do
hash = {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment