Merge pull request #1132 from loadimpact/fix/1103-credentials-leak-me…

…trics

fix(httpext): remove user credentials from metric tags

js/modules/k6/http/request_test.go

@@ -751,17 +751,19 @@ func TestRequestAndBatch(t *testing.T) {
 		t.Run("auth", func(t *testing.T) {
 			t.Run("basic", func(t *testing.T) {
 				url := sr("http://bob:pass@HTTPBIN_IP:HTTPBIN_PORT/basic-auth/bob/pass")
+				urlExpected := sr("http://****:****@HTTPBIN_IP:HTTPBIN_PORT/basic-auth/bob/pass")
 
 				_, err := common.RunString(rt, fmt.Sprintf(`
 				let res = http.request("GET", "%s", null, {});
 				if (res.status != 200) { throw new Error("wrong status: " + res.status); }
 				`, url))
 				assert.NoError(t, err)
-				assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "GET", url, "", 200, "")
+				assertRequestMetricsEmitted(t, stats.GetBufferedSamples(samples), "GET", urlExpected, "", 200, "")
 			})
 			t.Run("digest", func(t *testing.T) {
 				t.Run("success", func(t *testing.T) {
 					url := sr("http://bob:pass@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/auth/bob/pass")
+					urlExpected := sr("http://****:****@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/auth/bob/pass")
 
 					_, err := common.RunString(rt, fmt.Sprintf(`
 					let res = http.request("GET", "%s", null, { auth: "digest" });
@@ -771,8 +773,10 @@ func TestRequestAndBatch(t *testing.T) {
 					assert.NoError(t, err)
 
 					sampleContainers := stats.GetBufferedSamples(samples)
-					assertRequestMetricsEmitted(t, sampleContainers[0:1], "GET", sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), url, 401, "")
-					assertRequestMetricsEmitted(t, sampleContainers[1:2], "GET", sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), url, 200, "")
+					assertRequestMetricsEmitted(t, sampleContainers[0:1], "GET",
+						sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), urlExpected, 401, "")
+					assertRequestMetricsEmitted(t, sampleContainers[1:2], "GET",
+						sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), urlExpected, 200, "")
 				})
 				t.Run("failure", func(t *testing.T) {
 					url := sr("http://bob:pass@HTTPBIN_IP:HTTPBIN_PORT/digest-auth/failure")
@@ -1911,7 +1915,6 @@ func TestDigestAuthWithBody(t *testing.T) {
 		httpbin.New().DigestAuth(w, r) // this doesn't read the body
 	}))
 
-	// TODO: fix, the metric tags shouldn't have credentials (https://github.com/loadimpact/k6/issues/1103)
 	urlWithCreds := tb.Replacer.Replace(
 		"http://testuser:testpwd@HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd",
 	)
@@ -1923,8 +1926,12 @@ func TestDigestAuthWithBody(t *testing.T) {
 	`, urlWithCreds))
 	require.NoError(t, err)
 
-	expectedURL := tb.Replacer.Replace("HTTPBIN_IP_URL/digest-auth-with-post/auth/testuser/testpwd")
+	expectedURL := tb.Replacer.Replace(
+		"http://HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd")
+	expectedName := tb.Replacer.Replace(
+		"http://****:****@HTTPBIN_IP:HTTPBIN_PORT/digest-auth-with-post/auth/testuser/testpwd")
+
 	sampleContainers := stats.GetBufferedSamples(samples)
-	assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", expectedURL, urlWithCreds, 401, "")
-	assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", expectedURL, urlWithCreds, 200, "")
+	assertRequestMetricsEmitted(t, sampleContainers[0:1], "POST", expectedURL, expectedName, 401, "")
+	assertRequestMetricsEmitted(t, sampleContainers[1:2], "POST", expectedURL, expectedName, 200, "")
 }

lib/netext/httpext/request.go

@@ -49,16 +49,43 @@ type HTTPRequestCookie struct {
 
 // A URL wraps net.URL, and preserves the template (if any) the URL was constructed from.
 type URL struct {
-	u    *url.URL
-	Name string // http://example.com/thing/${}/
-	URL  string // http://example.com/thing/1234/
+	u        *url.URL
+	Name     string // http://example.com/thing/${}/
+	URL      string // http://example.com/thing/1234/
+	CleanURL string // URL with masked user credentials, used for output
 }
 
 // NewURL returns a new URL for the provided url and name. The error is returned if the url provided
 // can't be parsed
 func NewURL(urlString, name string) (URL, error) {
 	u, err := url.Parse(urlString)
-	return URL{u: u, Name: name, URL: urlString}, err
+	newURL := URL{u: u, Name: name, URL: urlString}
+	newURL.CleanURL = newURL.Clean()
+	if urlString == name {
+		newURL.Name = newURL.CleanURL
+	}
+	return newURL, err
+}
+
+// Clean returns an output-safe representation of URL
+func (u URL) Clean() string {
+	if u.CleanURL != "" {
+		return u.CleanURL
+	}
+
+	out := u.URL
+
+	if u.u != nil && u.u.User != nil {
+		schemeIndex := strings.Index(out, "://")
+		atIndex := strings.Index(out, "@")
+		if _, passwordOk := u.u.User.Password(); passwordOk {
+			out = out[:schemeIndex+3] + "****:****" + out[atIndex:]
+		} else {
+			out = out[:schemeIndex+3] + "****" + out[atIndex:]
+		}
+	}
+
+	return out
 }
 
 // GetURL returns the internal url.URL
@@ -212,7 +239,7 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
 		tags["method"] = preq.Req.Method
 	}
 	if state.Options.SystemTags["url"] {
-		tags["url"] = preq.URL.URL
+		tags["url"] = preq.URL.Clean()
 	}
 
 	// Only set the name system tag if the user didn't explicitly set it beforehand

lib/netext/httpext/request_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"github.com/pkg/errors"
@@ -82,3 +83,31 @@ func TestMakeRequestError(t *testing.T) {
 		require.Equal(t, err.Error(), "unknown compressionType CompressionType(13)")
 	})
 }
+
+func TestURL(t *testing.T) {
+	t.Run("Clean", func(t *testing.T) {
+		testCases := []struct {
+			url      string
+			expected string
+		}{
+			{"https://example.com/", "https://example.com/"},
+			{"https://example.com/${}", "https://example.com/${}"},
+			{"https://user@example.com/", "https://****@example.com/"},
+			{"https://user:pass@example.com/", "https://****:****@example.com/"},
+			{"https://user:pass@example.com/path?a=1&b=2", "https://****:****@example.com/path?a=1&b=2"},
+			{"https://user:pass@example.com/${}/${}", "https://****:****@example.com/${}/${}"},
+			{"@malformed/url", "@malformed/url"},
+			{"not a url", "not a url"},
+		}
+
+		for _, tc := range testCases {
+			tc := tc
+			t.Run(tc.url, func(t *testing.T) {
+				u, err := url.Parse(tc.url)
+				require.NoError(t, err)
+				ut := URL{u: u, URL: tc.url}
+				require.Equal(t, tc.expected, ut.Clean())
+			})
+		}
+	})
+}

lib/netext/httpext/transport.go

@@ -113,7 +113,8 @@ func (t *transport) measureAndEmitMetrics(unfReq *unfinishedRequest) *finishedRe
 		}
 	} else {
 		if enabledTags["url"] {
-			tags["url"] = unfReq.request.URL.String()
+			u := URL{u: unfReq.request.URL, URL: unfReq.request.URL.String()}
+			tags["url"] = u.Clean()
 		}
 		if enabledTags["status"] {
 			tags["status"] = strconv.Itoa(unfReq.response.StatusCode)