From b788a1b1d395870dc8d338ef34cad410d32a087f Mon Sep 17 00:00:00 2001 From: alliasgher Date: Tue, 14 Apr 2026 02:55:30 +0500 Subject: [PATCH] client: add RedactQueryParams to hide query string from logs and errors Query parameters often carry credentials (bearer tokens, presigned-URL signatures, OAuth tokens, etc.). Today every request URL -- query string included -- is copied verbatim into debug logs and into returned errors ("giving up after N attempt(s)"). Users that can't swap the logger still leak the query from errors, and the returned error wraps the URL. Add a new bool `Client.RedactQueryParams` (default false, so existing behavior is preserved). When set to true, the query string and fragment are stripped before the URL is rendered in logs and errors. Userinfo passwords continue to be redacted to "xxxxx" regardless of the setting. Internally redactURL now delegates to a parameterized redactURLFor so the stripping policy can be plumbed through without changing any exported surface besides the new field. All call sites in Do go through the new Client.loggableURL helper. Closes #206 Signed-off-by: alliasgher --- client.go | 41 ++++++++++++++++++---- client_test.go | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 7 deletions(-) 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()) + } +}