PLT-287: Harden HTTP logging and header limits

examples/http-client/go.mod

@@ -10,18 +10,21 @@ require (
 	github.com/CrisisTextLine/modular/modules/httpclient v0.1.0
 	github.com/CrisisTextLine/modular/modules/httpserver v0.1.1
 	github.com/CrisisTextLine/modular/modules/reverseproxy v1.1.0
+	github.com/stretchr/testify v1.11.1
 )
 
 require (
 	github.com/BurntSushi/toml v1.6.0 // indirect
 	github.com/cloudevents/sdk-go/v2 v2.16.2 // indirect
+	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/go-chi/chi/v5 v5.2.2 // indirect
 	github.com/gobwas/glob v0.2.3 // indirect
 	github.com/golobby/cast v1.3.3 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
+	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.27.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect

examples/http-client/gzip_logging_integration_test.go

@@ -0,0 +1,121 @@
+package main
+
+import (
+	"bytes"
+	"compress/gzip"
+	"io"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/http/httputil"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/CrisisTextLine/modular"
+	"github.com/CrisisTextLine/modular/modules/httpclient"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestGzipBodyOmittedInLogsWhenProxied is an end-to-end integration test
+// verifying that when a gzip-compressed response passes through a reverse
+// proxy using the httpclient module's transport (with verbose logging), the
+// log output contains the body-omission notice instead of raw binary bytes.
+//
+// This mirrors the real wiring: reverseproxy.module.go:1581 sets
+// proxy.Transport = m.httpClient.Transport, so the httpclient's
+// loggingTransport wraps every proxied request.
+func TestGzipBodyOmittedInLogsWhenProxied(t *testing.T) {
+	// 1. Create a gzip-compressed payload and a backend that serves it.
+	const payload = `{"status":"ok","data":"hello gzip integration"}`
+	var gzBuf bytes.Buffer
+	gw := gzip.NewWriter(&gzBuf)
+	_, err := gw.Write([]byte(payload))
+	require.NoError(t, err)
+	require.NoError(t, gw.Close())
+	gzBody := gzBuf.Bytes()
+
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.Header().Set("Content-Encoding", "gzip")
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write(gzBody)
+	}))
+	defer backend.Close()
+
+	// 2. Create a log-capturing buffer wired to a slog logger.
+	var logBuf bytes.Buffer
+	logger := slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelDebug}))
+
+	// 3. Bootstrap the modular app with the httpclient module.
+	app := modular.NewStdApplication(
+		modular.NewStdConfigProvider(struct{}{}),
+		logger,
+	)
+
+	// The httpclient module's RegisterConfig registers default values that
+	// overwrite any pre-registered config section. Use OnConfigLoaded to
+	// inject our test config after defaults are loaded but before Init.
+	stdApp := app.(*modular.StdApplication)
+	stdApp.OnConfigLoaded(func(_ modular.Application) error {
+		app.RegisterConfigSection("httpclient", modular.NewStdConfigProvider(&httpclient.Config{
+			Verbose:            true,
+			DisableCompression: true,
+			VerboseOptions: &httpclient.VerboseOptions{
+				LogHeaders: true,
+				LogBody:    true,
+			},
+		}))
+		return nil
+	})
+
+	app.RegisterModule(httpclient.NewHTTPClientModule())
+
+	// 4. Init wires the httpclient module (builds the loggingTransport).
+	err = app.Init()
+	require.NoError(t, err)
+
+	// 5. Retrieve the *http.Client from the service registry — this is the
+	// same client that reverseproxy receives via Constructor and uses as
+	// proxy.Transport = m.httpClient.Transport (module.go:1581).
+	var client *http.Client
+	err = stdApp.GetService("httpclient", &client)
+	require.NoError(t, err)
+	require.NotNil(t, client, "httpclient service should provide *http.Client")
+	require.NotNil(t, client.Transport, "httpclient should configure a custom transport")
+
+	// Verify the transport is NOT a plain *http.Transport (should be loggingTransport).
+	_, isPlainTransport := client.Transport.(*http.Transport)
+	require.False(t, isPlainTransport,
+		"expected httpclient to wrap the transport with loggingTransport when Verbose is true")
+
+	// 6. Build a reverse proxy that uses the httpclient's transport, exactly
+	// as the reverseproxy module does in createReverseProxyForBackend.
+	backendURL, err := url.Parse(backend.URL)
+	require.NoError(t, err)
+	proxy := httputil.NewSingleHostReverseProxy(backendURL)
+	proxy.Transport = client.Transport
+
+	// 7. Serve a request through the proxy.
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	proxy.ServeHTTP(rec, req)
+
+	assert.Equal(t, http.StatusOK, rec.Code)
+
+	// 8. Verify the response body is the raw gzip bytes (DisableCompression
+	// ensures the transport does not auto-decompress).
+	respBody, err := io.ReadAll(rec.Body)
+	require.NoError(t, err)
+	assert.Equal(t, gzBody, respBody, "proxy must pass gzip bytes through unchanged")
+
+	// 9. Assert the log buffer contains the body-omission notice.
+	logOutput := logBuf.String()
+	assert.Contains(t, logOutput, "[body omitted: Content-Encoding=gzip",
+		"expected gzip body-omission notice in logs")
+
+	// 10. Assert the log buffer does NOT contain gzip magic bytes (\x1f\x8b).
+	assert.False(t, strings.Contains(logOutput, "\x1f\x8b"),
+		"log output must not contain raw gzip magic bytes")
+}

