diff --git a/github/github.go b/github/github.go index 043b1dd88b1..ee974bf1339 100644 --- a/github/github.go +++ b/github/github.go @@ -1456,7 +1456,7 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro // If the error type is *url.Error, sanitize its URL before returning. if e, ok := errors.AsType[*url.Error](err); ok { if url, err := url.Parse(e.URL); err == nil { - e.URL = sanitizeURL(url).String() + e.URL = SanitizeURL(url).String() return response, e } } @@ -1787,7 +1787,7 @@ type ErrorBlock struct { func (r *ErrorResponse) Error() string { if r.Response != nil && r.Response.Request != nil { return fmt.Sprintf("%v %v: %v %v %+v", - r.Response.Request.Method, sanitizeURL(r.Response.Request.URL), + r.Response.Request.Method, SanitizeURL(r.Response.Request.URL), r.Response.StatusCode, r.Message, r.Errors) } @@ -1859,7 +1859,7 @@ type RateLimitError struct { func (r *RateLimitError) Error() string { return fmt.Sprintf("%v %v: %v %v %v", - r.Response.Request.Method, sanitizeURL(r.Response.Request.URL), + r.Response.Request.Method, SanitizeURL(r.Response.Request.URL), r.Response.StatusCode, r.Message, formatRateReset(time.Until(r.Rate.Reset.Time))) } @@ -1917,7 +1917,7 @@ func (r *AbuseRateLimitError) Error() string { retryInfo = fmt.Sprintf(" [retry after %v]", r.RetryAfter.Round(time.Second)) } return fmt.Sprintf("%v %v: %v %v%v", - r.Response.Request.Method, sanitizeURL(r.Response.Request.URL), + r.Response.Request.Method, SanitizeURL(r.Response.Request.URL), r.Response.StatusCode, r.Message, retryInfo) } @@ -1958,8 +1958,8 @@ type RedirectionError struct { func (r *RedirectionError) Error() string { return fmt.Sprintf("%v %v: %v location %v", - r.Response.Request.Method, sanitizeURL(r.Response.Request.URL), - r.StatusCode, sanitizeURL(r.Location)) + r.Response.Request.Method, SanitizeURL(r.Response.Request.URL), + r.StatusCode, SanitizeURL(r.Location)) } // Is returns whether the provided error equals this error. @@ -1976,13 +1976,16 @@ func (r *RedirectionError) Is(target error) bool { var sensitiveParams = []string{"client_secret", "access_token", "token"} -// sanitizeURL redacts sensitive parameters from the URL which may be -// exposed to the user. -func sanitizeURL(uri *url.URL) *url.URL { +// SanitizeURL returns a copy of uri with userinfo removed and sensitive query +// parameters redacted. It is safe to expose the result to users or to record +// it in telemetry because the input URL is not modified. +func SanitizeURL(uri *url.URL) *url.URL { if uri == nil { return nil } - params := uri.Query() + sanitized := *uri + sanitized.User = nil + params := sanitized.Query() var redacted bool for _, p := range sensitiveParams { if len(params.Get(p)) > 0 { @@ -1991,9 +1994,9 @@ func sanitizeURL(uri *url.URL) *url.URL { } } if redacted { - uri.RawQuery = params.Encode() + sanitized.RawQuery = params.Encode() } - return uri + return &sanitized } /* diff --git a/github/github_test.go b/github/github_test.go index 4d3b02edce3..465750ebf9e 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -3887,14 +3887,19 @@ func TestSanitizeURL(t *testing.T) { {"/?a=b&access_token=secret", "/?a=b&access_token=REDACTED"}, {"/?a=b&token=secret", "/?a=b&token=REDACTED"}, {"/?client_secret=s&access_token=t&token=u", "/?access_token=REDACTED&client_secret=REDACTED&token=REDACTED"}, + {"https://user:password@example.com/path?token=secret", "https://example.com/path?token=REDACTED"}, } for _, tt := range tests { inURL, _ := url.Parse(tt.in) want, _ := url.Parse(tt.want) + original := inURL.String() - if got := sanitizeURL(inURL); !cmp.Equal(got, want) { - t.Errorf("sanitizeURL(%v) returned %v, want %v", tt.in, got, want) + if got := SanitizeURL(inURL); !cmp.Equal(got, want) { + t.Errorf("SanitizeURL(%v) returned %v, want %v", tt.in, got, want) + } + if got := inURL.String(); got != original { + t.Errorf("SanitizeURL modified its input: got %v, want %v", got, original) } } } diff --git a/otel/transport.go b/otel/transport.go index e431dff3ba7..58ccc00aee1 100644 --- a/otel/transport.go +++ b/otel/transport.go @@ -61,7 +61,7 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { // Inject Attributes span.SetAttributes( attribute.String("http.method", req.Method), - attribute.String("http.url", req.URL.String()), + attribute.String("http.url", github.SanitizeURL(req.URL).String()), attribute.String("http.host", req.URL.Host), ) diff --git a/otel/transport_test.go b/otel/transport_test.go index 0164e95f002..36789dc343d 100644 --- a/otel/transport_test.go +++ b/otel/transport_test.go @@ -9,6 +9,8 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" "go.opentelemetry.io/otel" @@ -125,6 +127,68 @@ func TestRoundTrip_Spans(t *testing.T) { } } +func TestRoundTrip_RedactsSensitiveURL(t *testing.T) { + t.Parallel() + exporter := tracetest.NewInMemoryExporter() + tp := trace.NewTracerProvider(trace.WithSyncer(exporter)) + transport := NewTransport(&mockTransport{}, WithTracerProvider(tp)) + + req := httptest.NewRequest("GET", "https://alice:s3cr3t-password@api.github.com/repos/google/go-github?access_token=s3cr3t-access&client_secret=s3cr3t-client&token=s3cr3t-token&page=2", nil) + originalURL := req.URL.String() + if _, err := transport.RoundTrip(req); err != nil { + t.Fatalf("RoundTrip failed: %v", err) + } + if got := req.URL.String(); got != originalURL { + t.Errorf("RoundTrip modified the request URL: got %q, want %q", got, originalURL) + } + + spans := exporter.GetSpans() + if len(spans) != 1 { + t.Fatalf("Expected 1 span, got %v", len(spans)) + } + + var gotURL string + for _, a := range spans[0].Attributes { + if a.Key == "http.url" { + gotURL = a.Value.AsString() + } + } + if gotURL == "" { + t.Fatal("http.url attribute not recorded") + } + + for _, secret := range []string{"alice", "s3cr3t"} { + if strings.Contains(gotURL, secret) { + t.Errorf("http.url = %q, must not contain %q", gotURL, secret) + } + } + + u, err := url.Parse(gotURL) + if err != nil { + t.Fatalf("Parsing recorded http.url: %v", err) + } + if got, want := u.Scheme, "https"; got != want { + t.Errorf("URL scheme = %q, want %q", got, want) + } + if got, want := u.Host, "api.github.com"; got != want { + t.Errorf("URL host = %q, want %q", got, want) + } + if got, want := u.Path, "/repos/google/go-github"; got != want { + t.Errorf("URL path = %q, want %q", got, want) + } + if u.User != nil { + t.Errorf("URL userinfo = %v, want nil", u.User) + } + for _, p := range []string{"access_token", "client_secret", "token"} { + if got, want := u.Query().Get(p), "REDACTED"; got != want { + t.Errorf("Query parameter %q = %q, want %q", p, got, want) + } + } + if got, want := u.Query().Get("page"), "2"; got != want { + t.Errorf("Query parameter %q = %q, want %q", "page", got, want) + } +} + func TestRoundTrip_Error(t *testing.T) { t.Parallel() exporter := tracetest.NewInMemoryExporter()