Mask filterable parameters from sanitised URLsan-sanitize-url-parameters

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

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 }