diff --git a/internal/observability/tracing.go b/internal/observability/tracing.go index 299978e2a..25cefb55c 100644 --- a/internal/observability/tracing.go +++ b/internal/observability/tracing.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "net/url" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -15,6 +16,8 @@ import ( semconv "go.opentelemetry.io/otel/semconv/v1.4.0" oteltrace "go.opentelemetry.io/otel/trace" "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" ) // OTLP transport protocols. @@ -169,7 +172,7 @@ func (tm *TracingManager) HTTPMiddleware() func(http.Handler) http.Handler { ctx, span := tm.tracer.Start(ctx, spanName, oteltrace.WithAttributes( semconv.HTTPMethodKey.String(r.Method), - semconv.HTTPURLKey.String(r.URL.String()), + semconv.HTTPURLKey.String(traceSafeURL(r.URL)), semconv.HTTPSchemeKey.String(r.URL.Scheme), semconv.HTTPHostKey.String(r.Host), semconv.HTTPTargetKey.String(r.URL.Path), @@ -200,6 +203,16 @@ func (tm *TracingManager) HTTPMiddleware() func(http.Handler) http.Handler { } } +// traceSafeURL renders the request URL for the http.url span attribute with +// credential-bearing query parameters masked. ?apikey= is an accepted +// credential (the Web UI's SSE stream and the tray send the admin key that +// way), and spans are exported to an external collector. +func traceSafeURL(u *url.URL) string { + safe := *u + safe.RawQuery = oauth.LogSafeQueryString(u.RawQuery) + return safe.String() +} + // tracingResponseWriter wraps http.ResponseWriter to capture status code for tracing type tracingResponseWriter struct { http.ResponseWriter diff --git a/internal/observability/tracing_redact_test.go b/internal/observability/tracing_redact_test.go new file mode 100644 index 000000000..ca2fa50d2 --- /dev/null +++ b/internal/observability/tracing_redact_test.go @@ -0,0 +1,54 @@ +package observability + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + semconv "go.opentelemetry.io/otel/semconv/v1.4.0" + "go.uber.org/zap" +) + +// The Web UI's SSE stream and the tray send the admin key as ?apikey=, so the +// http.url span attribute must not carry it to the OTLP collector. +func TestHTTPMiddleware_MasksAPIKeyInSpanURL(t *testing.T) { + recorder := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(recorder)) + t.Cleanup(func() { _ = provider.Shutdown(t.Context()) }) + + tm := &TracingManager{ + logger: zap.NewNop().Sugar(), + enabled: true, + tracer: provider.Tracer("test"), + } + + const key = "3f9a0c1d2e4b5a6978c0d1e2f3a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6" + handler := tm.HTTPMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + req := httptest.NewRequest(http.MethodGet, "/events?apikey="+key+"&limit=5", nil) + handler.ServeHTTP(httptest.NewRecorder(), req) + + spans := recorder.Ended() + if len(spans) != 1 { + t.Fatalf("got %d ended spans, want 1", len(spans)) + } + var gotURL string + for _, kv := range spans[0].Attributes() { + if kv.Key == semconv.HTTPURLKey { + gotURL = kv.Value.AsString() + } + } + if gotURL == "" { + t.Fatal("span has no http.url attribute") + } + if strings.Contains(gotURL, key) { + t.Fatalf("http.url carries the API key: %q", gotURL) + } + if !strings.HasPrefix(gotURL, "/events?") || !strings.Contains(gotURL, "limit=5") { + t.Fatalf("http.url lost its path or non-secret parameters: %q", gotURL) + } +}