fix(observability): address review findings from improve-core-logging

- Pre-compute header name strings once at RequestLoggerMiddleware
  construction instead of per-request to avoid redundant allocations
- Add product_id to all operational log lines that had vendor_id and
  marketplace_id for consistent per-product filtering
- Replace sanitizeURL (kept path) with extractTargetHost (host only) in
  the transaction context parsed DEBUG log to prevent path segments from
  leaking sensitive data (e.g. /users/alice@example.com)
- Add target_host to allow-list rejection log and remove misleading
  comment that described intent that wasn't implemented

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: arnaugiralt

internal/observability/request_logger.go

@@ -108,6 +108,11 @@ func (rc *ResponseCapturer) Status() int {
 // Uses defer to ensure logging occurs even if downstream handlers panic
 // (when used with panic recovery middleware).
 func RequestLoggerMiddleware(logger *slog.Logger, headerPrefix string, next http.Handler) http.Handler {
+	vendorHdr := headerPrefix + "-Vendor-ID"
+	marketplaceHdr := headerPrefix + "-Marketplace-ID"
+	productHdr := headerPrefix + "-Product-ID"
+	targetURLHdr := headerPrefix + "-Target-URL"
+
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
 		ctx := r.Context()
@@ -123,10 +128,10 @@ func RequestLoggerMiddleware(logger *slog.Logger, headerPrefix string, next http
 				"path", r.URL.Path,
 				"status", capturer.Status(),
 				"latency_ms", time.Since(start).Milliseconds(),
-				"vendor_id", r.Header.Get(headerPrefix+"-Vendor-ID"),
-				"marketplace_id", r.Header.Get(headerPrefix+"-Marketplace-ID"),
-				"product_id", r.Header.Get(headerPrefix+"-Product-ID"),
-				"target_host", extractHost(r.Header.Get(headerPrefix+"-Target-URL")),
+				"vendor_id", r.Header.Get(vendorHdr),
+				"marketplace_id", r.Header.Get(marketplaceHdr),
+				"product_id", r.Header.Get(productHdr),
+				"target_host", extractHost(r.Header.Get(targetURLHdr)),
 				"client_ip", ClientIP(r),
 				"remote_addr", r.RemoteAddr,
 			)

internal/proxy/integration_test.go

@@ -2026,7 +2026,7 @@ func TestIntegration_SlowPath_LogsCredentialInjection(t *testing.T) {
 // Phase 6: DEBUG logpoints for context parsing
 // =============================================================================
 
-func TestProxy_ContextParsed_DebugLog_SanitizesURL(t *testing.T) {
+func TestProxy_ContextParsed_DebugLog_LogsHostOnly(t *testing.T) {
 	// Arrange - capture DEBUG log output
 	var logBuffer bytes.Buffer
 	logger := slog.New(slog.NewJSONHandler(&logBuffer, &slog.HandlerOptions{Level: slog.LevelDebug}))
@@ -2042,8 +2042,8 @@ func TestProxy_ContextParsed_DebugLog_SanitizesURL(t *testing.T) {
 	srv := mustNewServerForTarget(t, testConfig(), backend.URL)
 	handler := srv.Handler()
 
-	// Construct target URL with sensitive query params and credentials in userinfo
-	targetURL := backend.URL + "/v1/resource?api_key=supersecret&token=abc123"
+	// URL with a sensitive path segment and query params — only the host should appear in logs
+	targetURL := backend.URL + "/v1/users/alice@example.com?api_key=supersecret&token=abc123"
 
 	req := httptest.NewRequest(http.MethodGet, "/proxy", nil)
 	req.Header.Set("X-Connect-Target-URL", targetURL)
@@ -2053,8 +2053,11 @@ func TestProxy_ContextParsed_DebugLog_SanitizesURL(t *testing.T) {
 	// Act
 	handler.ServeHTTP(rec, req)
 
-	// Assert - target_url in the DEBUG log must not contain query params
+	// Assert - only the host appears; path, query, and userinfo must not leak
 	logOutput := logBuffer.String()
+	if !strings.Contains(logOutput, `"msg":"transaction context parsed"`) {
+		t.Errorf("expected 'transaction context parsed' debug log, got: %s", logOutput)
+	}
 	if strings.Contains(logOutput, "supersecret") {
 		t.Errorf("log must not contain sensitive query value 'supersecret', got: %s", logOutput)
 	}
@@ -2064,8 +2067,11 @@ func TestProxy_ContextParsed_DebugLog_SanitizesURL(t *testing.T) {
 	if strings.Contains(logOutput, "token=") {
 		t.Errorf("log must not contain 'token=' query param, got: %s", logOutput)
 	}
-	if !strings.Contains(logOutput, `"msg":"transaction context parsed"`) {
-		t.Errorf("expected 'transaction context parsed' debug log, got: %s", logOutput)
+	if strings.Contains(logOutput, "alice@example.com") {
+		t.Errorf("log must not contain path segment 'alice@example.com', got: %s", logOutput)
+	}
+	if strings.Contains(logOutput, "/v1/users") {
+		t.Errorf("log must not contain URL path, got: %s", logOutput)
 	}
 }
 

internal/proxy/server.go

@@ -515,7 +515,7 @@ func (s *Server) handleProxy(w http.ResponseWriter, r *http.Request) {
 		"vendor_id", txCtx.VendorID,
 		"marketplace_id", txCtx.MarketplaceID,
 		"product_id", txCtx.ProductID,
-		"target_url", sanitizeURL(txCtx.TargetURL),
+		"target_host", extractTargetHost(txCtx.TargetURL),
 	)
 
 	targetURL, err := url.Parse(txCtx.TargetURL)
@@ -701,18 +701,15 @@ func (s *Server) detectSlowPathInjections(r *http.Request, before http.Header) (
 	return r, len(injectedKeys)
 }
 
-// sanitizeURL strips the query string, fragment, and userinfo from a URL
-// to prevent token or credential leakage in log output.
-// Returns an empty string if the input is not a valid URL.
-func sanitizeURL(rawURL string) string {
+// extractTargetHost parses rawURL and returns only the host (with port if present).
+// Used in log output to avoid leaking sensitive path or query information.
+// Returns an empty string if the URL is invalid or has no host.
+func extractTargetHost(rawURL string) string {
 	u, err := url.Parse(rawURL)
-	if err != nil {
+	if err != nil || u.Host == "" {
 		return ""
 	}
-	u.RawQuery = ""
-	u.Fragment = ""
-	u.User = nil
-	return u.String()
+	return u.Host
 }
 
 // headerValuesEqual returns true if two header value slices are identical.
@@ -758,6 +755,7 @@ func (s *Server) handlePluginError(w http.ResponseWriter, traceID string, txCtx
 			"trace_id", traceID,
 			"vendor_id", txCtx.VendorID,
 			"marketplace_id", txCtx.MarketplaceID,
+			"product_id", txCtx.ProductID,
 			"target_host", targetHost,
 			"error", err,
 		)
@@ -770,6 +768,7 @@ func (s *Server) handlePluginError(w http.ResponseWriter, traceID string, txCtx
 			"trace_id", traceID,
 			"vendor_id", txCtx.VendorID,
 			"marketplace_id", txCtx.MarketplaceID,
+			"product_id", txCtx.ProductID,
 			"target_host", targetHost,
 		)
 		// Write 499 so RequestLoggerMiddleware logs the correct status instead of
@@ -782,6 +781,7 @@ func (s *Server) handlePluginError(w http.ResponseWriter, traceID string, txCtx
 		"trace_id", traceID,
 		"vendor_id", txCtx.VendorID,
 		"marketplace_id", txCtx.MarketplaceID,
+		"product_id", txCtx.ProductID,
 		"target_host", targetHost,
 		"error", err,
 	)
@@ -855,6 +855,7 @@ func (s *Server) createReverseProxy(target *url.URL, traceID string, txCtx *sdk.
 			"trace_id", traceID,
 			"vendor_id", txCtx.VendorID,
 			"marketplace_id", txCtx.MarketplaceID,
+			"product_id", txCtx.ProductID,
 			"target_host", target.Host,
 			"error", err,
 		)
@@ -900,6 +901,7 @@ func (s *Server) buildModifyResponse(traceID string, txCtx *sdk.TransactionConte
 					"trace_id", traceID,
 					"vendor_id", txCtx.VendorID,
 					"marketplace_id", txCtx.MarketplaceID,
+					"product_id", txCtx.ProductID,
 					"target_host", resp.Request.URL.Host,
 					"error", err,
 				)
@@ -925,6 +927,7 @@ func (s *Server) buildModifyResponse(traceID string, txCtx *sdk.TransactionConte
 			"content_length", resp.ContentLength,
 			"vendor_id", txCtx.VendorID,
 			"marketplace_id", txCtx.MarketplaceID,
+			"product_id", txCtx.ProductID,
 			"target_host", resp.Request.URL.Host,
 		)
 		return nil
@@ -946,6 +949,7 @@ func (s *Server) applyErrorNormalization(traceID string, txCtx *sdk.TransactionC
 			"trace_id", traceID,
 			"vendor_id", txCtx.VendorID,
 			"marketplace_id", txCtx.MarketplaceID,
+			"product_id", txCtx.ProductID,
 			"target_host", resp.Request.URL.Host,
 			"error", err,
 		)

internal/proxy/server_helpers_test.go

@@ -5,36 +5,36 @@ package proxy
 
 import "testing"
 
-func TestSanitizeURL(t *testing.T) {
+func TestExtractTargetHost(t *testing.T) {
 	tests := []struct {
 		name  string
 		input string
 		want  string
 	}{
 		{
-			name:  "strips query string",
-			input: "https://api.vendor.com/v1?api_key=secret",
-			want:  "https://api.vendor.com/v1",
+			name:  "returns host only, strips path",
+			input: "https://api.vendor.com/v1/users/123",
+			want:  "api.vendor.com",
 		},
 		{
-			name:  "strips fragment",
-			input: "https://api.vendor.com/v1#section",
-			want:  "https://api.vendor.com/v1",
+			name:  "returns host only, strips query string",
+			input: "https://api.vendor.com/v1?api_key=secret",
+			want:  "api.vendor.com",
 		},
 		{
-			name:  "strips userinfo",
+			name:  "returns host only, strips userinfo",
 			input: "https://user:pass@api.vendor.com/v1",
-			want:  "https://api.vendor.com/v1",
+			want:  "api.vendor.com",
 		},
 		{
-			name:  "preserves path",
-			input: "https://api.vendor.com/v1/users/123",
-			want:  "https://api.vendor.com/v1/users/123",
+			name:  "preserves port",
+			input: "https://api.vendor.com:8443/v1",
+			want:  "api.vendor.com:8443",
 		},
 		{
 			name:  "strips all sensitive parts together",
-			input: "https://user:pass@api.vendor.com/v1?token=abc#frag",
-			want:  "https://api.vendor.com/v1",
+			input: "https://user:pass@api.vendor.com/v1/users/alice@example.com?token=abc#frag",
+			want:  "api.vendor.com",
 		},
 		{
 			name:  "invalid URL returns empty",
@@ -50,9 +50,9 @@ func TestSanitizeURL(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := sanitizeURL(tt.input)
+			got := extractTargetHost(tt.input)
 			if got != tt.want {
-				t.Errorf("sanitizeURL(%q) = %q, want %q", tt.input, got, tt.want)
+				t.Errorf("extractTargetHost(%q) = %q, want %q", tt.input, got, tt.want)
 			}
 		})
 	}

internal/router/middleware.go

@@ -55,9 +55,9 @@ func (m *AllowListMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request)
 
 	// Validate target URL against allow list
 	if err := m.validator.Validate(targetURL); err != nil {
-		// Log the host but not the full URL to avoid leaking query params
 		slog.Warn("allow list validation failed",
 			"trace_id", observability.TraceIDFromContext(r.Context()),
+			"target_host", extractHostFromURL(targetURL),
 			"error", err.Error(),
 			"remote_addr", r.RemoteAddr,
 		)

internal/router/middleware_test.go

@@ -380,6 +380,9 @@ func TestAllowListMiddleware_ValidationFailed_HasTraceID(t *testing.T) {
 	if !strings.Contains(logOutput, `"trace_id":"trace-fail-456"`) {
 		t.Errorf("expected trace_id in failure log, got: %s", logOutput)
 	}
+	if !strings.Contains(logOutput, `"target_host":"evil.com"`) {
+		t.Errorf("expected target_host in failure log, got: %s", logOutput)
+	}
 }
 
 func TestAllowListMiddleware_EmptyAllowListDeniesAll(t *testing.T) {