Security review fixes (Opus audit)

bundles/configloader/bundle.go

@@ -86,6 +86,7 @@ import (
 	stderrors "errors"
 	"fmt"
 	"io"
+	"net/url"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -621,13 +622,23 @@ func (l *Loader) containsSensitiveKeyword(text string) bool {
 		"auth", "cert", "private", "jwt", "oauth", "api_key",
 		"access_key", "secret_key", "private_key", "client_secret",
 		"session", "cookie", "tls", "ssl", "pgp", "gpg",
+		"dsn", "database", "redis", "conn", "connection",
 	}
 
 	for _, keyword := range sensitiveKeywords {
 		if strings.Contains(text, keyword) {
 			return true
 		}
 	}
+
+	// Connection-string-style values (e.g. DATABASE_URL, REDIS_URL,
+	// ServiceURL) are commonly named with a "url"/"uri" suffix rather than
+	// containing an obviously sensitive keyword, but frequently embed
+	// credentials. Match the suffix rather than "contains" to avoid
+	// flagging every field that merely mentions a URL in passing.
+	if strings.HasSuffix(text, "url") || strings.HasSuffix(text, "uri") {
+		return true
+	}
 	return false
 }
 
@@ -637,6 +648,15 @@ func (l *Loader) looksLikeSensitiveData(value string) bool {
 		return false
 	}
 
+	// Connection-string-style URLs with embedded credentials (e.g.
+	// postgres://user:pass@host:5432/db, redis://:pass@host:6379/0) leak a
+	// password regardless of the field/env var name.
+	if u, err := url.Parse(value); err == nil && u.User != nil {
+		if _, hasPassword := u.User.Password(); hasPassword {
+			return true
+		}
+	}
+
 	// Base64-encoded data (likely sensitive)
 	if len(value) > 20 && len(value)%4 == 0 {
 		// Check if it's base64

bundles/configloader/bundle_test.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -182,6 +183,44 @@ func TestLoaderLoad_EnvVarOverride(t *testing.T) {
 	}
 }
 
+// TestLoaderLoad_RedactsConnectionStringSecrets verifies that DATABASE_URL /
+// REDIS_URL style values, which embed credentials but whose field/env names
+// don't contain an obviously sensitive keyword like "password" or "secret",
+// are redacted in LoadResult.EnvVarsUsed rather than logged in cleartext.
+func TestLoaderLoad_RedactsConnectionStringSecrets(t *testing.T) {
+	type TestCfg struct {
+		DatabaseURL string `env:"TEST_DATABASE_URL_CONFIGLOADER"`
+		RedisURL    string `env:"TEST_REDIS_URL_CONFIGLOADER"`
+	}
+
+	const dbPassword = "sup3rSecretPW"
+	t.Setenv("TEST_DATABASE_URL_CONFIGLOADER", "postgres://user:"+dbPassword+"@dbhost:5432/mydb")
+	t.Setenv("TEST_REDIS_URL_CONFIGLOADER", "redis://:"+dbPassword+"@redishost:6379/0")
+
+	l := loaderWithConfig(Config{
+		ConfigPaths:       []string{"./nonexistent.yaml"},
+		RequireConfigFile: false,
+		ValidateOnLoad:    false,
+		MaxFileSize:       1024 * 1024,
+		SecureLogging:     true,
+	})
+
+	var cfg TestCfg
+	result, err := l.Load(&cfg)
+	if err != nil {
+		t.Fatalf("Load failed: %v", err)
+	}
+
+	if len(result.EnvVarsUsed) == 0 {
+		t.Fatal("expected EnvVarsUsed to be populated")
+	}
+	for _, entry := range result.EnvVarsUsed {
+		if strings.Contains(entry, dbPassword) {
+			t.Errorf("expected password to be redacted from EnvVarsUsed, got %q", entry)
+		}
+	}
+}
+
 // TestLoaderLoad_NilDest tests that Load rejects nil destination.
 func TestLoaderLoad_NilDest(t *testing.T) {
 	l := loaderWithConfig(DefaultConfig())
@@ -688,6 +727,61 @@ func TestLoadFromFile_RelativePath(t *testing.T) {
 	}
 }
 
