Skip to content

Commit a010eca

Browse files
authored
fix(import): reject servers that fail config validation before persisting (RC4-IMPORT-001) (#1490)
# Pull Request ## Description Fixes **RC4-IMPORT-001**, from the codex QA of v0.70.0-rc.4. The bug also exists in rc.2. A malformed import was accepted and persisted, and then bricked the next startup. The trigger was a URL-only entry imported with a forced `claude-desktop` format and `skip_quarantine=true`: - `POST /api/v1/servers/import` returned HTTP 200 with `imported=1`, plus a "missing command" warning; - it persisted a stdio server with no command; - the next core start exited with code 4 (`command is required for stdio protocol`). **Fix** - New `config.ValidateServerForLoad(server)` runs the boot-path `validateDetailedCore` on a single server. - `configimport.Import` is the single choke point for REST import and CLI import, in daemon and direct-file mode. It now validates each mapped server before offering it for persistence. A server that fails is reported in `failed` with `invalid_server` and the reasons, and is never saved. Valid entries in the same payload import as before. - The REST import applies the caller's rename map after that validation, so renamed servers are re-validated before anything is persisted. - **Side effect, intended:** an entry whose `working_dir` doesn't exist on this machine is now rejected too, because startup would refuse it. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass - **New tests:** - configimport: the malformed entry is rejected with its reason, and a valid entry in the same payload still imports; - httpapi: `TestRunImport_RenameToInvalidNameIsRejected`. - **Suites:** `go test ./internal/configimport/ ./internal/httpapi/` passes. - **Review:** zcode found two lows. The rename re-validation is fixed here. The codex-fixture assertion is now environment-tolerant, because its `cwd=/home/user` entry passes or fails depending on the host. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 parent eef407c commit a010eca

7 files changed

Lines changed: 190 additions & 2 deletions

File tree

‎internal/config/config.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2335,6 +2335,25 @@ func (c *Config) ValidateDetailed() []ValidationError {
23352335
return errors
23362336
}
23372337

2338+
// ValidateServerForLoad runs the per-server checks config.Load applies at boot
2339+
// (the stdio command-required check, url-required for HTTP protocols, protocol
2340+
// and name validity, ...) against a single server and returns only the errors
2341+
// attributable to it. Write surfaces that persist a server built outside the
2342+
// config loader (e.g. imports) use it so they can never save an entry the next
2343+
// startup would refuse to load.
2344+
func ValidateServerForLoad(server *ServerConfig) []ValidationError {
2345+
cfg := DefaultConfig()
2346+
cfg.Servers = []*ServerConfig{server}
2347+
const prefix = "mcpServers[0]"
2348+
var out []ValidationError
2349+
for _, e := range cfg.validateDetailedCore() {
2350+
if strings.HasPrefix(e.Field, prefix) {
2351+
out = append(out, e)
2352+
}
2353+
}
2354+
return out
2355+
}
2356+
23382357
// oauthRedirectURIErrors reports every per-server `oauth.redirect_uri` that the
23392358
// loopback callback listener cannot honor.
23402359
//

‎internal/configimport/import.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@ package configimport
22

33
import (
44
"fmt"
5+
"strings"
56
"time"
7+
8+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
69
)
710

811
// Import parses configuration content and imports servers.
@@ -146,6 +149,22 @@ func Import(content []byte, opts *ImportOptions) (*ImportResult, error) {
146149
// Map to ServerConfig
147150
serverConfig, skipped, warnings := MapToServerConfig(parsed, opts.Now)
148151

152+
// Reject entries the next config.Load would refuse (e.g. a URL-only
153+
// entry forced through a stdio-only format yields a commandless stdio
154+
// server). Persisting one bricks the next startup with exit code 4.
155+
if verrs := config.ValidateServerForLoad(serverConfig); len(verrs) > 0 {
156+
msgs := make([]string, 0, len(verrs))
157+
for _, ve := range verrs {
158+
msgs = append(msgs, ve.Message)
159+
}
160+
result.Failed = append(result.Failed, FailedServer{
161+
Name: originalName,
162+
Error: "invalid_server",
163+
Details: strings.Join(msgs, "; "),
164+
})
165+
continue
166+
}
167+
149168
// Override quarantine if SkipQuarantine is set
150169
if opts.SkipQuarantine {
151170
serverConfig.Quarantined = false
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package configimport
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// RC4-IMPORT-001: a URL-only entry forced through the claude-desktop parser
11+
// maps to a commandless stdio server. config.Load rejects that on the next
12+
// boot (exit 4), so Import must report it as failed and never offer it for
13+
// persistence, while valid entries in the same payload still import.
14+
func TestImport_RejectsServerThatFailsConfigValidation(t *testing.T) {
15+
content := []byte(`{"mcpServers": {
16+
"broken": {"url": "https://example.com/mcp"},
17+
"good": {"command": "npx", "args": ["-y", "some-server"]}
18+
}}`)
19+
20+
for _, skipQuarantine := range []bool{false, true} {
21+
result, err := Import(content, &ImportOptions{FormatHint: FormatClaudeDesktop, SkipQuarantine: skipQuarantine})
22+
require.NoError(t, err)
23+
24+
require.Len(t, result.Imported, 1, "only the valid server may be importable")
25+
assert.Equal(t, "good", result.Imported[0].Server.Name)
26+
27+
require.Len(t, result.Failed, 1)
28+
assert.Equal(t, "broken", result.Failed[0].Name)
29+
assert.Equal(t, "invalid_server", result.Failed[0].Error)
30+
assert.Contains(t, result.Failed[0].Details, "command is required")
31+
assert.Equal(t, 1, result.Summary.Failed)
32+
assert.Equal(t, 1, result.Summary.Imported)
33+
}
34+
}

‎internal/configimport/import_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,14 @@ func TestImport(t *testing.T) {
5454
if result.Format != FormatCodex {
5555
t.Errorf("Format = %v, want %v", result.Format, FormatCodex)
5656
}
57-
if result.Summary.Imported != 5 {
58-
t.Errorf("Summary.Imported = %d, want 5", result.Summary.Imported)
57+
// The fixture's `filesystem` entry has cwd=/home/user, which fails the
58+
// same working_dir-exists check config.Load applies when that directory
59+
// is absent on the machine running the test, so it may be Failed.
60+
if got := result.Summary.Imported + result.Summary.Failed; got != 5 {
61+
t.Errorf("Summary.Imported+Failed = %d, want 5", got)
62+
}
63+
if result.Summary.Imported < 4 {
64+
t.Errorf("Summary.Imported = %d, want at least 4", result.Summary.Imported)
5965
}
6066
})
6167

‎internal/httpapi/import.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"strings"
1313
"time"
1414

15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
1516
"github.com/smart-mcp-proxy/mcpproxy-go/internal/configimport"
1617
"github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth"
1718
)
@@ -507,6 +508,28 @@ func (s *Server) runImport(r *http.Request, content []byte, formatHint string, s
507508
result.Imported[i].Server.Name = newName
508509
}
509510
}
511+
// configimport validated the pre-rename name; re-check the renamed
512+
// servers with the same boot-path validation so a rename cannot
513+
// persist an entry the next startup refuses (RC4-IMPORT-001).
514+
kept := result.Imported[:0]
515+
for _, imported := range result.Imported {
516+
if errs := config.ValidateServerForLoad(imported.Server); len(errs) > 0 {
517+
msgs := make([]string, 0, len(errs))
518+
for _, e := range errs {
519+
msgs = append(msgs, e.Error())
520+
}
521+
result.Failed = append(result.Failed, configimport.FailedServer{
522+
Name: imported.Server.Name,
523+
Error: "invalid_server",
524+
Details: strings.Join(msgs, "; "),
525+
})
526+
continue
527+
}
528+
kept = append(kept, imported)
529+
}
530+
result.Imported = kept
531+
result.Summary.Imported = len(result.Imported)
532+
result.Summary.Failed = len(result.Failed)
510533
}
511534

