diff --git a/client.go b/client.go index 91059f7..15b373c 100644 --- a/client.go +++ b/client.go @@ -432,6 +432,13 @@ type Client struct { // PrepareRetry can prepare the request for retry operation, for example re-sign it PrepareRetry PrepareRetry + // RedactQueryParams, when true, strips the query string from request URLs + // before they appear in logs and returned errors. This is useful because + // query parameters often carry credentials (tokens, signatures, etc.) and + // leaking them into error messages or log output can be a security hazard. + // Userinfo passwords are always redacted regardless of this setting. + RedactQueryParams bool + loggerInit sync.Once clientInit sync.Once } @@ -675,9 +682,9 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if logger != nil { switch v := logger.(type) { case LeveledLogger: - v.Debug("performing request", "method", req.Method, "url", redactURL(req.URL)) + v.Debug("performing request", "method", req.Method, "url", c.loggableURL(req.URL)) case Logger: - v.Printf("[DEBUG] %s %s", req.Method, redactURL(req.URL)) + v.Printf("[DEBUG] %s %s", req.Method, c.loggableURL(req.URL)) } } @@ -732,9 +739,9 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if err != nil { switch v := logger.(type) { case LeveledLogger: - v.Error("request failed", "error", err, "method", req.Method, "url", redactURL(req.URL)) + v.Error("request failed", "error", err, "method", req.Method, "url", c.loggableURL(req.URL)) case Logger: - v.Printf("[ERR] %s %s request failed: %v", req.Method, redactURL(req.URL), err) + v.Printf("[ERR] %s %s request failed: %v", req.Method, c.loggableURL(req.URL), err) } } else { // Call this here to maintain the behavior of logging all requests, @@ -770,7 +777,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { wait := c.Backoff(c.RetryWaitMin, c.RetryWaitMax, i, resp) if logger != nil { - desc := fmt.Sprintf("%s %s", req.Method, redactURL(req.URL)) + desc := fmt.Sprintf("%s %s", req.Method, c.loggableURL(req.URL)) if resp != nil { desc = fmt.Sprintf("%s (status: %d)", desc, resp.StatusCode) } @@ -835,11 +842,11 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // communicate why if err == nil { return nil, fmt.Errorf("%s %s giving up after %d attempt(s)", - req.Method, redactURL(req.URL), attempt) + req.Method, c.loggableURL(req.URL), attempt) } return nil, fmt.Errorf("%s %s giving up after %d attempt(s): %w", - req.Method, redactURL(req.URL), attempt, err) + req.Method, c.loggableURL(req.URL), attempt, err) } // Try to read the response body so we can reuse this connection. @@ -926,6 +933,15 @@ func (c *Client) StandardClient() *http.Client { // Taken from url.URL#Redacted() which was introduced in go 1.15. // We can switch to using it directly if we'll bump the minimum required go version. func redactURL(u *url.URL) string { + return redactURLFor(u, false) +} + +// redactURLFor produces a log- and error-safe rendering of u. Userinfo +// passwords are always redacted; when stripQuery is true the raw query and +// fragment are also stripped so that secrets embedded in query parameters +// (e.g. presigned-URL signatures, bearer tokens) don't leak through logs or +// returned errors. +func redactURLFor(u *url.URL, stripQuery bool) string { if u == nil { return "" } @@ -934,5 +950,16 @@ func redactURL(u *url.URL) string { if _, has := ru.User.Password(); has { ru.User = url.UserPassword(ru.User.Username(), "xxxxx") } + if stripQuery { + ru.RawQuery = "" + ru.Fragment = "" + ru.RawFragment = "" + } return ru.String() } + +// loggableURL returns the URL as it should appear in logs and error messages +// for this client, honoring Client.RedactQueryParams. +func (c *Client) loggableURL(u *url.URL) string { + return redactURLFor(u, c.RedactQueryParams) +} diff --git a/client_test.go b/client_test.go index 8c5c783..6237293 100644 --- a/client_test.go +++ b/client_test.go @@ -1403,3 +1403,96 @@ func TestClient_RedirectWithBody(t *testing.T) { t.Fatalf("Expected the client to be redirected 2 times, got: %d", atomic.LoadInt32(&redirects)) } } + +func TestRedactURLFor(t *testing.T) { + tests := []struct { + name string + raw string + stripQuery bool + want string + }{ + { + name: "plain_url_kept", + raw: "https://example.com/a", + stripQuery: false, + want: "https://example.com/a", + }, + { + name: "password_always_redacted", + raw: "https://user:secret@example.com/a", + stripQuery: false, + want: "https://user:xxxxx@example.com/a", + }, + { + name: "query_kept_when_strip_false", + raw: "https://example.com/a?token=hunter2", + stripQuery: false, + want: "https://example.com/a?token=hunter2", + }, + { + name: "query_stripped_when_strip_true", + raw: "https://example.com/a?token=hunter2", + stripQuery: true, + want: "https://example.com/a", + }, + { + name: "fragment_stripped_when_strip_true", + raw: "https://example.com/a?k=v#frag", + stripQuery: true, + want: "https://example.com/a", + }, + { + name: "password_and_query_both_redacted", + raw: "https://user:secret@example.com/a?token=hunter2", + stripQuery: true, + want: "https://user:xxxxx@example.com/a", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + u, err := url.Parse(tc.raw) + if err != nil { + t.Fatalf("parse: %v", err) + } + got := redactURLFor(u, tc.stripQuery) + if got != tc.want { + t.Fatalf("redactURLFor(%q, %v) = %q, want %q", tc.raw, tc.stripQuery, got, tc.want) + } + }) + } + + // Nil-safe path. + if got := redactURLFor(nil, true); got != "" { + t.Fatalf("redactURLFor(nil, true) = %q, want \"\"", got) + } +} + +func TestClient_RedactQueryParams_InError(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + + client := NewClient() + client.RetryWaitMin = 10 * time.Millisecond + client.RetryWaitMax = 10 * time.Millisecond + client.RetryMax = 1 + client.RedactQueryParams = true + + req, err := NewRequest("GET", ts.URL+"/api?token=supersecret", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + _, err = client.Do(req) + if err == nil { + t.Fatalf("expected an error after retries are exhausted") + } + if strings.Contains(err.Error(), "supersecret") { + t.Fatalf("error message leaked query secret: %q", err.Error()) + } + if strings.Contains(err.Error(), "token=") { + t.Fatalf("error message still contains query params: %q", err.Error()) + } +}