From 971560c4e7f0b87ad6cecc50f3aa06a3cfa6b3cf Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:32:26 +0300 Subject: [PATCH 1/2] refactor(httpapi): type the controller seams and fail closed without a config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The management service travelled from internal/runtime through httpapi.ServerController as interface{}, and twelve handler sites in internal/httpapi re-derived its method set with ad-hoc anonymous interface assertions. Five of those were unchecked, so a method-set drift became a panic recovered by chi into an opaque 500 with no log line naming the cause; the other seven degraded to a runtime ok=false and a 500. management.Service already describes exactly what those handlers call and internal/httpapi already imports internal/management, so the seam is typed to it end to end (runtime field + setter + getter, internal/server.Server, httpapi.ServerController) and all twelve assertions are deleted. The three equivalents on the MCP surface in internal/server/mcp.go go with them. apiKeyAuthMiddleware had two "allow through (testing scenario)" branches on GetCurrentConfig(). Typing the getter to *config.Config makes the non-*config.Config branch unrepresentable, and the remaining nil path now fails CLOSED with 503: no configuration means there is nothing to authenticate against, so the request cannot be authenticated. In production Runtime.GetCurrentConfig() wrapped a *config.Config and runtime.New rejects a nil config, so this was not an exploitable bypass there — but it was a live one for any other implementor, and 37 test requests authenticated by not authenticating. Three security tests (Spec 099 FR-018a's disclosure floor, and auth.AuthorizeServerOp's unrestricted-on-absent-AuthContext default) reached their handlers through the deleted branch. They are rewritten to drive the handler and the subtree gate directly with a no-AuthContext request, keeping the same assertions, rather than deleted with the branch. index_search_scoped does the same: the middleware would otherwise replace the AuthContext those tests inject and turn every scoped fixture into an admin one. Verified on a live instance: all twelve converted call sites exercised over REST and MCP, auth still required, no panics. Co-Authored-By: Claude Opus 5 --- internal/httpapi/activity.go | 5 +- internal/httpapi/activity_call_parity_test.go | 2 +- internal/httpapi/activity_handlers_test.go | 2 +- .../httpapi/activity_summary_scope_test.go | 2 +- internal/httpapi/activity_usage_test.go | 2 +- internal/httpapi/agent_token_gating_test.go | 2 +- internal/httpapi/annotation_coverage_test.go | 3 + internal/httpapi/auth_middleware_test.go | 42 +++++- .../auth_provider_probe_personal_test.go | 2 +- .../httpapi/code_exec_route_timeout_test.go | 2 +- internal/httpapi/code_exec_test.go | 5 +- internal/httpapi/code_scripts_test.go | 2 +- internal/httpapi/concurrency_shed_test.go | 2 +- .../httpapi/config_apply_error_status_test.go | 2 +- .../httpapi/config_patch_removed_keys_test.go | 4 +- internal/httpapi/contracts_test.go | 21 ++- internal/httpapi/diagnostics_fix_test.go | 2 +- .../diagnostics_per_server_redact_test.go | 2 +- internal/httpapi/docker_status_test.go | 2 +- internal/httpapi/handlers_test.go | 12 +- internal/httpapi/import_test.go | 2 +- internal/httpapi/index_search_scoped_test.go | 8 +- internal/httpapi/info_launched_by_test.go | 2 + internal/httpapi/info_update_policy_test.go | 1 + internal/httpapi/management_contract_test.go | 126 +++++++++++++++++ internal/httpapi/patch_server_test.go | 2 +- internal/httpapi/preflight_bench_test.go | 2 +- internal/httpapi/preflight_test.go | 2 +- internal/httpapi/profiles_test.go | 4 +- internal/httpapi/registry_add_test.go | 2 + internal/httpapi/registry_resilience_test.go | 10 ++ .../round9_config_import_doors_test.go | 4 +- internal/httpapi/routing_test.go | 2 +- internal/httpapi/scope_reveal_harness_test.go | 35 ++++- internal/httpapi/scope_reveal_test.go | 25 ++-- internal/httpapi/scope_round9_test.go | 11 +- internal/httpapi/search_tools_test.go | 1 + .../security_scanner_redaction_write_test.go | 2 + internal/httpapi/security_scanner_test.go | 41 +++++- internal/httpapi/security_test.go | 7 +- internal/httpapi/server.go | 127 ++++++++---------- internal/httpapi/server_config_patch_test.go | 2 +- internal/httpapi/server_global_tools_test.go | 4 +- internal/httpapi/server_logs_decode_test.go | 1 + .../httpapi/server_path_param_decode_test.go | 3 + internal/httpapi/session_principal_test.go | 2 +- internal/httpapi/sessions_handlers_test.go | 2 +- internal/httpapi/telemetry_payload_test.go | 2 +- .../httpapi/tenant_allowlist_walk_test.go | 2 +- internal/httpapi/tokens_test.go | 2 +- internal/httpapi/tool_hash_disclosure_test.go | 41 +++--- internal/httpapi/tool_quarantine_test.go | 6 +- internal/httpapi/update_failure_test.go | 2 +- internal/runtime/event_bus_payload_test.go | 8 ++ internal/runtime/runtime.go | 27 ++-- internal/server/mcp.go | 12 +- internal/server/server.go | 6 +- internal/server/update_failure.go | 2 +- .../api/user_activity_wired_test.go | 2 +- 59 files changed, 470 insertions(+), 190 deletions(-) create mode 100644 internal/httpapi/management_contract_test.go diff --git a/internal/httpapi/activity.go b/internal/httpapi/activity.go index 13906f848..ad361cf49 100644 --- a/internal/httpapi/activity.go +++ b/internal/httpapi/activity.go @@ -1127,9 +1127,8 @@ func (s *Server) handleActivityUsage(w http.ResponseWriter, r *http.Request) { // the value hot-reloads with config. func (s *Server) usageCacheTTL() time.Duration { def := time.Duration(config.DefaultObservabilityConfig().UsageCacheTTL) - cfgIface := s.controller.GetCurrentConfig() - cfg, ok := cfgIface.(*config.Config) - if !ok || cfg == nil || cfg.Observability == nil { + cfg := s.controller.GetCurrentConfig() + if cfg == nil || cfg.Observability == nil { return def } if d := time.Duration(cfg.Observability.UsageCacheTTL); d > 0 { diff --git a/internal/httpapi/activity_call_parity_test.go b/internal/httpapi/activity_call_parity_test.go index 5153436d9..fb79d29bb 100644 --- a/internal/httpapi/activity_call_parity_test.go +++ b/internal/httpapi/activity_call_parity_test.go @@ -45,7 +45,7 @@ type callParityController struct { snap *internalRuntime.UsageAggregate } -func (m *callParityController) GetCurrentConfig() any { +func (m *callParityController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "test-key", Observability: config.DefaultObservabilityConfig()} } diff --git a/internal/httpapi/activity_handlers_test.go b/internal/httpapi/activity_handlers_test.go index 08b3d79a0..c81055018 100644 --- a/internal/httpapi/activity_handlers_test.go +++ b/internal/httpapi/activity_handlers_test.go @@ -27,7 +27,7 @@ type mockActivityController struct { activities []*storage.ActivityRecord } -func (m *mockActivityController) GetCurrentConfig() any { +func (m *mockActivityController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } diff --git a/internal/httpapi/activity_summary_scope_test.go b/internal/httpapi/activity_summary_scope_test.go index 594867c42..163f0b060 100644 --- a/internal/httpapi/activity_summary_scope_test.go +++ b/internal/httpapi/activity_summary_scope_test.go @@ -25,7 +25,7 @@ type summaryScopeController struct { activities []*storage.ActivityRecord } -func (m *summaryScopeController) GetCurrentConfig() any { +func (m *summaryScopeController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "test-key"} } diff --git a/internal/httpapi/activity_usage_test.go b/internal/httpapi/activity_usage_test.go index fe1f81f92..38c79da73 100644 --- a/internal/httpapi/activity_usage_test.go +++ b/internal/httpapi/activity_usage_test.go @@ -32,7 +32,7 @@ type mockUsageController struct { scanCalls int } -func (m *mockUsageController) GetCurrentConfig() any { +func (m *mockUsageController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey, Observability: config.DefaultObservabilityConfig()} } diff --git a/internal/httpapi/agent_token_gating_test.go b/internal/httpapi/agent_token_gating_test.go index 3c401860a..bb5a14b2f 100644 --- a/internal/httpapi/agent_token_gating_test.go +++ b/internal/httpapi/agent_token_gating_test.go @@ -20,7 +20,7 @@ type adminConfigController struct { apiKey string } -func (c *adminConfigController) GetCurrentConfig() interface{} { +func (c *adminConfigController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: c.apiKey} } diff --git a/internal/httpapi/annotation_coverage_test.go b/internal/httpapi/annotation_coverage_test.go index 300593a55..148a965b8 100644 --- a/internal/httpapi/annotation_coverage_test.go +++ b/internal/httpapi/annotation_coverage_test.go @@ -84,6 +84,7 @@ func TestHandleAnnotationCoverage(t *testing.T) { srv := NewServer(ctrl, logger, nil) req := httptest.NewRequest("GET", "/api/v1/annotations/coverage", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) // Add API key bypass by setting request source to socket (trusted) req.Header.Set("X-Request-Source", "socket") w := httptest.NewRecorder() @@ -140,6 +141,7 @@ func TestAnnotationCoverage_EmptyServers(t *testing.T) { srv := NewServer(ctrl, logger, nil) req := httptest.NewRequest("GET", "/api/v1/annotations/coverage", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("X-Request-Source", "socket") w := httptest.NewRecorder() @@ -195,6 +197,7 @@ func TestAnnotationCoverage_TitleOnlyNotCounted(t *testing.T) { srv := NewServer(ctrl, logger, nil) req := httptest.NewRequest("GET", "/api/v1/annotations/coverage", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("X-Request-Source", "socket") w := httptest.NewRecorder() diff --git a/internal/httpapi/auth_middleware_test.go b/internal/httpapi/auth_middleware_test.go index eed501d29..6e3e67e5c 100644 --- a/internal/httpapi/auth_middleware_test.go +++ b/internal/httpapi/auth_middleware_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -47,7 +48,7 @@ type testControllerWithConfig struct { cfg *config.Config } -func (m *testControllerWithConfig) GetCurrentConfig() interface{} { +func (m *testControllerWithConfig) GetCurrentConfig() *config.Config { return m.cfg } @@ -399,3 +400,42 @@ func TestAPIKeyAuth_NoTokenStore_RejectsAgentToken(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, w.Code, "Agent token should be rejected when token store is not configured") } + +// --- SEC-02: the middleware must fail CLOSED when it cannot read a config --- + +// failClosedController models a ServerController that hands the middleware no +// configuration at all. apiKeyAuthMiddleware used to read that as a "testing +// scenario" and forward the request unauthenticated; there is no configuration +// to authenticate against, so the only safe answer is to refuse. +// +// reachedHandler is the real oracle: asserting only on the status code passes +// vacuously if some unrelated 503 fires before routing. +type failClosedController struct { + baseController + reachedHandler atomic.Bool +} + +func (c *failClosedController) GetCurrentConfig() *config.Config { return nil } + +// GetAllServers is the sentinel: GET /api/v1/servers falls back to it because +// baseController has no management service. +func (c *failClosedController) GetAllServers() ([]map[string]interface{}, error) { + c.reachedHandler.Store(true) + return []map[string]interface{}{}, nil +} + +func TestAPIKeyAuth_NilConfigFailsClosed(t *testing.T) { + logger := zap.NewNop().Sugar() + ctrl := &failClosedController{} + srv := NewServer(ctrl, logger, nil) + + // A plain TCP request with no credentials of any kind. + req := httptest.NewRequest("GET", "/api/v1/servers", nil) + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + + assert.Equal(t, http.StatusServiceUnavailable, w.Code, + "an unreadable config must refuse the request, not forward it unauthenticated") + assert.False(t, ctrl.reachedHandler.Load(), + "the handler must NOT run: a nil config means the request was never authenticated") +} diff --git a/internal/httpapi/auth_provider_probe_personal_test.go b/internal/httpapi/auth_provider_probe_personal_test.go index 363a96ea1..a4dd7b79c 100644 --- a/internal/httpapi/auth_provider_probe_personal_test.go +++ b/internal/httpapi/auth_provider_probe_personal_test.go @@ -39,7 +39,7 @@ type providerProbePersonalController struct { cfg *config.Config } -func (m *providerProbePersonalController) GetCurrentConfig() any { return m.cfg } +func (m *providerProbePersonalController) GetCurrentConfig() *config.Config { return m.cfg } func (m *providerProbePersonalController) GetConfig() (*config.Config, error) { return m.cfg, nil } diff --git a/internal/httpapi/code_exec_route_timeout_test.go b/internal/httpapi/code_exec_route_timeout_test.go index f2dfbee01..10049b3e7 100644 --- a/internal/httpapi/code_exec_route_timeout_test.go +++ b/internal/httpapi/code_exec_route_timeout_test.go @@ -25,7 +25,7 @@ type codeExecDeadlineController struct { hasDL bool } -func (c *codeExecDeadlineController) GetCurrentConfig() interface{} { return c.cfg } +func (c *codeExecDeadlineController) GetCurrentConfig() *config.Config { return c.cfg } func (c *codeExecDeadlineController) CallTool(ctx context.Context, _ string, _ map[string]interface{}) (interface{}, error) { c.deadline, c.hasDL = ctx.Deadline() diff --git a/internal/httpapi/code_exec_test.go b/internal/httpapi/code_exec_test.go index 0cbf569a9..458be56b4 100644 --- a/internal/httpapi/code_exec_test.go +++ b/internal/httpapi/code_exec_test.go @@ -11,6 +11,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" "github.com/smart-mcp-proxy/mcpproxy-go/internal/httpapi" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -70,7 +71,7 @@ func (m *mockController) GetConfigPath() string { return "" func (m *mockController) GetLogDir() string { return "" } func (m *mockController) TriggerOAuthLogin(serverName string) error { return nil } func (m *mockController) GetSecretResolver() interface{} { return nil } -func (m *mockController) GetCurrentConfig() interface{} { return nil } +func (m *mockController) GetCurrentConfig() *config.Config { return nil } func (m *mockController) NotifySecretsChanged(ctx context.Context, operation, secretName string) error { return nil } @@ -109,7 +110,7 @@ func (m *mockController) RemoveRegistrySourceRef(_ string) (*config.RegistryEntr func (m *mockController) EditRegistrySourceRef(_, _, _, _ string) (*config.RegistryEntry, *contracts.RegistryAddError, error) { return nil, nil, nil } -func (m *mockController) GetManagementService() interface{} { return nil } +func (m *mockController) GetManagementService() management.Service { return nil } func (m *mockController) GetRuntime() interface{} { return nil } func (m *mockController) GetSessions(limit, offset int) (interface{}, int, error) { return nil, 0, nil } func (m *mockController) GetSessionByID(id string) (interface{}, error) { return nil, nil } diff --git a/internal/httpapi/code_scripts_test.go b/internal/httpapi/code_scripts_test.go index ac143463a..1c0c0b181 100644 --- a/internal/httpapi/code_scripts_test.go +++ b/internal/httpapi/code_scripts_test.go @@ -25,7 +25,7 @@ type codeScriptsController struct { configPath string } -func (c *codeScriptsController) GetCurrentConfig() interface{} { +func (c *codeScriptsController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: c.apiKey} } diff --git a/internal/httpapi/concurrency_shed_test.go b/internal/httpapi/concurrency_shed_test.go index a4bf369a0..2946b6177 100644 --- a/internal/httpapi/concurrency_shed_test.go +++ b/internal/httpapi/concurrency_shed_test.go @@ -30,7 +30,7 @@ type shedController struct { err error } -func (m *shedController) GetCurrentConfig() any { +func (m *shedController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/config_apply_error_status_test.go b/internal/httpapi/config_apply_error_status_test.go index e2e6b4609..63241374d 100644 --- a/internal/httpapi/config_apply_error_status_test.go +++ b/internal/httpapi/config_apply_error_status_test.go @@ -26,7 +26,7 @@ type applyErrController struct { err error } -func (m *applyErrController) GetCurrentConfig() any { return &config.Config{APIKey: "k"} } +func (m *applyErrController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "k"} } func (m *applyErrController) GetConfig() (*config.Config, error) { return &config.Config{Listen: "127.0.0.1:8080"}, nil } diff --git a/internal/httpapi/config_patch_removed_keys_test.go b/internal/httpapi/config_patch_removed_keys_test.go index fd2560071..2b27c088f 100644 --- a/internal/httpapi/config_patch_removed_keys_test.go +++ b/internal/httpapi/config_patch_removed_keys_test.go @@ -51,7 +51,9 @@ type removedKeysController struct { applied int } -func (m *removedKeysController) GetCurrentConfig() any { return &config.Config{APIKey: "test-key"} } +func (m *removedKeysController) GetCurrentConfig() *config.Config { + return &config.Config{APIKey: "test-key"} +} func (m *removedKeysController) GetConfig() (*config.Config, error) { return m.live, nil } diff --git a/internal/httpapi/contracts_test.go b/internal/httpapi/contracts_test.go index 039739f61..e2c86304c 100644 --- a/internal/httpapi/contracts_test.go +++ b/internal/httpapi/contracts_test.go @@ -16,6 +16,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" internalRuntime "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" @@ -28,7 +29,7 @@ import ( type MockServerController struct{} // mockManagementService provides a test implementation of management service methods -type mockManagementService struct{} +type mockManagementService struct{ management.Service } func (m *mockManagementService) ListServers(ctx context.Context) ([]*contracts.Server, *contracts.ServerStats, error) { return []*contracts.Server{ @@ -91,7 +92,7 @@ func (m *mockManagementService) TriggerOAuthLogout(ctx context.Context, name str func (m *MockServerController) IsRunning() bool { return true } func (m *MockServerController) GetListenAddress() string { return ":8080" } -func (m *MockServerController) GetManagementService() interface{} { +func (m *MockServerController) GetManagementService() management.Service { return &mockManagementService{} } func (m *MockServerController) GetUpstreamStats() map[string]interface{} { @@ -236,7 +237,15 @@ func (m *MockServerController) GetSecretResolver() *secret.Resolver { return nil func (m *MockServerController) NotifySecretsChanged(_ context.Context, _, _ string) error { return nil } -func (m *MockServerController) GetCurrentConfig() interface{} { return map[string]interface{}{} } + +// mockControllerAPIKey is the admin key every request through a +// MockServerController-backed server must present: since SEC-02 the auth +// middleware refuses a request it cannot authenticate instead of forwarding it. +const mockControllerAPIKey = "mock-controller-admin-key" + +func (m *MockServerController) GetCurrentConfig() *config.Config { + return &config.Config{APIKey: mockControllerAPIKey} +} // Tool call history methods func (m *MockServerController) GetToolCalls(_ int, _ int, _ storage.ToolCallScope) ([]*contracts.ToolCallRecord, int, error) { @@ -436,6 +445,7 @@ func TestAPIContractCompliance(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Create request req := httptest.NewRequest(tt.method, tt.path, http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() // Execute request @@ -572,6 +582,7 @@ func TestEndpointResponseTypes(t *testing.T) { for _, tt := range actionTests { t.Run(tt.path, func(t *testing.T) { req := httptest.NewRequest(tt.method, tt.path, http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -598,6 +609,7 @@ func TestEndpointResponseTypes(t *testing.T) { // Test login endpoint (Spec 020: returns OAuthStartResponse instead of ServerActionResponse) t.Run("/api/v1/servers/test-server/login", func(t *testing.T) { req := httptest.NewRequest("POST", "/api/v1/servers/test-server/login", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -632,6 +644,7 @@ func TestInfoEndpointReturnsVersion(t *testing.T) { server := NewServer(controller, logger, nil) req := httptest.NewRequest("GET", "/api/v1/info", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -676,6 +689,7 @@ func TestInfoEndpointIncludesUpdateInfo(t *testing.T) { server := NewServer(controller, logger, nil) req := httptest.NewRequest("GET", "/api/v1/info", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -748,6 +762,7 @@ func BenchmarkAPIResponseMarshaling(b *testing.B) { server := NewServer(controller, logger, nil) req := httptest.NewRequest("GET", "/api/v1/servers", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) b.ResetTimer() for i := 0; i < b.N; i++ { diff --git a/internal/httpapi/diagnostics_fix_test.go b/internal/httpapi/diagnostics_fix_test.go index b5c36c080..0607fe394 100644 --- a/internal/httpapi/diagnostics_fix_test.go +++ b/internal/httpapi/diagnostics_fix_test.go @@ -22,7 +22,7 @@ type mockFixController struct { apiKey string } -func (m *mockFixController) GetCurrentConfig() any { +func (m *mockFixController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/diagnostics_per_server_redact_test.go b/internal/httpapi/diagnostics_per_server_redact_test.go index 4c4b875bc..7e37079c6 100644 --- a/internal/httpapi/diagnostics_per_server_redact_test.go +++ b/internal/httpapi/diagnostics_per_server_redact_test.go @@ -22,7 +22,7 @@ type mockPerServerDiagController struct { reveal bool } -func (m *mockPerServerDiagController) GetCurrentConfig() any { +func (m *mockPerServerDiagController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/docker_status_test.go b/internal/httpapi/docker_status_test.go index dabe64bdb..95a9f1ef4 100644 --- a/internal/httpapi/docker_status_test.go +++ b/internal/httpapi/docker_status_test.go @@ -27,7 +27,7 @@ type mockDockerStatusController struct { isolationEnabled bool } -func (m *mockDockerStatusController) GetCurrentConfig() any { +func (m *mockDockerStatusController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/handlers_test.go b/internal/httpapi/handlers_test.go index 7b25214e3..bdaa8189f 100644 --- a/internal/httpapi/handlers_test.go +++ b/internal/httpapi/handlers_test.go @@ -11,6 +11,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/core" @@ -243,7 +244,7 @@ type refreshAdminController struct { apiKey string } -func (c *refreshAdminController) GetCurrentConfig() any { +func (c *refreshAdminController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: c.apiKey} } @@ -401,7 +402,7 @@ type mockAddServerController struct { captured *config.ServerConfig } -func (m *mockAddServerController) GetCurrentConfig() any { +func (m *mockAddServerController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } @@ -422,7 +423,7 @@ type mockRemoveServerController struct { existsServer string } -func (m *mockRemoveServerController) GetCurrentConfig() any { +func (m *mockRemoveServerController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } @@ -565,6 +566,7 @@ func TestRequestIDInLogs(t *testing.T) { // mockOAuthManagementService implements TriggerOAuthLoginQuick for server login tests type mockOAuthManagementService struct { + management.Service triggerError error triggerResult *core.OAuthStartResult } @@ -591,13 +593,13 @@ type mockLoginController struct { mgmtSvc *mockOAuthManagementService } -func (m *mockLoginController) GetCurrentConfig() any { +func (m *mockLoginController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } } -func (m *mockLoginController) GetManagementService() interface{} { +func (m *mockLoginController) GetManagementService() management.Service { return m.mgmtSvc } diff --git a/internal/httpapi/import_test.go b/internal/httpapi/import_test.go index 3ab053fdf..02f9606c5 100644 --- a/internal/httpapi/import_test.go +++ b/internal/httpapi/import_test.go @@ -20,7 +20,7 @@ type mockImportController struct { apiKey string } -func (m *mockImportController) GetCurrentConfig() any { +func (m *mockImportController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } diff --git a/internal/httpapi/index_search_scoped_test.go b/internal/httpapi/index_search_scoped_test.go index 7c6899bd0..8c15213b7 100644 --- a/internal/httpapi/index_search_scoped_test.go +++ b/internal/httpapi/index_search_scoped_test.go @@ -147,7 +147,13 @@ func doScopedSearch(t *testing.T, controller ServerController, ctx context.Conte req = req.WithContext(ctx) } w := httptest.NewRecorder() - server.ServeHTTP(w, req) + // The handler is invoked directly rather than through ServeHTTP: these + // tests supply the caller's AuthContext themselves, and the auth + // middleware would replace it with whatever it derives from the request's + // credentials — turning every scoped fixture below into an admin one. + // (Before SEC-02 the middleware happened to forward this request + // untouched because the mock controller had no readable config.) + server.handleSearchTools(w, req) require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) diff --git a/internal/httpapi/info_launched_by_test.go b/internal/httpapi/info_launched_by_test.go index 558e5a4b7..b62e6f7d6 100644 --- a/internal/httpapi/info_launched_by_test.go +++ b/internal/httpapi/info_launched_by_test.go @@ -40,6 +40,7 @@ func TestInfoEndpointReportsLaunchedBy(t *testing.T) { server := NewServer(&MockServerController{}, logger, nil) req := httptest.NewRequest("GET", "/api/v1/info", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -78,6 +79,7 @@ func TestInfoEndpointReportsPID(t *testing.T) { server := NewServer(&MockServerController{}, logger, nil) req := httptest.NewRequest("GET", "/api/v1/info", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) diff --git a/internal/httpapi/info_update_policy_test.go b/internal/httpapi/info_update_policy_test.go index f28dd761c..17eaf9eba 100644 --- a/internal/httpapi/info_update_policy_test.go +++ b/internal/httpapi/info_update_policy_test.go @@ -58,6 +58,7 @@ func TestInfoEndpointAlwaysReportsUpdatePolicy(t *testing.T) { server := NewServer(&policyController{policy: tt.policy}, logger, nil) req := httptest.NewRequest("GET", "/api/v1/info", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) require.Equal(t, http.StatusOK, w.Code) diff --git a/internal/httpapi/management_contract_test.go b/internal/httpapi/management_contract_test.go new file mode 100644 index 000000000..3b20787ef --- /dev/null +++ b/internal/httpapi/management_contract_test.go @@ -0,0 +1,126 @@ +package httpapi + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/core" +) + +// ARC-04: the management service must reach the REST handlers through a +// COMPILE-TIME contract, not as an interface{} each handler re-derives with an +// ad-hoc anonymous type assertion. Twelve such assertions existed; five were +// unchecked and turned any method-set drift into a recovered panic surfacing as +// an opaque 500 with no log line naming the cause. +// +// This is the defect fix itself: it cannot compile while +// ServerController.GetManagementService() returns interface{}. +var _ = func(c ServerController) management.Service { return c.GetManagementService() } + +// fullMgmtService embeds management.Service so it satisfies the whole interface +// while implementing only the methods these routes drive. The embedded +// interface is nil, so an unexpected call panics loudly — the correct failure +// for a test reaching a method it never meant to exercise. +type fullMgmtService struct { + management.Service + calls map[string]int +} + +func (m *fullMgmtService) record(name string) { + if m.calls == nil { + m.calls = map[string]int{} + } + m.calls[name]++ +} + +func (m *fullMgmtService) RestartAll(context.Context) (*management.BulkOperationResult, error) { + m.record("RestartAll") + return &management.BulkOperationResult{Total: 2, Successful: 2}, nil +} + +func (m *fullMgmtService) EnableAll(context.Context) (*management.BulkOperationResult, error) { + m.record("EnableAll") + return &management.BulkOperationResult{Total: 2, Successful: 2}, nil +} + +func (m *fullMgmtService) DisableAll(context.Context) (*management.BulkOperationResult, error) { + m.record("DisableAll") + return &management.BulkOperationResult{Total: 2, Successful: 2}, nil +} + +func (m *fullMgmtService) TriggerOAuthLoginQuick(_ context.Context, name string) (*core.OAuthStartResult, error) { + m.record("TriggerOAuthLoginQuick") + return &core.OAuthStartResult{AuthURL: "https://example.test/authorize?server=" + name, BrowserOpened: true}, nil +} + +func (m *fullMgmtService) TriggerOAuthLogout(context.Context, string) error { + m.record("TriggerOAuthLogout") + return nil +} + +func (m *fullMgmtService) GetServerTools(context.Context, string) ([]map[string]interface{}, error) { + m.record("GetServerTools") + return []map[string]interface{}{{"name": "alpha", "description": "a tool"}}, nil +} + +const mgmtContractAPIKey = "mgmt-contract-admin-key" + +// typedMgmtController hands the server a real management.Service through the +// controller seam. +type typedMgmtController struct { + baseController + svc *fullMgmtService +} + +func (c *typedMgmtController) GetCurrentConfig() *config.Config { + return &config.Config{APIKey: mgmtContractAPIKey} +} + +func (c *typedMgmtController) GetManagementService() management.Service { return c.svc } + +// TestManagementHandlers_RouteThroughTypedService drives every management-only +// route and asserts each reaches the service and returns its success code — +// never the 500 "Management service not available" the ad-hoc assertions +// produced when a method was missing from the anonymous interface. +func TestManagementHandlers_RouteThroughTypedService(t *testing.T) { + svc := &fullMgmtService{} + srv := NewServer(&typedMgmtController{svc: svc}, zap.NewNop().Sugar(), nil) + + cases := []struct { + name string + method string + path string + expect string // management.Service method the route must reach + }{ + {"restart_all", http.MethodPost, "/api/v1/servers/restart_all", "RestartAll"}, + {"enable_all", http.MethodPost, "/api/v1/servers/enable_all", "EnableAll"}, + {"disable_all", http.MethodPost, "/api/v1/servers/disable_all", "DisableAll"}, + {"login", http.MethodPost, "/api/v1/servers/github/login", "TriggerOAuthLoginQuick"}, + {"logout", http.MethodPost, "/api/v1/servers/github/logout", "TriggerOAuthLogout"}, + {"tools", http.MethodGet, "/api/v1/servers/github/tools", "GetServerTools"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(tc.method, tc.path, nil) + req.Header.Set("X-API-Key", mgmtContractAPIKey) + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code, + "%s %s should reach the management service; body=%s", tc.method, tc.path, w.Body.String()) + assert.NotContains(t, w.Body.String(), "Management service not available", + "the typed seam must not report the service missing") + assert.Equal(t, 1, svc.calls[tc.expect], + fmt.Sprintf("route %s must call management.Service.%s exactly once", tc.path, tc.expect)) + }) + } +} diff --git a/internal/httpapi/patch_server_test.go b/internal/httpapi/patch_server_test.go index f9157d5ea..611c118d7 100644 --- a/internal/httpapi/patch_server_test.go +++ b/internal/httpapi/patch_server_test.go @@ -37,7 +37,7 @@ func (m *mockPatchServerController) GetAllServers() ([]map[string]interface{}, e return m.allServers, nil } -func (m *mockPatchServerController) GetCurrentConfig() any { +func (m *mockPatchServerController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/preflight_bench_test.go b/internal/httpapi/preflight_bench_test.go index da22e00f7..a8ba5ed28 100644 --- a/internal/httpapi/preflight_bench_test.go +++ b/internal/httpapi/preflight_bench_test.go @@ -129,7 +129,7 @@ func newBenchPreflightController(servers, toolsPerServer int) *benchPreflightCon } } -func (c *benchPreflightController) GetCurrentConfig() interface{} { +func (c *benchPreflightController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: preflightTestAPIKey} } diff --git a/internal/httpapi/preflight_test.go b/internal/httpapi/preflight_test.go index 67e467f57..0024bc4e7 100644 --- a/internal/httpapi/preflight_test.go +++ b/internal/httpapi/preflight_test.go @@ -50,7 +50,7 @@ type preflightController struct { recordErr error } -func (c *preflightController) GetCurrentConfig() interface{} { +func (c *preflightController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: preflightTestAPIKey} } diff --git a/internal/httpapi/profiles_test.go b/internal/httpapi/profiles_test.go index 13a282091..a5a83cd8f 100644 --- a/internal/httpapi/profiles_test.go +++ b/internal/httpapi/profiles_test.go @@ -21,7 +21,9 @@ type mockProfilesController struct { cfg *config.Config } -func (m *mockProfilesController) GetCurrentConfig() any { return &config.Config{APIKey: m.apiKey} } +func (m *mockProfilesController) GetCurrentConfig() *config.Config { + return &config.Config{APIKey: m.apiKey} +} func (m *mockProfilesController) GetConfig() (*config.Config, error) { return m.cfg, nil } diff --git a/internal/httpapi/registry_add_test.go b/internal/httpapi/registry_add_test.go index df4291a5f..8e51ac7dc 100644 --- a/internal/httpapi/registry_add_test.go +++ b/internal/httpapi/registry_add_test.go @@ -42,6 +42,7 @@ func TestAddFromRegistry_SlashServerIDUnescaped(t *testing.T) { // microsoft/markitdown, percent-encoded as a single path segment. req := httptest.NewRequest(http.MethodPost, "/api/v1/registries/github-mcp/servers/microsoft%2Fmarkitdown/add", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -65,6 +66,7 @@ func TestAddFromRegistry_NilConfigIsAnError(t *testing.T) { srv := NewServer(ctrl, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPost, "/api/v1/registries/reg1/servers/srv1/add", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("X-API-Key", "admin-secret") w := httptest.NewRecorder() srv.ServeHTTP(w, req) diff --git a/internal/httpapi/registry_resilience_test.go b/internal/httpapi/registry_resilience_test.go index 58b383949..d835067e7 100644 --- a/internal/httpapi/registry_resilience_test.go +++ b/internal/httpapi/registry_resilience_test.go @@ -60,6 +60,7 @@ func decodeData(t *testing.T, w *httptest.ResponseRecorder, into interface{}) { func TestSearchRegistryServers_KeyMissingIsUnavailableNot500(t *testing.T) { srv := NewServer(&keyMissingController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/registries/needs-key/servers", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -80,6 +81,7 @@ func TestSearchRegistryServers_KeyMissingIsUnavailableNot500(t *testing.T) { func TestSearchRegistryServers_CacheFreshnessSurfaced(t *testing.T) { srv := NewServer(&cachedController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/registries/pulse/servers", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -100,6 +102,7 @@ func TestSearchRegistryServers_CacheFreshnessSurfaced(t *testing.T) { func TestRefreshRegistryCache_Endpoint(t *testing.T) { srv := NewServer(&refreshCountController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPost, "/api/v1/registries/pulse/refresh", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -150,6 +153,7 @@ func (c *provenanceController) ListRegistries() ([]interface{}, error) { func TestListRegistries_SurfacesProvenanceAndTrusted(t *testing.T) { srv := NewServer(&provenanceController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/registries", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -234,6 +238,7 @@ func (c *removeController) RemoveRegistrySourceRef(id string) (*config.RegistryE func TestRemoveRegistrySource_RemovesCustom(t *testing.T) { srv := NewServer(&removeController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodDelete, "/api/v1/registries/acme", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -254,6 +259,7 @@ func TestRemoveRegistrySource_RemovesCustom(t *testing.T) { func TestRemoveRegistrySource_RefusesBuiltin(t *testing.T) { srv := NewServer(&removeController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodDelete, "/api/v1/registries/official", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -266,6 +272,7 @@ func TestRemoveRegistrySource_RefusesBuiltin(t *testing.T) { func TestRemoveRegistrySource_NotFound(t *testing.T) { srv := NewServer(&removeController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodDelete, "/api/v1/registries/ghost", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -305,6 +312,7 @@ func (c *editController) EditRegistrySourceRef(id, name, rawURL, _ string) (*con func TestEditRegistrySource_UpdatesCustom(t *testing.T) { srv := NewServer(&editController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPut, "/api/v1/registries/acme", strings.NewReader(`{"name":"Acme Prod","url":"https://acme.example/api"}`)) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -329,6 +337,7 @@ func TestEditRegistrySource_UpdatesCustom(t *testing.T) { func TestEditRegistrySource_RefusesBuiltin(t *testing.T) { srv := NewServer(&editController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPut, "/api/v1/registries/official", strings.NewReader(`{"name":"hijack"}`)) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -342,6 +351,7 @@ func TestEditRegistrySource_RefusesBuiltin(t *testing.T) { func TestEditRegistrySource_NotFound(t *testing.T) { srv := NewServer(&editController{&MockServerController{}}, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPut, "/api/v1/registries/ghost", strings.NewReader(`{"name":"x"}`)) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) diff --git a/internal/httpapi/round9_config_import_doors_test.go b/internal/httpapi/round9_config_import_doors_test.go index 9d68b25e9..94ae21b8c 100644 --- a/internal/httpapi/round9_config_import_doors_test.go +++ b/internal/httpapi/round9_config_import_doors_test.go @@ -68,7 +68,7 @@ type mockRound9ConfigController struct { applied int } -func (m *mockRound9ConfigController) GetCurrentConfig() any { +func (m *mockRound9ConfigController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "test-key"} } func (m *mockRound9ConfigController) GetConfig() (*config.Config, error) { return m.live, nil } @@ -288,7 +288,7 @@ type mockRound9ImportController struct { baseController } -func (m *mockRound9ImportController) GetCurrentConfig() any { +func (m *mockRound9ImportController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "test-key"} } func (m *mockRound9ImportController) AddServer(_ context.Context, _ *config.ServerConfig) error { diff --git a/internal/httpapi/routing_test.go b/internal/httpapi/routing_test.go index 874724ff6..fad080c07 100644 --- a/internal/httpapi/routing_test.go +++ b/internal/httpapi/routing_test.go @@ -51,7 +51,7 @@ func (m *mockRoutingController) GetDesiredConfig() (*config.Config, error) { return &clone, nil } -func (m *mockRoutingController) GetCurrentConfig() any { +func (m *mockRoutingController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } diff --git a/internal/httpapi/scope_reveal_harness_test.go b/internal/httpapi/scope_reveal_harness_test.go index f9bc1aced..4fd5f8e51 100644 --- a/internal/httpapi/scope_reveal_harness_test.go +++ b/internal/httpapi/scope_reveal_harness_test.go @@ -10,12 +10,14 @@ import ( "testing" "time" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" "go.uber.org/zap" "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" internalRuntime "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) @@ -110,6 +112,11 @@ func scopeFixtureConfig(reveal bool) *config.Config { // distinct type from mockManagementService so the fixture is under this test's // control. type scopeMgmtService struct { + // Embedded so the fixture satisfies the whole typed seam while + // implementing only the two methods these tests drive. The embedded + // interface is nil: an unexpected call panics loudly rather than + // silently returning a zero value. + management.Service servers []contracts.Server } @@ -152,7 +159,7 @@ type scopeController struct { subs []chan internalRuntime.Event } -func (c *scopeController) GetManagementService() interface{} { +func (c *scopeController) GetManagementService() management.Service { if !c.withManagement { return nil } @@ -162,7 +169,7 @@ func (c *scopeController) GetManagementService() interface{} { // GetCurrentConfig must return a real *config.Config or apiKeyAuthMiddleware // forwards the request with NO AuthContext at all and every scoped assertion // below would pass for the wrong reason. -func (c *scopeController) GetCurrentConfig() interface{} { return c.cfg } +func (c *scopeController) GetCurrentConfig() *config.Config { return c.cfg } func (c *scopeController) GetConfig() (*config.Config, error) { return c.cfg, nil } @@ -368,6 +375,30 @@ func scopeGet(t *testing.T, srv *Server, path, apiKey string) *httptest.Response return rec } +// noAuthContextRequest builds a GET that reaches a handler with NO AuthContext +// in its context, supplying chi URL params directly. +// +// Until SEC-02 the auth middleware manufactured exactly this shape: a request +// whose config it could not read was forwarded to the handler unauthenticated. +// It now refuses those (503), but the floors pinned by the tests that used to +// enter this way — Spec 099 FR-018a's disclosure tier, and +// auth.AuthorizeServerOp's unrestricted-on-absence default — are properties of +// the HANDLERS and the subtree gate, not of that branch. So those tests drive +// this request straight at the code under test instead of vanishing with the +// branch that used to reach it. +func noAuthContextRequest(t *testing.T, path string, urlParams map[string]string) *http.Request { + t.Helper() + req := httptest.NewRequest(http.MethodGet, path, http.NoBody) + rctx := chi.NewRouteContext() + for k, v := range urlParams { + rctx.URLParams.Add(k, v) + } + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + require.Nil(t, auth.AuthContextFromContext(req.Context()), + "precondition: the request must carry no AuthContext") + return req +} + // scopeDecodeData decodes the `data` object of the standard API envelope. func scopeDecodeData(t *testing.T, rec *httptest.ResponseRecorder) map[string]interface{} { t.Helper() diff --git a/internal/httpapi/scope_reveal_test.go b/internal/httpapi/scope_reveal_test.go index 412cd937e..61cc644c1 100644 --- a/internal/httpapi/scope_reveal_test.go +++ b/internal/httpapi/scope_reveal_test.go @@ -74,13 +74,14 @@ func TestGetServers_RevealRequiresAuthenticatedAdmin(t *testing.T) { }) t.Run("no auth context at all is unprivileged", func(t *testing.T) { - // The middleware's testing/bootstrap passthrough forwards with NO - // AuthContext. Absence of an identity must not satisfy a gate that a - // scoped token fails. + // A handler reached with NO AuthContext: absence of an identity must + // not satisfy a gate that a scoped token fails. See + // noAuthContextRequest for why this no longer goes through the router. ctrl := &scopeController{cfg: scopeFixtureConfig(true), servers: scopeFixtureServers(), withManagement: true} - srv := NewServer(passthroughController{ctrl}, zap.NewNop().Sugar(), nil) + srv := NewServer(ctrl, zap.NewNop().Sugar(), nil) - rec := scopeGet(t, srv, "/api/v1/servers", "") + rec := httptest.NewRecorder() + srv.handleGetServers(rec, noAuthContextRequest(t, "/api/v1/servers", nil)) require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) data := scopeDecodeData(t, rec) entry := scopeServerEntry(t, data, "alpha") @@ -91,13 +92,6 @@ func TestGetServers_RevealRequiresAuthenticatedAdmin(t *testing.T) { }) } -// passthroughController forces apiKeyAuthMiddleware down its no-usable-config -// branch (GetCurrentConfig returns a non-*config.Config), which forwards the -// request with a nil AuthContext. -type passthroughController struct{ *scopeController } - -func (p passthroughController) GetCurrentConfig() interface{} { return map[string]interface{}{} } - // TestGetServerDiagnostics_RevealRequiresAuthenticatedAdmin covers the // per-server diagnostics door, where health.detail echoes the raw connect error // with its URL credential. @@ -205,9 +199,10 @@ func TestGetServers_AdminContextsUnfiltered(t *testing.T) { assert.ElementsMatch(t, []string{"alpha", "beta"}, scopeServerNames(t, scopeDecodeData(t, rec))) }) - t.Run("no auth context passthrough", func(t *testing.T) { - srv := NewServer(passthroughController{newCtrl()}, zap.NewNop().Sugar(), nil) - rec := scopeGet(t, srv, "/api/v1/servers", "") + t.Run("no auth context is unrestricted", func(t *testing.T) { + srv := NewServer(newCtrl(), zap.NewNop().Sugar(), nil) + rec := httptest.NewRecorder() + srv.handleGetServers(rec, noAuthContextRequest(t, "/api/v1/servers", nil)) require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) assert.ElementsMatch(t, []string{"alpha", "beta"}, scopeServerNames(t, scopeDecodeData(t, rec)), "absence of a token must be treated as unrestricted, exactly as auth.AuthorizeServerOp does") diff --git a/internal/httpapi/scope_round9_test.go b/internal/httpapi/scope_round9_test.go index f4e0e96d3..d09181b91 100644 --- a/internal/httpapi/scope_round9_test.go +++ b/internal/httpapi/scope_round9_test.go @@ -172,9 +172,14 @@ func TestServerSubtree_AdminUnaffected(t *testing.T) { } }) - t.Run("no auth context passthrough is unrestricted", func(t *testing.T) { - srv := NewServer(passthroughController{newCtrl()}, zap.NewNop().Sugar(), nil) - rec := scopeGet(t, srv, "/api/v1/servers/beta/logs", "") + t.Run("no auth context is unrestricted", func(t *testing.T) { + // Driven through the real subtree gate (scopedServerSubtree) — that + // gate, not the auth middleware, is what must stay permissive when no + // AuthContext is present (auth.AuthorizeServerOp's default). + srv := NewServer(newCtrl(), zap.NewNop().Sugar(), nil) + rec := httptest.NewRecorder() + gated := srv.scopedServerSubtree(http.HandlerFunc(srv.handleGetServerLogs)) + gated.ServeHTTP(rec, noAuthContextRequest(t, "/api/v1/servers/beta/logs", map[string]string{"id": "beta"})) assert.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) assert.Contains(t, rec.Body.String(), betaLogSecret, "absence of a token must stay as permissive as it is today") diff --git a/internal/httpapi/search_tools_test.go b/internal/httpapi/search_tools_test.go index 093b56511..75ad2e487 100644 --- a/internal/httpapi/search_tools_test.go +++ b/internal/httpapi/search_tools_test.go @@ -46,6 +46,7 @@ func TestSearchTools_RestNameIsBare(t *testing.T) { server := NewServer(&canonicalSearchController{&MockServerController{}}, logger, nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/index/search?q=issue", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) diff --git a/internal/httpapi/security_scanner_redaction_write_test.go b/internal/httpapi/security_scanner_redaction_write_test.go index 49fdc6463..395cde5ff 100644 --- a/internal/httpapi/security_scanner_redaction_write_test.go +++ b/internal/httpapi/security_scanner_redaction_write_test.go @@ -25,6 +25,7 @@ const scannerRealSecret = "sk-live-REAL-VENDOR-KEY" func putScannerConfig(t *testing.T, srv *Server, body string) *httptest.ResponseRecorder { t.Helper() req := httptest.NewRequest(http.MethodPut, "/api/v1/security/scanners/mcp-scan/config", bytes.NewBufferString(body)) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") rec := httptest.NewRecorder() srv.ServeHTTP(rec, req) @@ -54,6 +55,7 @@ func TestConfigureScanner_RejectsRedactionSentinel(t *testing.T) { // 1. Read the document back the way any API client would. getReq := httptest.NewRequest(http.MethodGet, "/api/v1/security/scanners/mcp-scan/status", nil) + getReq.Header.Set("X-API-Key", mockControllerAPIKey) getRec := httptest.NewRecorder() srv.ServeHTTP(getRec, getReq) require.Equal(t, http.StatusOK, getRec.Code, "body: %s", getRec.Body.String()) diff --git a/internal/httpapi/security_scanner_test.go b/internal/httpapi/security_scanner_test.go index 64f17eefe..5c716f732 100644 --- a/internal/httpapi/security_scanner_test.go +++ b/internal/httpapi/security_scanner_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/zap" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/scanner" ) @@ -197,15 +198,17 @@ func (m *mockSecurityController) GetScanReportByJobID(_ context.Context, jobID s return nil, fmt.Errorf("no report found for job: %s", jobID) } -// secTestController embeds baseController and adds GetCurrentConfig -// returning nil to bypass auth middleware in tests. +// secTestController embeds baseController and supplies a real configuration +// with a real API key. It used to return nil to bypass the auth middleware +// entirely; since SEC-02 that path refuses the request instead of forwarding +// it, so these tests authenticate the way a real caller does. type secTestController struct { baseController servers []map[string]interface{} } -func (m *secTestController) GetCurrentConfig() interface{} { - return nil // nil config = testing scenario, bypasses auth +func (m *secTestController) GetCurrentConfig() *config.Config { + return &config.Config{APIKey: mockControllerAPIKey} } func (m *secTestController) GetAllServers() ([]map[string]interface{}, error) { @@ -254,6 +257,7 @@ func TestSecurityHandlerListScanners(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/scanners", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -271,6 +275,7 @@ func TestSecurityHandlerInstallScanner(t *testing.T) { body := bytes.NewBufferString(`{"id": "mcp-scan"}`) req := httptest.NewRequest("POST", "/api/v1/security/scanners/install", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -301,6 +306,7 @@ func TestSecurityHandlerEnableScannerDeepScanOffHint(t *testing.T) { enable := func(t *testing.T, srv *Server, id string) map[string]string { t.Helper() req := httptest.NewRequest("POST", "/api/v1/security/scanners/"+id+"/enable", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) require.Equal(t, http.StatusOK, w.Code) @@ -347,6 +353,7 @@ func TestSecurityHandlerInstallScannerError(t *testing.T) { body := bytes.NewBufferString(`{"id": "mcp-scan"}`) req := httptest.NewRequest("POST", "/api/v1/security/scanners/install", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -360,6 +367,7 @@ func TestSecurityHandlerInstallScannerMissingID(t *testing.T) { body := bytes.NewBufferString(`{}`) req := httptest.NewRequest("POST", "/api/v1/security/scanners/install", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -372,6 +380,7 @@ func TestSecurityHandlerRemoveScanner(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("DELETE", "/api/v1/security/scanners/mcp-scan", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -388,6 +397,7 @@ func TestSecurityHandlerConfigureScanner(t *testing.T) { body := bytes.NewBufferString(`{"env": {"API_KEY": "test-key"}}`) req := httptest.NewRequest("PUT", "/api/v1/security/scanners/mcp-scan/config", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -405,6 +415,7 @@ func TestSecurityHandlerConfigureScannerEmptyEnv(t *testing.T) { body := bytes.NewBufferString(`{"env": {}}`) req := httptest.NewRequest("PUT", "/api/v1/security/scanners/mcp-scan/config", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -421,6 +432,7 @@ func TestSecurityHandlerGetScannerStatus(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/scanners/mcp-scan/status", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -439,6 +451,7 @@ func TestSecurityHandlerGetScannerStatusNotFound(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/scanners/nonexistent/status", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -451,6 +464,7 @@ func TestSecurityHandlerStartScan(t *testing.T) { body := bytes.NewBufferString(`{"dry_run": true, "scanner_ids": ["mcp-scan"]}`) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/scan", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -470,6 +484,7 @@ func TestSecurityHandlerStartScanError(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/scan", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -489,6 +504,7 @@ func TestSecurityHandlerGetScanStatus(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/servers/my-server/scan/status", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -505,6 +521,7 @@ func TestSecurityHandlerGetScanStatusNotFound(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/servers/no-such-server/scan/status", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -525,6 +542,7 @@ func TestSecurityHandlerGetScanReport(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/servers/my-server/scan/report", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -542,6 +560,7 @@ func TestSecurityHandlerCancelScan(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/scan/cancel", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -555,6 +574,7 @@ func TestSecurityHandlerCancelScanError(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/scan/cancel", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -567,6 +587,7 @@ func TestSecurityHandlerApproveServer(t *testing.T) { body := bytes.NewBufferString(`{"force": false}`) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/security/approve", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -585,6 +606,7 @@ func TestSecurityHandlerApproveServerBlocked(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/security/approve", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -596,6 +618,7 @@ func TestSecurityHandlerRejectServer(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/servers/my-server/security/reject", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -617,6 +640,7 @@ func TestSecurityHandlerCheckIntegrity(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/servers/my-server/integrity", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -634,6 +658,7 @@ func TestSecurityHandlerCheckIntegrityNoBaseline(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/servers/my-server/integrity", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -658,6 +683,7 @@ func TestSecurityHandlerOverview(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/overview", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -678,6 +704,7 @@ func TestSecurityRoutesReturnNotImplementedWithoutController(t *testing.T) { // Do NOT set security controller req := httptest.NewRequest("GET", "/api/v1/security/scanners", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -707,6 +734,7 @@ func TestSecurityHandlerScanAll(t *testing.T) { body := bytes.NewBufferString(`{"scanner_ids": ["mcp-scan"]}`) req := httptest.NewRequest("POST", "/api/v1/security/scan-all", body) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -727,6 +755,7 @@ func TestSecurityHandlerScanAllAlreadyRunning(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/security/scan-all", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -754,6 +783,7 @@ func TestSecurityHandlerGetQueueProgress(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/queue", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -772,6 +802,7 @@ func TestSecurityHandlerGetQueueProgressEmpty(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("GET", "/api/v1/security/queue", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -787,6 +818,7 @@ func TestSecurityHandlerCancelAll(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/security/cancel-all", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) @@ -804,6 +836,7 @@ func TestSecurityHandlerCancelAllNoScan(t *testing.T) { srv := newTestServerWithSecurity(t, secCtrl) req := httptest.NewRequest("POST", "/api/v1/security/cancel-all", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() srv.ServeHTTP(w, req) diff --git a/internal/httpapi/security_test.go b/internal/httpapi/security_test.go index 952d16fc2..30955d572 100644 --- a/internal/httpapi/security_test.go +++ b/internal/httpapi/security_test.go @@ -11,6 +11,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" @@ -205,7 +206,7 @@ type mockControllerEmptyKey struct { baseController } -func (m *mockControllerEmptyKey) GetCurrentConfig() any { +func (m *mockControllerEmptyKey) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: "", // Empty API key } @@ -216,7 +217,7 @@ type mockControllerWithKey struct { apiKey string } -func (m *mockControllerWithKey) GetCurrentConfig() any { +func (m *mockControllerWithKey) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } @@ -253,7 +254,7 @@ func (m *baseController) GetQuarantinedServers() ([]map[string]interface{}, erro return nil, nil } func (m *baseController) UnquarantineServer(serverName string) error { return nil } -func (m *baseController) GetManagementService() interface{} { return nil } +func (m *baseController) GetManagementService() management.Service { return nil } func (m *baseController) GetServerTools(serverName string) ([]map[string]interface{}, error) { return nil, nil } diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 894093ae9..51bf1ce20 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -40,7 +40,6 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/telemetry" "github.com/smart-mcp-proxy/mcpproxy-go/internal/transport" "github.com/smart-mcp-proxy/mcpproxy-go/internal/updatecheck" - "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/core" "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/limiter" ) @@ -143,7 +142,11 @@ type ServerController interface { QuarantineServer(serverName string, quarantined bool) error GetQuarantinedServers() ([]map[string]interface{}, error) UnquarantineServer(serverName string) error - GetManagementService() interface{} // Returns the management service for unified operations + // GetManagementService returns the unified lifecycle/diagnostics service. + // Typed, not interface{}: handlers must get a compile-time contract rather + // than re-deriving a method set with ad-hoc assertions that fail at + // runtime (ARC-04). May be nil before the service is installed. + GetManagementService() management.Service DiscoverServerTools(ctx context.Context, serverName string) error // Tools and search @@ -165,7 +168,10 @@ type ServerController interface { // Secrets management GetSecretResolver() *secret.Resolver - GetCurrentConfig() interface{} + // GetCurrentConfig returns the live config snapshot, or nil when none is + // installed. Typed so apiKeyAuthMiddleware cannot be handed a value it + // fails to recognise and then wave through unauthenticated (SEC-02). + GetCurrentConfig() *config.Config NotifySecretsChanged(ctx context.Context, operation, secretName string) error // Tool call history. The ToolCallScope argument is the caller's server @@ -483,19 +489,17 @@ func (s *Server) apiKeyAuthMiddleware() func(http.Handler) http.Handler { return } - // Get config from controller - configInterface := s.controller.GetCurrentConfig() - if configInterface == nil { - // No config available (testing scenario) - allow through - next.ServeHTTP(w, r) - return - } - - // Cast to config type - cfg, ok := configInterface.(*config.Config) - if !ok { - // Config is not the expected type (testing scenario) - allow through - next.ServeHTTP(w, r) + // SECURITY: no configuration means there is nothing to + // authenticate against, so the request cannot be authenticated — + // refuse it. This used to forward the request to the handler + // ("testing scenario"), which is an unauthenticated REST API for + // any controller that returns nil here (SEC-02). + cfg := s.controller.GetCurrentConfig() + if cfg == nil { + s.logger.Errorw("Request rejected - configuration unavailable, cannot authenticate", + zap.String("path", r.URL.Path), + zap.String("remote_addr", r.RemoteAddr)) + s.writeError(w, r, http.StatusServiceUnavailable, "Configuration not available - cannot authenticate request") return } @@ -1686,9 +1690,7 @@ func (s *Server) handleGetServers(w http.ResponseWriter, r *http.Request) { // Try to use management service if available if mgmtSvc := s.controller.GetManagementService(); mgmtSvc != nil { // Use new management service path - servers, stats, err := mgmtSvc.(interface { - ListServers(context.Context) ([]*contracts.Server, *contracts.ServerStats, error) - }).ListServers(r.Context()) + servers, stats, err := mgmtSvc.ListServers(r.Context()) if err != nil { s.logger.Errorw("Failed to list servers via management service", "error", err) @@ -2224,19 +2226,17 @@ func (s *Server) handleAddServer(w http.ResponseWriter, r *http.Request) { // separate admission override); the request Quarantined boolean (#370) still // wins after, as a distinct pre-existing escape hatch. quarantined := true - if cfgIface := s.controller.GetCurrentConfig(); cfgIface != nil { - if cfg, ok := cfgIface.(*config.Config); ok && cfg != nil { - // Carry BOTH the explicit trust_mode AND the legacy - // auto_approve_tool_changes so EffectiveTrustMode() resolves the same - // admission decision it will resolve on the persisted server: a client - // that sets auto_approve_tool_changes:true but omits trust_mode must be - // admitted as auto (not left quarantined while the saved server reads - // auto — a contradictory state). (codex review, spec 086.) - quarantined = cfg.QuarantineDefaultForServer(&config.ServerConfig{ - TrustMode: req.TrustMode, - AutoApproveToolChanges: req.AutoApproveToolChanges, - }) - } + if cfg := s.controller.GetCurrentConfig(); cfg != nil { + // Carry BOTH the explicit trust_mode AND the legacy + // auto_approve_tool_changes so EffectiveTrustMode() resolves the same + // admission decision it will resolve on the persisted server: a client + // that sets auto_approve_tool_changes:true but omits trust_mode must be + // admitted as auto (not left quarantined while the saved server reads + // auto — a contradictory state). (codex review, spec 086.) + quarantined = cfg.QuarantineDefaultForServer(&config.ServerConfig{ + TrustMode: req.TrustMode, + AutoApproveToolChanges: req.AutoApproveToolChanges, + }) } if req.Quarantined != nil { quarantined = *req.Quarantined @@ -2865,9 +2865,7 @@ func (s *Server) handleEnableServer(w http.ResponseWriter, r *http.Request) { // Try to use management service if available if mgmtSvc := s.controller.GetManagementService(); mgmtSvc != nil { - err := mgmtSvc.(interface { - EnableServer(context.Context, string, bool) error - }).EnableServer(r.Context(), serverID, true) + err := mgmtSvc.EnableServer(r.Context(), serverID, true) if err != nil { s.logger.Errorw("Failed to enable server via management service", "server", serverID, "error", err) @@ -2932,9 +2930,7 @@ func (s *Server) handleDisableServer(w http.ResponseWriter, r *http.Request) { // Try to use management service if available if mgmtSvc := s.controller.GetManagementService(); mgmtSvc != nil { - err := mgmtSvc.(interface { - EnableServer(context.Context, string, bool) error - }).EnableServer(r.Context(), serverID, false) + err := mgmtSvc.EnableServer(r.Context(), serverID, false) if err != nil { s.logger.Errorw("Failed to disable server via management service", "server", serverID, "error", err) @@ -3021,10 +3017,8 @@ func (s *Server) handleForceReconnectServers(w http.ResponseWriter, r *http.Requ // @Router /api/v1/servers/restart_all [post] func (s *Server) handleRestartAll(w http.ResponseWriter, r *http.Request) { // Get management service from controller - mgmtSvc, ok := s.controller.GetManagementService().(interface { - RestartAll(ctx context.Context) (*management.BulkOperationResult, error) - }) - if !ok { + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { s.logger.Error("Failed to get management service") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return @@ -3053,10 +3047,8 @@ func (s *Server) handleRestartAll(w http.ResponseWriter, r *http.Request) { // @Router /api/v1/servers/enable_all [post] func (s *Server) handleEnableAll(w http.ResponseWriter, r *http.Request) { // Get management service from controller - mgmtSvc, ok := s.controller.GetManagementService().(interface { - EnableAll(ctx context.Context) (*management.BulkOperationResult, error) - }) - if !ok { + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { s.logger.Error("Failed to get management service") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return @@ -3085,10 +3077,8 @@ func (s *Server) handleEnableAll(w http.ResponseWriter, r *http.Request) { // @Router /api/v1/servers/disable_all [post] func (s *Server) handleDisableAll(w http.ResponseWriter, r *http.Request) { // Get management service from controller - mgmtSvc, ok := s.controller.GetManagementService().(interface { - DisableAll(ctx context.Context) (*management.BulkOperationResult, error) - }) - if !ok { + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { s.logger.Error("Failed to get management service") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return @@ -3127,9 +3117,7 @@ func (s *Server) handleRestartServer(w http.ResponseWriter, r *http.Request) { // Try to use management service if available if mgmtSvc := s.controller.GetManagementService(); mgmtSvc != nil { - err := mgmtSvc.(interface { - RestartServer(context.Context, string) error - }).RestartServer(r.Context(), serverID) + err := mgmtSvc.RestartServer(r.Context(), serverID) if err != nil { // Check if error is OAuth-related (expected state, not a failure) @@ -3367,11 +3355,9 @@ func (s *Server) handleServerLogin(w http.ResponseWriter, r *http.Request) { } // Call management service TriggerOAuthLoginQuick (Spec 020 fix: returns actual browser status) - mgmtSvc, ok := s.controller.GetManagementService().(interface { - TriggerOAuthLoginQuick(ctx context.Context, name string) (*core.OAuthStartResult, error) - }) - if !ok { - s.logger.Error("Management service not available or missing TriggerOAuthLoginQuick method") + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { + s.logger.Error("Management service not available") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return } @@ -3472,11 +3458,9 @@ func (s *Server) handleServerLogout(w http.ResponseWriter, r *http.Request) { } // Call management service TriggerOAuthLogout - mgmtSvc, ok := s.controller.GetManagementService().(interface { - TriggerOAuthLogout(ctx context.Context, name string) error - }) - if !ok { - s.logger.Error("Management service not available or missing TriggerOAuthLogout method") + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { + s.logger.Error("Management service not available") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return } @@ -3599,11 +3583,9 @@ func (s *Server) handleGetServerTools(w http.ResponseWriter, r *http.Request) { } // NEW: Call management service instead of controller (T016) - mgmtSvc, ok := s.controller.GetManagementService().(interface { - GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error) - }) - if !ok { - s.logger.Error("Management service not available or missing GetServerTools method") + mgmtSvc := s.controller.GetManagementService() + if mgmtSvc == nil { + s.logger.Error("Management service not available") s.writeError(w, r, http.StatusInternalServerError, "Management service not available") return } @@ -3730,9 +3712,8 @@ func (s *Server) handleGetGlobalTools(w http.ResponseWriter, r *http.Request) { // explicit guard in the loop below (#1064). Fall back to the controller // path when the management service is unavailable (keeps unit tests + // minimal deployments working). - mgmtSvc, hasMgmt := s.controller.GetManagementService().(interface { - GetServerTools(ctx context.Context, name string) ([]map[string]interface{}, error) - }) + mgmtSvc := s.controller.GetManagementService() + hasMgmt := mgmtSvc != nil getTools := func(name string) ([]map[string]interface{}, error) { if hasMgmt { return mgmtSvc.GetServerTools(r.Context(), name) @@ -4579,9 +4560,7 @@ func (s *Server) handleGetDiagnostics(w http.ResponseWriter, r *http.Request) { // Try to use management service if available if mgmtSvc := s.controller.GetManagementService(); mgmtSvc != nil { - diag, err := mgmtSvc.(interface { - Doctor(context.Context) (*contracts.Diagnostics, error) - }).Doctor(r.Context()) + diag, err := mgmtSvc.Doctor(r.Context()) if err != nil { s.logger.Errorw("Failed to get diagnostics via management service", "error", err) diff --git a/internal/httpapi/server_config_patch_test.go b/internal/httpapi/server_config_patch_test.go index 8df92ae96..7a48a1711 100644 --- a/internal/httpapi/server_config_patch_test.go +++ b/internal/httpapi/server_config_patch_test.go @@ -35,7 +35,7 @@ type mockPatchConfigController struct { validationErrs []config.ValidationError } -func (m *mockPatchConfigController) GetCurrentConfig() any { +func (m *mockPatchConfigController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/server_global_tools_test.go b/internal/httpapi/server_global_tools_test.go index eee23002c..5fb4827ea 100644 --- a/internal/httpapi/server_global_tools_test.go +++ b/internal/httpapi/server_global_tools_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) @@ -30,7 +31,7 @@ type globalToolsController struct { // GetManagementService returns nil so handleGetGlobalTools exercises the // controller GetServerTools path this mock controls. The management-service // path is verified end-to-end via the API E2E + curl verification. -func (m *globalToolsController) GetManagementService() interface{} { return nil } +func (m *globalToolsController) GetManagementService() management.Service { return nil } func (m *globalToolsController) GetAllServers() ([]map[string]interface{}, error) { return m.allServers, nil @@ -65,6 +66,7 @@ func doGlobalTools(t *testing.T, ctrl *globalToolsController) map[string]interfa t.Helper() srv := NewServer(ctrl, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest("GET", "/api/v1/tools", nil) + req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("X-Request-Source", "socket") w := httptest.NewRecorder() srv.router.ServeHTTP(w, req) diff --git a/internal/httpapi/server_logs_decode_test.go b/internal/httpapi/server_logs_decode_test.go index 7b248c3c7..695059f22 100644 --- a/internal/httpapi/server_logs_decode_test.go +++ b/internal/httpapi/server_logs_decode_test.go @@ -37,6 +37,7 @@ func TestGetServerLogs_SlashServerIDUnescaped(t *testing.T) { server := NewServer(controller, logger, nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/servers/io.github.evidai%2Fpolymarket-guard/logs?tail=10", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) diff --git a/internal/httpapi/server_path_param_decode_test.go b/internal/httpapi/server_path_param_decode_test.go index f3a2d4d20..653904183 100644 --- a/internal/httpapi/server_path_param_decode_test.go +++ b/internal/httpapi/server_path_param_decode_test.go @@ -61,6 +61,7 @@ func TestServerSubresource_SlashServerIDUnescaped(t *testing.T) { server := NewServer(&MockServerController{}, logger, nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/servers/"+encoded+"/tools", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -74,6 +75,7 @@ func TestServerSubresource_SlashServerIDUnescaped(t *testing.T) { server := NewServer(&MockServerController{}, logger, nil) req := httptest.NewRequest(http.MethodPost, "/api/v1/servers/"+encoded+"/restart", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) @@ -88,6 +90,7 @@ func TestServerSubresource_SlashServerIDUnescaped(t *testing.T) { server := NewServer(controller, logger, nil) req := httptest.NewRequest(http.MethodGet, "/api/v1/servers/"+encoded+"/tool-calls", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) w := httptest.NewRecorder() server.ServeHTTP(w, req) diff --git a/internal/httpapi/session_principal_test.go b/internal/httpapi/session_principal_test.go index c8f4a3ffd..b4380a249 100644 --- a/internal/httpapi/session_principal_test.go +++ b/internal/httpapi/session_principal_test.go @@ -46,7 +46,7 @@ type sessionTestController struct { cfg *config.Config } -func (m *sessionTestController) GetCurrentConfig() interface{} { return m.cfg } +func (m *sessionTestController) GetCurrentConfig() *config.Config { return m.cfg } func (m *sessionTestController) GetConfig() (*config.Config, error) { return m.cfg, nil } // principalProbeResponse reports exactly what apiKeyAuthMiddleware installed diff --git a/internal/httpapi/sessions_handlers_test.go b/internal/httpapi/sessions_handlers_test.go index 4420e794e..3678d0ddb 100644 --- a/internal/httpapi/sessions_handlers_test.go +++ b/internal/httpapi/sessions_handlers_test.go @@ -27,7 +27,7 @@ type mockSessionController struct { callCount int } -func (m *mockSessionController) GetCurrentConfig() any { +func (m *mockSessionController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/telemetry_payload_test.go b/internal/httpapi/telemetry_payload_test.go index fc45dbba0..c3a5df16e 100644 --- a/internal/httpapi/telemetry_payload_test.go +++ b/internal/httpapi/telemetry_payload_test.go @@ -24,7 +24,7 @@ type telemetryPayloadController struct { apiKey string } -func (m *telemetryPayloadController) GetCurrentConfig() any { +func (m *telemetryPayloadController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/tenant_allowlist_walk_test.go b/internal/httpapi/tenant_allowlist_walk_test.go index 2c66bc6e2..56b00f737 100644 --- a/internal/httpapi/tenant_allowlist_walk_test.go +++ b/internal/httpapi/tenant_allowlist_walk_test.go @@ -41,7 +41,7 @@ type tenantWalkController struct { baseController } -func (m *tenantWalkController) GetCurrentConfig() any { +func (m *tenantWalkController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: "admin-test-key"} } diff --git a/internal/httpapi/tokens_test.go b/internal/httpapi/tokens_test.go index fa229988c..1dfed7fab 100644 --- a/internal/httpapi/tokens_test.go +++ b/internal/httpapi/tokens_test.go @@ -132,7 +132,7 @@ type mockTokenController struct { profiles []string } -func (m *mockTokenController) GetCurrentConfig() interface{} { +func (m *mockTokenController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } diff --git a/internal/httpapi/tool_hash_disclosure_test.go b/internal/httpapi/tool_hash_disclosure_test.go index d6310a286..a55d0f332 100644 --- a/internal/httpapi/tool_hash_disclosure_test.go +++ b/internal/httpapi/tool_hash_disclosure_test.go @@ -15,6 +15,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) @@ -32,6 +33,7 @@ const toolHashTestAPIKey = "tool-hash-test-api-key" // toolHashMgmtService is the management service the per-server tools endpoint // prefers; it serves a fixed tool set for one server. type toolHashMgmtService struct { + management.Service tools map[string][]map[string]interface{} } @@ -46,11 +48,11 @@ type toolHashController struct { mgmt *toolHashMgmtService } -func (c *toolHashController) GetCurrentConfig() interface{} { +func (c *toolHashController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: toolHashTestAPIKey} } -func (c *toolHashController) GetManagementService() interface{} { +func (c *toolHashController) GetManagementService() management.Service { if c.mgmt == nil { return nil } @@ -115,7 +117,12 @@ func fetchTools(t *testing.T, srv *Server, path, token string) map[string]map[st w := httptest.NewRecorder() srv.ServeHTTP(w, req) require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + return decodeToolsByName(t, w) +} +// decodeToolsByName indexes the tool listing envelope by tool name. +func decodeToolsByName(t *testing.T, w *httptest.ResponseRecorder) map[string]map[string]interface{} { + t.Helper() var resp struct { Data struct { Tools []map[string]interface{} `json:"tools"` @@ -165,29 +172,27 @@ func TestToolHash_NeverDisclosedToAgentToken(t *testing.T) { } } -// Spec 099 FR-018a: a request that reached the handler with NO auth context — -// the middleware's no-config passthrough is the only way in — is not evidence of -// an admin, so it gets the agent-token tier and no pin. This is the second -// consumer of disclosureTier, and the one where a residual grant would publish -// hashes rather than merely widen a diagnosis. +// Spec 099 FR-018a: a request that reaches the handler with NO auth context is +// not evidence of an admin, so it gets the agent-token tier and no pin. This is +// the second consumer of disclosureTier, and the one where a residual grant +// would publish hashes rather than merely widen a diagnosis. +// +// The handler is invoked directly: since SEC-02 the auth middleware refuses a +// request it cannot authenticate instead of forwarding it, so the floor is +// pinned on the handler that owns it. See noAuthContextRequest. func TestToolHash_NoAuthContextGetsNoPin(t *testing.T) { - ctrl := &unconfiguredToolHashController{toolHashController: newToolHashController()} - srv := NewServer(ctrl, zaptest.NewLogger(t).Sugar(), nil) + srv := NewServer(newToolHashController(), zaptest.NewLogger(t).Sugar(), nil) - tools := fetchTools(t, srv, "/api/v1/tools", "") + rec := httptest.NewRecorder() + srv.handleGetGlobalTools(rec, noAuthContextRequest(t, "/api/v1/tools", nil)) + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + + tools := decodeToolsByName(t, rec) require.Contains(t, tools, "create_issue") assert.NotContains(t, tools["create_issue"], "hash", "no auth context is the disclosure floor, not the ceiling") } -// unconfiguredToolHashController reproduces the ONE middleware path that reaches -// a handler without installing an auth context: no readable config. -type unconfiguredToolHashController struct { - *toolHashController -} - -func (c *unconfiguredToolHashController) GetCurrentConfig() interface{} { return nil } - // The hash is proxy state, never upstream-supplied: a server declaring a tool // field literally named "hash" must not be able to publish a pin through the // listing (it would let a malicious upstream pin itself to a value the operator diff --git a/internal/httpapi/tool_quarantine_test.go b/internal/httpapi/tool_quarantine_test.go index 4325cd1bc..62da45508 100644 --- a/internal/httpapi/tool_quarantine_test.go +++ b/internal/httpapi/tool_quarantine_test.go @@ -46,7 +46,7 @@ type mockToolQuarantineController struct { blockedServer string } -func (m *mockToolQuarantineController) GetCurrentConfig() any { +func (m *mockToolQuarantineController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } @@ -702,7 +702,7 @@ type mockUnquarantineController struct { lastQuarantined bool } -func (m *mockUnquarantineController) GetCurrentConfig() any { +func (m *mockUnquarantineController) GetCurrentConfig() *config.Config { return &config.Config{ APIKey: m.apiKey, } @@ -824,7 +824,7 @@ type mockToolToggleController struct { mockToolQuarantineController } -func (m *mockToolToggleController) GetCurrentConfig() any { +func (m *mockToolToggleController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: m.apiKey} } diff --git a/internal/httpapi/update_failure_test.go b/internal/httpapi/update_failure_test.go index db477cd2d..9bcfec87c 100644 --- a/internal/httpapi/update_failure_test.go +++ b/internal/httpapi/update_failure_test.go @@ -25,7 +25,7 @@ type updateFailureController struct { err error } -func (c *updateFailureController) GetCurrentConfig() interface{} { +func (c *updateFailureController) GetCurrentConfig() *config.Config { return &config.Config{APIKey: c.apiKey} } diff --git a/internal/runtime/event_bus_payload_test.go b/internal/runtime/event_bus_payload_test.go index 12c6c8c44..a78bc7ecf 100644 --- a/internal/runtime/event_bus_payload_test.go +++ b/internal/runtime/event_bus_payload_test.go @@ -14,6 +14,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" ) @@ -21,6 +22,11 @@ import ( // used by emitServersChanged. It's stored on Runtime.managementService so the // emit path can type-assert and call ListServers. type fakeServersLister struct { + // Embedded so the fake satisfies the whole typed seam (the runtime now + // holds a management.Service, not an interface{}) while implementing only + // ListServers. The embedded interface is nil: an unexpected call panics + // loudly instead of silently returning a zero value. + management.Service servers []*contracts.Server stats *contracts.ServerStats err error @@ -203,6 +209,8 @@ func TestEmitServersChanged_RedactsEnvAndURLSecrets(t *testing.T) { // through buildServersChangedPayload — without that threading the call sits // on a detached 2-second timer regardless of app-shutdown cancellation. type ctxAwareLister struct { + // See fakeServersLister: embedded to satisfy the typed seam. + management.Service servers []*contracts.Server stats *contracts.ServerStats } diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 079dc2878..1b069f6d2 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -27,6 +27,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/experiments" "github.com/smart-mcp-proxy/mcpproxy-go/internal/health" "github.com/smart-mcp-proxy/mcpproxy-go/internal/index" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/management" "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" @@ -158,9 +159,14 @@ type Runtime struct { // outcome of the PREVIOUS process instance, derived exactly once in New // when the marker is armed (FR-010/FR-011) and handed to the telemetry // service in SetTelemetry. - prechurnStore telemetry.PreChurnStore - previousShutdown string - managementService interface{} // Initialized later to avoid import cycle + prechurnStore telemetry.PreChurnStore + previousShutdown string + // managementService is the unified lifecycle/diagnostics service. It is + // installed after New (SetManagementService) because the service is built + // on top of the Runtime, not because the type has to be erased: the + // management package does not import runtime, so the field carries the + // real interface and every consumer gets a compile-time contract. + managementService management.Service activityService *ActivityService // Activity logging service // rejectionMetric counts a concurrency shed SYNCHRONOUSLY at the rejection @@ -1118,8 +1124,9 @@ func (r *Runtime) NotifySecretsChanged(ctx context.Context, operation, secretNam return nil } -// GetCurrentConfig returns the current configuration -func (r *Runtime) GetCurrentConfig() interface{} { +// GetCurrentConfig returns the current configuration. It may be nil only +// before the config is installed; New rejects a nil config outright. +func (r *Runtime) GetCurrentConfig() *config.Config { r.mu.RLock() defer r.mu.RUnlock() return r.cfg @@ -2287,15 +2294,17 @@ func (r *Runtime) GetDockerRecoveryStatus() *storage.DockerRecoveryState { return r.upstreamManager.GetDockerRecoveryStatus() } -// SetManagementService stores the management service instance. -// This is called after runtime initialization to avoid import cycles. -func (r *Runtime) SetManagementService(svc interface{}) { +// SetManagementService stores the management service instance. It is called +// after runtime initialization because the service is CONSTRUCTED on top of +// the Runtime (it takes one as its RuntimeOperations), not because of an +// import cycle — internal/management does not import internal/runtime. +func (r *Runtime) SetManagementService(svc management.Service) { r.managementService = svc } // GetManagementService returns the management service instance. // Returns nil if service hasn't been set yet. -func (r *Runtime) GetManagementService() interface{} { +func (r *Runtime) GetManagementService() management.Service { return r.managementService } diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 93a6410b2..303cbc61d 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -4598,9 +4598,7 @@ func (p *MCPProxyServer) handleEnableUpstream(ctx context.Context, request mcp.C // Try to use management service if available if p.mainServer != nil && p.mainServer.runtime != nil { if mgmtSvc := p.mainServer.runtime.GetManagementService(); mgmtSvc != nil { - err := mgmtSvc.(interface { - EnableServer(context.Context, string, bool) error - }).EnableServer(ctx, serverName, enabled) + err := mgmtSvc.EnableServer(ctx, serverName, enabled) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to %s server '%s': %v", @@ -4641,9 +4639,7 @@ func (p *MCPProxyServer) handleRestartUpstream(ctx context.Context, request mcp. // Try to use management service if available if p.mainServer != nil && p.mainServer.runtime != nil { if mgmtSvc := p.mainServer.runtime.GetManagementService(); mgmtSvc != nil { - err := mgmtSvc.(interface { - RestartServer(context.Context, string) error - }).RestartServer(ctx, serverName) + err := mgmtSvc.RestartServer(ctx, serverName) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to restart server '%s': %v", serverName, err)), nil @@ -4707,9 +4703,7 @@ func (p *MCPProxyServer) handleDoctor(ctx context.Context, request mcp.CallToolR // Try to use management service if available if p.mainServer != nil && p.mainServer.runtime != nil { if mgmtSvc := p.mainServer.runtime.GetManagementService(); mgmtSvc != nil { - diag, err := mgmtSvc.(interface { - Doctor(context.Context) (*contracts.Diagnostics, error) - }).Doctor(ctx) + diag, err := mgmtSvc.Doctor(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to run diagnostics: %v", err)), nil diff --git a/internal/server/server.go b/internal/server/server.go index 1a9a776bd..39ae548d3 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -656,7 +656,7 @@ func (s *Server) UnsubscribeEvents(ch chan runtime.Event) { // GetManagementService returns the management service instance from runtime. // Returns nil if service hasn't been set yet. -func (s *Server) GetManagementService() interface{} { +func (s *Server) GetManagementService() management.Service { if s.runtime == nil { return nil } @@ -3141,7 +3141,7 @@ func (s *Server) startCustomHTTPServer(ctx context.Context, streamableServer *se // security_scan from every server on every SSE delivery — same bug // class as the pre-existing quarantine-stats staleness PR #463 // already fixes for Quarantine. - if mgmtSvc, ok := s.runtime.GetManagementService().(management.Service); ok && mgmtSvc != nil { + if mgmtSvc := s.runtime.GetManagementService(); mgmtSvc != nil { mgmtSvc.SetScanSummaryEnricher(&scanSummaryEnricherAdapter{scanner: secService}) } s.setSecurityScanner(secService) @@ -3924,7 +3924,7 @@ func (s *Server) EmitActiveProfileChanged(profile string) { } // GetCurrentConfig returns the current configuration -func (s *Server) GetCurrentConfig() interface{} { +func (s *Server) GetCurrentConfig() *config.Config { return s.runtime.GetCurrentConfig() } diff --git a/internal/server/update_failure.go b/internal/server/update_failure.go index 5694018ae..9d38c08f8 100644 --- a/internal/server/update_failure.go +++ b/internal/server/update_failure.go @@ -43,7 +43,7 @@ func (s *Server) RecordUpdateFailure(stage string) (bool, error) { db = ts.DiagnosticsCounterDB() } - cfg, _ := s.runtime.GetCurrentConfig().(*config.Config) + cfg := s.runtime.GetCurrentConfig() return recordUpdateFailure(cfg, httpapi.GetBuildVersion(), store, db, stage) } diff --git a/internal/serveredition/api/user_activity_wired_test.go b/internal/serveredition/api/user_activity_wired_test.go index ee21ddc71..2dd11ba2c 100644 --- a/internal/serveredition/api/user_activity_wired_test.go +++ b/internal/serveredition/api/user_activity_wired_test.go @@ -38,7 +38,7 @@ type stubServerController struct { cfg *config.Config } -func (c *stubServerController) GetCurrentConfig() interface{} { return c.cfg } +func (c *stubServerController) GetCurrentConfig() *config.Config { return c.cfg } // GetConfig backs internal/httpapi's trustedProxiesProvider (server.go:680), // which TagRequestMeta calls on EVERY /api/v1 request ahead of From eba0071f2f040b6eb8501498f61285ccb162edae Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 23 Sep 2026 06:57:50 +0300 Subject: [PATCH 2/2] fixup(httpapi): apply second-lens review findings on typed controller seams - remove dead X-API-Key Set in TestAddFromRegistry_NilConfigIsAnError (the first Set was immediately overwritten and asserted nothing) - TestAPIKeyAuth_NilConfigFailsClosed: assert the 503 body text, not just the status code, so an unrelated earlier 503 can't pass the same assertions for the wrong reason - GetCurrentConfig(): guard s.runtime == nil like its sibling GetManagementService(); unreachable in production but latent for a hand-built *Server in tests. Regression test added (get_current_config_test.go) reproducing the prior nil-pointer panic. Co-Authored-By: Claude Opus 5 --- internal/httpapi/auth_middleware_test.go | 2 ++ internal/httpapi/registry_add_test.go | 1 - internal/server/get_current_config_test.go | 18 ++++++++++++++++++ internal/server/server.go | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 internal/server/get_current_config_test.go diff --git a/internal/httpapi/auth_middleware_test.go b/internal/httpapi/auth_middleware_test.go index 6e3e67e5c..afbfb852d 100644 --- a/internal/httpapi/auth_middleware_test.go +++ b/internal/httpapi/auth_middleware_test.go @@ -436,6 +436,8 @@ func TestAPIKeyAuth_NilConfigFailsClosed(t *testing.T) { assert.Equal(t, http.StatusServiceUnavailable, w.Code, "an unreadable config must refuse the request, not forward it unauthenticated") + assert.Contains(t, w.Body.String(), "cannot authenticate", + "the 503 body must explain why, not just carry the right status code") assert.False(t, ctrl.reachedHandler.Load(), "the handler must NOT run: a nil config means the request was never authenticated") } diff --git a/internal/httpapi/registry_add_test.go b/internal/httpapi/registry_add_test.go index 8e51ac7dc..a587587f6 100644 --- a/internal/httpapi/registry_add_test.go +++ b/internal/httpapi/registry_add_test.go @@ -66,7 +66,6 @@ func TestAddFromRegistry_NilConfigIsAnError(t *testing.T) { srv := NewServer(ctrl, zaptest.NewLogger(t).Sugar(), nil) req := httptest.NewRequest(http.MethodPost, "/api/v1/registries/reg1/servers/srv1/add", nil) - req.Header.Set("X-API-Key", mockControllerAPIKey) req.Header.Set("X-API-Key", "admin-secret") w := httptest.NewRecorder() srv.ServeHTTP(w, req) diff --git a/internal/server/get_current_config_test.go b/internal/server/get_current_config_test.go new file mode 100644 index 000000000..9cd91f045 --- /dev/null +++ b/internal/server/get_current_config_test.go @@ -0,0 +1,18 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestGetCurrentConfig_NilRuntimeReturnsNil pins GetCurrentConfig's behavior +// to match its sibling GetManagementService: a hand-built *Server with no +// runtime (as tests construct via &Server{}) must return nil rather than +// panic on the nil pointer dereference inside runtime.GetCurrentConfig(). +// Unreachable in production (runtime.New rejects a nil config), but latent +// for any test that hand-builds a *Server. +func TestGetCurrentConfig_NilRuntimeReturnsNil(t *testing.T) { + s := &Server{} + assert.Nil(t, s.GetCurrentConfig()) +} diff --git a/internal/server/server.go b/internal/server/server.go index 39ae548d3..209b6ae16 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -3925,6 +3925,9 @@ func (s *Server) EmitActiveProfileChanged(profile string) { // GetCurrentConfig returns the current configuration func (s *Server) GetCurrentConfig() *config.Config { + if s.runtime == nil { + return nil + } return s.runtime.GetCurrentConfig() }