512535
// Apply the caller's field overrides (Paste tab env/header edits)
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package httpapi
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"encoding/json"
7+
"net/http"
8+
"net/http/httptest"
9+
"sync"
10+
"testing"
11+
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
"go.uber.org/zap"
15+
16+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
17+
)
18+
19+
type recordingImportController struct {
20+
mockImportController
21+
mu sync.Mutex
22+
added []string
23+
}
24+
25+
func (m *recordingImportController) AddServer(_ context.Context, s *config.ServerConfig) error {
26+
m.mu.Lock()
27+
defer m.mu.Unlock()
28+
m.added = append(m.added, s.Name)
29+
return nil
30+
}
31+
32+
// RC4-IMPORT-001: a URL-only entry imported with a forced claude-desktop
33+
// format and skip_quarantine=true used to be persisted as a commandless stdio
34+
// server, which made the next core startup exit with code 4.
35+
func TestImportServersJSON_RejectsInvalidServerNotPersisted(t *testing.T) {
36+
mock := &recordingImportController{mockImportController: mockImportController{apiKey: "test-key"}}
37+
server := NewServer(mock, zap.NewNop().Sugar(), nil)
38+
39+
body, _ := json.Marshal(ImportRequest{
40+
Format: "claude-desktop",
41+
Content: `{"mcpServers": {
42+
"broken": {"url": "https://example.com/mcp"},
43+
"good": {"command": "npx", "args": ["-y", "some-server"]}
44+
}}`,
45+
})
46+
req := httptest.NewRequest("POST", "/api/v1/servers/import/json?skip_quarantine=true", bytes.NewReader(body))
47+
req.Header.Set("Content-Type", "application/json")
48+
req.Header.Set("X-API-Key", "test-key")
49+
rr := httptest.NewRecorder()
50+
server.router.ServeHTTP(rr, req)
51+
52+
require.Equal(t, http.StatusOK, rr.Code, rr.Body.String())
53+
var wrapped wrappedImportResponse
54+
require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &wrapped))
55+
56+
require.Len(t, wrapped.Data.Imported, 1)
57+
assert.Equal(t, "good", wrapped.Data.Imported[0].Name)
58+
require.Len(t, wrapped.Data.Failed, 1)
59+
assert.Equal(t, "broken", wrapped.Data.Failed[0].Name)
60+
assert.Contains(t, wrapped.Data.Failed[0].Details, "command is required")
61+
62+
mock.mu.Lock()
63+
defer mock.mu.Unlock()
64+
assert.Equal(t, []string{"good"}, mock.added, "the invalid server must never reach AddServer")
65+
}

