fix(mcp): redact API query traces

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9ff1e38a-b018-4cf7-a5e9-5044a2efd03c
AchoArnold · Sep 3, 2026 · Sep 3, 2026 · Sep 3, 2026 · Sep 3, 2026 · Sep 3, 2026
commit 5e4d7fcfc9d4ca4903d868315f8f9584aeb8cb6d
diff --git a/mcp/internal/httpsms/client.go b/mcp/internal/httpsms/client.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -34,6 +35,24 @@ const (
 	// top of whatever deadline the caller's context already carries.
 	requestTimeout = 15 * time.Second
 
+	// dialTimeout bounds how long TCP connection establishment (DNS
+	// resolution plus connect) may take for a single dial, independently
+	// of the overall requestTimeout, so a slow or black-holed network path
+	// fails fast instead of consuming the whole request budget on dialing
+	// alone.
+	dialTimeout = 5 * time.Second
+
+	// tlsHandshakeTimeout bounds how long the TLS handshake may take once a
+	// TCP connection is established.
+	tlsHandshakeTimeout = 5 * time.Second
+
+	// responseHeaderTimeout bounds how long this client waits for the
+	// response status line and headers after the request (including its
+	// body, if any) has been fully written, so a server that accepts a
+	// connection but never responds cannot hold a call open until the
+	// overall requestTimeout.
+	responseHeaderTimeout = 10 * time.Second
+
 	maxIdleConns        = 100
 	maxIdleConnsPerHost = 10
 	idleConnTimeout     = 90 * time.Second
@@ -66,40 +85,49 @@ type Client interface {
 	RotateUserAPIKey(ctx context.Context, token string, userID string) (User, error)
 }
 
-// client is the Client implementation calling the httpSMS HTTP API.
-type client struct {
+// HTTPClient is the Client implementation calling the httpSMS HTTP API.
+type HTTPClient struct {
 	baseURL    string
 	httpClient *http.Client
 }
 
-var _ Client = (*client)(nil)
+var _ Client = (*HTTPClient)(nil)
 
-// NewClient returns a Client calling baseURL (for example
+// NewClient returns an *HTTPClient calling baseURL (for example
 // "https://api.httpsms.com"). The returned client is bounded and makes a
-// single attempt per call: an explicit request timeout, a size-limited
-// connection pool, OpenTelemetry context propagation through
-// otelhttp.Transport, and no automatic retries. Retrying automatically
-// would risk duplicating the side effect of a non-idempotent call such as
-// sending an SMS, creating a phone API key, or rotating the user's primary
-// API key.
-func NewClient(baseURL string) *client {
+// single attempt per call: an explicit overall request timeout plus
+// separate dial, TLS handshake, and response header timeouts, a
+// size-limited connection pool, OpenTelemetry context propagation through
+// otelhttp.Transport (with query string values redacted from span
+// attributes; see queryRedactingTransport), and no automatic retries.
+// Retrying automatically would risk duplicating the side effect of a
+// non-idempotent call such as sending an SMS, creating a phone API key, or
+// rotating the user's primary API key.
+func NewClient(baseURL string) *HTTPClient {
 	transport := &http.Transport{
-		MaxIdleConns:        maxIdleConns,
-		MaxIdleConnsPerHost: maxIdleConnsPerHost,
-		IdleConnTimeout:     idleConnTimeout,
+		MaxIdleConns:          maxIdleConns,
+		MaxIdleConnsPerHost:   maxIdleConnsPerHost,
+		IdleConnTimeout:       idleConnTimeout,
+		TLSHandshakeTimeout:   tlsHandshakeTimeout,
+		ResponseHeaderTimeout: responseHeaderTimeout,
+		DialContext: (&net.Dialer{
+			Timeout: dialTimeout,
+		}).DialContext,
 	}
 
-	return &client{
+	instrumented := otelhttp.NewTransport(&queryRestoringTransport{base: transport})
+
+	return &HTTPClient{
 		baseURL: strings.TrimRight(baseURL, "/"),
 		httpClient: &http.Client{
 			Timeout:   requestTimeout,
-			Transport: otelhttp.NewTransport(transport),
+			Transport: &queryRedactingTransport{next: instrumented},
 		},
 	}
 }
 
 // ListPhones calls GET /v1/phones.
-func (c *client) ListPhones(ctx context.Context, token string, params ListPhonesParams) ([]Phone, error) {
+func (c *HTTPClient) ListPhones(ctx context.Context, token string, params ListPhonesParams) ([]Phone, error) {
 	query := url.Values{}
 	setIntIfPositive(query, "skip", params.Skip)
 	setStringIfNotEmpty(query, "query", params.Query)
@@ -126,7 +154,7 @@ type messageSendRequest struct {
 }
 
 // SendSMS calls POST /v1/messages/send.
-func (c *client) SendSMS(ctx context.Context, token string, params SendSMSParams) (Message, error) {
+func (c *HTTPClient) SendSMS(ctx context.Context, token string, params SendSMSParams) (Message, error) {
 	body := messageSendRequest{
 		From:        params.From,
 		To:          params.To,
@@ -145,7 +173,7 @@ func (c *client) SendSMS(ctx context.Context, token string, params SendSMSParams
 }
 
 // ListMessageThreads calls GET /v1/message-threads.
-func (c *client) ListMessageThreads(ctx context.Context, token string, params ListMessageThreadsParams) ([]MessageThread, error) {
+func (c *HTTPClient) ListMessageThreads(ctx context.Context, token string, params ListMessageThreadsParams) ([]MessageThread, error) {
 	query := url.Values{}
 	setStringIfNotEmpty(query, "owner", params.Owner)
 	setBoolPointer(query, "is_archived", params.IsArchived)
@@ -162,7 +190,7 @@ func (c *client) ListMessageThreads(ctx context.Context, token string, params Li
 }
 
 // ListThreadMessages calls GET /v1/messages.
-func (c *client) ListThreadMessages(ctx context.Context, token string, params ListThreadMessagesParams) ([]Message, error) {
+func (c *HTTPClient) ListThreadMessages(ctx context.Context, token string, params ListThreadMessagesParams) ([]Message, error) {
 	query := url.Values{}
 	setStringIfNotEmpty(query, "owner", params.Owner)
 	setStringIfNotEmpty(query, "contact", params.Contact)
@@ -178,7 +206,7 @@ func (c *client) ListThreadMessages(ctx context.Context, token string, params Li
 }
 
 // ListIncomingMessages calls GET /v1/messages/incoming.
-func (c *client) ListIncomingMessages(ctx context.Context, token string, params ListIncomingMessagesParams) ([]Message, error) {
+func (c *HTTPClient) ListIncomingMessages(ctx context.Context, token string, params ListIncomingMessagesParams) ([]Message, error) {
 	query := url.Values{}
 	setRepeated(query, "owners", params.Owners)
 	setRepeated(query, "statuses", params.Statuses)
@@ -204,7 +232,7 @@ type phoneAPIKeyStoreRequest struct {
 }
 
 // CreatePhoneAPIKey calls POST /v1/phone-api-keys.
-func (c *client) CreatePhoneAPIKey(ctx context.Context, token string, params CreatePhoneAPIKeyParams) (PhoneAPIKey, error) {
+func (c *HTTPClient) CreatePhoneAPIKey(ctx context.Context, token string, params CreatePhoneAPIKeyParams) (PhoneAPIKey, error) {
 	body := phoneAPIKeyStoreRequest{Name: params.Name}
 
 	var key PhoneAPIKey
@@ -217,7 +245,7 @@ func (c *client) CreatePhoneAPIKey(ctx context.Context, token string, params Cre
 // RotateUserAPIKey calls DELETE /v1/users/{userID}/api-keys. userID is
 // always the authenticated subject's own Firebase UID; callers must never
 // accept it as untrusted tool input.
-func (c *client) RotateUserAPIKey(ctx context.Context, token string, userID string) (User, error) {
+func (c *HTTPClient) RotateUserAPIKey(ctx context.Context, token string, userID string) (User, error) {
 	path := "/v1/users/" + url.PathEscape(userID) + "/api-keys"
 
 	var user User
@@ -239,7 +267,7 @@ func (c *client) RotateUserAPIKey(ctx context.Context, token string, userID stri
 // it for non-idempotent operations (sending an SMS, creating a phone API
 // key, rotating the primary API key) without risking a duplicated side
 // effect from a transport-level retry.
-func (c *client) do(
+func (c *HTTPClient) do(
 	ctx context.Context,
 	token string,
 	method string,

diff --git a/mcp/internal/httpsms/client_test.go b/mcp/internal/httpsms/client_test.go
@@ -13,6 +13,10 @@ import (
 	"github.com/NdoleStudio/httpsms/mcp/internal/httpsms"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/propagation"
+	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 )
 
 // newTestServer starts an httptest.Server that runs assert (given the
@@ -424,6 +428,110 @@ func TestClient_PropagatesContextCancellation(t *testing.T) {
 	require.Error(t, err)
 }
 
+// TestNewClient_ReturnsAnExportedConcreteType is a compile-time assertion
+// that NewClient's declared return type is the exported *httpsms.HTTPClient
+// (not an unexported type), while *HTTPClient still satisfies Client. An
+// exported func returning an unexported type is a lint finding (the caller
+// cannot name the type, e.g. to embed it or declare a variable of it); this
+// would fail to compile if NewClient's signature regressed to an unexported
+// return type.
+func TestNewClient_ReturnsAnExportedConcreteType(t *testing.T) {
+	var typed *httpsms.HTTPClient = httpsms.NewClient("https://example.invalid")
+	var _ httpsms.Client = typed
+
+	assert.NotNil(t, typed)
+}
+
+// TestClient_RedactsQueryValuesFromOTelSpanAttributes is the regression
+// test for the critical review finding: query string values (which can
+// carry SMS content via the free-text "query" search filter, phone
+// numbers, or other sensitive filter values) must never be recorded as
+// OpenTelemetry span attributes, even though the real, unmodified query
+// string must still reach the httpSMS API on the wire and trace-context
+// propagation headers must still be injected.
+//
+// It uses an in-memory OTel span exporter to inspect every attribute of
+// every recorded span for a unique marker value used only as the "query"
+// filter, while independently capturing the raw query string the httptest
+// server actually received on the wire.
+func TestClient_RedactsQueryValuesFromOTelSpanAttributes(t *testing.T) {
+	const uniqueQueryValue = "otel-redaction-probe-4b9f9e6c-secret-sms-content"
+
+	var (
+		receivedRawQuery    string
+		receivedTraceparent string
+	)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		receivedRawQuery = r.URL.RawQuery
+		receivedTraceparent = r.Header.Get("Traceparent")
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		_ = json.NewEncoder(w).Encode(httpsms.Response[[]httpsms.Phone]{Status: "success", Data: []httpsms.Phone{}})
+	}))
+	t.Cleanup(server.Close)
+
+	exporter := tracetest.NewInMemoryExporter()
+	tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter))
+	t.Cleanup(func() { _ = tp.Shutdown(context.Background()) })
+
+	previousTracerProvider := otel.GetTracerProvider()
+	otel.SetTracerProvider(tp)
+	t.Cleanup(func() { otel.SetTracerProvider(previousTracerProvider) })
+
+	// The mcp binary's observability package registers a global W3C
+	// (tracecontext + baggage) propagator at startup (see
+	// internal/observability.New); replicate that here so this test
+	// exercises the same propagation path production traffic uses.
+	previousPropagator := otel.GetTextMapPropagator()
+	otel.SetTextMapPropagator(propagation.TraceContext{})
+	t.Cleanup(func() { otel.SetTextMapPropagator(previousPropagator) })
+
+	client := httpsms.NewClient(server.URL)
+	_, err := client.ListPhones(t.Context(), "token", httpsms.ListPhonesParams{Query: uniqueQueryValue, Limit: 10})
+	require.NoError(t, err)
+
+	// The real network request must still carry the unredacted query and a
+	// propagated trace context: redaction must be a span-attribute-only
+	// concern, not a change to what is actually sent over the wire.
+	assert.Contains(t, receivedRawQuery, uniqueQueryValue, "the httptest server must still receive the real, unredacted query")
+	assert.NotEmpty(t, receivedTraceparent, "trace-context propagation must still work despite query redaction")
+
+	spans := exporter.GetSpans()
+	require.Len(t, spans, 1, "expected exactly one span per call: redaction must not create a second otel span")
+
+	for _, span := range spans {
+		for _, attr := range span.Attributes {
+			assert.NotContains(t, attr.Value.Emit(), uniqueQueryValue,
+				"span attribute %q must not contain the redacted query value", attr.Key)
+		}
+	}
+}
+
+// TestClient_ResponseHeaderTimeoutFiresBeforeTheOverallRequestTimeout proves
+// the response header timeout is wired into the client's transport (not
+// just the overall http.Client.Timeout): a server that accepts the
+// connection and the request body but never writes a response must fail
+// well before the 15s overall request timeout, since the 10s response
+// header timeout fires first.
+func TestClient_ResponseHeaderTimeoutFiresBeforeTheOverallRequestTimeout(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		select {
+		case <-time.After(12 * time.Second):
+		case <-r.Context().Done():
+		}
+	}))
+	t.Cleanup(server.Close)
+
+	client := httpsms.NewClient(server.URL)
+
+	start := time.Now()
+	_, err := client.ListPhones(context.Background(), "token", httpsms.ListPhonesParams{})
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.Less(t, elapsed, 13*time.Second, "expected the ~10s response header timeout to fire well before the 15s overall request timeout")
+}
+
 func readAll(r *http.Request) ([]byte, error) {
 	if r.Body == nil {
 		return nil, nil

diff --git a/mcp/internal/httpsms/transport.go b/mcp/internal/httpsms/transport.go
@@ -0,0 +1,89 @@
+package httpsms
+
+import (
+	"context"
+	"net/http"
+)
+
+// rawQueryContextKey is the context key queryRedactingTransport uses to
+// smuggle a request's real, unmodified RawQuery past otelhttp.Transport to
+// queryRestoringTransport. It is unexported and unique to this package, so
+// it can never collide with a context value set by a caller or by another
+// package.
+type rawQueryContextKey struct{}
+
+// queryRedactingTransport wraps an otelhttp-instrumented transport so that
+// query string values (for example the free-text "query" search filter,
+// which can contain SMS content, phone numbers, or other sensitive filter
+// values) are never recorded as OpenTelemetry span attributes, while the
+// real, unmodified query string is still sent to the httpSMS API on the
+// wire and trace-context propagation headers are still injected as usual.
+//
+// otelhttp.Transport.RoundTrip derives every request span attribute
+// (including the full request URL, via semconv.URLFull) from the exact
+// *http.Request instance it is handed, and then forwards that same
+// instance (after Clone-ing it to attach the span's context) one layer
+// further down to its own configured base transport. There is therefore no
+// exported option to give otelhttp one URL for its attributes and a
+// different one for the real network call: the only seam available is
+// between "what otelhttp is handed" and "what otelhttp's own base
+// transport sends", which is exactly what this pair of transports uses.
+//
+//   - queryRedactingTransport (this type) sits in front of otelhttp.
+//     It clones the incoming request, strips RawQuery from the clone's
+//     URL, stashes the real RawQuery on the clone's context, and hands
+//     that sanitized clone to otelhttp. otelhttp's span attributes are
+//     therefore built from a query-free URL.
+//   - queryRestoringTransport sits behind otelhttp, installed as the base
+//     transport passed to otelhttp.NewTransport. It reads the real
+//     RawQuery back out of the request's context and restores it onto the
+//     request's URL immediately before delegating to the real network
+//     transport (*http.Transport), so the httpSMS API still receives the
+//     original, unmodified query string.
+//
+// Only one otelhttp.Transport is ever involved, so exactly one span is
+// created per call: queryRedactingTransport itself does not start a span.
+// Neither transport mutates the *http.Request a caller passed to
+// http.Client.Do: queryRedactingTransport clones before making any change,
+// and queryRestoringTransport only ever sees clones (first otelhttp's own
+// Clone of queryRedactingTransport's clone).
+type queryRedactingTransport struct {
+	next http.RoundTripper // otelhttp.NewTransport(&queryRestoringTransport{...})
+}
+
+// RoundTrip implements http.RoundTripper.
+func (t *queryRedactingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if req.URL == nil || req.URL.RawQuery == "" {
+		// Nothing to redact: forward unchanged so GET requests without a
+		// query string (and all POST/DELETE calls) skip the clone.
+		return t.next.RoundTrip(req)
+	}
+
+	ctx := context.WithValue(req.Context(), rawQueryContextKey{}, req.URL.RawQuery)
+	sanitized := req.Clone(ctx)
+
+	sanitizedURL := *req.URL
+	sanitizedURL.RawQuery = ""
+	sanitized.URL = &sanitizedURL
+
+	return t.next.RoundTrip(sanitized)
+}
+
+// queryRestoringTransport restores the real query string (stashed by
+// queryRedactingTransport) onto the request's URL immediately before
+// handing it to the real network transport, so the httpSMS API still
+// receives the original, unmodified query even though otelhttp only ever
+// saw a query-free URL.
+type queryRestoringTransport struct {
+	base http.RoundTripper // the real network transport (*http.Transport)
+}
+
+// RoundTrip implements http.RoundTripper.
+func (t *queryRestoringTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	if rawQuery, ok := req.Context().Value(rawQueryContextKey{}).(string); ok {
+		restoredURL := *req.URL
+		restoredURL.RawQuery = rawQuery
+		req.URL = &restoredURL
+	}
+	return t.base.RoundTrip(req)
+}