From 9748de8a7ad9be79f6ef152036d3e340819a75ce Mon Sep 17 00:00:00 2001 From: AdamMagued Date: Sat, 3 Oct 2026 15:08:28 +0300 Subject: [PATCH] fix: detect config changes for require_mcp_auth and residuals Detect changes to require_mcp_auth and related hot-reloadable fields in DetectConfigChanges, propagate missing config errors in registry commands, apply TLS and listen environment overrides on fallback CLI loaders, redact credentials following apostrophes in doctor URLs, and return an empty preview for empty JSON imports. --- cmd/mcpproxy/cli_config.go | 1 + cmd/mcpproxy/cli_config_resolve_test.go | 38 +++++++++ cmd/mcpproxy/doctor_redact.go | 9 +- cmd/mcpproxy/doctor_redact_test.go | 24 ++++++ cmd/mcpproxy/registry_cmd.go | 4 + internal/config/loader.go | 7 ++ internal/httpapi/import.go | 3 + .../httpapi/import_preview_fields_test.go | 24 ++++++ internal/httpapi/server_config_patch_test.go | 32 +++++++ internal/runtime/config_hotreload.go | 18 ++++ internal/runtime/config_hotreload_test.go | 84 +++++++++++++++++++ 11 files changed, 242 insertions(+), 2 deletions(-) diff --git a/cmd/mcpproxy/cli_config.go b/cmd/mcpproxy/cli_config.go index 6008a661e..c6c8c135d 100644 --- a/cmd/mcpproxy/cli_config.go +++ b/cmd/mcpproxy/cli_config.go @@ -33,6 +33,7 @@ func loadCLIConfig(explicitPath string) (*config.Config, error) { // $HOME/.mcpproxy/mcp_config.json and report the HOME defaults, which // contradicts the data dir the operator named. cfg = config.DefaultConfig() + config.ApplyTLSEnvOverrides(cfg) } else { cfg, err = config.Load() } diff --git a/cmd/mcpproxy/cli_config_resolve_test.go b/cmd/mcpproxy/cli_config_resolve_test.go index b73c2807c..30cdac00f 100644 --- a/cmd/mcpproxy/cli_config_resolve_test.go +++ b/cmd/mcpproxy/cli_config_resolve_test.go @@ -199,3 +199,41 @@ func TestResolveCLIConfigPathPrecedence(t *testing.T) { t.Errorf("-d without a config file falls back to discovery, got %q", got) } } + +func TestLoadRegistryConfig_ExplicitConfigErrorPropagated(t *testing.T) { + resetLoaderGlobals(t) + configFile = filepath.Join(t.TempDir(), "nonexistent.json") + + _, err := loadRegistryConfig() + if err == nil { + t.Fatal("expected error when explicit global -c points to nonexistent file, got nil") + } + + configFile = "" + registryConfigPath = filepath.Join(t.TempDir(), "nonexistent-local.json") + _, err = loadRegistryConfig() + if err == nil { + t.Fatal("expected error when explicit --config points to nonexistent file, got nil") + } +} + +func TestLoadCLIConfig_DataDirAppliesTLSEnvOverrides(t *testing.T) { + resetLoaderGlobals(t) + home := t.TempDir() + t.Setenv("HOME", home) + t.Chdir(t.TempDir()) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:23456") + t.Setenv("MCPPROXY_TLS_ENABLED", "false") + dataDir = filepath.Join(t.TempDir(), "new-datadir") + + cfg, err := loadCLIConfig("") + if err != nil { + t.Fatalf("loadCLIConfig failed: %v", err) + } + if cfg.Listen != "127.0.0.1:23456" { + t.Errorf("expected Listen to be 127.0.0.1:23456 from env override, got %q", cfg.Listen) + } + if cfg.TLS != nil && cfg.TLS.Enabled { + t.Errorf("expected TLS to be disabled from env override, got enabled") + } +} diff --git a/cmd/mcpproxy/doctor_redact.go b/cmd/mcpproxy/doctor_redact.go index ae89d0729..6dac6b75e 100644 --- a/cmd/mcpproxy/doctor_redact.go +++ b/cmd/mcpproxy/doctor_redact.go @@ -20,7 +20,7 @@ const doctorRedactionMask = "REDACTED" // in doctor output even outside a URL. runDoctor sets it for the run. var doctorSecretLiterals []string -var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"'<>]+`) +var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"<>]+`) func doctorMask(string) string { return doctorRedactionMask } @@ -32,7 +32,12 @@ func redactDoctorString(s string) string { } if strings.Contains(s, "://") { s = doctorURLPattern.ReplaceAllStringFunc(s, func(u string) string { - return oauth.RedactURLQueryParamsWith(u, doctorMask) + trailing := "" + for strings.HasSuffix(u, "'") { + trailing = "'" + trailing + u = u[:len(u)-1] + } + return oauth.RedactURLQueryParamsWith(u, doctorMask) + trailing }) } for _, lit := range doctorSecretLiterals { diff --git a/cmd/mcpproxy/doctor_redact_test.go b/cmd/mcpproxy/doctor_redact_test.go index 161bdfceb..acb0b1955 100644 --- a/cmd/mcpproxy/doctor_redact_test.go +++ b/cmd/mcpproxy/doctor_redact_test.go @@ -150,3 +150,27 @@ func TestStatusOutput_MasksKeyInEveryFormat(t *testing.T) { } } } + +func TestDoctorRedact_ApostropheURL(t *testing.T) { + input := "connect failed: dial https://h.example/mcp?label=Bob's&%74oken=secret-tok-123456 failed" + redacted := redactDoctorString(input) + if strings.Contains(redacted, "secret-tok-123456") { + t.Fatalf("credential after apostrophe leaked: %s", redacted) + } + if !strings.Contains(redacted, "Bob's") { + t.Fatalf("label before token should be preserved: %s", redacted) + } + if !strings.Contains(redacted, "REDACTED") { + t.Fatalf("expected REDACTED in output: %s", redacted) + } + + // Enclosed in quotes + inQuotes := "check 'https://h.example/mcp?label=Bob's&token=mysecret' for info" + redactedQuotes := redactDoctorString(inQuotes) + if strings.Contains(redactedQuotes, "mysecret") { + t.Fatalf("credential in quoted URL leaked: %s", redactedQuotes) + } + if !strings.HasSuffix(strings.TrimSpace(redactedQuotes), "' for info") { + t.Fatalf("trailing quote and prose should be preserved: %s", redactedQuotes) + } +} diff --git a/cmd/mcpproxy/registry_cmd.go b/cmd/mcpproxy/registry_cmd.go index b8589d537..46188a6d2 100644 --- a/cmd/mcpproxy/registry_cmd.go +++ b/cmd/mcpproxy/registry_cmd.go @@ -627,8 +627,12 @@ func loadRegistryConfig() (*config.Config, error) { // $HOME/.mcpproxy/mcp_config.json. cfg, err := loadCLIConfig(registryConfigPath) if err != nil { + if resolveCLIConfigPath(registryConfigPath) != "" { + return nil, err + } // Discovery should still work with defaults if no config is present. cfg = config.DefaultConfig() + config.ApplyTLSEnvOverrides(cfg) if dataDir != "" { cfg.DataDir = dataDir } diff --git a/internal/config/loader.go b/internal/config/loader.go index e3d23014f..4b422dfbd 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -743,6 +743,13 @@ func expandDataDir(cfg *Config) { cfg.DataDir = resolved } +// ApplyTLSEnvOverrides applies the MCPPROXY_* environment overrides to cfg. +// Exported for CLI loaders that fall back to DefaultConfig() when no config +// file exists. +func ApplyTLSEnvOverrides(cfg *Config) { + applyTLSEnvOverrides(cfg) +} + // applyTLSEnvOverrides applies the MCPPROXY_* environment overrides. Each one // goes through OverrideForProcess so no save path persists it (see // process_overrides.go); the env-sourced set is rebuilt from scratch on every diff --git a/internal/httpapi/import.go b/internal/httpapi/import.go index a4a47f975..b99b6fa23 100644 --- a/internal/httpapi/import.go +++ b/internal/httpapi/import.go @@ -487,6 +487,9 @@ func (s *Server) runImport(r *http.Request, content []byte, formatHint string, s // Run import result, err := configimport.Import(content, opts) if err != nil { + if preview && bytes.Equal(bytes.TrimSpace(content), []byte("{}")) { + return emptyImportPreview(formatHint), nil + } return nil, err } diff --git a/internal/httpapi/import_preview_fields_test.go b/internal/httpapi/import_preview_fields_test.go index d484b5cd6..266f7ad90 100644 --- a/internal/httpapi/import_preview_fields_test.go +++ b/internal/httpapi/import_preview_fields_test.go @@ -237,3 +237,27 @@ func TestImportPreview_RedactsCredentialShapedCommandFromSummary(t *testing.T) { func bytesContains(haystack []byte, needle string) bool { return bytes.Contains(haystack, []byte(needle)) } + +func TestImportPreview_EmptyObjectWithoutFormatHint(t *testing.T) { + logger := zap.NewNop().Sugar() + mock := &mockImportController{apiKey: "test-key"} + server := NewServer(mock, logger, nil) + + body, _ := json.Marshal(ImportRequest{Content: "{}"}) + req := httptest.NewRequest("POST", "/api/v1/servers/import/json?preview=true", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Key", "test-key") + rr := httptest.NewRecorder() + server.router.ServeHTTP(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var wrapped wrappedImportResponse + if err := json.Unmarshal(rr.Body.Bytes(), &wrapped); err != nil { + t.Fatalf("decode: %v", err) + } + if len(wrapped.Data.Imported) != 0 { + t.Fatalf("expected 0 imported servers, got %d", len(wrapped.Data.Imported)) + } +} diff --git a/internal/httpapi/server_config_patch_test.go b/internal/httpapi/server_config_patch_test.go index 7a48a1711..ccd3dd761 100644 --- a/internal/httpapi/server_config_patch_test.go +++ b/internal/httpapi/server_config_patch_test.go @@ -353,3 +353,35 @@ func TestHandlePatchConfig_MergesOntoDesiredConfig(t *testing.T) { "a restart-pending routing mode the client never mentioned must survive the patch") assert.Equal(t, "secret-key", ctrl.captured.APIKey, "secrets still preserved verbatim") } + +func TestHandlePatchConfig_RequireMCPAuthChangeDetected(t *testing.T) { + live := &config.Config{ + APIKey: "secret-key", + RequireMCPAuth: false, + } + ctrl := &mockPatchConfigController{ + apiKey: "secret-key", + live: live, + detectChanges: true, + } + srv := NewServer(ctrl, zap.NewNop().Sugar(), nil) + + body, err := json.Marshal(map[string]interface{}{ + "require_mcp_auth": true, + }) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPatch, "/api/v1/config", bytes.NewReader(body)) + req.Header.Set("X-API-Key", "secret-key") + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code) + + res := decodePatchResult(t, w.Body.Bytes()) + assert.Equal(t, true, res["success"]) + assert.Equal(t, true, res["applied_immediately"]) + changed, ok := res["changed_fields"].([]interface{}) + require.True(t, ok) + assert.Contains(t, changed, "require_mcp_auth") +} diff --git a/internal/runtime/config_hotreload.go b/internal/runtime/config_hotreload.go index a4c2d208e..69fc59bf6 100644 --- a/internal/runtime/config_hotreload.go +++ b/internal/runtime/config_hotreload.go @@ -232,6 +232,9 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { if oldCfg.CallToolTimeout != newCfg.CallToolTimeout { result.ChangedFields = append(result.ChangedFields, "call_tool_timeout") } + if !reflect.DeepEqual(oldCfg.MaxResultSizeChars, newCfg.MaxResultSizeChars) { + result.ChangedFields = append(result.ChangedFields, "max_result_size_chars") + } // TOON output (spec 084, FR-001 — hot-reloadable). The call_tool_* encoder // seam reads ToonOutput/ToonMinSavingsPct fresh on every call (same pattern @@ -326,6 +329,9 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { if !reflect.DeepEqual(oldCfg.ToolDiscoveryInterval, newCfg.ToolDiscoveryInterval) { result.ChangedFields = append(result.ChangedFields, "tool_discovery_interval") } + if !reflect.DeepEqual(oldCfg.InitTimeout, newCfg.InitTimeout) { + result.ChangedFields = append(result.ChangedFields, "init_timeout") + } // Concurrency limits (spec 093 / GH #955 — hot-reloadable, FR-021). The // limiter registry re-publishes one generation from the new snapshot on @@ -385,6 +391,9 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { if !reflect.DeepEqual(oldCfg.Logging, newCfg.Logging) { result.ChangedFields = append(result.ChangedFields, "logging") } + if oldCfg.DebugSearch != newCfg.DebugSearch { + result.ChangedFields = append(result.ChangedFields, "debug_search") + } // Docker isolation configuration (can be hot-reloaded for new servers). // Compared via jsonEqual, not reflect.DeepEqual: the PATCH /api/v1/config @@ -412,6 +421,12 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { if oldCfg.AllowServerRemove != newCfg.AllowServerRemove { result.ChangedFields = append(result.ChangedFields, "allow_server_remove") } + if oldCfg.RequireMCPAuth != newCfg.RequireMCPAuth { + result.ChangedFields = append(result.ChangedFields, "require_mcp_auth") + } + if oldCfg.IsQuarantineEnabled() != newCfg.IsQuarantineEnabled() { + result.ChangedFields = append(result.ChangedFields, "quarantine_enabled") + } // trusted_hosts (GH #898 — hot-reloadable). hostValidationMiddleware reads // the live snapshot per request, so reporting the change is all the // propagation needed. slices.Equal, not DeepEqual: the PATCH round-trip @@ -432,6 +447,9 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { if !reflect.DeepEqual(oldCfg.Environment, newCfg.Environment) { result.ChangedFields = append(result.ChangedFields, "environment") } + if oldCfg.ForwardProxyEnv != newCfg.ForwardProxyEnv { + result.ChangedFields = append(result.ChangedFields, "forward_proxy_env") + } // Observability cadence (Spec 069 A2 — can be hot-reloaded; the usage flush // loop re-reads the interval each cycle, so applying it is just a setter). diff --git a/internal/runtime/config_hotreload_test.go b/internal/runtime/config_hotreload_test.go index 3eedecd2b..bc7b48698 100644 --- a/internal/runtime/config_hotreload_test.go +++ b/internal/runtime/config_hotreload_test.go @@ -1201,3 +1201,87 @@ func TestDetectConfigChanges_AdvertiseUpstreamServers(t *testing.T) { assert.NotContains(t, DetectConfigChanges(mk(nil), mk(&on)).ChangedFields, "advertise_upstream_servers") } + +func TestDetectConfigChanges_Issue1466Residuals(t *testing.T) { + base := func() *config.Config { + return &config.Config{ + Listen: "127.0.0.1:8080", + DataDir: "/d", + TLS: &config.TLSConfig{}, + } + } + + t.Run("require_mcp_auth toggle detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + oldCfg.RequireMCPAuth = false + newCfg := base() + newCfg.RequireMCPAuth = true + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "require_mcp_auth") + assert.False(t, result.RequiresRestart) + assert.True(t, result.AppliedImmediately) + }) + + t.Run("quarantine_enabled toggle detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + disabled := false + newCfg := base() + newCfg.QuarantineEnabled = &disabled + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "quarantine_enabled") + assert.False(t, result.RequiresRestart) + }) + + t.Run("init_timeout change detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + dur := config.Duration(45 * time.Second) + newCfg := base() + newCfg.InitTimeout = &dur + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "init_timeout") + assert.False(t, result.RequiresRestart) + }) + + t.Run("max_result_size_chars change detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + val := 250000 + newCfg := base() + newCfg.MaxResultSizeChars = &val + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "max_result_size_chars") + assert.False(t, result.RequiresRestart) + }) + + t.Run("forward_proxy_env change detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + oldCfg.ForwardProxyEnv = false + newCfg := base() + newCfg.ForwardProxyEnv = true + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "forward_proxy_env") + assert.False(t, result.RequiresRestart) + }) + + t.Run("debug_search change detected hot-reloadable", func(t *testing.T) { + oldCfg := base() + oldCfg.DebugSearch = false + newCfg := base() + newCfg.DebugSearch = true + + result := DetectConfigChanges(oldCfg, newCfg) + require.True(t, result.Success) + assert.Contains(t, result.ChangedFields, "debug_search") + assert.False(t, result.RequiresRestart) + }) +} +