From 4dcc71ea65a8b45ba38ef0733d7003d60735a071 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 3 Oct 2026 20:36:24 +0300 Subject: [PATCH 1/2] fix(import): reject servers that fail config validation before persisting POST /api/v1/servers/import with a URL-only entry forced through the claude-desktop format mapped to a commandless stdio server, which was persisted (notably with skip_quarantine=true) and made the next core startup exit with code 4 (command is required for stdio protocol). configimport.Import, the single choke point behind REST import and the CLI upstream import (daemon and direct config-file mode), now runs the same per-server checks config.Load applies (new config.ValidateServerForLoad) and reports failing entries as Failed (invalid_server, with the reason) instead of offering them for persistence. Valid entries in the same payload import as before. --- internal/config/config.go | 19 ++++++ internal/configimport/import.go | 19 ++++++ .../import_invalid_server_test.go | 34 ++++++++++ internal/configimport/import_test.go | 10 ++- .../httpapi/import_invalid_server_test.go | 65 +++++++++++++++++++ 5 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 internal/configimport/import_invalid_server_test.go create mode 100644 internal/httpapi/import_invalid_server_test.go diff --git a/internal/config/config.go b/internal/config/config.go index beda82923..28420d2ea 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2335,6 +2335,25 @@ func (c *Config) ValidateDetailed() []ValidationError { return errors } +// ValidateServerForLoad runs the per-server checks config.Load applies at boot +// (the stdio command-required check, url-required for HTTP protocols, protocol +// and name validity, ...) against a single server and returns only the errors +// attributable to it. Write surfaces that persist a server built outside the +// config loader (e.g. imports) use it so they can never save an entry the next +// startup would refuse to load. +func ValidateServerForLoad(server *ServerConfig) []ValidationError { + cfg := DefaultConfig() + cfg.Servers = []*ServerConfig{server} + const prefix = "mcpServers[0]" + var out []ValidationError + for _, e := range cfg.validateDetailedCore() { + if strings.HasPrefix(e.Field, prefix) { + out = append(out, e) + } + } + return out +} + // oauthRedirectURIErrors reports every per-server `oauth.redirect_uri` that the // loopback callback listener cannot honor. // diff --git a/internal/configimport/import.go b/internal/configimport/import.go index b6394573d..994b92e35 100644 --- a/internal/configimport/import.go +++ b/internal/configimport/import.go @@ -2,7 +2,10 @@ package configimport import ( "fmt" + "strings" "time" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" ) // Import parses configuration content and imports servers. @@ -146,6 +149,22 @@ func Import(content []byte, opts *ImportOptions) (*ImportResult, error) { // Map to ServerConfig serverConfig, skipped, warnings := MapToServerConfig(parsed, opts.Now) + // Reject entries the next config.Load would refuse (e.g. a URL-only + // entry forced through a stdio-only format yields a commandless stdio + // server). Persisting one bricks the next startup with exit code 4. + if verrs := config.ValidateServerForLoad(serverConfig); len(verrs) > 0 { + msgs := make([]string, 0, len(verrs)) + for _, ve := range verrs { + msgs = append(msgs, ve.Message) + } + result.Failed = append(result.Failed, FailedServer{ + Name: originalName, + Error: "invalid_server", + Details: strings.Join(msgs, "; "), + }) + continue + } + // Override quarantine if SkipQuarantine is set if opts.SkipQuarantine { serverConfig.Quarantined = false diff --git a/internal/configimport/import_invalid_server_test.go b/internal/configimport/import_invalid_server_test.go new file mode 100644 index 000000000..5d3f6bb06 --- /dev/null +++ b/internal/configimport/import_invalid_server_test.go @@ -0,0 +1,34 @@ +package configimport + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// RC4-IMPORT-001: a URL-only entry forced through the claude-desktop parser +// maps to a commandless stdio server. config.Load rejects that on the next +// boot (exit 4), so Import must report it as failed and never offer it for +// persistence, while valid entries in the same payload still import. +func TestImport_RejectsServerThatFailsConfigValidation(t *testing.T) { + content := []byte(`{"mcpServers": { + "broken": {"url": "https://example.com/mcp"}, + "good": {"command": "npx", "args": ["-y", "some-server"]} + }}`) + + for _, skipQuarantine := range []bool{false, true} { + result, err := Import(content, &ImportOptions{FormatHint: FormatClaudeDesktop, SkipQuarantine: skipQuarantine}) + require.NoError(t, err) + + require.Len(t, result.Imported, 1, "only the valid server may be importable") + assert.Equal(t, "good", result.Imported[0].Server.Name) + + require.Len(t, result.Failed, 1) + assert.Equal(t, "broken", result.Failed[0].Name) + assert.Equal(t, "invalid_server", result.Failed[0].Error) + assert.Contains(t, result.Failed[0].Details, "command is required") + assert.Equal(t, 1, result.Summary.Failed) + assert.Equal(t, 1, result.Summary.Imported) + } +} diff --git a/internal/configimport/import_test.go b/internal/configimport/import_test.go index 56eff0ed0..ca78dbb00 100644 --- a/internal/configimport/import_test.go +++ b/internal/configimport/import_test.go @@ -54,8 +54,14 @@ func TestImport(t *testing.T) { if result.Format != FormatCodex { t.Errorf("Format = %v, want %v", result.Format, FormatCodex) } - if result.Summary.Imported != 5 { - t.Errorf("Summary.Imported = %d, want 5", result.Summary.Imported) + // The fixture's `filesystem` entry has cwd=/home/user, which fails the + // same working_dir-exists check config.Load applies when that directory + // is absent on the machine running the test, so it may be Failed. + if got := result.Summary.Imported + result.Summary.Failed; got != 5 { + t.Errorf("Summary.Imported+Failed = %d, want 5", got) + } + if result.Summary.Imported < 4 { + t.Errorf("Summary.Imported = %d, want at least 4", result.Summary.Imported) } }) diff --git a/internal/httpapi/import_invalid_server_test.go b/internal/httpapi/import_invalid_server_test.go new file mode 100644 index 000000000..772c9e24a --- /dev/null +++ b/internal/httpapi/import_invalid_server_test.go @@ -0,0 +1,65 @@ +package httpapi + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +type recordingImportController struct { + mockImportController + mu sync.Mutex + added []string +} + +func (m *recordingImportController) AddServer(_ context.Context, s *config.ServerConfig) error { + m.mu.Lock() + defer m.mu.Unlock() + m.added = append(m.added, s.Name) + return nil +} + +// RC4-IMPORT-001: a URL-only entry imported with a forced claude-desktop +// format and skip_quarantine=true used to be persisted as a commandless stdio +// server, which made the next core startup exit with code 4. +func TestImportServersJSON_RejectsInvalidServerNotPersisted(t *testing.T) { + mock := &recordingImportController{mockImportController: mockImportController{apiKey: "test-key"}} + server := NewServer(mock, zap.NewNop().Sugar(), nil) + + body, _ := json.Marshal(ImportRequest{ + Format: "claude-desktop", + Content: `{"mcpServers": { + "broken": {"url": "https://example.com/mcp"}, + "good": {"command": "npx", "args": ["-y", "some-server"]} + }}`, + }) + req := httptest.NewRequest("POST", "/api/v1/servers/import/json?skip_quarantine=true", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Key", "test-key") + rr := httptest.NewRecorder() + server.router.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + var wrapped wrappedImportResponse + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &wrapped)) + + require.Len(t, wrapped.Data.Imported, 1) + assert.Equal(t, "good", wrapped.Data.Imported[0].Name) + require.Len(t, wrapped.Data.Failed, 1) + assert.Equal(t, "broken", wrapped.Data.Failed[0].Name) + assert.Contains(t, wrapped.Data.Failed[0].Details, "command is required") + + mock.mu.Lock() + defer mock.mu.Unlock() + assert.Equal(t, []string{"good"}, mock.added, "the invalid server must never reach AddServer") +} From b73f95de8b0b898942be529109c8d2109974fd98 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 3 Oct 2026 20:57:19 +0300 Subject: [PATCH 2/2] fix(import): re-validate renamed servers before persisting configimport validates each server under its pre-rename name; the REST import then applies the caller's rename map. Re-run the boot-path validation after renaming so a rename to a name the next startup refuses (e.g. containing ':') fails the entry instead of persisting it. From the zcode review of RC4-IMPORT-001. --- internal/httpapi/import.go | 23 +++++++++++++++++++++++ internal/httpapi/import_test.go | 22 ++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/internal/httpapi/import.go b/internal/httpapi/import.go index d54492a0c..a4a47f975 100644 --- a/internal/httpapi/import.go +++ b/internal/httpapi/import.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/configimport" "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" ) @@ -507,6 +508,28 @@ func (s *Server) runImport(r *http.Request, content []byte, formatHint string, s result.Imported[i].Server.Name = newName } } + // configimport validated the pre-rename name; re-check the renamed + // servers with the same boot-path validation so a rename cannot + // persist an entry the next startup refuses (RC4-IMPORT-001). + kept := result.Imported[:0] + for _, imported := range result.Imported { + if errs := config.ValidateServerForLoad(imported.Server); len(errs) > 0 { + msgs := make([]string, 0, len(errs)) + for _, e := range errs { + msgs = append(msgs, e.Error()) + } + result.Failed = append(result.Failed, configimport.FailedServer{ + Name: imported.Server.Name, + Error: "invalid_server", + Details: strings.Join(msgs, "; "), + }) + continue + } + kept = append(kept, imported) + } + result.Imported = kept + result.Summary.Imported = len(result.Imported) + result.Summary.Failed = len(result.Failed) } // Apply the caller's field overrides (Paste tab env/header edits) diff --git a/internal/httpapi/import_test.go b/internal/httpapi/import_test.go index 60c531dd1..9c9815c22 100644 --- a/internal/httpapi/import_test.go +++ b/internal/httpapi/import_test.go @@ -739,3 +739,25 @@ func TestImportServersJSON_EmptyMcpServersStill400(t *testing.T) { t.Errorf("Expected status 400, got %d: %s", rr.Code, rr.Body.String()) } } + +// RC4-IMPORT-001 follow-up: configimport validated the pre-rename name, so a +// rename to a name the boot path refuses (':' is the qualified-name +// separator) must fail the entry instead of persisting it. +func TestRunImport_RenameToInvalidNameIsRejected(t *testing.T) { + const content = `{"mcpServers": {"good": {"command": "good-mcp"}}}` + mock := &mockImportController{apiKey: "test-key"} + server := NewServer(mock, zap.NewNop().Sugar(), nil) + req := httptest.NewRequest("POST", "/api/v1/servers/import/path", http.NoBody) + req.Header.Set("X-API-Key", "test-key") + + resp, err := server.runImport(req, []byte(content), "claude-desktop", nil, true, map[string]string{"good": "bad:name"}, nil, false) + if err != nil { + t.Fatalf("runImport returned error: %v", err) + } + if len(resp.Imported) != 0 { + t.Fatalf("a rename to an invalid name must not be imported, got %+v", resp.Imported) + } + if len(resp.Failed) != 1 || resp.Failed[0].Error != "invalid_server" { + t.Fatalf("expected one invalid_server failure, got %+v", resp.Failed) + } +}