diff --git a/internal/contracts/types.go b/internal/contracts/types.go index ad38b568d..047c405c2 100644 --- a/internal/contracts/types.go +++ b/internal/contracts/types.go @@ -2,11 +2,16 @@ package contracts import ( + "errors" "time" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) +// ErrServerNotFound is returned (wrapped) by controllers when a named upstream +// server does not exist, so the REST layer can answer 404 via errors.Is. +var ErrServerNotFound = errors.New("server not found") + // APIResponse is the standard wrapper for all API responses type APIResponse struct { Success bool `json:"success"` diff --git a/internal/httpapi/config_funnel.go b/internal/httpapi/config_funnel.go index 80344715e..9a5b9a821 100644 --- a/internal/httpapi/config_funnel.go +++ b/internal/httpapi/config_funnel.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "strings" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" internalRuntime "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" @@ -45,6 +46,32 @@ func refuseLockedTelemetryChange(stored, edited *config.Config) error { } } +// refuseAmbiguousLockedTelemetryKeys refuses, while the env telemetry lock is +// active, a raw document that spells the telemetry section more than once +// under case variants ("telemetry" and "Telemetry"). encoding/json decodes keys +// case-insensitively, so which spelling wins would depend on map iteration / +// marshal order; a locked setting must not hinge on that. +func refuseAmbiguousLockedTelemetryKeys(document map[string]interface{}) error { + disabled, reason := telemetry.IsDisabledByEnv() + if !disabled { + return nil + } + n := 0 + for k := range document { + if strings.EqualFold(k, "telemetry") { + n++ + } + } + if n < 2 { + return nil + } + return &configMutationRefusal{ + status: http.StatusUnprocessableEntity, + msg: "telemetry is locked: the document repeats the telemetry section under different key spellings, which is ambiguous while telemetry is disabled by " + + string(reason) + " in the environment.", + } +} + // configFunnelController is the optional controller capability behind every // config-writing REST path (Spec 108-f F2): the read of the desired config, the // caller's mutation, the FR-008a guard and the write all run under one lock, diff --git a/internal/httpapi/config_funnel_test.go b/internal/httpapi/config_funnel_test.go index 1a70e2629..779d32ca0 100644 --- a/internal/httpapi/config_funnel_test.go +++ b/internal/httpapi/config_funnel_test.go @@ -134,3 +134,25 @@ func TestPatchConfig_FunnelValidationErrorIs400WithField(t *testing.T) { assert.Equal(t, "anonymous_profile", body["field"]) assert.Equal(t, `unknown profile "ghost"`, body["error"]) } + +// #1466: with the env telemetry lock active, a case-variant duplicate of the +// locked key cannot smuggle a change past the lock — the comparison is on the +// typed value the document resolves to. +func TestApplyConfig_TelemetryLockRefusesCaseVariantDuplicate(t *testing.T) { + t.Setenv("DO_NOT_TRACK", "1") + off := false + ctrl := &funnelController{desired: &config.Config{Listen: "127.0.0.1:8080", Telemetry: &config.TelemetryConfig{Enabled: &off}}} + srv := NewServer(ctrl, zap.NewNop().Sugar(), nil) + + docs := []string{ + `{"telemetry":{"enabled":false},"Telemetry":{"ENABLED":true}}`, + `{"Telemetry":{"ENABLED":true},"telemetry":{"anonymous_id":"x"}}`, + `{"telemetry":{"anonymous_id":"x"},"Telemetry":{"ENABLED":true}}`, + } + for _, doc := range docs { + w := funnelRequest(t, srv, http.MethodPost, "/api/v1/config/apply", doc) + assert.Equal(t, http.StatusUnprocessableEntity, w.Code, "doc %s: %s", doc, w.Body.String()) + } + require.NotNil(t, ctrl.desired.Telemetry.Enabled) + assert.False(t, *ctrl.desired.Telemetry.Enabled, "the locked value was not changed") +} diff --git a/internal/httpapi/scope_filters.go b/internal/httpapi/scope_filters.go index bc78746d5..d74073e40 100644 --- a/internal/httpapi/scope_filters.go +++ b/internal/httpapi/scope_filters.go @@ -72,7 +72,7 @@ func scopeFilterListContains(list []string, name string) bool { func rejectUnsupportedScopeFilters(w http.ResponseWriter, r *http.Request, honoured ...string) bool { q := r.URL.Query() for _, name := range []string{"profile", "client", "token", "agent"} { - if q.Get(name) == "" { + if !q.Has(name) { continue } diff --git a/internal/httpapi/scope_filters_gate_test.go b/internal/httpapi/scope_filters_gate_test.go index e1c3e7e73..872598021 100644 --- a/internal/httpapi/scope_filters_gate_test.go +++ b/internal/httpapi/scope_filters_gate_test.go @@ -150,3 +150,18 @@ func TestStatus_FeaturesScopeFiltersEqualsList(t *testing.T) { require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) assert.Equal(t, scopeFilterSupportedFilters, body.Data.Features.ScopeFilters) } + +// #1394: a present-but-empty scope parameter (?token=) on a handler that does +// not honour it is rejected like a valued one, not silently served unfiltered. +func TestRejectUnsupportedScopeFilters_EmptyValueStillRejected(t *testing.T) { + for _, name := range []string{"profile", "client", "token", "agent"} { + req := httptest.NewRequest(http.MethodGet, "/api/v1/activity/summary?"+name+"=", nil) + rec := httptest.NewRecorder() + assert.Falsef(t, rejectUnsupportedScopeFilters(rec, req), "?%s= must be rejected", name) + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "unsupported_scope_filter") + } + // Honoured parameters stay accepted when empty. + req := httptest.NewRequest(http.MethodGet, "/api/v1/activity?token=", nil) + assert.True(t, rejectUnsupportedScopeFilters(httptest.NewRecorder(), req, "token")) +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index ca8224f16..f06552233 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -4159,6 +4159,10 @@ func (s *Server) handleGetServerLogs(w http.ResponseWriter, r *http.Request) { logEntries, err := s.controller.GetServerLogs(serverID, tail) if err != nil { + if errors.Is(err, contracts.ErrServerNotFound) { + s.writeError(w, r, http.StatusNotFound, fmt.Sprintf("Server not found: %s", serverID)) + return + } s.logger.Errorw("Failed to get server logs", "server", serverID, "error", err) s.writeError(w, r, http.StatusInternalServerError, fmt.Sprintf("Failed to get logs: %v", err)) return @@ -5709,6 +5713,9 @@ func (s *Server) handleApplyConfig(w http.ResponseWriter, r *http.Request) { // door's masks over the operator's credentials. The unmask runs INSIDE the // config funnel, against the desired config it read under the lock. result, ok := s.mutateConfig(w, r, "Failed to apply configuration", func(stored *config.Config) error { + if err := refuseAmbiguousLockedTelemetryKeys(document); err != nil { + return err + } resolved, err := oauth.UnmaskLiveConfigDocument(document, stored) if err != nil { s.logger.Warnw("Refused a configuration write carrying an unbindable mask", "error", err) @@ -5846,6 +5853,9 @@ func (s *Server) handlePatchConfig(w http.ResponseWriter, r *http.Request) { // Direct and then changed any other setting lost the routing switch with no // warning, on disk, with a success toast. result, ok := s.mutateConfig(w, r, "Failed to apply configuration patch", func(cfg *config.Config) error { + if err := refuseAmbiguousLockedTelemetryKeys(patchMap); err != nil { + return err + } merged, refusal := s.mergeConfigPatch(cfg, patchMap) if refusal != nil { return refusal diff --git a/internal/httpapi/server_logs_notfound_test.go b/internal/httpapi/server_logs_notfound_test.go new file mode 100644 index 000000000..b20b5246e --- /dev/null +++ b/internal/httpapi/server_logs_notfound_test.go @@ -0,0 +1,46 @@ +package httpapi + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" +) + +type logsErrController struct { + *MockServerController + err error +} + +func (c *logsErrController) GetServerLogs(_ string, _ int) ([]contracts.LogEntry, error) { + return nil, c.err +} + +// #1466: an unknown server is a 404 (as the swagger comment promises), any +// other failure stays a 500. +func TestGetServerLogs_NotFoundIs404(t *testing.T) { + cases := []struct { + name string + err error + want int + }{ + {"not found", fmt.Errorf("%w: ghost", contracts.ErrServerNotFound), http.StatusNotFound}, + {"other error", fmt.Errorf("failed to read log"), http.StatusInternalServerError}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctrl := &logsErrController{MockServerController: &MockServerController{}, err: tc.err} + srv := NewServer(ctrl, zap.NewNop().Sugar(), nil) + req := httptest.NewRequest("GET", "/api/v1/servers/ghost/logs", http.NoBody) + req.Header.Set("X-API-Key", mockControllerAPIKey) + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + assert.Equal(t, tc.want, w.Code, w.Body.String()) + }) + } +} diff --git a/internal/runtime/config_hotreload.go b/internal/runtime/config_hotreload.go index a4c2d208e..a2ef8bcbe 100644 --- a/internal/runtime/config_hotreload.go +++ b/internal/runtime/config_hotreload.go @@ -6,6 +6,7 @@ import ( "fmt" "reflect" "slices" + "strings" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) @@ -519,6 +520,36 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult { } } + // Remaining top-level fields (#1435). Each of these used to be edited, + // saved and adopted into the live config but computed an empty + // ChangedFields, so the apply answered "No configuration changes detected" + // (applied_immediately:false) for a change that had in fact been made. The + // list is checked by TestDetectConfigChanges_EveryTopLevelFieldIsDiffed: + // adding a config.Config field there forces a clause here (or an entry in + // that test's documented exclusion list). jsonEqual, not DeepEqual, for the + // PATCH round-trip reason documented on it. + for _, name := range undiffedHotConfigFields { + if !jsonEqual(configFieldValue(oldCfg, name), configFieldValue(newCfg, name)) { + result.ChangedFields = append(result.ChangedFields, configFieldJSONName(name)) + } + } + // These are bound once at startup (the unix-socket listener, the tray + // endpoint, the search index debug flag, the tokenizer, and the MCP + // prompts capability / server instructions read at server construction): an edit is saved + // and takes effect on the next start. They are pinned to the live value by + // pinRestartGated, like the other restart-gated fields. + for _, name := range restartGatedConfigFields { + if !jsonEqual(configFieldValue(oldCfg, name), configFieldValue(newCfg, name)) { + field := configFieldJSONName(name) + result.ChangedFields = append(result.ChangedFields, field) + result.RequiresRestart = true + result.AppliedImmediately = false + if result.RestartReason == "" { + result.RestartReason = field + " is bound at startup - requires restart" + } + } + } + // If no changes detected if len(result.ChangedFields) == 0 { result.AppliedImmediately = false @@ -559,3 +590,35 @@ func (r *ConfigApplyResult) FormatChangedFields() string { // For 3+ fields, show "field1, field2, and N others" return fmt.Sprintf("%s, %s, and %d others", r.ChangedFields[0], r.ChangedFields[1], len(r.ChangedFields)-2) } + +// undiffedHotConfigFields are the top-level config.Config fields (Go names) +// with no dedicated clause in DetectConfigChanges: a change is saved and +// adopted into the live config, and reported by its JSON key. +var undiffedHotConfigFields = []string{ + "EnableTray", "TopK", "MaxResultSizeChars", "InitTimeout", "ForwardProxyEnv", + "RequireMCPAuth", "CheckServerRepo", "DockerRecovery", + "RegistriesLocked", "AllowPrivateRegistryFetch", "Features", + "ToolResponseSessionRiskWarning", "OAuthExpiryWarningHours", + "ActivityRetentionDays", "ActivityMaxRecords", "ActivityMaxSizeMB", + "ActivityMaxResponseSize", "ActivityCleanupIntervalMin", + "ToolCallMaxResponseSize", "ToolCallMaxRecordsPerServer", + "IntentDeclaration", "SensitiveDataDetection", "OutputValidation", + "OutputSanitisation", "Telemetry", + "QuarantineEnabled", "RevealSecretHeaders", +} + +// restartGatedConfigFields are bound once at startup; see pinRestartGated. +var restartGatedConfigFields = []string{"TrayEndpoint", "EnableSocket", "DebugSearch", "Tokenizer", "EnablePrompts", "Instructions"} + +func configFieldValue(cfg *config.Config, goName string) interface{} { + return reflect.ValueOf(cfg).Elem().FieldByName(goName).Interface() +} + +func configFieldJSONName(goName string) string { + f, _ := reflect.TypeOf(config.Config{}).FieldByName(goName) + name, _, _ := strings.Cut(f.Tag.Get("json"), ",") + if name == "" { + return goName + } + return name +} diff --git a/internal/runtime/config_hotreload_fields_test.go b/internal/runtime/config_hotreload_fields_test.go new file mode 100644 index 000000000..45dd01e58 --- /dev/null +++ b/internal/runtime/config_hotreload_fields_test.go @@ -0,0 +1,87 @@ +package runtime + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// #1435: an edit that DetectConfigChanges does not recognise computes an empty +// ChangedFields and is answered "No configuration changes detected" — a silent +// no-op the Settings UIs render as success. This guard mutates every top-level +// field of config.Config in isolation and asserts the diff notices it. +// +// configFieldsNotDiffed lists the fields that are intentionally NOT diffed, +// each with the reason. Adding a config field forces a choice: teach +// DetectConfigChanges about it, or document here why an edit is not an +// applicable change. +var configFieldsNotDiffed = map[string]string{ + "ServerEdition": "diffed by the dedicated server_edition clauses (restart projection, admin_emails, access); a zero-value block projects the same bytes as nil", + "TLS": "diffed (restart-gated) by the dedicated tls clause; the baseline here already carries a TLS block, so a zero-value edit is not a change", +} + +// mutateConfigField changes one top-level field of cfg to a value that differs +// from its zero value and reports whether it knew how. +func mutateConfigField(cfg *config.Config, f reflect.StructField, tryFalse bool) bool { + v := reflect.ValueOf(cfg).Elem().FieldByName(f.Name) + switch v.Kind() { + case reflect.String: + v.SetString("changed-value") + case reflect.Bool: + v.SetBool(!v.Bool()) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + v.SetInt(v.Int() + 7) + case reflect.Float32, reflect.Float64: + v.SetFloat(v.Float() + 7) + case reflect.Slice: + elem := reflect.New(v.Type().Elem()).Elem() + if elem.Kind() == reflect.String { + elem.SetString("changed-value") + } + v.Set(reflect.Append(v, elem)) + case reflect.Pointer: + p := reflect.New(v.Type().Elem()) + switch p.Elem().Kind() { + case reflect.Bool: + p.Elem().SetBool(!tryFalse) + case reflect.Int, reflect.Int64: + p.Elem().SetInt(7) + } + v.Set(p) + default: + return false + } + return true +} + +func TestDetectConfigChanges_EveryTopLevelFieldIsDiffed(t *testing.T) { + typ := reflect.TypeOf(config.Config{}) + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + if !f.IsExported() || f.Tag.Get("json") == "-" { + continue + } + t.Run(f.Name, func(t *testing.T) { + if reason, ok := configFieldsNotDiffed[f.Name]; ok { + assert.NotEmpty(t, reason) + t.Skipf("intentionally not diffed: %s", reason) + } + oldCfg := &config.Config{Listen: "127.0.0.1:8080", DataDir: "/d", TLS: &config.TLSConfig{}} + edited := &config.Config{Listen: "127.0.0.1:8080", DataDir: "/d", TLS: &config.TLSConfig{}} + if !mutateConfigField(edited, f, false) { + t.Fatalf("test cannot mutate field %s (kind %s): extend mutateConfigField", f.Name, f.Type.Kind()) + } + result := DetectConfigChanges(oldCfg, edited) + if len(result.ChangedFields) == 0 && f.Type.Kind() == reflect.Pointer && f.Type.Elem().Kind() == reflect.Bool { + // A *bool whose unset state means "true": the edit that matters is false. + edited = &config.Config{Listen: "127.0.0.1:8080", DataDir: "/d", TLS: &config.TLSConfig{}} + mutateConfigField(edited, f, true) + result = DetectConfigChanges(oldCfg, edited) + } + assert.NotEmpty(t, result.ChangedFields, "editing %s alone is reported as \"No configuration changes detected\"", f.Name) + }) + } +} diff --git a/internal/runtime/restart_gated.go b/internal/runtime/restart_gated.go index c218984a4..9a17f0ffd 100644 --- a/internal/runtime/restart_gated.go +++ b/internal/runtime/restart_gated.go @@ -54,6 +54,15 @@ func pinRestartGated(live, desired *config.Config) *config.Config { // pending a restart, leaving Runtime.Config() readers disagreeing with // the sink actually still writing). pinned.AuditLog = live.AuditLog + // Bound once at startup (#1435): the unix-socket listener, the tray + // endpoint, the search debug flag and the tokenizer. + pinned.TrayEndpoint = live.TrayEndpoint + pinned.EnableSocket = live.EnableSocket + pinned.DebugSearch = live.DebugSearch + pinned.Tokenizer = live.Tokenizer + // Read once at MCP server construction (mcp.go), never re-read. + pinned.EnablePrompts = live.EnablePrompts + pinned.Instructions = live.Instructions // server_edition's restart-pinned subset (enabled, oauth.*, public_url, // session_cookie_secure, session_ttl, bearer_token_ttl, // credential_encryption_key — Spec 107 FR-039 part 2) is bound at login diff --git a/internal/runtime/restart_gated_test.go b/internal/runtime/restart_gated_test.go index 13be66f95..3adc89497 100644 --- a/internal/runtime/restart_gated_test.go +++ b/internal/runtime/restart_gated_test.go @@ -48,6 +48,10 @@ func TestPinRestartGatedCoversEveryRestartGatedField(t *testing.T) { // the apply as pending a restart, leaving Runtime.Config() readers // disagreeing with the sink actually in effect. "audit_log": func(c *config.Config) { c.AuditLog = &config.AuditLogConfig{Path: "/tmp/two.jsonl"} }, + // Read once when each MCP server is constructed (mcp.go), never + // re-read: a PATCH must report requires_restart, not applied. + "enable_prompts": func(c *config.Config) { c.EnablePrompts = !c.EnablePrompts }, + "instructions": func(c *config.Config) { c.Instructions = "changed instructions" }, } for name, mutate := range mutations { diff --git a/internal/server/server.go b/internal/server/server.go index 184b9dfa5..aed53f67b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -3916,7 +3916,7 @@ func (s *Server) GetServerLogs(serverName string, tail int) ([]contracts.LogEntr // Check if server exists _, exists := s.runtime.UpstreamManager().GetClient(serverName) if !exists { - return nil, fmt.Errorf("server not found: %s", serverName) + return nil, fmt.Errorf("%w: %s", contracts.ErrServerNotFound, serverName) } // Read from server-specific log file