fix: Add connection string validation and prevent credential exposure (fixes #37) (#65)

Security improvements to NewTypeProviderWithConnection:

1. **Add connection string length validation**
   - Enforce maximum length of 1000 characters (aligns with ODBC standard)
   - Prevents resource exhaustion attacks
   - Covers 99%+ of legitimate use cases

2. **CRITICAL: Fix credential exposure vulnerability (HIGH severity)**
   - Remove error wrapping (%w) at pg/provider.go:60
   - Prevents exposure of connection strings with passwords in error messages
   - Addresses CWE-209 (Information Exposure) and CWE-532 (Sensitive Info in Logs)
   - Based on research showing pgx issues #1271 and #1428 expose credentials

3. **Add comprehensive security test suite**
   - TestConnectionStringLengthValidation - validates limit enforcement
   - TestMalformedConnectionStringsNoCredentialExposure - verifies no leakage
   - TestValidConnectionWithPostgreSQL17 - ensures backward compatibility
   - TestInvalidConnectionFormats - tests various invalid inputs
   - TestConnectionStringSecurityProperties - security property verification
   - All tests pass with PostgreSQL 17 testcontainers

Security Impact:
- Eliminates HIGH severity credential exposure vulnerability
- Aligns with OWASP, NIST SP 800-53, and CWE security standards
- Maintains backward compatibility (errors still returned, sanitized)

References:
- pgx #1271: jackc/pgx#1271
- pgx #1428: jackc/pgx#1428
- CWE-209: https://cwe.mitre.org/data/definitions/209.html
- CWE-532: https://cwe.mitre.org/data/definitions/532.html

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>

Author: richardwooding

pg/provider.go

@@ -19,6 +19,11 @@ import (
 const (
 	typeJSON  = "json"
 	typeJSONB = "jsonb"
+
+	// MaxConnectionStringLength limits connection string length to prevent
+	// resource exhaustion and align with ODBC standard (1024 chars).
+	// Legitimate PostgreSQL connection strings rarely exceed a few hundred characters.
+	MaxConnectionStringLength = 1000
 )
 
 // FieldSchema represents a PostgreSQL field type with name, type, and optional nested schema.
@@ -55,9 +60,17 @@ func NewTypeProvider(schemas map[string]Schema) TypeProvider {
 
 // NewTypeProviderWithConnection creates a new PostgreSQL type provider that can introspect database schemas
 func NewTypeProviderWithConnection(ctx context.Context, connectionString string) (TypeProvider, error) {
+	// Validate connection string length to prevent DoS and align with industry standards
+	if len(connectionString) > MaxConnectionStringLength {
+		return nil, fmt.Errorf("connection string exceeds maximum length of %d characters", MaxConnectionStringLength)
+	}
+
 	pool, err := pgxpool.New(ctx, connectionString)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create connection pool: %w", err)
+		// Security: Don't wrap the error with %w to prevent exposing connection details
+		// (credentials, hostnames, database names) in error messages or logs.
+		// See pgx issues #1271 and #1428, CWE-209, CWE-532.
+		return nil, errors.New("failed to create connection pool")
 	}
 
 	return &typeProvider{

pg/provider_connection_security_test.go

@@ -0,0 +1,305 @@
+package pg_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/testcontainers/testcontainers-go"
+	"github.com/testcontainers/testcontainers-go/modules/postgres"
+	"github.com/testcontainers/testcontainers-go/wait"
+
+	"github.com/spandigital/cel2sql/v2/pg"
+)
+
+// TestConnectionStringLengthValidation tests that connection strings exceeding
+// the maximum length are rejected
+func TestConnectionStringLengthValidation(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		name      string
+		connStr   string
+		expectErr bool
+		errMsg    string
+	}{
+		{
+			name:      "valid_short_connection_string",
+			connStr:   "postgresql://user:pass@localhost:5432/db",
+			expectErr: false,
+		},
+		{
+			name:      "valid_connection_string_at_limit",
+			connStr:   "postgresql://user:pass@localhost:5432/db?" + strings.Repeat("a", 959), // Total = 1000
+			expectErr: false,
+		},
+		{
+			name:      "connection_string_exceeds_limit",
+			connStr:   "postgresql://user:pass@localhost:5432/db?" + strings.Repeat("a", 960), // Total = 1001
+			expectErr: true,
+			errMsg:    "connection string exceeds maximum length",
+		},
+		{
+			name:      "very_long_connection_string",
+			connStr:   strings.Repeat("postgresql://user:pass@localhost:5432/db?", 100), // Way over limit
+			expectErr: true,
+			errMsg:    "connection string exceeds maximum length",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			provider, err := pg.NewTypeProviderWithConnection(ctx, tt.connStr)
+
+			if tt.expectErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.errMsg)
+				assert.Nil(t, provider)
+			} else {
+				// Note: These will fail to connect since they're not real DBs,
+				// but we're testing length validation which happens first
+				if err != nil {
+					// Should fail with connection error, not length error
+					assert.NotContains(t, err.Error(), "exceeds maximum length")
+				}
+				if provider != nil {
+					provider.Close()
+				}
+			}
+		})
+	}
+}
+
+// TestMalformedConnectionStringsNoCredentialExposure tests that malformed
+// connection strings don't expose credentials in error messages
+func TestMalformedConnectionStringsNoCredentialExposure(t *testing.T) {
+	ctx := context.Background()
+
+	// Test various truly malformed connection strings that will cause parsing errors
+	testCases := []struct {
+		name       string
+		connStr    string
+		secrets    []string // Strings that should NOT appear in error
+	}{
+		{
+			name:    "invalid_syntax_with_credentials",
+			connStr: "postgresql://admin:secret@::invalid::/database",
+			secrets: []string{"secret", "admin"},
+		},
+		{
+			name:    "malformed_uri_with_password",
+			connStr: "postgressql://user:P@ss123!@[invalid",  // Invalid URI syntax
+			secrets: []string{"P@ss123!", "user"},
+		},
+		{
+			name:    "broken_url_encoding",
+			connStr: "postgresql://app:pass%@host/db", // Invalid URL encoding
+			secrets: []string{"pass%", "app"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			provider, err := pg.NewTypeProviderWithConnection(ctx, tc.connStr)
+
+			// These should fail during parsing
+			if err != nil {
+				// Critical security check: error message must NOT contain any secrets
+				errorMsg := err.Error()
+				t.Logf("Error message (should be generic): %s", errorMsg)
+
+				for _, secret := range tc.secrets {
+					assert.NotContains(t, errorMsg, secret,
+						"Error message MUST NOT expose credential: %s", secret)
+				}
+
+				// Verify we get generic error message (either length or connection error)
+				assert.True(t,
+					errorMsg == "failed to create connection pool" ||
+						strings.Contains(errorMsg, "exceeds maximum length"),
+					"Error should be generic, got: %s", errorMsg)
+			}
+
+			if provider != nil {
+				provider.Close()
+			}
+		})
+	}
+}
+
+// TestValidConnectionWithPostgreSQL17 tests that valid connection strings work
+// correctly with PostgreSQL 17 and don't break existing functionality
+func TestValidConnectionWithPostgreSQL17(t *testing.T) {
+	ctx := context.Background()
+
+	// Create a PostgreSQL 17 container
+	container, err := postgres.Run(ctx,
+		"postgres:17",
+		postgres.WithDatabase("testdb"),
+		postgres.WithUsername("testuser"),
+		postgres.WithPassword("testpass"),
+		testcontainers.WithWaitStrategy(
+			wait.ForLog("database system is ready to accept connections").
+				WithOccurrence(2).
+				WithStartupTimeout(time.Second*60),
+		),
+	)
+	require.NoError(t, err)
+
+	defer func() {
+		if err := container.Terminate(ctx); err != nil {
+			t.Errorf("failed to terminate container: %v", err)
+		}
+	}()
+
+	// Get connection string
+	connStr, err := container.ConnectionString(ctx, "sslmode=disable")
+	require.NoError(t, err)
+	t.Logf("Connection string length: %d characters", len(connStr))
+
+	// Verify connection string is within limit
+	assert.LessOrEqual(t, len(connStr), pg.MaxConnectionStringLength,
+		"Generated connection string should be within limit")
+
+	// Test that connection succeeds with valid connection string
+	provider, err := pg.NewTypeProviderWithConnection(ctx, connStr)
+	require.NoError(t, err, "Valid connection string should not error")
+	require.NotNil(t, provider, "Provider should be created")
+	defer provider.Close()
+
+	// Verify provider is functional by accessing schemas
+	schemas := provider.GetSchemas()
+	assert.NotNil(t, schemas)
+}
+
+// TestInvalidConnectionFormats tests various invalid connection string formats
+func TestInvalidConnectionFormats(t *testing.T) {
+	ctx := context.Background()
+
+	testCases := []struct {
+		name    string
+		connStr string
+	}{
+		{
+			name:    "invalid_scheme",
+			connStr: "mysql://user:pass@localhost/db",
+		},
+		{
+			name:    "invalid_port",
+			connStr: "postgresql://user:pass@localhost:abc/db",
+		},
+		{
+			name:    "garbage_string",
+			connStr: "this is not a connection string",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			provider, err := pg.NewTypeProviderWithConnection(ctx, tc.connStr)
+
+			// Verify error is generic if one occurs
+			if err != nil {
+				errorMsg := err.Error()
+				// Should be either length error or generic connection error
+				assert.True(t,
+					errorMsg == "failed to create connection pool" ||
+						strings.Contains(errorMsg, "exceeds maximum length"),
+					"Error should be generic, got: %s", errorMsg)
+
+				// Verify no connection details leaked
+				assert.NotContains(t, errorMsg, tc.connStr)
+			}
+
+			if provider != nil {
+				provider.Close()
+			}
+		})
+	}
+}
+
+// TestConnectionStringSecurityProperties verifies security properties of connection handling
+func TestConnectionStringSecurityProperties(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("error_does_not_leak_connection_string", func(t *testing.T) {
+		// Use a malformed connection string that will actually cause a parsing error
+		connStr := "postgresql://UNIQUE_USER_12345:UNIQUE_PASS_67890@[::invalid/UNIQUE_DB_ABC"
+
+		provider, err := pg.NewTypeProviderWithConnection(ctx, connStr)
+
+		if err != nil {
+			errorMsg := err.Error()
+			t.Logf("Error message: %s", errorMsg)
+
+			// Verify none of the unique markers appear in error
+			assert.NotContains(t, errorMsg, "UNIQUE_USER_12345")
+			assert.NotContains(t, errorMsg, "UNIQUE_PASS_67890")
+			assert.NotContains(t, errorMsg, "UNIQUE_DB_ABC")
+
+			// Should be our generic message
+			assert.Equal(t, "failed to create connection pool", errorMsg)
+		}
+
+		if provider != nil {
+			provider.Close()
+		}
+	})
+
+	t.Run("length_validation_happens_before_parsing", func(t *testing.T) {
+		// Create a very long invalid connection string
+		longConnStr := "postgresql://user:password@host/db?" + strings.Repeat("x", 2000)
+
+		provider, err := pg.NewTypeProviderWithConnection(ctx, longConnStr)
+		require.Error(t, err)
+		require.Nil(t, provider)
+
+		// Should fail with length error, not parsing error
+		assert.Contains(t, err.Error(), "exceeds maximum length")
+		assert.NotContains(t, err.Error(), "user")
+		assert.NotContains(t, err.Error(), "password")
+	})
+
+	t.Run("no_connection_details_in_error_type", func(t *testing.T) {
+		// Use malformed string that will cause parse error
+		connStr := "postgresql://secret_user:secret_pass@[invalid"
+
+		provider, err := pg.NewTypeProviderWithConnection(ctx, connStr)
+
+		if err != nil {
+			// Error should not expose details
+			errorMsg := err.Error()
+			t.Logf("Error message: %s", errorMsg)
+
+			assert.Equal(t, "failed to create connection pool", errorMsg)
+
+			// Verify it's a plain error without wrapped details
+			assert.NotContains(t, errorMsg, "secret_user")
+			assert.NotContains(t, errorMsg, "secret_pass")
+		}
+
+		if provider != nil {
+			provider.Close()
+		}
+	})
+}
+
+// TestConnectionStringLengthConstant verifies the constant value is reasonable
+func TestConnectionStringLengthConstant(t *testing.T) {
+	// Verify constant is set to expected value
+	assert.Equal(t, 1000, pg.MaxConnectionStringLength,
+		"MaxConnectionStringLength should be 1000 (aligns with ODBC standard of 1024)")
+
+	// Verify typical connection strings fit well within limit
+	typicalConnStr := "postgresql://username:password@hostname.example.com:5432/database_name?sslmode=require&connect_timeout=10"
+	assert.Less(t, len(typicalConnStr), pg.MaxConnectionStringLength,
+		"Typical connection string should fit within limit")
+
+	// Verify even complex connection strings fit
+	complexConnStr := "postgresql://my_application_user:My$ecureP@ssw0rd!@prod-db-primary.us-east-1.company.internal:5432/application_database?sslmode=verify-full&sslrootcert=/etc/ssl/certs/ca-bundle.crt&connect_timeout=30&application_name=MyApp"
+	assert.Less(t, len(complexConnStr), pg.MaxConnectionStringLength,
+		"Complex connection string should fit within limit")
+}