diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index dc4cf5b49..4aafe1470 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -432,16 +432,6 @@ func runServer(cmd *cobra.Command, _ []string) error { // Get flag values from command (handles both global and local flags) cmdLogLevel, _ := cmd.Flags().GetString("log-level") cmdLogToFile, _ := cmd.Flags().GetBool("log-to-file") - cmdLogDir, _ := cmd.Flags().GetString("log-dir") - cmdDebugSearch, _ := cmd.Flags().GetBool("debug-search") - cmdToolResponseLimit, _ := cmd.Flags().GetInt("tool-response-limit") - cmdRequireMCPAuth, _ := cmd.Flags().GetBool("require-mcp-auth") - cmdReadOnlyMode, _ := cmd.Flags().GetBool("read-only") - cmdDisableManagement, _ := cmd.Flags().GetBool("disable-management") - cmdAllowServerAdd, _ := cmd.Flags().GetBool("allow-server-add") - cmdAllowServerRemove, _ := cmd.Flags().GetBool("allow-server-remove") - cmdEnablePrompts, _ := cmd.Flags().GetBool("enable-prompts") - cmdAggregateUpstreamPrompts, _ := cmd.Flags().GetBool("aggregate-upstream-prompts") // Load configuration first to get logging settings cfg, saver, err := loadConfig(cmd) @@ -449,51 +439,7 @@ func runServer(cmd *cobra.Command, _ []string) error { return fmt.Errorf("failed to load configuration: %w", err) } - // Override logging settings from command line - if cfg.Logging == nil { - // Use command-specific default level (INFO for server command) - defaultLevel := cmdLogLevel - if defaultLevel == "" { - defaultLevel = defaultLogLevel // Server command defaults to INFO - } - - cfg.Logging = &config.LogConfig{ - Level: defaultLevel, - EnableFile: !cmd.Flags().Changed("log-to-file") || cmdLogToFile, // Default true for serve, unless explicitly disabled - EnableConsole: true, - Filename: "main.log", - MaxSize: 10, - MaxBackups: 5, - MaxAge: 30, - Compress: true, - JSONFormat: false, - } - } else { - // Override specific fields from command line - if cmdLogLevel != "" { - cfg.Logging.Level = cmdLogLevel - } else if cfg.Logging.Level == "" { - cfg.Logging.Level = defaultLogLevel // Server command defaults to INFO - } - - // For serve mode: Enable file logging by default, only disable if explicitly set to false - if cmd.Flags().Changed("log-to-file") { - cfg.Logging.EnableFile = cmdLogToFile - } else { - cfg.Logging.EnableFile = true // Default to true for serve mode - } - - if cfg.Logging.Filename == "" || cfg.Logging.Filename == "mcpproxy.log" { - cfg.Logging.Filename = "main.log" - } - } - - // Resolve the log directory. An explicit --log-dir wins; otherwise a - // non-default data dir co-locates logs under /logs so that - // tests/e2e/harness `serve` runs do not pollute the shared OS-standard - // prod log (root cause of the phantom "core restarts every 10s" in - // MCP-2250). The default data dir keeps the OS-standard location. - cfg.Logging.LogDir = resolveServeLogDir(cmdLogDir, cfg.Logging.LogDir, cfg.DataDir, defaultDataDirPath()) + applyServeLoggingFlags(cmd, cfg) // Setup logger with new logging system logger, err := logs.SetupLogger(cfg.Logging) @@ -545,38 +491,11 @@ func runServer(cmd *cobra.Command, _ []string) error { // Issue #566: registries (e.g. Pulse) require a versioned User-Agent. registries.SetVersion(version) - // Override other settings from command line - cfg.DebugSearch = cmdDebugSearch - - if cmdToolResponseLimit != 0 { - cfg.ToolResponseLimit = cmdToolResponseLimit - } - - // Apply security settings from command line ONLY if explicitly set - if cmd.Flags().Changed("require-mcp-auth") { - cfg.RequireMCPAuth = cmdRequireMCPAuth - } - if cmd.Flags().Changed("read-only") { - cfg.ReadOnlyMode = cmdReadOnlyMode - } - if cmd.Flags().Changed("disable-management") { - cfg.DisableManagement = cmdDisableManagement - } - if cmd.Flags().Changed("allow-server-add") { - cfg.AllowServerAdd = cmdAllowServerAdd - } - if cmd.Flags().Changed("allow-server-remove") { - cfg.AllowServerRemove = cmdAllowServerRemove - } - if cmd.Flags().Changed("enable-prompts") { - cfg.EnablePrompts = cmdEnablePrompts - } - if cmd.Flags().Changed("aggregate-upstream-prompts") { - cfg.AggregateUpstreamPrompts = cmdAggregateUpstreamPrompts - } + applyServeRuntimeFlags(cmd, cfg) logger.Info("Configuration loaded", zap.String("data_dir", cfg.DataDir), + zap.Strings("process_overrides", config.ProcessOverrideFields()), zap.Int("servers_count", len(cfg.Servers)), zap.Bool("require_mcp_auth", cfg.RequireMCPAuth), zap.Bool("read_only_mode", cfg.ReadOnlyMode), @@ -822,9 +741,11 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { // never persist a one-off CLI choice. saver := newServeConfigSaver(cfg, loadedPath) - // Override with command line flags ONLY if they were explicitly set + // Override with command line flags ONLY if they were explicitly set. Each + // one is a process-only override (config.OverrideForProcess): the effective + // config carries it, no save path persists it — see process_overrides.go. if dataDir != "" { - cfg.DataDir = dataDir + config.OverrideForProcess(cfg, config.FieldDataDir, config.OverrideSourceFlag, dataDir) } if cmd.Flags().Changed("listen") { listenFlag, _ := cmd.Flags().GetString("listen") @@ -836,18 +757,18 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { if listenFlag == "" { listenFlag = ":0" } - cfg.Listen = listenFlag + config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, listenFlag) } if cmd.Flags().Changed("tray-endpoint") { trayEndpointFlag, _ := cmd.Flags().GetString("tray-endpoint") - cfg.TrayEndpoint = trayEndpointFlag + config.OverrideForProcess(cfg, config.FieldTrayEndpoint, config.OverrideSourceFlag, trayEndpointFlag) } if cmd.Flags().Changed("enable-socket") { enableSocketFlag, _ := cmd.Flags().GetBool("enable-socket") - cfg.EnableSocket = enableSocketFlag + config.OverrideForProcess(cfg, config.FieldEnableSocket, config.OverrideSourceFlag, enableSocketFlag) } if toolResponseLimit != 0 { - cfg.ToolResponseLimit = toolResponseLimit + config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, toolResponseLimit) } applyToolResponseModeFlag(cfg, cmd.Flags().Changed("tool-response-mode"), toolResponseMode) applyDirectToolResponseModeFlag(cfg, cmd.Flags().Changed("direct-tool-response-mode"), directToolResponseMode) @@ -867,7 +788,7 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { // invalid values with a tool_response_mode error. func applyToolResponseModeFlag(cfg *config.Config, changed bool, mode string) { if changed { - cfg.ToolResponseMode = mode + config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, mode) } } @@ -879,7 +800,89 @@ func applyToolResponseModeFlag(cfg *config.Config, changed bool, mode string) { // rejects invalid values with a direct_tool_response_mode error. func applyDirectToolResponseModeFlag(cfg *config.Config, changed bool, mode string) { if changed { - cfg.DirectToolResponseMode = mode + config.OverrideForProcess(cfg, config.FieldDirectToolResponseMode, config.OverrideSourceFlag, mode) + } +} + +// applyServeLoggingFlags fills serve's logging defaults and layers the +// --log-level / --log-to-file / --log-dir flags on top. The flags are +// process-only overrides (config.OverrideForProcess) so no save path writes +// them into the file; the serve defaults (INFO, file logging on, main.log) +// are plain in-memory fills, as they always were. +func applyServeLoggingFlags(cmd *cobra.Command, cfg *config.Config) { + cmdLogLevel, _ := cmd.Flags().GetString("log-level") + cmdLogToFile, _ := cmd.Flags().GetBool("log-to-file") + cmdLogDir, _ := cmd.Flags().GetString("log-dir") + + if cfg.Logging == nil { + cfg.Logging = &config.LogConfig{ + EnableConsole: true, + Filename: "main.log", + MaxSize: 10, + MaxBackups: 5, + MaxAge: 30, + Compress: true, + JSONFormat: false, + } + } + + if cmdLogLevel != "" { + config.OverrideForProcess(cfg, config.FieldLogLevel, config.OverrideSourceFlag, cmdLogLevel) + } else if cfg.Logging.Level == "" { + cfg.Logging.Level = defaultLogLevel // Server command defaults to INFO + } + + // For serve mode: Enable file logging by default, only disable if explicitly set to false + if cmd.Flags().Changed("log-to-file") { + config.OverrideForProcess(cfg, config.FieldLogEnableFile, config.OverrideSourceFlag, cmdLogToFile) + } else { + cfg.Logging.EnableFile = true // Default to true for serve mode + } + + if cfg.Logging.Filename == "" || cfg.Logging.Filename == "mcpproxy.log" { + cfg.Logging.Filename = "main.log" + } + + // Resolve the log directory. An explicit --log-dir wins; otherwise a + // non-default data dir co-locates logs under /logs so that + // tests/e2e/harness `serve` runs do not pollute the shared OS-standard + // prod log (root cause of the phantom "core restarts every 10s" in + // MCP-2250). The default data dir keeps the OS-standard location. + logDir := resolveServeLogDir(cmdLogDir, cfg.Logging.LogDir, cfg.DataDir, defaultDataDirPath()) + if cmdLogDir != "" { + config.OverrideForProcess(cfg, config.FieldLogDir, config.OverrideSourceFlag, logDir) + } else { + cfg.Logging.LogDir = logDir + } +} + +// applyServeRuntimeFlags layers the remaining serve flags onto the loaded +// config, each as a process-only override (config.OverrideForProcess). +func applyServeRuntimeFlags(cmd *cobra.Command, cfg *config.Config) { + flags := cmd.Flags() + + // --debug-search has always applied unconditionally (its default is + // false), so it is recorded unconditionally too. + cmdDebugSearch, _ := flags.GetBool("debug-search") + config.OverrideForProcess(cfg, config.FieldDebugSearch, config.OverrideSourceFlag, cmdDebugSearch) + + // Apply security settings from command line ONLY if explicitly set + for _, f := range []struct { + flag string + field config.Field[bool] + }{ + {"require-mcp-auth", config.FieldRequireMCPAuth}, + {"read-only", config.FieldReadOnlyMode}, + {"disable-management", config.FieldDisableManagement}, + {"allow-server-add", config.FieldAllowServerAdd}, + {"allow-server-remove", config.FieldAllowServerRemove}, + {"enable-prompts", config.FieldEnablePrompts}, + {"aggregate-upstream-prompts", config.FieldAggregateUpstreamPrompts}, + } { + if flags.Changed(f.flag) { + v, _ := flags.GetBool(f.flag) + config.OverrideForProcess(cfg, f.field, config.OverrideSourceFlag, v) + } } } diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index d5793fd2f..c67a11277 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -356,3 +356,139 @@ func TestServeSaverUsesTheDiscoveredConfigPath(t *testing.T) { untouched := readConfigFileJSON(t, unrelated) assert.Nil(t, untouched["telemetry"], "save landed in the unrelated config") } + +// newServeRuntimeFlagTestCmd adds the flags runServer (not loadConfig) applies +// onto the loaded config. +func newServeRuntimeFlagTestCmd() *cobra.Command { + cmd := newServeFlagTestCmd() + cmd.Flags().String("log-level", "", "") + cmd.Flags().Bool("log-to-file", true, "") + cmd.Flags().String("log-dir", "", "") + cmd.Flags().Bool("debug-search", false, "") + cmd.Flags().Bool("require-mcp-auth", false, "") + cmd.Flags().Bool("read-only", false, "") + cmd.Flags().Bool("disable-management", false, "") + cmd.Flags().Bool("allow-server-add", true, "") + cmd.Flags().Bool("allow-server-remove", true, "") + cmd.Flags().Bool("enable-prompts", true, "") + cmd.Flags().Bool("aggregate-upstream-prompts", false, "") + return cmd +} + +// The serve saver covers serve's OWN three saves. Every other persist path — +// the runtime's SaveConfiguration on a server enable, telemetry's first-run +// anonymous_id write — goes through config.SaveConfig with the live config. +// Those must not persist the flag overrides either, which is what registering +// every flag as a process-only override (config.OverrideForProcess) buys: the +// central save seam writes the file's value back. +func TestServeFlagsAreRegisteredAsProcessOverrides(t *testing.T) { + saveServeGlobals(t) + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeRuntimeFlagTestCmd() + require.NoError(t, cmd.ParseFlags([]string{ + "--listen", ":0", + "--tray-endpoint", "unix:///tmp/x.sock", + "--enable-socket=false", + "--tool-response-limit", "500", + "--tool-response-mode", "compact", + "--direct-tool-response-mode", "deferred", + "--log-level", "debug", + "--log-to-file=false", + "--log-dir", t.TempDir(), + "--debug-search", + "--require-mcp-auth", + "--read-only", + "--disable-management", + "--allow-server-add=false", + "--allow-server-remove=false", + "--enable-prompts=false", + "--aggregate-upstream-prompts", + })) + + cfg, _, err := loadConfig(cmd) + require.NoError(t, err) + applyServeLoggingFlags(cmd, cfg) + applyServeRuntimeFlags(cmd, cfg) + + // The effective config carries every flag… + assert.Equal(t, ":0", cfg.Listen) + assert.Equal(t, "debug", cfg.Logging.Level) + assert.False(t, cfg.Logging.EnableFile) + assert.True(t, cfg.ReadOnlyMode) + assert.True(t, cfg.DebugSearch) + assert.False(t, cfg.AllowServerAdd) + + // …and a plain runtime-style save writes none of them. + cfg.ToolsLimit = 42 // a genuine in-memory change that must persist + require.NoError(t, config.SaveConfig(cfg, path)) + + file := readConfigFileJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", file["listen"]) + assert.Nil(t, file["tray_endpoint"]) + assert.Equal(t, true, file["enable_socket"]) + assert.Equal(t, float64(20000), file["tool_response_limit"]) + assert.Equal(t, "full", file["tool_response_mode"]) + assert.Equal(t, "full", file["direct_tool_response_mode"]) + logging, _ := file["logging"].(map[string]any) + assert.Equal(t, "info", logging["level"]) + assert.Equal(t, true, logging["enable_file"]) + assert.NotEqual(t, cfg.Logging.LogDir, logging["log_dir"], "--log-dir leaked") + assert.NotEqual(t, true, file["debug_search"]) + assert.NotEqual(t, true, file["require_mcp_auth"]) + assert.NotEqual(t, true, file["read_only_mode"]) + assert.NotEqual(t, true, file["disable_management"]) + assert.NotEqual(t, false, file["allow_server_add"]) + assert.NotEqual(t, false, file["allow_server_remove"]) + assert.NotEqual(t, false, file["enable_prompts"]) + assert.NotEqual(t, true, file["aggregate_upstream_prompts"]) + assert.Equal(t, float64(42), file["tools_limit"]) +} + +// A field the API edits after the flag was applied is a real change and is +// persisted, flag or no flag. +func TestServeFlagOverrideEditedViaAPIIsPersisted(t *testing.T) { + saveServeGlobals(t) + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeRuntimeFlagTestCmd() + require.NoError(t, cmd.ParseFlags([]string{"--listen", ":0", "--read-only"})) + cfg, _, err := loadConfig(cmd) + require.NoError(t, err) + applyServeRuntimeFlags(cmd, cfg) + + cfg.Listen = "127.0.0.1:9090" // the Settings page + cfg.ReadOnlyMode = false // toggled back off + require.NoError(t, config.SaveConfig(cfg, path)) + + file := readConfigFileJSON(t, path) + assert.Equal(t, "127.0.0.1:9090", file["listen"]) + assert.Equal(t, false, file["read_only_mode"]) +} + +// Both loadConfig and runServer apply --tool-response-limit; the second +// registration must not replace the recorded file value (the fallback when +// the file cannot be read at save time) with the flag's own value. +func TestServeFlagsRegisteredTwiceKeepTheFileFallback(t *testing.T) { + saveServeGlobals(t) + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeRuntimeFlagTestCmd() + require.NoError(t, cmd.ParseFlags([]string{"--tool-response-limit", "500"})) + cfg, _, err := loadConfig(cmd) + require.NoError(t, err) + applyServeRuntimeFlags(cmd, cfg) + require.Equal(t, 500, cfg.ToolResponseLimit) + + persisted := config.PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, 20000, persisted.ToolResponseLimit) +} diff --git a/internal/config/config.go b/internal/config/config.go index b53619333..63e621b92 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2224,7 +2224,13 @@ func (c *Config) EnsureAPIKey() (apiKey string, wasGenerated bool, source APIKey // Check environment variable for API key first - this overrides config file // Use LookupEnv to distinguish between "not set" and "set to empty string" if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envAPIKey != "" { - c.APIKey = envAPIKey + // A process-only override of whatever the file holds: no save path + // may replace the file's key with it (see process_overrides.go). + // Already equal means Validate recorded it (or the file literally + // holds the env key); re-recording would lose the file value. + if c.APIKey != envAPIKey { + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) + } return c.APIKey, false, APIKeySourceEnvironment } @@ -2734,7 +2740,10 @@ func (c *Config) Validate() error { // Check environment variable for API key // Use LookupEnv to distinguish between "not set" and "set to empty string" if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists { - c.APIKey = envAPIKey // Allow empty string to explicitly disable authentication + // Allow empty string to explicitly disable authentication. A + // process-only override: no save path may write it into api_key + // (see process_overrides.go). + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) } } diff --git a/internal/config/loader.go b/internal/config/loader.go index e750231fb..62869e35b 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "time" "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" @@ -454,6 +455,46 @@ func atomicWriteFile(path string, data []byte, perm os.FileMode) error { // SaveConfig saves configuration to file func SaveConfig(cfg *Config, path string) error { + return SaveConfigWithEdits(cfg, nil, path) +} + +// SaveConfigWithEdits is SaveConfig for the save that persists an API edit: +// the overridden fields that moved between mergeBase (the config the edit was +// merged onto) and cfg are the caller's edits and are written as they are; +// every other overridden field is written back from the file as in +// SaveConfig. See PersistableConfigWithEdits. +// +// Never persists a process-only override (serve flag, MCPPROXY_* env, env +// API key): the file's value is written back for every field still carrying +// one (see process_overrides.go). That makes every save a read-modify-write +// of the file, so in-process savers — the runtime, telemetry, serve's own +// saves — are serialised: a save always reads the file the previous save +// wrote, and an API edit can never be reverted by a concurrent save that had +// read the file before it landed. (A stale config saved whole still +// overwrites unrelated fields with what it holds — the residual window +// telemetry.persistConfig documents — but a field it merely carries from an +// override is restored from the file, never from that stale copy.) +func SaveConfigWithEdits(cfg, mergeBase *Config, path string) error { + saveConfigMu.Lock() + defer saveConfigMu.Unlock() + persisted := PersistableConfigWithEdits(cfg, mergeBase, path) + if saveConfigTestHook != nil { + saveConfigTestHook() + } + return writeConfigFile(persisted, path) +} + +// saveConfigMu serialises the read-base-then-write of every in-process save. +// Leaf-level: nothing under it takes another lock except the override +// registry's RWMutex (a leaf itself). +var saveConfigMu sync.Mutex + +// saveConfigTestHook, when set, runs between a save's base read and its write +// (under saveConfigMu). Tests only. +var saveConfigTestHook func() + +// writeConfigFile marshals cfg exactly as given and writes it atomically. +func writeConfigFile(cfg *Config, path string) error { data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return fmt.Errorf("failed to marshal config: %w", err) @@ -689,8 +730,14 @@ func expandDataDir(cfg *Config) { cfg.DataDir = resolved } -// applyTLSEnvOverrides applies environment variable overrides for TLS configuration +// 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 +// load so a reload reflects the variables set now. func applyTLSEnvOverrides(cfg *Config) { + b := &envOverrideBatch{} + defer b.commit() + // Ensure TLS config is initialized if cfg.TLS == nil { cfg.TLS = &TLSConfig{ @@ -702,41 +749,35 @@ func applyTLSEnvOverrides(cfg *Config) { } // Override listen address from environment - if value := os.Getenv("MCPPROXY_LISTEN"); value != "" { - cfg.Listen = value - } + value := os.Getenv("MCPPROXY_LISTEN") + envOverride(b, cfg, FieldListen, value != "", value) // Override TLS enabled from environment - if value := os.Getenv("MCPPROXY_TLS_ENABLED"); value != "" { - cfg.TLS.Enabled = (value == trueValue || value == "1") - } + value = os.Getenv("MCPPROXY_TLS_ENABLED") + envOverride(b, cfg, FieldTLSEnabled, value != "", value == trueValue || value == "1") // Override TLS client cert requirement from environment - if value := os.Getenv("MCPPROXY_TLS_REQUIRE_CLIENT_CERT"); value != "" { - cfg.TLS.RequireClientCert = (value == trueValue || value == "1") - } + value = os.Getenv("MCPPROXY_TLS_REQUIRE_CLIENT_CERT") + envOverride(b, cfg, FieldTLSRequireClientCert, value != "", value == trueValue || value == "1") // Override TLS certificates directory from environment - if value := os.Getenv("MCPPROXY_CERTS_DIR"); value != "" { - cfg.TLS.CertsDir = value - } + value = os.Getenv("MCPPROXY_CERTS_DIR") + envOverride(b, cfg, FieldTLSCertsDir, value != "", value) // Override data directory from environment (for backward compatibility) - if value := os.Getenv("MCPPROXY_DATA"); value != "" { - cfg.DataDir = value - } + value = os.Getenv("MCPPROXY_DATA") + envOverride(b, cfg, FieldDataDir, value != "", value) // Override trusted hosts for reverse-proxy deployments (GH #898). // Comma-separated list of Host header values accepted on loopback listeners. - if value := os.Getenv("MCPPROXY_TRUSTED_HOSTS"); value != "" { - var hosts []string - for _, h := range strings.Split(value, ",") { - if h = strings.TrimSpace(h); h != "" { - hosts = append(hosts, h) - } + var hosts []string + value = os.Getenv("MCPPROXY_TRUSTED_HOSTS") + for _, h := range strings.Split(value, ",") { + if h = strings.TrimSpace(h); h != "" { + hosts = append(hosts, h) } - cfg.TrustedHosts = hosts } + envOverride(b, cfg, FieldTrustedHosts, value != "", hosts) // Override trusted proxies (Spec 107 FR-027). Comma-separated CIDRs or // IPs; an empty variable leaves the file value. Entries are validated by @@ -754,12 +795,8 @@ func applyTLSEnvOverrides(cfg *Config) { // (spec 086 FR-019). Explicit MCPPROXY_* alias per the loader convention; // the env value wins over the file value, and materializes the security // block so a config with no `security` key can still point at a corpus. - if value := os.Getenv(EnvTPABundlePath); value != "" { - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.TPABundlePath = value - } + value = os.Getenv(EnvTPABundlePath) + envOverride(b, cfg, FieldTPABundlePath, value != "", value) // Override the automatic informational baseline-scan kill switch from // environment. Materializes the security block so an install with no @@ -771,35 +808,29 @@ func applyTLSEnvOverrides(cfg *Config) { // bare `value != ""` check would have materialized `false` here and silently // turned automatic scanning off for a config that had explicitly enabled it, // because the accessor then reads the overwritten field rather than the env. + var autoScan *bool switch os.Getenv(EnvAutoBaselineScan) { case trueValue, "1": enabled := true - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.AutoBaselineScan = &enabled + autoScan = &enabled case falseValue, "0": enabled := false - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.AutoBaselineScan = &enabled + autoScan = &enabled } + envOverride(b, cfg, FieldAutoBaselineScan, autoScan != nil, autoScan) // Override retrieve_tools serialization mode from environment (Spec 085). // Explicit MCPPROXY_* alias per the established loader convention; the // value is validated by cfg.Validate() right after these overrides apply. - if value := os.Getenv("MCPPROXY_TOOL_RESPONSE_MODE"); value != "" { - cfg.ToolResponseMode = value - } + value = os.Getenv("MCPPROXY_TOOL_RESPONSE_MODE") + envOverride(b, cfg, FieldToolResponseMode, value != "", value) // Override DIRECT-surface serialization mode from environment (Spec 102). // A separate variable from MCPPROXY_TOOL_RESPONSE_MODE above, matching the // separate config axis: that one governs retrieve_tools, this one governs // the direct enumeration surface. Setting one must never move the other. - if value := os.Getenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE"); value != "" { - cfg.DirectToolResponseMode = value - } + value = os.Getenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE") + envOverride(b, cfg, FieldDirectToolResponseMode, value != "", value) // Override the GLOBAL aggregate concurrency limiter from environment // (spec 093 FR-022, GH #955). Only this scope has an env scheme: the @@ -807,28 +838,12 @@ func applyTLSEnvOverrides(cfg *Config) { // An explicit 0 is meaningful (it disables the limiter), so the value is // materialized as a pointer; malformed values are warned about and ignored // so a typo cannot silently reshape the proxy's admission behavior. - if value := os.Getenv("MCPPROXY_MAX_CONCURRENT_REQUESTS"); value != "" { - if n, err := strconv.Atoi(value); err == nil && n >= 0 { - cfg.MaxConcurrentRequests = &n - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_MAX_CONCURRENT_REQUESTS=%q (want a non-negative integer)\n", value) - } - } - if value := os.Getenv("MCPPROXY_QUEUE_SIZE"); value != "" { - if n, err := strconv.Atoi(value); err == nil && n >= 0 { - cfg.QueueSize = &n - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_SIZE=%q (want a non-negative integer)\n", value) - } - } - if value := os.Getenv("MCPPROXY_QUEUE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - qt := Duration(d) - cfg.QueueTimeout = &qt - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_TIMEOUT=%q (want a duration such as \"30s\")\n", value) - } - } + maxReqSet, maxReq := envNonNegativeInt("MCPPROXY_MAX_CONCURRENT_REQUESTS", "want a non-negative integer") + envOverride(b, cfg, FieldMaxConcurrentRequests, maxReqSet, maxReq) + queueSizeSet, queueSize := envNonNegativeInt("MCPPROXY_QUEUE_SIZE", "want a non-negative integer") + envOverride(b, cfg, FieldQueueSize, queueSizeSet, queueSize) + queueTimeoutSet, queueTimeout := envNonNegativeDuration("MCPPROXY_QUEUE_TIMEOUT", "want a duration such as \"30s\"") + envOverride(b, cfg, FieldQueueTimeout, queueTimeoutSet, queueTimeout) // Override the HTTP server's request deadlines from environment (GH #965) // — the escape hatch for operators who cannot edit the config file. An @@ -837,28 +852,42 @@ func applyTLSEnvOverrides(cfg *Config) { // ResolveHTTPIdleTimeout), so the value is materialized as a pointer; // malformed values are warned about and ignored so a typo cannot silently // reintroduce a response-truncating deadline. - if value := os.Getenv("MCPPROXY_HTTP_READ_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - rt := Duration(d) - cfg.HTTPReadTimeout = &rt - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_READ_TIMEOUT=%q (want a duration such as \"120s\", or \"0s\" to disable)\n", value) - } + readTimeoutSet, readTimeout := envNonNegativeDuration("MCPPROXY_HTTP_READ_TIMEOUT", "want a duration such as \"120s\", or \"0s\" to disable") + envOverride(b, cfg, FieldHTTPReadTimeout, readTimeoutSet, readTimeout) + writeTimeoutSet, writeTimeout := envNonNegativeDuration("MCPPROXY_HTTP_WRITE_TIMEOUT", "want a duration such as \"300s\", or \"0s\" to disable") + envOverride(b, cfg, FieldHTTPWriteTimeout, writeTimeoutSet, writeTimeout) + idleTimeoutSet, idleTimeout := envNonNegativeDuration("MCPPROXY_HTTP_IDLE_TIMEOUT", "want a duration such as \"180s\"; \"0s\" falls back to the read timeout") + envOverride(b, cfg, FieldHTTPIdleTimeout, idleTimeoutSet, idleTimeout) +} + +// envNonNegativeInt reads a non-negative integer env override; malformed +// values are warned about and ignored so a typo cannot silently reshape the +// proxy's behavior. +func envNonNegativeInt(name, want string) (bool, *int) { + value := os.Getenv(name) + if value == "" { + return false, nil } - if value := os.Getenv("MCPPROXY_HTTP_WRITE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - wt := Duration(d) - cfg.HTTPWriteTimeout = &wt - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_WRITE_TIMEOUT=%q (want a duration such as \"300s\", or \"0s\" to disable)\n", value) - } + n, err := strconv.Atoi(value) + if err != nil || n < 0 { + fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid %s=%q (%s)\n", name, value, want) + return false, nil } - if value := os.Getenv("MCPPROXY_HTTP_IDLE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - it := Duration(d) - cfg.HTTPIdleTimeout = &it - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_IDLE_TIMEOUT=%q (want a duration such as \"180s\"; \"0s\" falls back to the read timeout)\n", value) - } + return true, &n +} + +// envNonNegativeDuration reads a non-negative duration env override; malformed +// values are warned about and ignored. +func envNonNegativeDuration(name, want string) (bool, *Duration) { + value := os.Getenv(name) + if value == "" { + return false, nil + } + d, err := time.ParseDuration(value) + if err != nil || d < 0 { + fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid %s=%q (%s)\n", name, value, want) + return false, nil } + dur := Duration(d) + return true, &dur } diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go new file mode 100644 index 000000000..9c65bbec1 --- /dev/null +++ b/internal/config/process_overrides.go @@ -0,0 +1,492 @@ +package config + +import ( + "reflect" + "sort" + "sync" +) + +// Process-only overrides: the persisted-vs-effective split. +// +// The config a process runs with is the file PLUS a layer of one-off choices — +// `serve` CLI flags (--listen, --read-only, --tool-response-mode, ...), the +// MCPPROXY_* environment variables the loader applies, and the MCPPROXY_API_KEY +// that Validate copies into api_key. That effective config is the only one the +// daemon holds: the runtime, the telemetry service and every API handler read +// and — crucially — SAVE it. Before this existed, any persist path (first-run +// anonymous_id generation, a server enable from the Web UI, the startup-outcome +// stamp) wrote the overrides into mcp_config.json, and the next unflagged start +// — or the tray-launched core — inherited a choice that was meant for one run +// (`--listen :0` used to leave `"listen": ":0"` behind and the core silently +// booted in stdio mode). +// +// The registry below records, per overridden field, the value this process was +// given and the value the file held when it was applied. PersistableConfig then +// answers "what should go to disk": for every recorded field whose EFFECTIVE +// value still equals the override, the file's value is written back; a field +// that has since been edited (the Settings page changing listen, the tray +// picking an alternate port) no longer matches the override and is persisted +// as the edit it is. That is what keeps the API's legitimate edits of the very +// same fields working — a blanket "always restore the file value" would have +// thrown those edits away. +// +// SaveConfig applies the split centrally so every persist path — the runtime, +// telemetry, the server edition's admin handlers, the CLI subcommands that +// load-modify-save — is covered without each having to remember. Callers that +// need to know the exact bytes going to disk (the runtime's config-watcher +// self-write markers) call PersistableConfig themselves first; the mapping is +// idempotent, so SaveConfig re-applying it is harmless. + +// OverrideSource says where a process-only override came from. +type OverrideSource string + +const ( + // OverrideSourceFlag is a `serve` command-line flag. + OverrideSourceFlag OverrideSource = "flag" + // OverrideSourceEnv is a MCPPROXY_* environment variable, including the + // MCPPROXY_API_KEY that Validate folds into api_key. + OverrideSourceEnv OverrideSource = "env" +) + +// Field names a configuration value a process-only override can shadow. Set +// copies nested structs before writing (copy-on-write) so a PersistableConfig +// result never writes through a pointer it shares with the effective config. +type Field[T any] struct { + Name string + Get func(*Config) T + Set func(*Config, T) +} + +// The overridable fields. Keep in lockstep with the overrides applied in +// cmd/mcpproxy (serve flags), applyTLSEnvOverrides (env) and Validate (env API +// key): an override applied without going through OverrideForProcess is +// persisted by every save path, which is the bug this file exists to close. +var ( + FieldListen = scalar("listen", func(c *Config) *string { return &c.Listen }) + FieldDataDir = scalar("data_dir", func(c *Config) *string { return &c.DataDir }) + FieldAPIKey = scalar("api_key", func(c *Config) *string { return &c.APIKey }) + FieldTrayEndpoint = scalar("tray_endpoint", func(c *Config) *string { return &c.TrayEndpoint }) + FieldEnableSocket = scalar("enable_socket", func(c *Config) *bool { return &c.EnableSocket }) + FieldToolResponseLimit = scalar("tool_response_limit", func(c *Config) *int { return &c.ToolResponseLimit }) + FieldToolResponseMode = scalar("tool_response_mode", func(c *Config) *string { return &c.ToolResponseMode }) + FieldDirectToolResponseMode = scalar("direct_tool_response_mode", func(c *Config) *string { return &c.DirectToolResponseMode }) + FieldDebugSearch = scalar("debug_search", func(c *Config) *bool { return &c.DebugSearch }) + FieldRequireMCPAuth = scalar("require_mcp_auth", func(c *Config) *bool { return &c.RequireMCPAuth }) + FieldReadOnlyMode = scalar("read_only_mode", func(c *Config) *bool { return &c.ReadOnlyMode }) + FieldDisableManagement = scalar("disable_management", func(c *Config) *bool { return &c.DisableManagement }) + FieldAllowServerAdd = scalar("allow_server_add", func(c *Config) *bool { return &c.AllowServerAdd }) + FieldAllowServerRemove = scalar("allow_server_remove", func(c *Config) *bool { return &c.AllowServerRemove }) + FieldEnablePrompts = scalar("enable_prompts", func(c *Config) *bool { return &c.EnablePrompts }) + FieldAggregateUpstreamPrompts = scalar("aggregate_upstream_prompts", func(c *Config) *bool { return &c.AggregateUpstreamPrompts }) + FieldTrustedHosts = scalar("trusted_hosts", func(c *Config) *[]string { return &c.TrustedHosts }) + FieldMaxConcurrentRequests = scalar("max_concurrent_requests", func(c *Config) **int { return &c.MaxConcurrentRequests }) + FieldQueueSize = scalar("queue_size", func(c *Config) **int { return &c.QueueSize }) + FieldQueueTimeout = scalar("queue_timeout", func(c *Config) **Duration { return &c.QueueTimeout }) + FieldHTTPReadTimeout = scalar("http_read_timeout", func(c *Config) **Duration { return &c.HTTPReadTimeout }) + FieldHTTPWriteTimeout = scalar("http_write_timeout", func(c *Config) **Duration { return &c.HTTPWriteTimeout }) + FieldHTTPIdleTimeout = scalar("http_idle_timeout", func(c *Config) **Duration { return &c.HTTPIdleTimeout }) + + FieldLogLevel = Field[string]{ + Name: "logging.level", + Get: func(c *Config) string { return loggingOf(c).Level }, + Set: func(c *Config, v string) { l := cowLogging(c); l.Level = v }, + } + FieldLogEnableFile = Field[bool]{ + Name: "logging.enable_file", + Get: func(c *Config) bool { return loggingOf(c).EnableFile }, + Set: func(c *Config, v bool) { l := cowLogging(c); l.EnableFile = v }, + } + FieldLogDir = Field[string]{ + Name: "logging.log_dir", + Get: func(c *Config) string { return loggingOf(c).LogDir }, + Set: func(c *Config, v string) { l := cowLogging(c); l.LogDir = v }, + } + FieldTLSEnabled = Field[bool]{ + Name: "tls.enabled", + Get: func(c *Config) bool { return tlsOf(c).Enabled }, + Set: func(c *Config, v bool) { t := cowTLS(c); t.Enabled = v }, + } + FieldTLSRequireClientCert = Field[bool]{ + Name: "tls.require_client_cert", + Get: func(c *Config) bool { return tlsOf(c).RequireClientCert }, + Set: func(c *Config, v bool) { t := cowTLS(c); t.RequireClientCert = v }, + } + FieldTLSCertsDir = Field[string]{ + Name: "tls.certs_dir", + Get: func(c *Config) string { return tlsOf(c).CertsDir }, + Set: func(c *Config, v string) { t := cowTLS(c); t.CertsDir = v }, + } + FieldTPABundlePath = Field[string]{ + Name: "security.tpa_bundle_path", + Get: func(c *Config) string { return securityOf(c).TPABundlePath }, + Set: func(c *Config, v string) { s := cowSecurity(c); s.TPABundlePath = v }, + } + FieldAutoBaselineScan = Field[*bool]{ + Name: "security.auto_baseline_scan", + Get: func(c *Config) *bool { return securityOf(c).AutoBaselineScan }, + Set: func(c *Config, v *bool) { + if c.Security == nil && v == nil { + return // nothing to unset; do not materialize an empty block + } + s := cowSecurity(c) + s.AutoBaselineScan = v + }, + } +) + +// scalar builds a Field for a top-level value addressed by pointer. +func scalar[T any](name string, ptr func(*Config) *T) Field[T] { + return Field[T]{ + Name: name, + Get: func(c *Config) T { return *ptr(c) }, + Set: func(c *Config, v T) { *ptr(c) = v }, + } +} + +// loggingOf/tlsOf/securityOf read a nested block, treating a missing one as +// its zero value; cowLogging/cowTLS/cowSecurity replace the block with a copy +// before a write so the pointer shared with the effective config is untouched. +func loggingOf(c *Config) LogConfig { + if c.Logging == nil { + return LogConfig{} + } + return *c.Logging +} + +func cowLogging(c *Config) *LogConfig { + l := loggingOf(c) + c.Logging = &l + return c.Logging +} + +func tlsOf(c *Config) TLSConfig { + if c.TLS == nil { + return TLSConfig{} + } + return *c.TLS +} + +func cowTLS(c *Config) *TLSConfig { + t := tlsOf(c) + c.TLS = &t + return c.TLS +} + +func securityOf(c *Config) SecurityConfig { + if c.Security == nil { + return SecurityConfig{} + } + return *c.Security +} + +func cowSecurity(c *Config) *SecurityConfig { + s := securityOf(c) + c.Security = &s + return c.Security +} + +// processOverride is one recorded override. +type processOverride interface { + name() string + source() OverrideSource + // restore writes the persisted value of the field into out when out still + // carries the process value. base is the file as it stands now (nil when + // unreadable, in which case the value the file held at override time is + // used). + restore(out, base *Config) + // loadedValue is the file value recorded when the override was applied. + loadedValue() any + // supersededBy reports whether live no longer carries the process value. + supersededBy(live *Config) bool + // movedBetween reports whether the field differs between base and next. + movedBetween(base, next *Config) bool + // reapply layers the process value back onto a freshly loaded cfg and + // returns the entry with its recorded file value refreshed (loaded is + // what cfg held before, or fileValue when the caller knows better). + reapply(cfg *Config, fileValue any, useFileValue bool) processOverride +} + +type typedOverride[T any] struct { + field Field[T] + src OverrideSource + process T // the value this process runs with + loaded T // the file's value when the override was applied +} + +func (o typedOverride[T]) name() string { return o.field.Name } +func (o typedOverride[T]) source() OverrideSource { return o.src } +func (o typedOverride[T]) loadedValue() any { return o.loaded } + +func (o typedOverride[T]) restore(out, base *Config) { + if !reflect.DeepEqual(o.field.Get(out), o.process) { + return // edited since the override was applied: a real change, persist it + } + fileValue := o.loaded + if base != nil { + fileValue = o.field.Get(base) + } + o.field.Set(out, fileValue) +} + +// supersededBy reports whether live no longer carries the process value. +func (o typedOverride[T]) supersededBy(live *Config) bool { + return !reflect.DeepEqual(o.field.Get(live), o.process) +} + +// movedBetween reports whether the field differs between base and next. +func (o typedOverride[T]) movedBetween(base, next *Config) bool { + return !reflect.DeepEqual(o.field.Get(base), o.field.Get(next)) +} + +func (o typedOverride[T]) reapply(cfg *Config, fileValue any, useFileValue bool) processOverride { + o.loaded = o.field.Get(cfg) + if v, ok := fileValue.(T); ok && useFileValue { + o.loaded = v + } + o.field.Set(cfg, o.process) + return o +} + +// overrideKey identifies one registry entry: the same field may be overridden +// by env AND by a flag (the flag, applied later, wins in memory), and both +// records have to survive — a reload rebuilds the env set, and dropping the +// flag record with it would turn the flag's value into "an edit" on the next +// save. +type overrideKey struct { + field string + source OverrideSource +} + +var ( + processOverridesMu sync.RWMutex + processOverrides = map[overrideKey]processOverride{} +) + +// OverrideForProcess sets field f on cfg to value for THIS PROCESS ONLY and +// records it, so PersistableConfig (and therefore SaveConfig) writes the file's +// value back as long as the effective value still equals the override. +func OverrideForProcess[T any](cfg *Config, f Field[T], source OverrideSource, value T) { + if cfg == nil { + return + } + entry := newOverride(cfg, f, source, value) + f.Set(cfg, value) + + processOverridesMu.Lock() + processOverrides[overrideKey{f.Name, source}] = entry + processOverridesMu.Unlock() +} + +// newOverride builds the record for applying value to f WITHOUT setting it. +// The recorded file value is what cfg holds now — unless this is a flag +// layered over an env override of the same field, whose record already +// knows the real file value. +// +// A repeated registration of the same override (loadConfig and runServer both +// apply --tool-response-limit; Validate runs twice) finds the previous +// override already in place, so the file value is inherited from the +// existing record rather than read off cfg. +func newOverride[T any](cfg *Config, f Field[T], source OverrideSource, value T) typedOverride[T] { + loaded := f.Get(cfg) + processOverridesMu.RLock() + prev, hasPrev := processOverrides[overrideKey{f.Name, source}] + env, hasEnv := processOverrides[overrideKey{f.Name, OverrideSourceEnv}] + processOverridesMu.RUnlock() + if hasPrev { + if p, ok := prev.(typedOverride[T]); ok && reflect.DeepEqual(loaded, p.process) { + loaded = p.loaded + } + } + if source == OverrideSourceFlag && hasEnv { + if v, ok := env.loadedValue().(T); ok { + loaded = v + } + } + return typedOverride[T]{field: f, src: source, process: value, loaded: loaded} +} + +// envOverrideBatch collects the env overrides of one load and commits them in +// ONE registry update. The loader rebuilds the env set on every load (so a +// reload reflects the variables set now); clearing and re-adding under +// separate locks would leave a window in which a save on another goroutine — +// telemetry, the runtime — sees no env entries at all and persists them. +type envOverrideBatch struct { + managed []string // every field the loader consults, set or not + entries []processOverride // the ones that are set +} + +// consider applies an env override for f when present and, either way, marks +// f as env-managed so a stale entry for it is dropped at commit. +func envOverride[T any](b *envOverrideBatch, cfg *Config, f Field[T], present bool, value T) { + b.managed = append(b.managed, f.Name) + if !present { + return + } + b.entries = append(b.entries, newOverride(cfg, f, OverrideSourceEnv, value)) + f.Set(cfg, value) +} + +// commit atomically replaces the env entries of every managed field with the +// batch. Entries of other sources (flags) and env entries the loader does not +// manage (api_key, recorded by Validate/EnsureAPIKey) are untouched. +func (b *envOverrideBatch) commit() { + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for _, name := range b.managed { + delete(processOverrides, overrideKey{name, OverrideSourceEnv}) + } + for _, e := range b.entries { + processOverrides[overrideKey{e.name(), OverrideSourceEnv}] = e + } +} + +// ReapplyFlagOverrides layers every flag-sourced override back onto cfg — a +// config freshly loaded from the file, on which the loader has already +// re-applied the env overrides but knows nothing about the serve flags — and +// refreshes each record's file value. Without it a hot reload after an +// external edit silently switched `--read-only` (or any other flag) off. +// +// live is the config this process was running before the reload. A flag the +// live config no longer carries was superseded by an API edit (which is on +// disk by now) and is not resurrected over that edit. +func ReapplyFlagOverrides(cfg, live *Config) { + if cfg == nil { + return + } + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for key, o := range processOverrides { + if key.source != OverrideSourceFlag { + continue + } + if live != nil && o.supersededBy(live) { + // Superseded by an API edit that is on disk by now: the file + // value stands. The record is KEPT — a stale config still + // carrying the flag value (telemetry's, an in-flight save's) + // must go on restoring the file value rather than persist it. + continue + } + // Over an env override of the same field the config already carries + // the env value; the env record knows what the file said. + env, hasEnv := processOverrides[overrideKey{key.field, OverrideSourceEnv}] + var fileValue any + if hasEnv { + fileValue = env.loadedValue() + } + processOverrides[key] = o.reapply(cfg, fileValue, hasEnv) + } +} + +// effectiveOverridesLocked returns the one override that is in force per +// field: a flag shadows an env override of the same field (the flag is +// applied after the env value, so it is what the process actually runs with). +// Only the winner may decide whether the field was edited — the shadowed env +// record would otherwise intercept an API edit that happens to equal the env +// value. Caller must hold processOverridesMu (read or write). +func effectiveOverridesLocked() []processOverride { + winners := make(map[string]processOverride, len(processOverrides)) + for key, o := range processOverrides { + if prev, ok := winners[key.field]; ok && prev.source() == OverrideSourceFlag { + continue + } + winners[key.field] = o + } + out := make([]processOverride, 0, len(winners)) + for _, o := range winners { + out = append(out, o) + } + return out +} + +// ResetProcessOverrides forgets every recorded override. For tests. +func ResetProcessOverrides() { + processOverridesMu.Lock() + processOverrides = map[overrideKey]processOverride{} + processOverridesMu.Unlock() +} + +// ProcessOverrideFields lists the names of the fields currently overridden for +// this process, sorted and de-duplicated, for diagnostics and logging. +func ProcessOverrideFields() []string { + processOverridesMu.RLock() + defer processOverridesMu.RUnlock() + seen := make(map[string]struct{}, len(processOverrides)) + names := make([]string, 0, len(processOverrides)) + for key := range processOverrides { + if _, dup := seen[key.field]; dup { + continue + } + seen[key.field] = struct{}{} + names = append(names, key.field) + } + sort.Strings(names) + return names +} + +// PersistableConfig returns the config that should go to disk at path for the +// given effective config: a shallow copy in which every field that still +// carries its process-only override is replaced by the value the file at path +// holds now (or held when the override was applied, if the file cannot be read). +// Fields that were edited since keep the edit. With no overrides recorded the +// effective config is returned as is. +// +// The copy shares Servers, Registries and every nested block it does not touch +// with effective; the ones it restores are copied first. Callers must not +// mutate the shared structures. +// +// A save that carries an API edit uses PersistableConfigWithEdits instead, so +// the edited fields are persisted whatever they equal. +func PersistableConfig(effective *Config, path string) *Config { + return PersistableConfigWithEdits(effective, nil, path) +} + +// PersistableConfigWithEdits is PersistableConfig for the save that persists +// an API edit: mergeBase is the config the edit was merged onto (the desired +// config for PUT/PATCH /api/v1/config), and every overridden field that +// MOVED between mergeBase and effective is the caller's edit — persisted as +// is, whatever it moved to. Moving listen from the file's address to the +// flag's own address is the operator making that address permanent +// (base != next == override is distinguishable, unlike a plain round trip); +// moving it back to the file's value is an edit too. A field that merely +// round-tripped a value mergeBase already held — the file's listen after a +// disk reload, say — keeps restoring the file value, whatever it equals. +// +// The override records are never removed by an edit. The one save that +// carries the edit ignores them; every other save — a concurrent telemetry +// write of the still-live config, the next server enable — keeps restoring +// the CURRENT file value, which after the edit's save is the edit itself. +// That is what makes an edit of api_key under MCPPROXY_API_KEY safe: there +// is no window in which a stale config carrying the env secret is +// unprotected. +// +// Known limitation: an edit that sets an overridden field to exactly the +// override's value while mergeBase already holds that value is +// indistinguishable from a round trip and is not persisted — unreachable +// from the Web UI, which already shows the override as the current value. +func PersistableConfigWithEdits(effective, mergeBase *Config, path string) *Config { + if effective == nil { + return nil + } + processOverridesMu.RLock() + overrides := effectiveOverridesLocked() + processOverridesMu.RUnlock() + if len(overrides) == 0 { + return effective + } + + var base *Config + if path != "" { + if onDisk, err := DecodeConfigFile(path); err == nil { + base = onDisk + } + } + + out := *effective + for _, o := range overrides { + if mergeBase != nil && o.movedBetween(mergeBase, effective) { + continue // the caller's edit + } + o.restore(&out, base) + } + return &out +} diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go new file mode 100644 index 000000000..620a4f316 --- /dev/null +++ b/internal/config/process_overrides_test.go @@ -0,0 +1,576 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A process-only override (CLI flag, MCPPROXY_* env, env API key) shadows the +// file value for this one process. The effective config carries the override; +// the persisted config must not — unless something edited the field afterwards +// (an API PUT of a new listen address), which is a real change to persist. + +func writeOverrideTestFile(t *testing.T, raw string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "mcp_config.json") + require.NoError(t, os.WriteFile(path, []byte(raw), 0o600)) + return path +} + +func readJSON(t *testing.T, path string) map[string]any { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + return m +} + +func TestOverrideForProcess_SetsEffectiveValueAndRecordsIt(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Listen = "127.0.0.1:8080" + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + assert.Equal(t, ":0", cfg.Listen, "the override is the effective value") + assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) +} + +func TestPersistableConfig_RestoresFileValueWhileOverrideStillApplies(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + persisted := PersistableConfig(cfg, path) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) + assert.Equal(t, ":0", cfg.Listen, "the effective config is left untouched") +} + +func TestPersistableConfig_KeepsAnEditOfTheOverriddenField(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + // An API edit: the effective value no longer matches the override. + edited := *cfg + edited.Listen = "127.0.0.1:9090" + persisted := PersistableConfig(&edited, path) + assert.Equal(t, "127.0.0.1:9090", persisted.Listen, "an edit of an overridden field is persisted") +} + +func TestPersistableConfig_PrefersTheCurrentFileOverTheLoadTimeValue(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + // Something else (an API edit, an external editor) moved the file on. + require.NoError(t, os.WriteFile(path, []byte(`{"listen": "127.0.0.1:7070", "mcpServers": []}`), 0o600)) + + persisted := PersistableConfig(cfg, path) + assert.Equal(t, "127.0.0.1:7070", persisted.Listen, "a round-tripped override must not resurrect the load-time file value") +} + +func TestPersistableConfig_FallsBackToLoadTimeValueWhenFileUnreadable(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Listen = "127.0.0.1:8080" + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) +} + +func TestPersistableConfig_NestedFieldsAreCopiedNotMutated(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Logging = &LogConfig{Level: "info"} + cfg.TLS.Enabled = false + OverrideForProcess(cfg, FieldLogLevel, OverrideSourceFlag, "debug") + OverrideForProcess(cfg, FieldTLSEnabled, OverrideSourceEnv, true) + require.Equal(t, "debug", cfg.Logging.Level) + require.True(t, cfg.TLS.Enabled) + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "info", persisted.Logging.Level) + assert.False(t, persisted.TLS.Enabled) + assert.Equal(t, "debug", cfg.Logging.Level, "restoring must not write through the shared Logging pointer") + assert.True(t, cfg.TLS.Enabled, "restoring must not write through the shared TLS pointer") + assert.NotSame(t, cfg.Logging, persisted.Logging) + assert.NotSame(t, cfg.TLS, persisted.TLS) +} + +func TestPersistableConfig_NoOverridesReturnsInput(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + cfg := DefaultConfig() + assert.Same(t, cfg, PersistableConfig(cfg, "/nonexistent")) +} + +func TestSaveConfig_DoesNotPersistProcessOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "read_only_mode": false, "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + OverrideForProcess(cfg, FieldAPIKey, OverrideSourceEnv, "env-secret") + cfg.ToolsLimit = 42 // an ordinary in-memory edit, persisted as usual + + require.NoError(t, SaveConfig(cfg, path)) + + m := readJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", m["listen"]) + assert.NotEqual(t, true, m["read_only_mode"]) + assert.NotEqual(t, "env-secret", m["api_key"]) + assert.Equal(t, float64(42), m["tools_limit"]) +} + +func TestLoadFromFile_RecordsEnvOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "0.0.0.0:9999") + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + t.Setenv("MCPPROXY_API_KEY", "from-env") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "0.0.0.0:9999", cfg.Listen) + require.Equal(t, "compact", cfg.ToolResponseMode) + require.Equal(t, "from-env", cfg.APIKey) + + require.NoError(t, SaveConfig(cfg, path)) + m := readJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", m["listen"]) + assert.Equal(t, "full", m["tool_response_mode"]) + assert.NotEqual(t, "from-env", m["api_key"]) +} + +func TestLoadFromFile_ReplacesEnvOverridesButKeepsFlagOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + assert.ElementsMatch(t, []string{"listen", "tool_response_mode"}, ProcessOverrideFields()) + + // A reload with the variable gone drops the env entry and keeps the flag. + os.Unsetenv("MCPPROXY_TOOL_RESPONSE_MODE") + _, err = LoadFromFile(path) + require.NoError(t, err) + assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) +} + +// Round-1 review findings. + +// EnsureAPIKey lets MCPPROXY_API_KEY win over a key the FILE holds; that is +// an override like Validate's and must not replace the file key on disk. +func TestEnsureAPIKey_EnvKeyOverFileKeyIsNotPersisted(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "api_key": "file-key", "mcpServers": []}`) + t.Setenv("MCPPROXY_API_KEY", "env-key") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "file-key", cfg.APIKey, "Validate only fills an EMPTY api_key from env") + + key, generated, source := cfg.EnsureAPIKey() + require.Equal(t, "env-key", key) + require.False(t, generated) + require.Equal(t, APIKeySourceEnvironment, source) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "file-key", readJSON(t, path)["api_key"]) +} + +// A key EnsureAPIKey generated is the one thing that must persist. +func TestEnsureAPIKey_GeneratedKeyIsPersisted(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + os.Unsetenv("MCPPROXY_API_KEY") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + key, generated, _ := cfg.EnsureAPIKey() + require.True(t, generated) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, key, readJSON(t, path)["api_key"]) +} + +// The same field overridden by env AND a flag keeps both records: the flag +// wins in memory, neither leaks, and a reload (which rebuilds the env set) +// must not turn the flag's value into "an edit" by dropping its record. +func TestOverrides_EnvAndFlagOnTheSameFieldBothSurviveReload(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:9000") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9999") + require.Equal(t, "127.0.0.1:9999", cfg.Listen) + + // The reload: the loader rebuilds the env entries on a fresh config. + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9000", reloaded.Listen, "the loader applies env") + ReapplyFlagOverrides(reloaded, nil) + assert.Equal(t, "127.0.0.1:9999", reloaded.Listen, "flag overrides are re-applied on reload") + + require.NoError(t, SaveConfig(reloaded, path)) + assert.Equal(t, "127.0.0.1:8080", readJSON(t, path)["listen"]) + + // The original (pre-reload) effective config saves the same way. + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "127.0.0.1:8080", readJSON(t, path)["listen"]) +} + +// The flag record's load-time fallback is the FILE value, not the env value +// the flag happened to be layered over. +func TestOverrides_FlagOverEnvFallsBackToFileValue(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:9000") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9999") + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) +} + +// ReapplyFlagOverrides re-layers every flag-sourced override onto a freshly +// loaded config and refreshes the recorded file value. +func TestReapplyFlagOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "logging": {"level": "info"}, "mcpServers": []}`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + OverrideForProcess(cfg, FieldLogLevel, OverrideSourceFlag, "debug") + + // The file changed and was reloaded. + require.NoError(t, os.WriteFile(path, []byte(`{"read_only_mode": false, "logging": {"level": "warn"}, "mcpServers": []}`), 0o600)) + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "warn", reloaded.Logging.Level) + ReapplyFlagOverrides(reloaded, nil) + assert.True(t, reloaded.ReadOnlyMode) + assert.Equal(t, "debug", reloaded.Logging.Level) + + persisted := PersistableConfig(reloaded, filepath.Join(t.TempDir(), "missing.json")) + assert.False(t, persisted.ReadOnlyMode) + assert.Equal(t, "warn", persisted.Logging.Level, "the fallback tracks the RELOADED file value") +} + +// Rebuilding the env set on a reload must be atomic with respect to saves on +// other goroutines: a save that lands mid-rebuild must never see an empty (or +// half-built) registry and persist the overrides. +func TestOverrides_EnvRebuildIsAtomicWithSaves(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "0.0.0.0:9999") + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + } + _, _ = LoadFromFile(path) // rebuilds the env entries + } + }() + for i := 0; i < 200; i++ { + persisted := PersistableConfig(cfg, path) + if persisted.Listen != "127.0.0.1:8080" || persisted.ToolResponseMode != "full" { + close(stop) + <-done + t.Fatalf("iteration %d: env override leaked mid-rebuild: listen=%q mode=%q", i, persisted.Listen, persisted.ToolResponseMode) + } + } + close(stop) + <-done +} + +// Round-2 review findings. + +// A flag the API superseded in this process (a hot edit to a different +// value) must not come back on a reload: the edit is on disk, the flag is +// retired. +func TestReapplyFlagOverrides_SkipsAFlagTheLiveConfigSuperseded(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "tool_response_mode": "full", "mcpServers": []}`) + + live, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldReadOnlyMode, OverrideSourceFlag, false) // explicit --read-only=false + OverrideForProcess(live, FieldToolResponseMode, OverrideSourceFlag, "compact") + + live.ReadOnlyMode = true // the API edit, applied hot and persisted + require.NoError(t, os.WriteFile(path, []byte(`{"read_only_mode": true, "tool_response_mode": "full", "tools_limit": 5, "mcpServers": []}`), 0o600)) + + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + ReapplyFlagOverrides(reloaded, live) + assert.True(t, reloaded.ReadOnlyMode, "the superseded flag must not be resurrected") + assert.Equal(t, "compact", reloaded.ToolResponseMode, "the untouched flag is re-applied") + assert.ElementsMatch(t, []string{"read_only_mode", "tool_response_mode"}, ProcessOverrideFields(), + "the record stays: a stale config still carrying the flag value must keep restoring the file") +} + +// Registering the same override twice (loadConfig and runServer both apply +// --tool-response-limit; Validate runs twice) must keep the FILE value as the +// fallback, not the flag value the second registration finds in place. +func TestOverrideForProcess_RepeatedRegistrationKeepsTheFileFallback(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.ToolResponseLimit = 20000 + OverrideForProcess(cfg, FieldToolResponseLimit, OverrideSourceFlag, 500) + OverrideForProcess(cfg, FieldToolResponseLimit, OverrideSourceFlag, 500) + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, 20000, persisted.ToolResponseLimit) +} + +// Round-4 review findings. + +// With env AND a flag on the same field only the flag is effective; the env +// record must not intercept an API edit that happens to equal the env value. +func TestPersistableConfig_StackedEnvAndFlag_EditToTheEnvValuePersists(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + + cfg.DirectToolResponseMode = "deferred" // the API edit: visibly different from "compact" + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) +} + +// Round-5 review finding. + +// Edit-aware saves (review rounds 2-8). An override is never "retired": the +// save that persists an API edit ignores the overrides of the fields the +// caller MOVED relative to its merge base, and every other save keeps +// restoring the file value. No mutable state means no window in which a +// concurrent save of the still-live config is unprotected. + +func TestSaveConfigWithEdits_PersistsAnEditBackToTheOverrideValue(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"tool_response_mode": "full", "listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceFlag, "compact") + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + base := *cfg + next := base + next.ToolResponseMode = "full" // away from the flag + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "full", readJSON(t, path)["tool_response_mode"]) + + base = next + next.ToolResponseMode = "compact" // and back to it: still an edit + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + m := readJSON(t, path) + assert.Equal(t, "compact", m["tool_response_mode"]) + assert.Equal(t, "127.0.0.1:8080", m["listen"], "the untouched override is still not persisted") +} + +func TestSaveConfigWithEdits_RoundTripIsNotAnEdit(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + + base := *cfg + next := base + next.ToolsLimit = 42 // the only thing the caller changed + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + m := readJSON(t, path) + assert.NotEqual(t, true, m["read_only_mode"]) + assert.Equal(t, float64(42), m["tools_limit"]) +} + +// Moving a field from a base value to the override's own value is an edit +// (base != next == override is distinguishable, unlike a round trip). +func TestSaveConfigWithEdits_MovingToTheOverrideValueIsAnEdit(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9000") + + base := *cfg + base.Listen = "127.0.0.1:8080" // the desired config after a disk reload + next := base + next.Listen = "127.0.0.1:9000" // the operator makes the flag's address permanent + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "127.0.0.1:9000", readJSON(t, path)["listen"]) +} + +// With env AND a flag on the same field only the flag is effective; the env +// record must not intercept a plain save of an edit that equals the env value +// once that edit is on disk. +func TestPersistableConfig_StackedEnvAndFlag_RestoresFromTheFile(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + + base := *cfg + next := base + next.DirectToolResponseMode = "deferred" // visibly different from "compact" + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) + + // A later plain save of the same effective config keeps what is on disk. + require.NoError(t, SaveConfig(&next, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) +} + +// While an API save persists a rotated api_key, a concurrent plain save of +// the still-live config (telemetry, another runtime path) must never write +// the env secret: the override record is never removed, only ignored by the +// one save that edits the field. +func TestSaveConfigWithEdits_ConcurrentStaleSaveNeverLeaksTheEnvKey(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + live, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + } + _ = SaveConfig(live, path) // the stale live config, env-secret and all + } + }() + for i := 0; i < 100; i++ { + base := *live + next := base + next.APIKey = "rotated-key" + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + if got := readJSON(t, path)["api_key"]; got == "env-secret" { + close(stop) + <-done + t.Fatalf("iteration %d: env API key leaked into the file", i) + } + } + close(stop) + <-done + assert.NotEqual(t, "env-secret", readJSON(t, path)["api_key"]) +} + +// Round-9 review finding: a plain save reads the file as its base and writes +// a whole replacement. Without serialisation an API edit that lands between +// that read and that write is reverted. In-process writers (the runtime, +// telemetry, serve's own saves) are serialised through one mutex, so the +// later save always reads the earlier save's file. +func TestSaveConfig_ReadBaseAndWriteAreSerialisedAgainstOtherSaves(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + t.Cleanup(func() { saveConfigTestHook = nil }) + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + live, err := DecodeConfigFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") + + var once bool + editDone := make(chan error, 1) + saveConfigTestHook = func() { + if once { + return + } + once = true + // The stale save has read its base ("" for api_key). Now an API edit + // rotates the key concurrently; it must not be able to land between + // this read and the stale save's write. + go func() { + base := *live + next := base + next.APIKey = "rotated-key" + editDone <- SaveConfigWithEdits(&next, &base, path) + }() + time.Sleep(100 * time.Millisecond) + } + + require.NoError(t, SaveConfig(live, path)) // the stale, telemetry-style save + require.NoError(t, <-editDone) + + assert.Equal(t, "rotated-key", readJSON(t, path)["api_key"], "the API edit must not be reverted by the stale save") +} diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index f588d39e5..952524432 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -131,8 +131,19 @@ type selfWriteEntry struct { // does; a marshal failure just skips recording (the write itself would have // failed the same way). Recording an already-present payload refreshes its // timestamp; when full, the oldest entry is evicted. -func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { - data, err := json.MarshalIndent(cfg, "", " ") +// +// path is the file the save targets: the payload is the PERSISTABLE form of +// cfg (config.PersistableConfig), which is what SaveConfig actually writes when +// a serve flag or MCPPROXY_* env override is in force. +func (r *Runtime) noteConfigSelfWrite(cfg *config.Config, path string) { + r.noteConfigSelfWriteWithEdits(cfg, nil, path) +} + +// noteConfigSelfWriteWithEdits is noteConfigSelfWrite for a save that carries +// an API edit (config.SaveConfigWithEdits): mergeBase is the config the edit +// was merged onto, so the marker matches the bytes that save writes. +func (r *Runtime) noteConfigSelfWriteWithEdits(cfg, mergeBase *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfigWithEdits(cfg, mergeBase, path), "", " ") if err != nil { return } @@ -158,8 +169,14 @@ func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { // those bytes never reached disk, so a later byte-identical write of them is // a genuine external edit the watcher must reload. Only the failed payload is // removed — markers pre-armed by other (successful) saves stay live. -func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config) { - data, err := json.MarshalIndent(cfg, "", " ") +func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config, path string) { + r.forgetConfigSelfWriteWithEdits(cfg, nil, path) +} + +// forgetConfigSelfWriteWithEdits is forgetConfigSelfWrite's counterpart to +// noteConfigSelfWriteWithEdits. +func (r *Runtime) forgetConfigSelfWriteWithEdits(cfg, mergeBase *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfigWithEdits(cfg, mergeBase, path), "", " ") if err != nil { return } @@ -235,8 +252,12 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { // event came from our own save. If a future save path diverges, this // degrades to one redundant (idempotent) reload — never a loop, since // ReloadConfiguration never writes the file. + // Compared in its PERSISTABLE form: with a serve flag or MCPPROXY_* env + // override in force the file legitimately differs from memory in exactly + // those fields (config.PersistableConfig), and reading that difference as + // an external edit would reload the file over the override on every save. trimmedDisk := bytes.TrimSpace(diskBytes) - if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { + if current, merr := json.MarshalIndent(config.PersistableConfig(r.ConfigSnapshot().Config, absPath), "", " "); merr == nil { if bytes.Equal(bytes.TrimSpace(current), trimmedDisk) { // The file now matches memory. If that content matches none of // the recorded self-writes, the file has moved past our saves diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 7bfbe8730..4a8909d5d 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -464,7 +464,7 @@ func TestConfigWatcher_BackToBackSelfWritesBothSuppressed(t *testing.T) { // this window). With a single slot this evicts A's record. cfgB := editedConfig(initialCfg, 72222) cfgB.Listen = "127.0.0.1:2" - rt.noteConfigSelfWrite(cfgB) + rt.noteConfigSelfWrite(cfgB, cfgPath) // A's debounce fires while disk still holds A. It must stay suppressed. time.Sleep(1200 * time.Millisecond) diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 0a54db641..c86f17b0e 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1,7 +1,9 @@ package runtime import ( + "bytes" "context" + "encoding/json" "fmt" "strings" "time" @@ -1476,9 +1478,9 @@ func (r *Runtime) SaveConfiguration() error { // the configSvc snapshot (the running config). Marked as our own // write first, or the watcher reads the pending value back as an // external edit and hot-applies what we deliberately deferred. - r.noteConfigSelfWrite(diskCopy) + r.noteConfigSelfWrite(diskCopy, snapshot.Path) if err := config.SaveConfig(diskCopy, snapshot.Path); err != nil { - r.forgetConfigSelfWrite(diskCopy) + r.forgetConfigSelfWrite(diskCopy, snapshot.Path) r.logger.Error("Failed to save config to file (pending-aware path)", zap.Error(err)) return err } @@ -1492,7 +1494,7 @@ func (r *Runtime) SaveConfiguration() error { // Fallback to legacy save (no configSvc store to keep in sync) if diskCopy != nil { configCopy = diskCopy - r.noteConfigSelfWrite(diskCopy) + r.noteConfigSelfWrite(diskCopy, snapshot.Path) } if err := config.SaveConfig(configCopy, snapshot.Path); err != nil { r.logger.Error("Failed to save config to file (legacy path)", zap.Error(err)) @@ -1583,6 +1585,10 @@ func (r *Runtime) ReloadConfiguration() error { if loadErr != nil { return fmt.Errorf("failed to reload config: %w", loadErr) } + r.mu.RLock() + live := r.cfg + r.mu.RUnlock() + config.ReapplyFlagOverrides(newConfig, live) // Already holding configCommitMu; use the locked helper so we don't // re-acquire the non-reentrant mutex (would deadlock). r.updateConfigLocked(newConfig, cfgPath) @@ -1600,6 +1606,15 @@ func (r *Runtime) ReloadConfiguration() error { config.LogLoadDiagnostics(newSnapshot.Config, r.logger) } + // fileCfg is the file as reloaded — the DESIRED config. running is what + // this process adopts from it: restart-gated fields pinned to the live + // values and the serve flags re-applied. They coincide unless something is + // pending or a flag is in force; every per-component side effect below + // follows running (parity with ApplyConfig, which applies hotCfg), while + // the restart-required warning diffs the file. + fileCfg := newSnapshot.Config + running := newSnapshot.Config + // Sync the legacy r.cfg/r.cfgPath fields too: Runtime.GetConfig() still // backs GET/PATCH /api/v1/config and other httpapi handlers. Without this, // a disk reload only lands in the configsvc snapshot — the API keeps @@ -1616,27 +1631,44 @@ func (r *Runtime) ReloadConfiguration() error { // a surface nobody was being served, and pinRestartGated on the apply // path would pin to a value that was never live. r.mu.RLock() - pinned := pinRestartGated(r.cfg, newSnapshot.Config) + live := r.cfg + pinned := pinRestartGated(live, newSnapshot.Config) r.mu.RUnlock() + // The loader re-applied the MCPPROXY_* env overrides but knows nothing + // about the serve flags; the RUNNING config is the file plus both, so + // a hand edit of an unrelated key must not switch `--read-only` off. + // Applied to the pinned copy only (nested blocks copy-on-write): the + // desired config below stays the file, so a pending file edit of a + // restart-gated flag field (listen) is still reported as pending. + config.ReapplyFlagOverrides(pinned, live) + // Republish so the configsvc snapshot and r.cfg cannot disagree: // ReloadFromFile has already published the RAW file, which live // subscribers would read as the running configuration. Skipped when - // nothing is pending — the common case, where pinned is equivalent to - // what ReloadFromFile just published. - if DetectConfigChanges(newSnapshot.Config, pinned).RequiresRestart { + // nothing is pending and no flag differs — the common case, where + // pinned is equivalent to what ReloadFromFile just published. + if DetectConfigChanges(fileCfg, pinned).RequiresRestart || !configsEquivalent(fileCfg, pinned) { if uerr := r.configSvc.Update(pinned, configsvc.UpdateTypeModify, "reload_pin_restart_gated"); uerr != nil { r.logger.Error("Failed to republish the pinned configuration after reload", zap.Error(uerr)) + } else { + newSnapshot = r.configSvc.Current() } } + running = pinned r.mu.Lock() r.cfg = pinned // The file IS the desired configuration, so a disk reload resets it — // including over an API change that was still waiting for a restart: // whoever edited the file wins, and nothing may keep merging onto a - // base the file no longer agrees with. - r.desiredCfg = newSnapshot.Config + // base the file no longer agrees with. The hot serve flags ride along + // exactly as they do in the startup desired config (the effective + // one): every PUT/PATCH round-trips this document, and a base that + // had lost --read-only would hand the file's value back as an "edit". + // Restart-gated fields stay the file's, so a pending edit of listen + // is still reported as pending. + r.desiredCfg = pinRestartGated(fileCfg, pinned) if newSnapshot.Path != "" { r.cfgPath = newSnapshot.Path } @@ -1650,8 +1682,8 @@ func (r *Runtime) ReloadConfiguration() error { // ConfigApplyResult; the disk path had no channel at all, so at least make // it loud in the log. Log-only on purpose: auto-restarting on a file save // would be far more surprising than a stale deadline. - if oldSnapshot != nil && oldSnapshot.Config != nil && newSnapshot != nil && newSnapshot.Config != nil { - if result := DetectConfigChanges(oldSnapshot.Config, newSnapshot.Config); result.RequiresRestart { + if oldSnapshot != nil && oldSnapshot.Config != nil && fileCfg != nil { + if result := DetectConfigChanges(oldSnapshot.Config, fileCfg); result.RequiresRestart { r.logger.Warn("Config file change includes restart-required fields; the running server keeps the old values until restart", zap.Strings("changed_fields", result.ChangedFields), zap.String("reason", result.RestartReason)) @@ -1664,7 +1696,7 @@ func (r *Runtime) ReloadConfiguration() error { // health_check_interval from this, so external edits must reach it too — // not only API applies. if r.upstreamManager != nil { - r.upstreamManager.SetGlobalConfig(newSnapshot.Config) + r.upstreamManager.SetGlobalConfig(running) } // Parity with ApplyConfig's live per-component side effects (PR #857 @@ -1673,7 +1705,7 @@ func (r *Runtime) ReloadConfiguration() error { // external edit lands in the snapshot/API while the running components // keep their stale values. r.mu.Lock() - r.applyComponentConfigLocked(oldSnapshot.Config, newSnapshot.Config) + r.applyComponentConfigLocked(oldSnapshot.Config, running) r.mu.Unlock() if err := r.LoadConfiguredServers(nil); err != nil { @@ -1687,12 +1719,12 @@ func (r *Runtime) ReloadConfiguration() error { // fsnotify config file watcher (config_watcher.go), which funnels external // file edits into this method. nil-safe + fire-and-forget. if r.telemetryService != nil { - r.telemetryService.NotifyConfigChanged(newSnapshot.Config) + r.telemetryService.NotifyConfigChanged(running) } // Spec 079 FR-012: re-gate the update checker on the disk-reload path too // (ApplyConfig covers the API path). SetConfig no-ops when unchanged. - r.applyUpdateCheckConfig(newSnapshot.Config) + r.applyUpdateCheckConfig(running) go r.postConfigReload() @@ -2258,3 +2290,11 @@ func (r *Runtime) supervisorEventForwarder() { } } } + +// configsEquivalent reports whether two configs marshal to the same JSON — +// the same comparison the config watcher uses to recognise its own saves. +func configsEquivalent(a, b *config.Config) bool { + ja, errA := json.Marshal(a) + jb, errB := json.Marshal(b) + return errA == nil && errB == nil && bytes.Equal(ja, jb) +} diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go new file mode 100644 index 000000000..af81dc337 --- /dev/null +++ b/internal/runtime/process_overrides_persist_test.go @@ -0,0 +1,420 @@ +package runtime + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// The runtime's persist paths (SaveConfiguration on every API-driven server +// change, ApplyConfig on PUT /api/v1/config) write the live config, which +// carries the `serve` CLI flag and MCPPROXY_* env overrides. Those are +// process-only and must not reach the file — while a genuine API edit of the +// same field (the Settings page changing listen) still must. +func newOverriddenRuntime(t *testing.T) (*Runtime, string) { + t.Helper() + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + initial := config.DefaultConfig() + initial.Listen = "127.0.0.1:8080" + initial.DataDir = tmp + initial.ToolResponseMode = "full" + require.NoError(t, config.SaveConfig(initial, cfgPath)) + + cfg, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") + config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") + config.OverrideForProcess(cfg, config.FieldReadOnlyMode, config.OverrideSourceFlag, true) + config.OverrideForProcess(cfg, config.FieldAPIKey, config.OverrideSourceEnv, "env-secret") + + rt, err := New(cfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + return rt, cfgPath +} + +func readConfigJSON(t *testing.T, path string) map[string]any { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + return m +} + +func assertNoOverridesOnDisk(t *testing.T, m map[string]any) { + t.Helper() + assert.Equal(t, "127.0.0.1:8080", m["listen"], "--listen must not be persisted") + assert.Equal(t, "full", m["tool_response_mode"], "--tool-response-mode must not be persisted") + assert.NotEqual(t, true, m["read_only_mode"], "--read-only must not be persisted") + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not be persisted") +} + +func TestSaveConfiguration_DoesNotPersistProcessOverrides(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + require.NoError(t, rt.SaveConfiguration()) + + assertNoOverridesOnDisk(t, readConfigJSON(t, cfgPath)) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "the effective config keeps the override") + assert.Equal(t, "compact", live.ToolResponseMode) + assert.True(t, live.ReadOnlyMode) + assert.Equal(t, "env-secret", live.APIKey) +} + +// GET /config returns the desired config — which at startup IS the effective +// one, overrides included — and a PUT round-trips it. A field that comes back +// unchanged from that round trip is not an edit. +func TestApplyConfig_RoundTrippedOverrideIsNotPersisted(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, ":0", desired.Listen, "GET /config serves the effective value") + desired.ToolsLimit = 42 // the actual edit + + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(42), m["tools_limit"], "the real edit is persisted") +} + +// The Settings page changing listen while `--listen` is in force is a real +// edit: it must reach the file (restart-gated, so it stays pending in memory). +func TestApplyConfig_EditOfAnOverriddenFieldIsPersisted(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.Listen = "127.0.0.1:9090" + desired.ToolResponseMode = "full" // hot: turning the flag's choice back off + + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + m := readConfigJSON(t, cfgPath) + assert.Equal(t, "127.0.0.1:9090", m["listen"]) + assert.Equal(t, "full", m["tool_response_mode"]) + assert.NotEqual(t, true, m["read_only_mode"], "the untouched override is still not persisted") + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "listen is restart-gated: memory keeps the bound value") + assert.Equal(t, "full", live.ToolResponseMode, "the hot edit is live") + + // A later unrelated save must not revert the persisted edit. + require.NoError(t, rt.SaveConfiguration()) + m = readConfigJSON(t, cfgPath) + assert.Equal(t, "127.0.0.1:9090", m["listen"]) +} + +// The config watcher compares the file with memory to recognise the daemon's +// own saves. With overrides in force the file legitimately differs from memory +// in exactly those fields; that difference must not read as an external edit, +// or every API save would trigger a reload that drops the hot overrides. +func TestConfigWatcher_OwnSaveWithOverridesIsNotAnExternalEdit(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + require.NoError(t, rt.SaveConfiguration()) + rt.clearSelfWrites() // the marker path is tested elsewhere; force the snapshot comparison + + rt.reloadFromDiskIfChanged(cfgPath) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, "compact", live.ToolResponseMode, "the daemon's own save must not reload the file over the flag") + assert.True(t, live.ReadOnlyMode) +} + +// A genuine external edit reloads the file; the loader re-applies env +// overrides, and the runtime must re-apply the serve flags the same way, or a +// hand edit of an unrelated key silently switches --read-only off. +func TestReloadConfiguration_KeepsFlagOverridesEffective(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 // the external edit + require.NoError(t, config.SaveConfig(edited, cfgPath)) + + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, 77, live.ToolsLimit, "the external edit is adopted") + assert.True(t, live.ReadOnlyMode, "--read-only survives a reload") + assert.Equal(t, "compact", live.ToolResponseMode, "--tool-response-mode survives a reload") + assert.Equal(t, ":0", live.Listen) + + // …and the next save still does not persist them. + require.NoError(t, rt.SaveConfiguration()) + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(77), m["tools_limit"]) +} + +// A hot API edit of an overridden field supersedes the flag for this process: +// a later external edit of an unrelated key must not resurrect it on reload. +func TestReloadConfiguration_DoesNotResurrectAFlagTheAPISuperseded(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "full" // the API turns the flag's choice off + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + require.Equal(t, "full", edited.ToolResponseMode) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, "full", live.ToolResponseMode, "the API edit survives the reload") + assert.True(t, live.ReadOnlyMode, "the untouched flag survives the reload") +} + +// Toggling an overridden hot field away from the flag and back again via the +// API: the second edit is a real edit and must persist, not be swapped for +// the file's value. +func TestApplyConfig_TogglingAnOverriddenFieldBackPersists(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "full" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + assert.Equal(t, "full", readConfigJSON(t, cfgPath)["tool_response_mode"]) + + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "compact" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + assert.Equal(t, "compact", readConfigJSON(t, cfgPath)["tool_response_mode"]) +} + +// A reload must keep the DESIRED config equal to the file: a restart-gated +// flag (listen) is re-applied to the running config only, so a pending file +// edit of that field is still reported as pending and not clobbered. +func TestReloadConfiguration_RestartGatedFlagDoesNotHideAPendingFileEdit(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.Listen = "127.0.0.1:9090" + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "the bound listener keeps the flag value") + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, "127.0.0.1:9090", desired.Listen, "the desired config is the file") + + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, "127.0.0.1:9090", readConfigJSON(t, cfgPath)["listen"], "a later save keeps the pending edit") +} + +// The reload's per-component side effects (upstream manager global config, +// truncator, logging, telemetry) must follow the RUNNING config — the file +// plus the flags — exactly as ApplyConfig applies hotCfg, not the raw file. +func TestReloadConfiguration_ComponentsFollowTheRunningConfig(t *testing.T) { + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + initial := config.DefaultConfig() + initial.DataDir = tmp + initial.ToolResponseLimit = 20000 + initial.ToolResponseMode = "full" + require.NoError(t, config.SaveConfig(initial, cfgPath)) + + cfg, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, 500) + config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") + + rt, err := New(cfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + require.Equal(t, 500, rt.Truncator().Limit()) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + assert.Equal(t, 500, rt.Truncator().Limit(), "the truncator must not be rebuilt from the file's limit") + if um := rt.upstreamManager; um != nil { + assert.Equal(t, "compact", um.GlobalConfig().ToolResponseMode, "the upstream manager must see the running config") + } + snap := rt.ConfigSnapshot() + assert.Equal(t, "compact", snap.Config.ToolResponseMode, "the published snapshot is the running config") +} + +// An API edit of a restart-gated overridden field (listen under --listen) +// ends the override for this process even though the listener stays bound: +// a second edit back to the flag's value is then persisted as asked, so disk, +// the desired config and the API result agree. +func TestApplyConfig_EditingListenUnderAFlagEndsTheOverride(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.Listen = "127.0.0.1:9090" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9090", readConfigJSON(t, cfgPath)["listen"]) + + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9090", desired.Listen) + desired.Listen = ":0" // cancel: back to what this process is bound to + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "the explicit edit is persisted as asked") + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, ":0", desired.Listen) + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "a later unrelated save keeps it") +} + +// After a disk reload the desired config must still carry the hot flags, as +// it does at startup, so a GET→PUT round trip of an unrelated edit neither +// retires --read-only nor hot-applies the file's value over it. +func TestApplyConfig_UnrelatedEditAfterReloadKeepsHotFlags(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + assert.True(t, desired.ReadOnlyMode, "GET /config shows the effective hot flag after a reload") + assert.Equal(t, "compact", desired.ToolResponseMode) + desired.ToolsLimit = 99 // the unrelated edit + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.True(t, live.ReadOnlyMode, "--read-only must survive an unrelated API edit") + assert.Equal(t, "compact", live.ToolResponseMode) + assert.Equal(t, 99, live.ToolsLimit) + + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(99), m["tools_limit"]) +} + +// A round trip of the file's listen value after a reload is not an edit of +// listen: the --listen override survives and is still not persisted. +func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:8080", desired.Listen, "restart-gated: the desired config is the file") + desired.ToolsLimit = 99 + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, "127.0.0.1:8080", readConfigJSON(t, cfgPath)["listen"]) +} + +// After a reload the desired listen is the file's; an API edit that sets it +// to the flag's own address is a distinguishable, legitimate edit and must +// reach the file. +func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:8080", desired.Listen) + desired.Listen = ":0" // make the flag's address permanent + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "the edit must reach the file") + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, ":0", desired.Listen) +} + +// A failed save must not leave an override retired: the env API key would +// otherwise leak into the file on the next unrelated save once disk recovers. +func TestApplyConfig_FailedSaveKeepsTheOverrideProtected(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + tmp := filepath.Dir(cfgPath) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.APIKey = "rotated-key" + + // Induce a real, cross-platform write failure by pointing this ONE save + // at a path whose parent is a plain file instead of a directory, rather + // than chmod'ing an existing directory read-only: Windows does not enforce + // Unix-style directory permission bits via os.Chmod the way POSIX does, so + // the GitHub Actions windows-latest runner can still write into a + // "read-only" directory and the save silently succeeds. writeConfigFile's + // os.MkdirAll(dir, 0700) pre-flight does `Stat(dir); if err == nil && + // !IsDir() { return ENOTDIR }` — pure Go logic that runs identically on + // every OS, so a file sitting where the save's directory should be fails + // the same way on Linux, macOS and Windows. This targets only this one + // ApplyConfig call: cfgPath (the runtime's own saved-to path, still a real + // writable directory) is untouched, so the later SaveConfiguration below + // exercises "disk recovered" for real. + blockedDir := filepath.Join(tmp, "blocked-save-dir") + require.NoError(t, os.WriteFile(blockedDir, []byte("not a directory"), 0o644)) + brokenPath := filepath.Join(blockedDir, "mcp_config.json") + + _, err = rt.ApplyConfig(desired, brokenPath) + require.Error(t, err, "the save must fail") + + require.NoError(t, rt.SaveConfiguration()) // disk recovered; an unrelated save + m := readConfigJSON(t, cfgPath) + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not leak") + assert.NotEqual(t, "rotated-key", m["api_key"], "the failed edit must not appear either") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 369d9df1a..079dc2878 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1737,16 +1737,23 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // (config_watcher.go). If the save fails, its entry is removed again on // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. - r.noteConfigSelfWrite(newCfg) - - saveErr := config.SaveConfig(newCfg, savePath) + // + // The overridden fields this apply MOVED relative to its merge base are + // the caller's edits and are persisted as they are — whatever they moved + // to, including a serve flag's own value; a round trip of a value the + // base already held keeps restoring the file value. The override records + // stay: a concurrent save of the still-live config must keep restoring + // the file value (config.PersistableConfigWithEdits). + r.noteConfigSelfWriteWithEdits(newCfg, baseCfg, savePath) + + saveErr := config.SaveConfigWithEdits(newCfg, baseCfg, savePath) if saveErr != nil { // Drop the pre-armed self-write entry: the save never landed, so no // future fs event for these bytes can be our own echo. Keeping it // would suppress a genuine external write of byte-identical JSON. // Only this payload is forgotten — markers from other still-pending // successful saves stay live. - r.forgetConfigSelfWrite(newCfg) + r.forgetConfigSelfWriteWithEdits(newCfg, baseCfg, savePath) r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) diff --git a/internal/telemetry/process_overrides_persist_test.go b/internal/telemetry/process_overrides_persist_test.go new file mode 100644 index 000000000..f19572cae --- /dev/null +++ b/internal/telemetry/process_overrides_persist_test.go @@ -0,0 +1,56 @@ +package telemetry + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// The first-run anonymous_id generation persists the WHOLE config the daemon +// handed to telemetry.New — the effective one, carrying every `serve` flag and +// MCPPROXY_* env override. Those apply to this process only and must never +// reach the file, or the next unflagged start inherits them. +func TestEnsureAnonymousID_DoesNotPersistProcessOverrides(t *testing.T) { + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + raw := `{"listen": "127.0.0.1:8080", "data_dir": ` + jsonQuote(tmp) + `, "read_only_mode": false, "mcpServers": []}` + require.NoError(t, os.WriteFile(cfgPath, []byte(raw), 0o600)) + + cfg, err := config.DecodeConfigFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") + config.OverrideForProcess(cfg, config.FieldReadOnlyMode, config.OverrideSourceFlag, true) + config.OverrideForProcess(cfg, config.FieldAPIKey, config.OverrideSourceEnv, "env-secret") + + svc := New(cfg, cfgPath, "v1.0.0", "personal", zap.NewNop()) + svc.ensureAnonymousID() + require.NotEmpty(t, cfg.GetAnonymousID()) + + data, err := os.ReadFile(cfgPath) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + + tel, _ := m["telemetry"].(map[string]any) + require.NotNil(t, tel, "the generated id must be persisted") + assert.Equal(t, cfg.GetAnonymousID(), tel["anonymous_id"]) + + assert.Equal(t, "127.0.0.1:8080", m["listen"], "--listen must not be persisted") + assert.NotEqual(t, true, m["read_only_mode"], "--read-only must not be persisted") + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not be persisted") +} + +func jsonQuote(s string) string { + b, _ := json.Marshal(s) + return string(b) +} diff --git a/internal/truncate/truncator.go b/internal/truncate/truncator.go index de93d61d6..b7487c2b8 100644 --- a/internal/truncate/truncator.go +++ b/internal/truncate/truncator.go @@ -28,6 +28,11 @@ func NewTruncator(limit int) *Truncator { return &Truncator{limit: limit} } +// Limit returns the character limit this truncator applies. +func (t *Truncator) Limit() int { + return t.limit +} + // Truncate analyzes and truncates a tool response if it exceeds the limit. // The record array the resulting cache handle pages is inferred from the // payload; callers that already know which array their truncation banner