Repoint handler tests at the WithSecurity constructors

docs/changelog.md

@@ -253,6 +253,18 @@ project adheres to
   512M; the collector connection-pool and timeout options are now
   documented in the sample configuration. (#308)
 
+### Removed
+
+- Remove the superseded `NewConnectionHandler` and
+  `NewNotificationChannelHandler` constructors, along with the
+  `DefaultHostValidator` helper that only they used. The server has
+  wired both handlers through the `NewConnectionHandlerWithSecurity` and
+  `NewNotificationChannelHandlerWithSecurity` variants for some time, so
+  the shorter forms were unreachable in production whilst 49 test call
+  sites still used them. Those call sites now use the `WithSecurity`
+  constructors directly, passing the same host-validation settings the
+  removed helper supplied, so test behaviour is unchanged.
+
 ### Security
 
 - Ignore a blank password when updating a database connection, so an

server/src/internal/api/connection_handlers.go

@@ -36,16 +36,6 @@ type ConnectionHandler struct {
 	visibilityListerFn func() auth.ConnectionVisibilityLister
 }
 
-// NewConnectionHandler creates a new connection handler
-func NewConnectionHandler(datastore *database.Datastore, authStore *auth.AuthStore, rbacChecker *auth.RBACChecker) *ConnectionHandler {
-	return &ConnectionHandler{
-		datastore:     datastore,
-		authStore:     authStore,
-		hostValidator: DefaultHostValidator(),
-		rbacChecker:   rbacChecker,
-	}
-}
-
 // NewConnectionHandlerWithSecurity creates a new connection handler with custom security settings
 func NewConnectionHandlerWithSecurity(datastore *database.Datastore, authStore *auth.AuthStore,
 	rbacChecker *auth.RBACChecker, allowInternal bool, allowedHosts, blockedHosts []string) *ConnectionHandler {

server/src/internal/api/connection_handlers_test.go

@@ -26,7 +26,7 @@ import (
 )
 
 func TestNewConnectionHandler(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 	if handler == nil {
 		t.Fatal("NewConnectionHandler returned nil")
 	}
@@ -79,7 +79,7 @@ func TestConnectionHandler_HandleNotConfigured(t *testing.T) {
 }
 
 func TestConnectionHandler_HandleConnections_MethodNotAllowed(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	tests := []struct {
 		name   string
@@ -111,7 +111,7 @@ func TestConnectionHandler_HandleConnections_MethodNotAllowed(t *testing.T) {
 }
 
 func TestConnectionHandler_HandleConnectionSubpath_InvalidID(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/connections/abc", nil)
 	rec := httptest.NewRecorder()
@@ -133,7 +133,7 @@ func TestConnectionHandler_HandleConnectionSubpath_InvalidID(t *testing.T) {
 }
 
 func TestConnectionHandler_HandleConnectionSubpath_MethodNotAllowed(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodPatch, "/api/v1/connections/1", nil)
 	rec := httptest.NewRecorder()
@@ -151,7 +151,7 @@ func TestConnectionHandler_HandleConnectionSubpath_MethodNotAllowed(t *testing.T
 }
 
 func TestConnectionHandler_HandleConnectionSubpath_DatabasesMethodNotAllowed(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodPost, "/api/v1/connections/1/databases", nil)
 	rec := httptest.NewRecorder()
@@ -169,7 +169,7 @@ func TestConnectionHandler_HandleConnectionSubpath_DatabasesMethodNotAllowed(t *
 }
 
 func TestConnectionHandler_HandleCurrentConnection_MethodNotAllowed(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodPatch, "/api/v1/connections/current", nil)
 	req.Header.Set("Authorization", "Bearer testtoken")
@@ -188,7 +188,7 @@ func TestConnectionHandler_HandleCurrentConnection_MethodNotAllowed(t *testing.T
 }
 
 func TestConnectionHandler_HandleCurrentConnection_MissingAuth(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/connections/current", nil)
 	rec := httptest.NewRecorder()
@@ -210,7 +210,7 @@ func TestConnectionHandler_HandleCurrentConnection_MissingAuth(t *testing.T) {
 }
 
 func TestConnectionHandler_RegisterRoutes_NotConfigured(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 	mux := http.NewServeMux()
 	noopWrapper := func(h http.HandlerFunc) http.HandlerFunc { return h }
 
@@ -237,7 +237,7 @@ func TestConnectionHandler_RegisterRoutes_NotConfigured(t *testing.T) {
 func TestConnectionHandler_CreateConnection_NoAuth(t *testing.T) {
 	// Test that createConnection requires authentication
 	rbac := auth.NewRBACChecker(nil)
-	handler := NewConnectionHandler(nil, nil, rbac)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, rbac, false, nil, nil)
 
 	body, _ := json.Marshal(ConnectionCreateRequest{
 		Name:         "test",
@@ -273,7 +273,7 @@ func TestConnectionHandler_CreateConnection_NoAuth(t *testing.T) {
 func TestConnectionHandler_UpdateConnection_NoAuth(t *testing.T) {
 	// Test that updateConnection requires authentication
 	rbac := auth.NewRBACChecker(nil)
-	handler := NewConnectionHandler(nil, nil, rbac)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, rbac, false, nil, nil)
 
 	body, _ := json.Marshal(ConnectionFullUpdateRequest{})
 	req := httptest.NewRequest(http.MethodPut, "/api/v1/connections/1",
@@ -299,7 +299,7 @@ func TestConnectionHandler_UpdateConnection_NoAuth(t *testing.T) {
 }
 
 func TestConnectionHandler_SetCurrentConnection_InvalidConnectionID(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	body, _ := json.Marshal(CurrentConnectionRequest{ConnectionID: 0})
 	req := httptest.NewRequest(http.MethodPost, "/api/v1/connections/current",
@@ -453,7 +453,7 @@ func TestCurrentConnectionResponse_JSON(t *testing.T) {
 }
 
 func TestConnectionHandler_HandleSubpath_NotFound(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	// Test unknown subpath
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/connections/1/unknown", nil)
@@ -467,7 +467,7 @@ func TestConnectionHandler_HandleSubpath_NotFound(t *testing.T) {
 }
 
 func TestConnectionHandler_HandleSubpath_EmptyPath(t *testing.T) {
-	handler := NewConnectionHandler(nil, nil, nil)
+	handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/connections/", nil)
 	rec := httptest.NewRecorder()
@@ -649,7 +649,7 @@ func TestListConnectionsScopedTokenReturnsScopedConnection(t *testing.T) {
 			return ds.GetConnectionSharingInfo(ctx, id)
 		},
 	)
-	handler := NewConnectionHandler(ds, store, checker)
+	handler := NewConnectionHandlerWithSecurity(ds, store, checker, false, nil, nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/v1/connections", nil)
 	ctx := req.Context()
@@ -761,7 +761,7 @@ func newListConnectionsIssue68Handler(ds *database.Datastore, store *auth.AuthSt
 			return ds.GetConnectionSharingInfo(ctx, id)
 		},
 	)
-	return NewConnectionHandler(ds, store, checker)
+	return NewConnectionHandlerWithSecurity(ds, store, checker, false, nil, nil)
 }
 
 // TestListConnections_Issue68_Superuser_ReturnsAllConnections locks in

server/src/internal/api/host_validation.go

@@ -196,10 +196,3 @@ func (v *HostValidator) ValidatePort(port int) error {
 
 	return nil
 }
-
-// DefaultHostValidator returns a validator with secure defaults:
-// - Blocks internal network connections
-// - No allowed/blocked host lists
-func DefaultHostValidator() *HostValidator {
-	return NewHostValidator(false, nil, nil)
-}

server/src/internal/api/host_validation_test.go

@@ -67,22 +67,6 @@ func TestNewHostValidator(t *testing.T) {
 	}
 }
 
-func TestDefaultHostValidator(t *testing.T) {
-	v := DefaultHostValidator()
-	if v == nil {
-		t.Fatal("DefaultHostValidator returned nil")
-	}
-	if v.AllowInternalNetworks {
-		t.Error("Default validator should not allow internal networks")
-	}
-	if len(v.AllowedHosts) != 0 {
-		t.Error("Default validator should have empty allowed hosts")
-	}
-	if len(v.BlockedHosts) != 0 {
-		t.Error("Default validator should have empty blocked hosts")
-	}
-}
-
 func TestHostValidator_ValidateHost(t *testing.T) {
 	tests := []struct {
 		name          string
@@ -239,7 +223,7 @@ func TestHostValidator_ValidateHost(t *testing.T) {
 }
 
 func TestHostValidator_ValidatePort(t *testing.T) {
-	v := DefaultHostValidator()
+	v := NewHostValidator(false, nil, nil)
 
 	tests := []struct {
 		name        string
@@ -403,7 +387,7 @@ func TestHostValidator_CIDRParsing(t *testing.T) {
 }
 
 func TestHostValidator_InternalNetworksList(t *testing.T) {
-	v := DefaultHostValidator()
+	v := NewHostValidator(false, nil, nil)
 
 	// Test that common internal ranges are blocked
 	internalIPs := []string{

server/src/internal/api/issue269_connection_name_test.go

@@ -216,7 +216,7 @@ func TestConnectionHandler_UpdateConnection_Issue269_InvalidChars(t *testing.T)
 	seedIssue269Connection(t, pool, connID, owner, "valid-name")
 
 	checker := auth.NewRBACChecker(store)
-	handler := NewConnectionHandler(ds, store, checker)
+	handler := NewConnectionHandlerWithSecurity(ds, store, checker, false, nil, nil)
 
 	invalid := issue269InvalidName
 	body, _ := json.Marshal(ConnectionFullUpdateRequest{Name: &invalid})
@@ -254,7 +254,7 @@ func TestConnectionHandler_UpdateConnection_Issue269_ValidNameSucceeds(t *testin
 	seedIssue269Connection(t, pool, connID, owner, "old-name")
 
 	checker := auth.NewRBACChecker(store)
-	handler := NewConnectionHandler(ds, store, checker)
+	handler := NewConnectionHandlerWithSecurity(ds, store, checker, false, nil, nil)
 
 	valid := "New Cluster (primary) - east_1.db"
 	body, _ := json.Marshal(ConnectionFullUpdateRequest{Name: &valid})

server/src/internal/api/notification_channel_handlers.go

@@ -32,20 +32,6 @@ type NotificationChannelHandler struct {
 	checkPermission func(http.ResponseWriter, *http.Request) bool
 }
 
-// NewNotificationChannelHandler creates a new notification channel handler
-func NewNotificationChannelHandler(datastore *database.Datastore, authStore *auth.AuthStore, rbacChecker *auth.RBACChecker) *NotificationChannelHandler {
-	h := &NotificationChannelHandler{
-		datastore:     datastore,
-		authStore:     authStore,
-		rbacChecker:   rbacChecker,
-		hostValidator: DefaultHostValidator(),
-	}
-	if rbacChecker != nil {
-		h.checkPermission = RequireAdminPermission(rbacChecker, auth.PermManageNotificationChannels, "manage notification channels")
-	}
-	return h
-}
-
 // NewNotificationChannelHandlerWithSecurity creates a new notification channel handler with custom security settings
 func NewNotificationChannelHandlerWithSecurity(datastore *database.Datastore, authStore *auth.AuthStore,
 	rbacChecker *auth.RBACChecker, allowInternal bool, allowedHosts, blockedHosts []string) *NotificationChannelHandler {

server/src/internal/api/notification_channel_handlers_test.go

@@ -39,7 +39,7 @@ import (
 // the handler is constructed without a datastore, every route under
 // `/api/v1/notification-channels` returns 503.
 func TestNotificationChannelHandler_NotConfiguredRoutes(t *testing.T) {
-	handler := NewNotificationChannelHandler(nil, nil, nil)
+	handler := NewNotificationChannelHandlerWithSecurity(nil, nil, nil, false, nil, nil)
 	mux := http.NewServeMux()
 	noopWrapper := func(h http.HandlerFunc) http.HandlerFunc { return h }
 	handler.RegisterRoutes(mux, noopWrapper)
@@ -67,7 +67,7 @@ func TestNotificationChannelHandler_NotConfiguredRoutes(t *testing.T) {
 func TestNotificationChannelHandler_MethodNotAllowed(t *testing.T) {
 	authStore, cleanup := newAuthStoreForChannelTests(t)
 	defer cleanup()
-	handler := NewNotificationChannelHandler(nil, authStore, auth.NewRBACChecker(authStore))
+	handler := NewNotificationChannelHandlerWithSecurity(nil, authStore, auth.NewRBACChecker(authStore), false, nil, nil)
 
 	cases := []struct {
 		path        string
@@ -115,7 +115,7 @@ func TestNotificationChannelHandler_MethodNotAllowed(t *testing.T) {
 func TestNotificationChannelHandler_InvalidIDs(t *testing.T) {
 	authStore, cleanup := newAuthStoreForChannelTests(t)
 	defer cleanup()
-	handler := NewNotificationChannelHandler(nil, authStore, auth.NewRBACChecker(authStore))
+	handler := NewNotificationChannelHandlerWithSecurity(nil, authStore, auth.NewRBACChecker(authStore), false, nil, nil)
 
 	cases := []struct {
 		path string
@@ -147,7 +147,7 @@ func TestNotificationChannelHandler_InvalidIDs(t *testing.T) {
 func TestNotificationChannelHandler_PermissionRequired(t *testing.T) {
 	authStore, cleanup := newAuthStoreForChannelTests(t)
 	defer cleanup()
-	handler := NewNotificationChannelHandler(nil, authStore, auth.NewRBACChecker(authStore))
+	handler := NewNotificationChannelHandlerWithSecurity(nil, authStore, auth.NewRBACChecker(authStore), false, nil, nil)
 
 	for _, method := range []string{http.MethodGet, http.MethodPost} {
 		req := httptest.NewRequest(method, "/api/v1/notification-channels", nil)
@@ -172,7 +172,7 @@ func TestNotificationChannelHandler_PermissionRequired(t *testing.T) {
 func TestNotificationChannelHandler_NotFoundPaths(t *testing.T) {
 	authStore, cleanup := newAuthStoreForChannelTests(t)
 	defer cleanup()
-	handler := NewNotificationChannelHandler(nil, authStore, auth.NewRBACChecker(authStore))
+	handler := NewNotificationChannelHandlerWithSecurity(nil, authStore, auth.NewRBACChecker(authStore), false, nil, nil)
 
 	paths := []string{
 		"/api/v1/notification-channels/",
@@ -303,7 +303,7 @@ func setupChannelHandler(t *testing.T, ds *database.Datastore) (*NotificationCha
 	userID := setupUserWithPermission(t, authStore, "channel_admin",
 		auth.PermManageNotificationChannels)
 	checker := auth.NewRBACChecker(authStore)
-	handler := NewNotificationChannelHandler(ds, authStore, checker)
+	handler := NewNotificationChannelHandlerWithSecurity(ds, authStore, checker, false, nil, nil)
 	return handler, userID, cleanup
 }
 
@@ -1178,7 +1178,7 @@ func TestCreateChannel_ValidationErrors(t *testing.T) {
 	userID := setupUserWithPermission(t, authStore, "ch_validator",
 		auth.PermManageNotificationChannels)
 	checker := auth.NewRBACChecker(authStore)
-	handler := NewNotificationChannelHandler(nil, authStore, checker)
+	handler := NewNotificationChannelHandlerWithSecurity(nil, authStore, checker, false, nil, nil)
 
 	cases := []struct {
 		name string

server/src/internal/api/query_handlers_test.go

@@ -29,7 +29,7 @@ import (
 // RBAC checks pass without requiring a database.
 func newTestConnectionHandlerWithRBAC() *ConnectionHandler {
 	rbac := auth.NewRBACChecker(nil)
-	return NewConnectionHandler(nil, nil, rbac)
+	return NewConnectionHandlerWithSecurity(nil, nil, rbac, false, nil, nil)
 }
 
 func TestExecuteQuery_MethodNotAllowed(t *testing.T) {