diff options
| -rw-r--r-- | changelogs/unreleased/an-sanitize-url-parameters.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/url_sanitizer.rb | 27 | ||||
| -rw-r--r-- | spec/lib/gitlab/url_sanitizer_spec.rb | 37 |
3 files changed, 55 insertions, 14 deletions
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 } |