PLT-287: Harden HTTP logging and header limits (#191)

* PLT-287: Harden HTTP logging and header limits

Skip body logging for compressed responses (gzip, br, deflate) in both
logResponse() and handleFileLogging() — log headers with a [body omitted]
notice instead of dumping garbled binary bytes to structured logs.

Cap MaxBodyLogSize default to 10KB when LogBody is enabled and no
explicit limit is set, preventing unbounded body logging.

Add MaxHeaderBytes to httpserver (default 32KB) to reject oversized
request headers with 431 before reaching application code.

* Address Copilot PR feedback on MaxHeaderBytes tests

- Validate MaxHeaderBytes <= 0 (not just == 0) so negative values
  cannot silently bypass the 32KB default
- Replace NUL-byte header payload with valid ASCII; the original test
  was passing due to client-side rejection, not server-side enforcement
- Account for Go's 4096-byte internal overhead in MaxHeaderBytes check
- Assert network error type on connection-reset path instead of silently
  passing on unexpected errors

* Fix gosec lint findings in httpclient logger

- Sanitize id parameter in LogRequest, LogResponse, and
  LogTransactionToFile before using in file paths (G703)
- Add nolint directives for G705 false positives — fmt.Fprintf writes
  to local log files, not HTTP responses

* Potential fix for code scanning alert no. 224: Log entries created from user input

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* Lower MaxBodyLogSize default to 1KB; omit binary content types from logs

- Reduce MaxBodyLogSize default from 10KB to 1KB to keep structured log
  entries reasonable for log aggregators
- Extract shouldOmitResponseBody() helper to check both Content-Encoding
  and Content-Type, used by logResponse and handleFileLogging
- Omit body for binary content types (images, PDFs, protobuf,
  octet-stream, etc.) even without Content-Encoding
- Allow text-based types: text/*, application/json, +json, +xml, etc.

* Add E2E integration test for gzip body logging with httpclient proxy

Add an integration test in examples/http-client that bootstraps the
httpclient module through the modular framework with verbose logging
and DisableCompression enabled, then exercises the transport through
an httputil.ReverseProxy — the same wiring reverseproxy uses at
module.go:1581. Asserts that log output contains the body-omission
notice for gzip responses and does not contain raw binary bytes.

* Fix gosec lint findings in reverseproxy module

Add nolint annotations for:
- G704 (SSRF) in composite.go and dryrun.go: URLs are built from
  configured backend addresses, not user input
- G118 (context leak) in health_checker.go: cancel func is stored
  on the struct and called in Stop()

* Fix flaky TestAffiliateBackendOverrideRouting test

The test captured the first HandleFunc("/*") from the mock router,
but setupBackendRoutes registers one per backend using map iteration
(random order). When the "chimera" handler was captured instead of
"legacy", the proxy tried to reach bing.com and failed with 500.

Fix by capturing the last "/*" handler, which is the catch-all from
registerBasicRoutes that properly routes all paths via pattern matching.

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Author: kristopherchun

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 {