+// TestLooksLikeSensitiveData_ConnectionStringPassword verifies that URL-shaped
+// values with embedded userinfo credentials are flagged regardless of the
+// field/env name that carries them (defense-in-depth alongside the
+// name-based keyword check).
+func TestLooksLikeSensitiveData_ConnectionStringPassword(t *testing.T) {
+	l := loaderWithConfig(DefaultConfig())
+
+	cases := []struct {
+		name     string
+		value    string
+		expected bool
+	}{
+		{"postgres with password", "postgres://user:pass@dbhost:5432/mydb", true},
+		{"redis with password only", "redis://:pass@redishost:6379/0", true},
+		{"http url without credentials", "http://example.com/path", false},
+		{"url with user but no password", "postgres://user@dbhost:5432/mydb", false},
+		{"plain non-url string", "hello", false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := l.looksLikeSensitiveData(tc.value); got != tc.expected {
+				t.Errorf("looksLikeSensitiveData(%q) = %v, want %v", tc.value, got, tc.expected)
+			}
+		})
+	}
+}
+
+// TestContainsSensitiveKeyword_URLSuffix verifies the url/dsn/uri suffix
+// matching added for connection-string-style field names, and that it
+// doesn't over-match names that merely contain "url" as an infix.
+func TestContainsSensitiveKeyword_URLSuffix(t *testing.T) {
+	l := loaderWithConfig(DefaultConfig())
+
+	cases := []struct {
+		name     string
+		text     string
+		expected bool
+	}{
+		{"database_url env tag", "database_url", true},
+		{"redis_url env tag", "redis_url", true},
+		{"DatabaseURL field name lowered", "databaseurl", true},
+		{"dsn suffix", "connectiondsn", true},
+		{"plain normal field", "normal", false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := l.containsSensitiveKeyword(tc.text); got != tc.expected {
+				t.Errorf("containsSensitiveKeyword(%q) = %v, want %v", tc.text, got, tc.expected)
+			}
+		})
+	}
+}
+
 func TestMustLoadConfig_Panics(t *testing.T) {
 	defer func() {
 		if r := recover(); r == nil {

bundles/httpclient/bundle.go

@@ -151,6 +151,13 @@ type Config struct {
 	// Secure credential provider (replaces plain text APIKey)
 	CredentialProvider CredentialProvider // Provider for secure credential access
 
+	// AllowedHosts optionally restricts requests (including RawRequest and
+	// resolved BaseURL+path lookups) to this set of hosts. This is opt-in
+	// defense-in-depth against SSRF for services that pass user-influenced
+	// URLs or paths through the client. Empty/nil (default) preserves the
+	// existing unrestricted behavior.
+	AllowedHosts []string
+
 	// Retry configuration
 	RetryConfig RetryConfig
 
@@ -370,8 +377,9 @@ func (b *Bundle) Initialize(app *framework.App) error {
 
 	// Create HTTP client
 	httpClient := &http.Client{
-		Transport: transport,
-		Timeout:   b.config.Timeout,
+		Transport:     transport,
+		Timeout:       b.config.Timeout,
+		CheckRedirect: redirectHeaderStripper(b.config.APIKeyHeader),
 	}
 
 	// Create enhanced client wrapper
@@ -411,6 +419,34 @@ func (b *Bundle) Close() error {
 	return b.Stop(ctx)
 }
 
+// maxRedirects matches the net/http default redirect limit.
+const maxRedirects = 10
+
+// redirectHeaderStripper returns an http.Client CheckRedirect function that
+// removes credential headers whenever a redirect crosses to a different host.
+// Go's default redirect handling already strips "Authorization" on cross-host
+// redirects but forwards custom headers like the configured API-key header
+// unchanged, allowing a malicious or compromised upstream to harvest
+// credentials via a 302 to an attacker-controlled host. Same-host redirects
+// are unaffected and continue to carry auth headers as before.
+func redirectHeaderStripper(apiKeyHeader string) func(*http.Request, []*http.Request) error {
+	return func(req *http.Request, via []*http.Request) error {
+		if len(via) >= maxRedirects {
+			return fmt.Errorf("stopped after %d redirects", maxRedirects)
+		}
+
+		prev := via[len(via)-1]
+		if prev.URL.Host != req.URL.Host {
+			req.Header.Del("Authorization")
+			if apiKeyHeader != "" {
+				req.Header.Del(apiKeyHeader)
+			}
+		}
+
+		return nil
+	}
+}
+
 // createBackoffStrategy creates the configured backoff strategy.
 func (b *Bundle) createBackoffStrategy() backoff.BackOff {
 	exponentialBackoff := backoff.NewExponentialBackOff()
@@ -475,6 +511,7 @@ func (e *HTTPError) IsRetryableError() bool {
 var (
 	ErrCircuitBreakerOpen = stderrors.New("circuit breaker is open")
 	ErrMaxRetriesExceeded = stderrors.New("maximum retries exceeded")
+	ErrHostNotAllowed     = stderrors.New("host not allowed")
 )
 
 // Get performs a GET request and unmarshals the response into dest.
@@ -505,6 +542,10 @@ func (c *Client) request(ctx context.Context, method, path string, body interfac
 		return fmt.Errorf("failed to build URL: %w", err)
 	}
 
+	if err := c.checkHostAllowed(fullURL); err != nil {
+		return err
+	}
+
 	// Execute request with circuit breaker protection
 	_, err = c.circuitBreaker.Execute(func() (interface{}, error) {
 		return nil, c.executeWithRetry(ctx, method, fullURL, body, dest)
@@ -722,7 +763,7 @@ func (c *Client) sanitizeURLForLogging(rawURL string) string {
 func (c *Client) logRequestError(method, url string, duration time.Duration, err error) {
 	c.logger.Error().
 		Str("method", method).
-		Str("url", url).
+		Str("url", c.sanitizeURLForLogging(url)).
 		Dur("duration", duration).
 		Err(err).
 		Msg("HTTP Request Error")
@@ -747,6 +788,32 @@ func (c *Client) buildURL(path string) (string, error) {
 	return baseURL.ResolveReference(pathURL).String(), nil
 }
 
+// checkHostAllowed validates rawURL's host against config.AllowedHosts. It is
+// an opt-in SSRF safeguard: when AllowedHosts is empty (the default), every
+// host is permitted, preserving existing behavior. When non-empty, requests
+// to any other host are rejected with ErrHostNotAllowed. This covers both the
+// shared request path (buildURL results, which may be overridden by an
+// absolute path) and RawRequest, where the caller supplies the URL directly.
+func (c *Client) checkHostAllowed(rawURL string) error {
+	if len(c.config.AllowedHosts) == 0 {
+		return nil
+	}
+
+	parsedURL, err := url.Parse(rawURL)
+	if err != nil {
+		return fmt.Errorf("invalid URL: %w", err)
+	}
+
+	host := parsedURL.Hostname()
+	for _, allowed := range c.config.AllowedHosts {
+		if host == allowed {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("%w: %s", ErrHostNotAllowed, host)
+}
+
 // addAuthHeaders adds authentication headers to the request.
 func (c *Client) addAuthHeaders(ctx context.Context, req *http.Request) error {
 	// Add API key if credential provider is configured
@@ -823,6 +890,10 @@ func (b *Bundle) EnableJWTIntegration(jwtBundle interface{}) error {
 
 // RawRequest performs a raw HTTP request with full control over request/response.
 func (c *Client) RawRequest(ctx context.Context, method, url string, headers map[string]string, body io.Reader) (*http.Response, error) {
+	if err := c.checkHostAllowed(url); err != nil {
+		return nil, err
+	}
+
 	req, err := http.NewRequestWithContext(ctx, method, url, body)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create HTTP request: %w", err)