modules/httpclient/README.md

@@ -29,7 +29,7 @@ httpclient:
   verbose_options:                # Options for verbose logging (when verbose is true)
     log_headers: true             # Log request and response headers
     log_body: true                # Log request and response bodies
-    max_body_log_size: 10000      # Maximum size of logged bodies (bytes)
+    max_body_log_size: 1024       # Maximum size of logged bodies (bytes, default 1KB)
     log_to_file: false            # Whether to log to files instead of application logger
     log_file_path: "/tmp/logs"    # Directory path for log files (required when log_to_file is true)
 ```

modules/httpclient/config.go

@@ -112,9 +112,11 @@ type VerboseOptions struct {
 	LogBody bool `yaml:"log_body" json:"log_body" env:"LOG_BODY"`
 
 	// MaxBodyLogSize limits the size of logged request and response bodies.
-	// Bodies larger than this size will be truncated in logs. Set to 0 for no limit.
+	// Bodies larger than this size will be truncated in logs.
 	// Helps prevent log spam from large file uploads or downloads.
-	// Default: 0 (no limit)
+	// When LogBody is enabled and this is left at 0, Validate() applies a
+	// safe default of 1024 (1KB) to prevent unbounded log output.
+	// Default: 1024 (1KB) when LogBody is enabled
 	MaxBodyLogSize int `yaml:"max_body_log_size" json:"max_body_log_size" env:"MAX_BODY_LOG_SIZE"`
 
 	// LogToFile enables logging to files instead of just the application logger.
@@ -159,11 +161,18 @@ func (c *Config) Validate() error {
 		c.VerboseOptions = &VerboseOptions{
 			LogHeaders:     true,
 			LogBody:        true,
-			MaxBodyLogSize: 10000, // 10KB
+			MaxBodyLogSize: 1024, // 1KB
 			LogToFile:      false,
 		}
 	}
 
+	// Apply a safe default body log size when verbose options are explicitly provided
+	// but MaxBodyLogSize is left at 0 (unlimited). Unlimited body logging can flood
+	// structured log systems with large or binary payloads.
+	if c.Verbose && c.VerboseOptions != nil && c.VerboseOptions.LogBody && c.VerboseOptions.MaxBodyLogSize == 0 {
+		c.VerboseOptions.MaxBodyLogSize = 1024 // 1KB default cap
+	}
+
 	// Validate verbose log file path if logging to file is enabled
 	if c.Verbose && c.VerboseOptions != nil && c.VerboseOptions.LogToFile && c.VerboseOptions.LogFilePath == "" {
 		return fmt.Errorf("config validation error: %w", ErrLogFilePathRequired)

modules/httpclient/logger.go

@@ -92,17 +92,25 @@ func NewFileLogger(baseDir string, logger modular.Logger) (*FileLogger, error) {
 
 // LogRequest writes request data to a file.
 func (f *FileLogger) LogRequest(id string, data []byte) error {
-	requestFile := filepath.Join(f.requestDir, fmt.Sprintf("request_%s_%d.log", id, time.Now().UnixNano()))
-	if err := os.WriteFile(requestFile, data, 0600); err != nil {
+	safeID := sanitizeForFilename(id)
+	if safeID == "" {
+		return fmt.Errorf("request ID %q: %w", id, ErrUnsafeFilename)
+	}
+	requestFile := filepath.Join(f.requestDir, fmt.Sprintf("request_%s_%d.log", safeID, time.Now().UnixNano()))
+	if err := os.WriteFile(requestFile, data, 0600); err != nil { //nolint:gosec // path components are sanitized above
 		return fmt.Errorf("failed to write request log file %s: %w", requestFile, err)
 	}
 	return nil
 }
 
 // LogResponse writes response data to a file.
 func (f *FileLogger) LogResponse(id string, data []byte) error {
-	responseFile := filepath.Join(f.responseDir, fmt.Sprintf("response_%s_%d.log", id, time.Now().UnixNano()))
-	if err := os.WriteFile(responseFile, data, 0600); err != nil {
+	safeID := sanitizeForFilename(id)
+	if safeID == "" {
+		return fmt.Errorf("response ID %q: %w", id, ErrUnsafeFilename)
+	}
+	responseFile := filepath.Join(f.responseDir, fmt.Sprintf("response_%s_%d.log", safeID, time.Now().UnixNano()))
+	if err := os.WriteFile(responseFile, data, 0600); err != nil { //nolint:gosec // path components are sanitized above
 		return fmt.Errorf("failed to write response log file %s: %w", responseFile, err)
 	}
 	return nil
@@ -111,14 +119,18 @@ func (f *FileLogger) LogResponse(id string, data []byte) error {
 // LogTransactionToFile logs both request and response data to a single file for easier analysis.
 func (f *FileLogger) LogTransactionToFile(id string, reqData, respData []byte, duration time.Duration, url string) error {
 	// Create a filename that's safe for the filesystem
+	safeID := sanitizeForFilename(id)
+	if safeID == "" {
+		return fmt.Errorf("transaction ID %q: %w", id, ErrUnsafeFilename)
+	}
 	safeURL := sanitizeForFilename(url)
 	if safeURL == "" {
 		return fmt.Errorf("URL %q: %w", url, ErrUnsafeFilename)
 	}
 
-	txnFile := filepath.Join(f.txnDir, fmt.Sprintf("txn_%s_%s_%d.log", id, safeURL, time.Now().UnixNano()))
+	txnFile := filepath.Join(f.txnDir, fmt.Sprintf("txn_%s_%s_%d.log", safeID, safeURL, time.Now().UnixNano()))
 
-	file, err := os.Create(txnFile)
+	file, err := os.Create(txnFile) //nolint:gosec // path components are sanitized above
 	if err != nil {
 		return fmt.Errorf("failed to create transaction log file: %w", err)
 	}
@@ -130,7 +142,7 @@ func (f *FileLogger) LogTransactionToFile(id string, reqData, respData []byte, d
 	}()
 
 	// Write transaction metadata
-	if _, err := fmt.Fprintf(file, "Transaction ID: %s\n", id); err != nil {
+	if _, err := fmt.Fprintf(file, "Transaction ID: %s\n", id); err != nil { //nolint:gosec // G705 false positive: writing to local file, not HTTP response
 		return fmt.Errorf("failed to write transaction ID to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "URL: %s\n", url); err != nil {
@@ -139,7 +151,7 @@ func (f *FileLogger) LogTransactionToFile(id string, reqData, respData []byte, d
 	if _, err := fmt.Fprintf(file, "Time: %s\n", time.Now().Format(time.RFC3339)); err != nil {
 		return fmt.Errorf("failed to write timestamp to log file: %w", err)
 	}
-	if _, err := fmt.Fprintf(file, "Duration: %d ms\n", duration.Milliseconds()); err != nil {
+	if _, err := fmt.Fprintf(file, "Duration: %d ms\n", duration.Milliseconds()); err != nil { //nolint:gosec // G705 false positive: writing to local file, not HTTP response
 		return fmt.Errorf("failed to write duration to log file: %w", err)
 	}
 	if _, err := fmt.Fprintf(file, "\n----- REQUEST -----\n\n"); err != nil {