From f15a5b2c270cc06ffa57d505d4fba161998889cd Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Mon, 21 Jan 2019 19:59:21 +0200 Subject: Mask filterable parameters from sanitised URLs Sanitised URLS are used for logging and display purposes only, and are intended to prevent sensitive information, such as credentials and access tokens. This change ensures that if URLs contain certain parameters, as configured by `Rails.application.config.filter_parameters`, these parameters in the sanitised URL will be masked with the phrase `[FILTERED]`. This is required for distributed tracing, which emits the `http.url` field, which is intended to include the full URL including querystring parameters. Since we want to avoid sensitive information was as `?private_token` values leaking, we need to mask the URL --- .../unreleased/an-sanitize-url-parameters.yml | 5 +++ lib/gitlab/url_sanitizer.rb | 27 +++++++++++++++- spec/lib/gitlab/url_sanitizer_spec.rb | 37 ++++++++++++++-------- 3 files changed, 55 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/an-sanitize-url-parameters.yml diff --git a/changelogs/unreleased/an-sanitize-url-parameters.yml b/changelogs/unreleased/an-sanitize-url-parameters.yml new file mode 100644 index 00000000000..bb5dc280944 --- /dev/null +++ b/changelogs/unreleased/an-sanitize-url-parameters.yml @@ -0,0 +1,5 @@ +--- +title: Mask filterable parameters from sanitised URLs +merge_request: 24537 +author: +type: security diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb index 880712de5fe..cc58a69ee2e 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -3,11 +3,25 @@ module Gitlab class UrlSanitizer ALLOWED_SCHEMES = %w[http https ssh git].freeze + SINGLE_QUOTE = "'" def self.sanitize(content) regexp = URI::DEFAULT_PARSER.make_regexp(ALLOWED_SCHEMES) - content.gsub(regexp) { |url| new(url).masked_url } + content.gsub(regexp) do |url| + # Unfortunately, URI::DEFAULT_PARSER.make_regexp returns a regular expression which hungrily consumes + # single quotes at the end of a string. When the querystring is filtered, the quote is converted into a %27 + # which can affect error messages which quote the URL with singlequotes. + # + # Note the regular expression issue does not affect double-quotes nor backticks. + if url.end_with?(SINGLE_QUOTE) + url_without_quote = url.delete_suffix(SINGLE_QUOTE) + masked_unquoted_url = new(url_without_quote).masked_url + masked_unquoted_url + SINGLE_QUOTE + else + new(url).masked_url + end + end rescue Addressable::URI::InvalidURIError content.gsub(regexp, '') end @@ -40,6 +54,7 @@ module Gitlab url = @url.dup url.password = "*****" if url.password.present? url.user = "*****" if url.user.present? + url.query = filter_query(url.query) if url.query.present? url.to_s end @@ -53,6 +68,16 @@ module Gitlab private + def param_filter + @param_filter ||= ActionDispatch::Http::ParameterFilter.new(Rails.application.config.filter_parameters) + end + + def filter_query(query) + qs = CGI.parse(query) + qs = param_filter.filter(qs) + URI.encode_www_form(qs) + end + def parse_url(url) url = url.to_s.strip match = url.match(%r{\A(?:git|ssh|http(?:s?))\://(?:(.+)(?:@))?(.+)}) diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb index 6e98a999766..b2176a18918 100644 --- a/spec/lib/gitlab/url_sanitizer_spec.rb +++ b/spec/lib/gitlab/url_sanitizer_spec.rb @@ -4,12 +4,18 @@ describe Gitlab::UrlSanitizer do using RSpec::Parameterized::TableSyntax describe '.sanitize' do - def sanitize_url(url) + def generate_message(url) # We want to try with multi-line content because is how error messages are formatted - described_class.sanitize(%Q{ - remote: Not Found - fatal: repository '#{url}' not found - }) + %Q{ + remote: Not Found + fatal: repository '#{url}' not found. + With Double Quotes: "#{url}" + With backticks: \`#{url}\` + } + end + + def sanitize_url(url) + described_class.sanitize(generate_message(url)) end where(:input, :output) do @@ -27,10 +33,15 @@ describe Gitlab::UrlSanitizer do # return an empty string for invalid URLs 'ssh://' | '' + + # Filter secrets from the querystring + 'http://gitlab.com/api/v4/projects?private_token=1234&a=b' | 'http://gitlab.com/api/v4/projects?private_token=%5BFILTERED%5D&a=b' + 'http://gitlab.com/api/v4/projects?password=1234&a=b' | 'http://gitlab.com/api/v4/projects?password=%5BFILTERED%5D&a=b' + 'http://gitlab.com/api/v4/projects?token=1234&a=b' | 'http://gitlab.com/api/v4/projects?token=%5BFILTERED%5D&a=b' end with_them do - it { expect(sanitize_url(input)).to include("repository '#{output}' not found") } + it { expect(sanitize_url(input)).to eq(generate_message(output)) } end end @@ -51,6 +62,7 @@ describe Gitlab::UrlSanitizer do true | 'git://foo:bar@example.com/group/group/project.git' true | 'http://foo:bar@example.com/group/group/project.git' true | 'https://foo:bar@example.com/group/group/project.git' + true | 'https://foo:bar@example.com/group/group/project.git?token=password' end with_them do @@ -92,14 +104,13 @@ describe Gitlab::UrlSanitizer do context 'credentials in URL' do where(:url, :credentials) do - 'http://foo:bar@example.com' | { user: 'foo', password: 'bar' } + 'http://foo:bar@example.com' | { user: 'foo', password: 'bar' } 'http://foo:bar:baz@example.com' | { user: 'foo', password: 'bar:baz' } - 'http://:bar@example.com' | { user: nil, password: 'bar' } - 'http://foo:@example.com' | { user: 'foo', password: nil } - 'http://foo@example.com' | { user: 'foo', password: nil } - 'http://:@example.com' | { user: nil, password: nil } - 'http://@example.com' | { user: nil, password: nil } - 'http://example.com' | { user: nil, password: nil } + 'http://foo:@example.com' | { user: 'foo', password: nil } + 'http://foo@example.com' | { user: 'foo', password: nil } + 'http://:@example.com' | { user: nil, password: nil } + 'http://@example.com' | { user: nil, password: nil } + 'http://example.com' | { user: nil, password: nil } # Other invalid URLs nil | { user: nil, password: nil } -- cgit v1.2.1