Merge branch 'security-leaking-repo-mirror-credentials' into 'master'

[Security] Improve sanitization of Gitaly exceptions

See merge request gitlab/gitaly!16

5 files changed, 21 insertions, 11 deletions

diff --git a/internal/logsanitizer/url.go b/internal/logsanitizer/url.go
index ae56ddd83..8640570f5 100644
--- a/internal/logsanitizer/url.go
+++ b/internal/logsanitizer/url.go
@@ -8,7 +8,7 @@ import (
 
 // Pattern taken from Regular Expressions Cookbook, slightly modified though
 //                                        |Scheme                |User                         |Named/IPv4 host|IPv6+ host
-var hostPattern = regexp.MustCompile(`(?i)([a-z][a-z0-9+\-.]*://)([a-z0-9\-._~%!$&'()*+,;=:]+@)([a-z0-9\-._~%]+|\[[a-z0-9\-._~%!$&'()*+,;=:]+\])`)
+var hostPattern = regexp.MustCompile(`(?i)([a-z][a-z0-9+\-.]*://)?([a-z0-9\-._~%!$&'()*+,;=:]+@)([a-z0-9\-._~%]+|\[[a-z0-9\-._~%!$&'()*+,;=:]+\])`)
 
 // URLSanitizerHook stores which gRPC methods to perform sanitization for.
 type URLSanitizerHook struct {
diff --git a/internal/logsanitizer/url_test.go b/internal/logsanitizer/url_test.go
index b3fc116fd..5b21d36f0 100644
--- a/internal/logsanitizer/url_test.go
+++ b/internal/logsanitizer/url_test.go
@@ -17,6 +17,7 @@ func TestUrlSanitizerHook(t *testing.T) {
 	urlSanitizer.AddPossibleGrpcMethod(
 		"UpdateRemoteMirror",
 		"CreateRepositoryFromURL",
+		"FetchRemote",
 	)
 
 	logger := log.New()
@@ -58,6 +59,15 @@ func TestUrlSanitizerHook(t *testing.T) {
 			expectedString: "asked for: https://[FILTERED]@gitlab.com/foo/bar",
 		},
 		{
+			desc: "with URL without scheme output",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "FetchRemote",
+				}).Info("fatal: unable to look up foo:bar@non-existent.org (port 9418) (nodename nor servname provided, or not known")
+			},
+			expectedString: "unable to look up [FILTERED]@non-existent.org (port 9418) (nodename nor servname provided, or not known",
+		},
+		{
 			desc: "with gRPC method not added to the list",
 			logFunc: func() {
 				logger.WithFields(log.Fields{
diff --git a/ruby/lib/gitaly_server/utils.rb b/ruby/lib/gitaly_server/utils.rb
index 968bb17ca..9748522bc 100644
--- a/ruby/lib/gitaly_server/utils.rb
+++ b/ruby/lib/gitaly_server/utils.rb
@@ -1,7 +1,7 @@
 module GitalyServer
   module Utils
     # See internal/logsanitizer/url.go for credits and explanation.
-    URL_HOST_PATTERN = %r{([a-z][a-z0-9+\-.]*://)([a-z0-9\-._~%!$&'()*+,;=:]+@)([a-z0-9\-._~%]+|\[[a-z0-9\-._~%!$&'()*+,;=:]+\])}i
+    URL_HOST_PATTERN = %r{([a-z][a-z0-9+\-.]*://)?([a-z0-9\-._~%!$&'()*+,;=:]+@)([a-z0-9\-._~%]+|\[[a-z0-9\-._~%!$&'()*+,;=:]+\])}i
 
     def gitaly_commit_from_rugged(rugged_commit)
       message_split = rugged_commit.message.b.split("\n", 2)
diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb
index 3c7453961..b02cdc5ec 100644
--- a/ruby/lib/gitlab/git/gitlab_projects.rb
+++ b/ruby/lib/gitlab/git/gitlab_projects.rb
@@ -112,15 +112,6 @@ module Gitlab
         false
       end
 
-      def mask_password_in_url(url)
-        result = URI(url)
-        result.password = "*****" unless result.password.nil?
-        result.user = "*****" unless result.user.nil? # it's needed for oauth access_token
-        result
-      rescue
-        url
-      end
-
       def remove_origin_in_repo
         cmd = %W(#{Gitlab.config.git.bin_path} remote rm origin)
         run(cmd, repository_absolute_path)
diff --git a/ruby/spec/lib/gitaly_server/exception_sanitizer_interceptor_spec.rb b/ruby/spec/lib/gitaly_server/exception_sanitizer_interceptor_spec.rb
index 06fabffc8..2e5516173 100644
--- a/ruby/spec/lib/gitaly_server/exception_sanitizer_interceptor_spec.rb
+++ b/ruby/spec/lib/gitaly_server/exception_sanitizer_interceptor_spec.rb
@@ -17,6 +17,15 @@ describe GitalyServer::ExceptionSanitizerInterceptor do
     end
   end
 
+  context 'with incomplete url in exception' do
+    let(:ex) { "unable to look up user:pass@non-existent.org (port 9418)" }
+    let(:ex_sanitized_message) { "unable to look up [FILTERED]@non-existent.org (port 9418)" }
+
+    it 'sanitizes exception message' do
+      expect { subject }.to raise_error(ex_sanitized_message)
+    end
+  end
+
   context 'GRPC::BadStatus exception' do
     let(:ex) { GRPC::Unknown.new(super().message) }