Merge branch 'security-182-remove-token' into 'master'

[master] Redact sensitive url params as in Rails

See merge request gitlab/gitlab-workhorse!4

CHANGELOG

@@ -14,6 +14,10 @@ v 7.2.0
 - Update CI matrix to go1.10 + go1.11 and fix ResponseWriter bugs !309
 - Add support for Redis URLs (redis:// and rediss://) in Workhorse !321
 
+v 7.1.3
+
+- Redact sensitive url params as in Rails
+
 v 7.1.1
 
 Bad release, use 7.2.0 instead.
@@ -32,10 +36,18 @@ v 7.1.0
 - Pass Correlation-Ids down to backend systems !311
 - Don't fail if /home/git/repositories already exists in Gitaly container !317
 
+v 7.0.1
+
+- Redact sensitive url params as in Rails
+
 v 7.0.0
 
 - Use the new Gitaly auth scheme (v2) !298
 
+v 6.1.2
+
+- Redact sensitive url params as in Rails
+
 v 6.1.1
 
 - Allow custom error messages to pass through to Rails !300

internal/helper/helpers.go

@@ -19,8 +19,6 @@ import (
 
 const NginxResponseBufferHeader = "X-Accel-Buffering"
 
-var scrubRegexp = regexp.MustCompile(`(?i)([\?&]((?:private|authenticity|rss)[\-_]token)|(?:X-AMZ-)?Signature)=[^&]*`)
-
 func Fail500(w http.ResponseWriter, r *http.Request, err error) {
 	http.Error(w, "Internal server error", 500)
 	if err != nil {
@@ -196,8 +194,82 @@ func CloneRequestWithNewBody(r *http.Request, body []byte) *http.Request {
 	return &newReq
 }
 
+// Based on https://stackoverflow.com/a/52965552/474597
 // ScrubURLParams replaces the content of any sensitive query string parameters
 // in an URL with `[FILTERED]`
-func ScrubURLParams(url string) string {
-	return scrubRegexp.ReplaceAllString(url, "$1=[FILTERED]")
+func ScrubURLParams(originalURL string) string {
+	u, err := url.Parse(originalURL)
+	if err != nil {
+		return "<invalid URL>"
+	}
+
+	buf := bytes.NewBuffer(make([]byte, 0, len(originalURL)))
+
+	for i, queryPart := range bytes.Split([]byte(u.RawQuery), []byte("&")) {
+		if i != 0 {
+			buf.WriteByte(byte('&'))
+		}
+
+		splitParam := bytes.SplitN(queryPart, []byte("="), 2)
+
+		if len(splitParam) == 2 {
+			buf.Write(splitParam[0])
+			buf.WriteByte(byte('='))
+
+			if isParamSensitive(splitParam[0]) {
+				buf.Write([]byte("[FILTERED]"))
+			} else {
+				buf.Write(splitParam[1])
+			}
+		} else {
+			buf.Write(queryPart)
+		}
+	}
+	u.RawQuery = buf.String()
+	return u.String()
+}
+
+// Remember to keep in sync with Rails' filter_parameters
+
+var sensitiveRegexps = []*regexp.Regexp{
+	regexp.MustCompile(`token$`),
+	regexp.MustCompile(`key$`),
+	regexp.MustCompile(`(?i)(?:X\-AMZ\-)?Signature`),
+}
+
+// Not in regexp due to SA6004.
+// Not in string for performance.
+var sensitivePartialMatch = [][]byte{
+	[]byte("password"),
+	[]byte("secret"),
+}
+
+var sensitiveExactMatch = []string{
+	"certificate",
+	"hook",
+	"import_url",
+	"otp_attempt",
+	"sentry_dsn",
+	"trace",
+	"variables",
+	"content",
+}
+
+func isParamSensitive(name []byte) bool {
+	for _, s := range sensitiveExactMatch {
+		if string(name) == s {
+			return true
+		}
+	}
+	for _, r := range sensitiveRegexps {
+		if r.Match(name) {
+			return true
+		}
+	}
+	for _, s := range sensitivePartialMatch {
+		if bytes.Contains(name, s) {
+			return true
+		}
+	}
+	return false
 }

internal/helper/helpers_test.go

@@ -115,9 +115,12 @@ func TestScrubURLParams(t *testing.T) {
 	for before, expected := range map[string]string{
 		"http://example.com":                                                "http://example.com",
 		"http://example.com?foo=1":                                          "http://example.com?foo=1",
+		"http://example.com?title=token":                                    "http://example.com?title=token",
 		"http://example.com?authenticity_token=1":                           "http://example.com?authenticity_token=[FILTERED]",
 		"http://example.com?private_token=1":                                "http://example.com?private_token=[FILTERED]",
 		"http://example.com?rss_token=1":                                    "http://example.com?rss_token=[FILTERED]",
+		"http://example.com?access_token=1":                                 "http://example.com?access_token=[FILTERED]",
+		"http://example.com?refresh_token=1":                                "http://example.com?refresh_token=[FILTERED]",
 		"http://example.com?foo&authenticity_token=blahblah&bar":            "http://example.com?foo&authenticity_token=[FILTERED]&bar",
 		"http://example.com?private-token=1":                                "http://example.com?private-token=[FILTERED]",
 		"http://example.com?foo&private-token=blahblah&bar":                 "http://example.com?foo&private-token=[FILTERED]&bar",
@@ -128,10 +131,22 @@ func TestScrubURLParams(t *testing.T) {
 		"?private-token=foo&authenticity_token=bar":                         "?private-token=[FILTERED]&authenticity_token=[FILTERED]",
 		"?private_token=foo&authenticity-token=bar":                         "?private_token=[FILTERED]&authenticity-token=[FILTERED]",
 		"?X-AMZ-Signature=foo":                                              "?X-AMZ-Signature=[FILTERED]",
-		"&X-AMZ-Signature=foo":                                              "&X-AMZ-Signature=[FILTERED]",
 		"?x-amz-signature=foo":                                              "?x-amz-signature=[FILTERED]",
-		"&Signature=foo":                                                    "&Signature=[FILTERED]",
 		"?Signature=foo":                                                    "?Signature=[FILTERED]",
+		"?confirmation_password=foo":                                        "?confirmation_password=[FILTERED]",
+		"?pos_secret_number=foo":                                            "?pos_secret_number=[FILTERED]",
+		"?book_key=foo":                                                     "?book_key=[FILTERED]",
+		"?certificate=foo":                                                  "?certificate=[FILTERED]",
+		"?hook=foo":                                                         "?hook=[FILTERED]",
+		"?import_url=foo":                                                   "?import_url=[FILTERED]",
+		"?otp_attempt=foo":                                                  "?otp_attempt=[FILTERED]",
+		"?sentry_dsn=foo":                                                   "?sentry_dsn=[FILTERED]",
+		"?trace=foo":                                                        "?trace=[FILTERED]",
+		"?variables=foo":                                                    "?variables=[FILTERED]",
+		"?content=foo":                                                      "?content=[FILTERED]",
+		"?content=e=mc2":                                                    "?content=[FILTERED]",
+		"?formula=e=mc2":                                                    "?formula=e=mc2",
+		"http://%41:8080/":                                                  "<invalid URL>",
 	} {
 		after := ScrubURLParams(before)
 		assert.Equal(t, expected, after, "Scrubbing %q", before)

internal/helper/logging_test.go

@@ -23,9 +23,13 @@ func Test_statsCollectingResponseWriter_accessLogFields(t *testing.T) {
 		"private-token=%s",
 		"authenticity-token=%s",
 		"rss-token=%s",
+		"access-token=%s",
+		"refresh-token=%s",
 		"private_token=%s",
 		"authenticity_token=%s",
-		"rss-token=%s",
+		"rss_token=%s",
+		"access_token=%s",
+		"refresh_token=%s",
 		"private-token=%s&authenticity-token=%s",
 		"private_token=%s&authenticity_token=%s",
 		"param=not_private&private-token=%s", // Non-private fields prefixed