‎internal/httpapi/import_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,3 +739,25 @@ func TestImportServersJSON_EmptyMcpServersStill400(t *testing.T) {
739739
t.Errorf("Expected status 400, got %d: %s", rr.Code, rr.Body.String())
740740
}
741741
}
742+
743+
// RC4-IMPORT-001 follow-up: configimport validated the pre-rename name, so a
744+
// rename to a name the boot path refuses (':' is the qualified-name
745+
// separator) must fail the entry instead of persisting it.
746+
func TestRunImport_RenameToInvalidNameIsRejected(t *testing.T) {
747+
const content = `{"mcpServers": {"good": {"command": "good-mcp"}}}`
748+
mock := &mockImportController{apiKey: "test-key"}
749+
server := NewServer(mock, zap.NewNop().Sugar(), nil)
750+
req := httptest.NewRequest("POST", "/api/v1/servers/import/path", http.NoBody)
751+
req.Header.Set("X-API-Key", "test-key")
752+
753+
resp, err := server.runImport(req, []byte(content), "claude-desktop", nil, true, map[string]string{"good": "bad:name"}, nil, false)
754+
if err != nil {
755+
t.Fatalf("runImport returned error: %v", err)
756+
}
757+
if len(resp.Imported) != 0 {
758+
t.Fatalf("a rename to an invalid name must not be imported, got %+v", resp.Imported)
759+
}
760+
if len(resp.Failed) != 1 || resp.Failed[0].Error != "invalid_server" {
761+
t.Fatalf("expected one invalid_server failure, got %+v", resp.Failed)
762+
}
763+
}

0 commit comments

Comments
 (0)