From 53f6f188017078e726a6378f61e24e2a8d5c721b Mon Sep 17 00:00:00 2001 From: Ri-go Date: Fri, 18 Sep 2026 20:28:16 +0800 Subject: [PATCH] Keep request URL credentials out of HTTP errors --- internal/forwarder/broadcast.go | 8 +- internal/forwarder/hedge.go | 2 +- internal/forwarder/http.go | 14 +-- internal/forwarder/redaction_test.go | 156 +++++++++++++++++++++++++++ internal/log/errors.go | 117 ++++++++++++++++++++ internal/log/errors_test.go | 114 ++++++++++++++++++++ internal/server/cmt_rpc/server.go | 2 +- 7 files changed, 400 insertions(+), 13 deletions(-) create mode 100644 internal/forwarder/redaction_test.go create mode 100644 internal/log/errors.go create mode 100644 internal/log/errors_test.go diff --git a/internal/forwarder/broadcast.go b/internal/forwarder/broadcast.go index ffc9e7d..a474836 100644 --- a/internal/forwarder/broadcast.go +++ b/internal/forwarder/broadcast.go @@ -45,7 +45,7 @@ func (f *HTTP) Broadcast(w http.ResponseWriter, r *http.Request, key types.Route bodyBytes, err = io.ReadAll(r.Body) _ = r.Body.Close() if err != nil { - writeJSONError(w, http.StatusBadRequest, "read body: "+err.Error()) + writeJSONError(w, http.StatusBadRequest, "read body: "+log.ErrorMessage(err)) return } } @@ -196,12 +196,12 @@ func pickErrReport(failures []broadcastResult) string { // Prefer transport errors (most informative); fall back to first 5xx. for _, f := range failures { if f.err != nil && !errors.Is(f.err, context.Canceled) { - return fmt.Sprintf("backend %s: %v", f.backend, f.err) + return fmt.Sprintf("backend %s: %s", f.backend, log.ErrorMessage(f.err)) } } for _, f := range failures { if f.err != nil { - return fmt.Sprintf("backend %s: %v", f.backend, f.err) + return fmt.Sprintf("backend %s: %s", f.backend, log.ErrorMessage(f.err)) } } for _, f := range failures { @@ -217,7 +217,7 @@ func failureReports(failures []broadcastResult) []string { for _, f := range failures { switch { case f.err != nil: - out = append(out, fmt.Sprintf("%s: %v", f.backend, f.err)) + out = append(out, fmt.Sprintf("%s: %s", f.backend, log.ErrorMessage(f.err))) case f.resp != nil: out = append(out, fmt.Sprintf("%s: status %d", f.backend, f.resp.StatusCode)) default: diff --git a/internal/forwarder/hedge.go b/internal/forwarder/hedge.go index c17a7e6..90225b5 100644 --- a/internal/forwarder/hedge.go +++ b/internal/forwarder/hedge.go @@ -50,7 +50,7 @@ func (f *HTTP) Hedge(w http.ResponseWriter, r *http.Request, key types.RouteKey) bodyBytes, err = io.ReadAll(r.Body) _ = r.Body.Close() if err != nil { - writeJSONError(w, http.StatusBadRequest, "read body: "+err.Error()) + writeJSONError(w, http.StatusBadRequest, "read body: "+log.ErrorMessage(err)) return } } diff --git a/internal/forwarder/http.go b/internal/forwarder/http.go index 589a244..4825ffa 100644 --- a/internal/forwarder/http.go +++ b/internal/forwarder/http.go @@ -63,7 +63,7 @@ type ErrAllAttemptsFailed struct { } func (e *ErrAllAttemptsFailed) Error() string { - return fmt.Sprintf("all %d attempts failed: %v", e.Attempts, e.Last) + return fmt.Sprintf("all %d attempts failed: %s", e.Attempts, log.ErrorMessage(e.Last)) } func (e *ErrAllAttemptsFailed) Unwrap() error { return e.Last } @@ -86,7 +86,7 @@ func (f *HTTP) Forward(w http.ResponseWriter, r *http.Request, key types.RouteKe bodyBytes, err = io.ReadAll(r.Body) _ = r.Body.Close() if err != nil { - writeJSONError(w, http.StatusBadRequest, "read body: "+err.Error()) + writeJSONError(w, http.StatusBadRequest, "read body: "+log.ErrorMessage(err)) return } } @@ -165,7 +165,7 @@ func (f *HTTP) Forward(w http.ResponseWriter, r *http.Request, key types.RouteKe metrics.FailoverAttempts.WithLabelValues(b.Name, "next", classifyErr(err)).Inc() log.FromCtx(r.Context()).Warn("upstream attempt failed", "backend", b.Name, - "err", err.Error(), + "err", log.ErrorMessage(err), "attempt", attempts, ) continue @@ -213,7 +213,7 @@ func (f *HTTP) Forward(w http.ResponseWriter, r *http.Request, key types.RouteKe metrics.RequestDuration.WithLabelValues(string(key.Protocol), key.Class.String(), b.Name).Observe(dur.Seconds()) log.FromCtx(r.Context()).Warn("upstream body truncated mid-relay", "backend", b.Name, - "err", body.err.Error(), + "err", log.ErrorMessage(body.err), "attempt", attempts, ) return @@ -227,7 +227,7 @@ func (f *HTTP) Forward(w http.ResponseWriter, r *http.Request, key types.RouteKe metrics.RequestDuration.WithLabelValues(string(key.Protocol), key.Class.String(), b.Name).Observe(dur.Seconds()) log.FromCtx(r.Context()).Debug("client write failed mid-relay", "backend", b.Name, - "err", copyErr.Error(), + "err", log.ErrorMessage(copyErr), ) return } @@ -250,9 +250,9 @@ func (f *HTTP) Forward(w http.ResponseWriter, r *http.Request, key types.RouteKe } log.FromCtx(r.Context()).Error("all upstream attempts failed", "attempts", attempts, - "err", lastErr.Error(), + "err", log.ErrorMessage(lastErr), ) - writeJSONError(w, http.StatusBadGateway, lastErr.Error()) + writeJSONError(w, http.StatusBadGateway, log.ErrorMessage(lastErr)) metrics.RequestsTotal.WithLabelValues(string(key.Protocol), key.Class.String(), "-", "all_failed").Inc() } diff --git a/internal/forwarder/redaction_test.go b/internal/forwarder/redaction_test.go new file mode 100644 index 0000000..090b9c3 --- /dev/null +++ b/internal/forwarder/redaction_test.go @@ -0,0 +1,156 @@ +package forwarder + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "testing" + "time" + + "github.com/InjectiveLabs/stitch/internal/backend" + "github.com/InjectiveLabs/stitch/internal/log" + "github.com/InjectiveLabs/stitch/internal/types" +) + +type forwardCall func(*HTTP, http.ResponseWriter, *http.Request, types.RouteKey) + +var redactionForwardCalls = map[string]forwardCall{ + "ordinary": (*HTTP).Forward, + "hedge": (*HTTP).Hedge, + "broadcast": (*HTTP).Broadcast, +} + +func TestForwardErrorsDoNotExposeURLCredentials(t *testing.T) { + const query = "height=75&api_key=QuerySecret&other=OtherSecret" + const endpoint = "http://PrivateUser:PrivatePassword@archive.example" //nolint:gosec // Synthetic credentials used to test redaction. + for name, call := range redactionForwardCalls { + t.Run(name, func(t *testing.T) { + var logs bytes.Buffer + if err := log.Init("debug", "json", &logs); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = log.Init("info", "text", os.Stderr) }) + candidates := []*backend.Backend{mkBackend("first", endpoint), mkBackend("second", endpoint)} + fwd := newHedgeForwarder(stubSelector{cands: candidates}, time.Millisecond) + for _, candidate := range candidates { + transport := fwd.pool.Transport(candidate.Name) + transport.Proxy = nil + transport.DialContext = func(context.Context, string, string) (net.Conn, error) { + // The inner diagnostic repeats the URL, in addition to the + // url.Error added by http.Client.Do. + return nil, fmt.Errorf("redirect %s/status?%s: %w", endpoint, query, context.DeadlineExceeded) + } + } + r := httptest.NewRequest(http.MethodGet, "/status?"+query, nil) + w := httptest.NewRecorder() + call(fwd, w, r, types.RouteKey{Protocol: types.ProtoRPC, Method: "status", Idempotent: true}) + if w.Code != http.StatusBadGateway { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + for source, text := range map[string]string{"response": w.Body.String(), "log": logs.String()} { + for _, secret := range []string{"PrivateUser", "PrivatePassword", "QuerySecret", "OtherSecret", "api_key", "height="} { + if strings.Contains(text, secret) { + t.Fatalf("%s leaked %q: %s", source, secret, text) + } + } + for _, diagnostic := range []string{"archive.example/status", "context deadline exceeded"} { + if !strings.Contains(text, diagnostic) { + t.Errorf("%s lost diagnostic %q: %s", source, diagnostic, text) + } + } + } + if r.URL.RawQuery != query { + t.Fatal("request query changed") + } + }) + } +} + +func TestForwardingPreservesQueryAndUpstreamCredentials(t *testing.T) { + const query = "height=75&api_key=QuerySecret&filter=a%2Bb&duplicate=1&duplicate=2" + for name, call := range redactionForwardCalls { + t.Run(name, func(t *testing.T) { + seen := make(chan string, 2) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + user, password, ok := r.BasicAuth() + if !ok || user != "UpstreamUser" || password != "UpstreamPassword" { + t.Error("upstream credentials were changed") + } + seen <- r.URL.RawQuery + _, _ = w.Write([]byte("ok")) + })) + defer upstream.Close() + endpoint := strings.Replace(upstream.URL, "://", "://UpstreamUser:UpstreamPassword@", 1) + fwd := newHedgeForwarder(stubSelector{cands: []*backend.Backend{ + mkBackend("first", endpoint), mkBackend("second", endpoint), + }}, time.Millisecond) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/status?"+query, nil) + call(fwd, w, r, types.RouteKey{Protocol: types.ProtoRPC, Method: "status", Idempotent: true}) + if w.Code != http.StatusOK || w.Body.String() != "ok" { + t.Fatalf("status=%d body=%s", w.Code, w.Body.String()) + } + if got := <-seen; got != query { + t.Fatalf("forwarded query %q, want %q", got, query) + } + }) + } +} + +func TestRedactionPreservesErrorClassification(t *testing.T) { + for _, cause := range []error{context.Canceled, context.DeadlineExceeded, io.ErrUnexpectedEOF} { + err := &url.Error{Op: "Get", URL: "http://archive.example/?api_key=Secret", Err: cause} + wrapped := &ErrAllAttemptsFailed{Attempts: 2, Last: err} + before := classifyErr(wrapped) + if strings.Contains(wrapped.Error(), "Secret") { + t.Fatal("aggregate error leaked a query") + } + _ = log.ErrorMessage(wrapped) + var original *url.Error + if classifyErr(wrapped) != before || !errors.Is(wrapped, cause) || !errors.As(wrapped, &original) || original != err { + t.Fatal("diagnostic formatting changed error classification or identity") + } + } +} + +func TestMalformedRedirectErrorsDoNotExposeQuery(t *testing.T) { + for name, call := range redactionForwardCalls { + t.Run(name, func(t *testing.T) { + var logs bytes.Buffer + if err := log.Init("debug", "json", &logs); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = log.Init("info", "text", os.Stderr) }) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", "/%zz?"+r.URL.RawQuery) + w.WriteHeader(http.StatusFound) + })) + defer upstream.Close() + fwd := newHedgeForwarder(stubSelector{cands: []*backend.Backend{ + mkBackend("first", upstream.URL), mkBackend("second", upstream.URL), + }}, time.Millisecond) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/status?api_key=QuerySecret", nil) + call(fwd, w, r, types.RouteKey{Protocol: types.ProtoRPC, Method: "status", Idempotent: true}) + if w.Code != http.StatusBadGateway { + t.Fatalf("status=%d body=%s", w.Code, w.Body.String()) + } + for source, text := range map[string]string{"response": w.Body.String(), "log": logs.String()} { + if strings.Contains(text, "QuerySecret") || strings.Contains(text, "api_key") { + t.Fatalf("%s leaked redirect query: %s", source, text) + } + if !strings.Contains(text, "failed to parse Location") { + t.Fatalf("%s lost parse diagnosis: %s", source, text) + } + } + }) + } +} diff --git a/internal/log/errors.go b/internal/log/errors.go new file mode 100644 index 0000000..67991c7 --- /dev/null +++ b/internal/log/errors.go @@ -0,0 +1,117 @@ +package log + +import ( + "fmt" + "net/url" + "regexp" + "sort" + "strconv" + "strings" +) + +// ErrorMessage formats an error for logs or client responses without URL +// credentials, query strings or fragments. It does not change the original +// error, so callers can still classify it with errors.Is and errors.As. +func ErrorMessage(err error) string { + // Wrappers can copy a relative or malformed URL into their own text. + // Collect the structured values before rendering so those copies cannot + // evade redaction, including Go-quoted copies containing escapes. + var redactions []urlRedaction + collectErrorURLs(err, &redactions) + // A short URL can prefix a longer one. Replacing it first could expose + // the longer URL's remaining query parameters as ordinary error text. + sort.SliceStable(redactions, func(i, j int) bool { return len(redactions[i].raw) > len(redactions[j].raw) }) + replacements := make([]string, 0, 2*len(redactions)) + for _, r := range redactions { + replacements = append(replacements, r.raw, r.safe) + } + return errorMessage(err, strings.NewReplacer(replacements...)) +} + +func errorMessage(err error, urls *strings.Replacer) string { + if err == nil { + return "" + } + var message string + switch e := err.(type) { + case *url.Error: + // Format the structured URL separately: it can be relative, malformed, + // or repeated inside an underlying transport error. + message = fmt.Sprintf("%s %q: %s", e.Op, diagnosticURL(e.URL), errorMessage(e.Err, urls)) + case interface{ Unwrap() []error }: + message = err.Error() + for _, inner := range e.Unwrap() { + message = replaceInnerError(message, inner, urls) + } + case interface{ Unwrap() error }: + message = replaceInnerError(err.Error(), e.Unwrap(), urls) + default: + message = err.Error() + } + // Wrapping errors may copy a URL into their own message, and transports + // may include a second URL in an unstructured cause. Redact those too. + message = urls.Replace(message) + // net/http flattens redirect Location parse errors into ordinary text. + // Their quoted URL can be relative and absent from the structured tree. + message = diagnosticQuotedPattern.ReplaceAllStringFunc(message, func(quoted string) string { + raw, err := strconv.Unquote(quoted) + if err != nil || (!strings.ContainsAny(raw, "?#") && !strings.Contains(raw, "://") && !strings.HasPrefix(raw, "/")) { + return quoted + } + return strconv.Quote(diagnosticURL(raw)) + }) + return diagnosticURLPattern.ReplaceAllStringFunc(message, diagnosticURLToken) +} + +func replaceInnerError(message string, inner error, urls *strings.Replacer) string { + if inner == nil || inner.Error() == "" { + return message + } + return strings.ReplaceAll(message, inner.Error(), errorMessage(inner, urls)) +} + +type urlRedaction struct{ raw, safe string } + +func collectErrorURLs(err error, redactions *[]urlRedaction) { + if err == nil { + return + } + if e, ok := err.(*url.Error); ok && e.URL != "" { + safe := diagnosticURL(e.URL) + *redactions = append(*redactions, + urlRedaction{strconv.Quote(e.URL), strconv.Quote(safe)}, + urlRedaction{strconv.QuoteToASCII(e.URL), strconv.Quote(safe)}, + urlRedaction{e.URL, safe}) + } + switch e := err.(type) { + case interface{ Unwrap() []error }: + for _, inner := range e.Unwrap() { + collectErrorURLs(inner, redactions) + } + case interface{ Unwrap() error }: + collectErrorURLs(e.Unwrap(), redactions) + } +} + +var diagnosticURLPattern = regexp.MustCompile(`[A-Za-z][A-Za-z0-9+.-]*://[^\s<>]+`) +var diagnosticQuotedPattern = regexp.MustCompile(`"(?:[^"\\]|\\.)*"`) + +func diagnosticURLToken(raw string) string { + // Keep error-message punctuation around an unstructured URL. Quotes and + // backslashes inside the query remain part of the URL being discarded. + trimmed := strings.TrimRight(raw, `"'\),.;:!?`) + return diagnosticURL(trimmed) + raw[len(trimmed):] +} + +func diagnosticURL(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Opaque != "" { + return "[redacted URL]" + } + u.User = nil + u.RawQuery = "" + u.ForceQuery = false + u.Fragment = "" + u.RawFragment = "" + return u.String() +} diff --git a/internal/log/errors_test.go b/internal/log/errors_test.go new file mode 100644 index 0000000..77571e1 --- /dev/null +++ b/internal/log/errors_test.go @@ -0,0 +1,114 @@ +package log + +import ( + "context" + "errors" + "fmt" + "net/url" + "strings" + "testing" +) + +func TestErrorMessageRedactsURLCredentials(t *testing.T) { + const raw = "https://private-user:private-pass@archive.example/status?api_key=secret-key&height=75#private-fragment" //nolint:gosec // Synthetic credentials used to test redaction. + innerURL := "https://other-user:other-pass@other.example/redirect?token=other-key" //nolint:gosec // Synthetic credentials used to test redaction. + transport := &url.Error{Op: "Get", URL: raw, Err: context.DeadlineExceeded} + tests := []struct { + name string + err error + want string + }{ + {"transport", transport, `Get "https://archive.example/status": context deadline exceeded`}, + {"wrapped", fmt.Errorf("retry exhausted: %w", transport), `retry exhausted: Get "https://archive.example/status": context deadline exceeded`}, + {"nested", &url.Error{Op: "Post", URL: innerURL, Err: transport}, `Post "https://other.example/redirect": Get "https://archive.example/status": context deadline exceeded`}, + {"joined", errors.Join(errors.New("first candidate failed"), transport), "first candidate failed\nGet \"https://archive.example/status\": context deadline exceeded"}, + {"inner repeats URL", &url.Error{Op: "Get", URL: raw, Err: fmt.Errorf("redirect from %s to %s failed", raw, innerURL)}, `Get "https://archive.example/status": redirect from https://archive.example/status to https://other.example/redirect failed`}, + {"wrapper repeats URL", fmt.Errorf("request %s: %w", innerURL, transport), `request https://other.example/redirect: Get "https://archive.example/status": context deadline exceeded`}, + {"relative", &url.Error{Op: "Get", URL: "/status?api_key=secret-key", Err: errors.New("unsupported protocol")}, `Get "/status": unsupported protocol`}, + {"malformed", &url.Error{Op: "parse", URL: "https://private-user:private-pass@%zz/?api_key=secret-key", Err: errors.New("invalid URL escape")}, `parse "[redacted URL]": invalid URL escape`}, + {"opaque", &url.Error{Op: "parse", URL: "http:private-user:private-pass?api_key=secret-key", Err: errors.New("invalid URL")}, `parse "[redacted URL]": invalid URL`}, + {"ordinary", errors.New("connection reset by peer"), "connection reset by peer"}, + {"nil", nil, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ErrorMessage(tt.err) + for _, secret := range []string{"private-user", "private-pass", "secret-key", "private-fragment", "other-user", "other-pass", "other-key", "api_key", "height=", "token="} { + if strings.Contains(got, secret) { + t.Fatalf("credential or query leaked: %q", got) + } + } + if got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } + if transport.URL != raw || transport.Err != context.DeadlineExceeded { + t.Fatal("formatting changed the original error") + } + if !errors.Is(transport, context.DeadlineExceeded) { + t.Fatal("timeout classification changed") + } +} + +func TestErrorMessageRedactsCopiedAndQuotedURLs(t *testing.T) { + for _, raw := range []string{ + "/status?api_key=Secret", + `https://archive.example/status?api_key=before"Secret`, + `https://archive.example/status?api_key=before\Secret`, + "https://archive.example/status?first=before\napi_key=Secret", + } { + for _, format := range []string{"request %s: %w", "request %q: %w", "request %+q: %w"} { + t.Run(fmt.Sprintf("%q/%s", raw, format), func(t *testing.T) { + inner := &url.Error{Op: "Get", URL: raw, Err: context.DeadlineExceeded} + got := ErrorMessage(fmt.Errorf(format, raw, inner)) + if strings.Contains(got, "Secret") || strings.Contains(got, "api_key") { + t.Fatalf("copied URL leaked: %q", got) + } + }) + } + } +} + +func TestErrorMessageRedactsURLsRepeatedByUnstructuredCause(t *testing.T) { + for _, raw := range []string{ + `https://other.example/status?api_key=before"Secret`, + `https://other.example/status?api_key=before\Secret`, + } { + for _, format := range []string{"redirect %s failed", "redirect %q failed"} { + cause := fmt.Errorf(format, raw) + err := &url.Error{Op: "Get", URL: "https://archive.example/status?api_key=OuterSecret", Err: cause} + got := ErrorMessage(err) + if strings.Contains(got, "Secret") || strings.Contains(got, "api_key") { + t.Fatalf("underlying URL leaked: %q", got) + } + } + } +} + +func TestErrorMessageRedactsOverlappingURLs(t *testing.T) { + short := "https://archive.example/status?api_key=Secret" + long := short + "&token=OtherSecret" + inner := &url.Error{Op: "Get", URL: long, Err: context.DeadlineExceeded} + outer := &url.Error{Op: "Get", URL: short, Err: fmt.Errorf("redirect %s: %w", long, inner)} + for _, err := range []error{outer, errors.Join(outer, inner), fmt.Errorf("request %q: %w", long, outer)} { + got := ErrorMessage(err) + if strings.Contains(got, "Secret") || strings.Contains(got, "token=") || strings.Contains(got, "api_key") { + t.Fatalf("overlapping URL leaked: %q", got) + } + } +} + +func TestErrorMessageRedactsFlattenedRelativeRedirect(t *testing.T) { + for _, location := range []string{"/%zz?api_key=Secret", "%zz?api_key=Secret", "?api_key=Secret#%zz", "/%zz#Secret"} { + cause := fmt.Errorf("failed to parse Location header %q: parse %q: invalid URL escape %%zz", location, location) + err := &url.Error{Op: "Get", URL: "http://archive.example/status", Err: cause} + got := ErrorMessage(err) + if strings.Contains(got, "Secret") || strings.Contains(got, "api_key") { + t.Fatalf("flattened redirect URL leaked: %q", got) + } + if !strings.Contains(got, "invalid URL escape %zz") { + t.Fatalf("parse diagnosis lost: %q", got) + } + } +} diff --git a/internal/server/cmt_rpc/server.go b/internal/server/cmt_rpc/server.go index 7281573..daed04d 100644 --- a/internal/server/cmt_rpc/server.go +++ b/internal/server/cmt_rpc/server.go @@ -91,7 +91,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { d, err := decode(r) if err != nil { - writeJSONRPCError(w, http.StatusBadRequest, err.Error()) + writeJSONRPCError(w, http.StatusBadRequest, log.ErrorMessage(err)) return } ctx = log.WithMethod(ctx, d.key.Method)