Keep request URL credentials out of HTTP errors

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:

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
 		}
 	}

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()
 }
 

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)
+				}
+			}
+		})
+	}
+}

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()
+}