diff --git a/internal/connect/backup.go b/internal/connect/backup.go index 00d6ba55d..f006f5c6d 100644 --- a/internal/connect/backup.go +++ b/internal/connect/backup.go @@ -68,7 +68,26 @@ func backupFile(path string) (string, error) { // atomicWriteFile writes data to path atomically by writing to a temp file // in the same directory and renaming. This prevents partial writes. -func atomicWriteFile(path string, data []byte, perm os.FileMode) error { +// atomicWriteFile stages data into a temp file in the same directory and +// renames it over path, so a reader never observes a partially-written file. +// +// preRename, when non-nil, is called AFTER every real filesystem operation +// that stages the temp file (MkdirAll, CreateTemp, Write, Close, Chmod) and +// IMMEDIATELY before the rename that actually replaces path's content — the +// last point at which this function can still back out. A non-nil return +// aborts: the temp file is removed and path is left untouched. This exists +// for connectJSON/connectTOML's servers-section race guard (Spec 091 FR-005 +// gap, round-5 cross-model review of PR #1340): those callers' own re-checks +// before calling this function still left a real, I/O-bearing gap (temp-file +// staging) between the check and the rename; running the SAME check here, at +// this exact point, closes that gap down to the (unavoidable without an +// OS-level lock across the whole read-modify-write sequence) span between +// this call and the os.Rename two lines below. Go's os.Rename itself still +// does a metadata lookup (Lstat on Unix) before the actual rename/replace +// syscall, so this is not literally zero I/O, but it is the practical +// floor: one fast local metadata lookup, not a copy or any work an external +// writer could meaningfully race against. +func atomicWriteFile(path string, data []byte, perm os.FileMode, preRename func() error) error { dir := filepath.Dir(path) // Ensure the directory exists @@ -102,6 +121,12 @@ func atomicWriteFile(path string, data []byte, perm os.FileMode) error { return fmt.Errorf("chmod temp file: %w", err) } + if preRename != nil { + if err := preRename(); err != nil { + return err + } + } + if err := os.Rename(tmpName, path); err != nil { return fmt.Errorf("rename temp to target: %w", err) } diff --git a/internal/connect/clients.go b/internal/connect/clients.go index 539396a0e..4e4e28359 100644 --- a/internal/connect/clients.go +++ b/internal/connect/clients.go @@ -220,27 +220,57 @@ func serversMapPath(clientID string) []string { return nil } -// getServersMap resolves a client's servers map from parsed config data, -// following serversMapPath when the client needs one and falling back to the -// flat client.ServerKey lookup otherwise. -func getServersMap(client *ClientDef, data map[string]interface{}) (map[string]interface{}, bool) { +// resolveServersMapState classifies a client's servers-map location within +// parsed config data (following serversMapPath, or the flat client.ServerKey +// lookup for every client that doesn't need one), distinguishing three +// outcomes that getServersMap's plain (map, bool) collapses into two: +// +// - found=true: the full path resolved to an object; serversMap is it. +// - found=false, malformed=false: some key along the path is simply +// ABSENT — a legitimate "nothing here yet, safe to create" case. +// - found=false, malformed=true: a key along the path is PRESENT but its +// value is not an object (a hand-edited string/number/array/bool, or — +// for a nested path like ZCode's mcp.servers — an intermediate level +// that isn't a table either). This must never be treated the same as +// "absent": collapsing the two let a non-object value be silently +// replaced by a fresh empty map on write, with drift detection never +// getting a chance to refuse (Spec 091 FR-005 gap, PR #1340). +// +// This is the single place that distinction is computed, so both flat-key +// clients and any future nested-path client (see serversMapPath) get it for +// free — callers that only need the old two-way "found or not" question can +// still use getServersMap, which is defined in terms of this. +func resolveServersMapState(client *ClientDef, data map[string]interface{}) (serversMap map[string]interface{}, found, malformed bool) { path := serversMapPath(client.ID) if path == nil { - m, ok := data[client.ServerKey].(map[string]interface{}) - return m, ok + path = []string{client.ServerKey} } cur := data for i, key := range path { - m, ok := cur[key].(map[string]interface{}) + raw, present := cur[key] + if !present { + return nil, false, false + } + m, ok := raw.(map[string]interface{}) if !ok { - return nil, false + return nil, false, true } if i == len(path)-1 { - return m, true + return m, true, false } cur = m } - return nil, false + return nil, false, false +} + +// getServersMap resolves a client's servers map from parsed config data, +// following serversMapPath when the client needs one and falling back to the +// flat client.ServerKey lookup otherwise. Callers that need to distinguish +// "absent" from "present but not an object" should use +// resolveServersMapState instead. +func getServersMap(client *ClientDef, data map[string]interface{}) (map[string]interface{}, bool) { + m, found, _ := resolveServersMapState(client, data) + return m, found } // setServersMap writes serversMap back into data at a client's servers diff --git a/internal/connect/connect.go b/internal/connect/connect.go index 5ad1737d6..6e7cd97c4 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -495,6 +495,38 @@ func (s *Service) ConnectWithPrecondition(clientID, serverName string, force boo return nil, s.asAccessError(client, cfgPath, err) } + // Deliberately NOT refusing here on a malformed accessState: this resolution + // and the write's own read (inside connectJSON/connectTOML) are two + // independent reads of the file, so a check here alone would leave a TOCTOU + // window — the config could still be object-shaped now and mutated to a + // non-object section before the write's own read runs, bypassing an + // upstream-only guard. The write functions instead re-check this repeatedly, + // close to each I/O step that could widen the window (see + // refuseIfServersSectionRaced and its call sites in connectJSON/ + // connectTOML) — shrinking, not eliminating, the drift window; the residual + // gap immediately before atomicWriteFile's own rename is documented there. + // + // KNOWN, PRE-EXISTING, OUT-OF-SCOPE LIMITATION (surfaced by cross-model + // review of this fix, not introduced by it — present since Spec 091 + // shipped): the precondition TOKEN itself is only checked against THIS + // read, not against the write's own later, independent read. An entry that + // is still object-shaped on both reads but whose VALUE changed between them + // is not re-validated — with force=true the write clobbers content the + // user's token did not actually describe. Closing that requires threading + // one shared `data` read through preWriteState AND connectJSON/connectTOML + // (today they each call s.read independently), which is a larger, + // security-sensitive refactor of the FR-005 precondition mechanism itself + // and deserves its own dedicated PR + review cycle rather than being folded + // into this one — the same reasoning that scoped THIS fix to the + // non-object-section gap in the first place. Tracked separately. + // + // A second, narrower instance of the same two-independent-reads shape: + // guardJsoncComments (below, inside connectJSON) reads the file once to + // detect comments, then readOrCreateJSON reads it again; a file that + // gains comments in between is parsed leniently and rewritten as plain + // JSON, silently stripping them. Also pre-existing (the comment guard + // predates this fix) and also tracked separately rather than fixed here. + // Precondition check BEFORE any backup or write, so a refusal is completely // inert (Spec 091 FR-005). if preconditionToken != "" { @@ -624,6 +656,72 @@ func (s *Service) guardJsoncComments(cfgPath string) error { return nil } +// refuseIfServersSectionRaced re-reads cfgPath and reports whether client's +// servers section (following serversMapPath for a nested-schema client like +// ZCode, or the flat client.ServerKey lookup otherwise — the same resolution +// resolveServersMapState uses everywhere else) has become present-but-not- +// an-object since an earlier read. connectJSON/ +// connectTOML each call this TWICE against the same drift class (Spec 091 +// FR-005 gap) — on top of the type assertion the function body's own read +// already does for its existence/force/adoption decisions — because that +// first read is not adjacent to the actual write: several real I/O steps +// happen in between: +// +// - immediately before backupFile: a fast-fail so an already-bad section +// (unchanged since the top-of-function read) does not even earn a +// backup file, and so a change landing during the EARLIER part of the +// function body (existence/adoption decisions, which do no I/O of their +// own) is caught. +// - as atomicWriteFile's preRename hook (NOT a call made before +// atomicWriteFile — round-5 cross-model review found that placement +// alone still left atomicWriteFile's own temp-file staging +// (MkdirAll/CreateTemp/Write/Close/Chmod) as a real, I/O-bearing window): +// backupFile performs its own Stat/Open/copy, genuinely slow enough on a +// loaded filesystem to be practically raceable (round-4), and then +// atomicWriteFile's staging adds more of the same (round-5) — so a +// change landing during EITHER is caught by this call running at the +// true last moment before the rename that actually replaces the file. +// +// A residual gap remains between this second call (inside atomicWriteFile, +// immediately before os.Rename) and the rename itself — practically just the +// single Lstat os.Rename performs internally on Unix before replacing the +// file, not a copy or anything an external writer could meaningfully race +// against. Fully eliminating even that needs an OS-level file lock (e.g. +// flock) held across the whole read-modify-write sequence, which is a larger +// architectural change deserving its own review, not folded into this fix +// (tracked alongside the other deferred TOCTOU findings — see the comment +// block in ConnectWithPrecondition). +// +// This is deliberately forgiving about everything except the one thing it +// exists to catch: a vanished file, a still-absent-or-object-shaped section, +// or any read/parse failure all return nil — those are not this guard's +// class of problem, and the imminent backup/write attempt (or its own +// pre-existing error handling) is what surfaces them. Only "parsed fine AND +// the key is present AND it is not the right container type" refuses. +func (s *Service) refuseIfServersSectionRaced(client *ClientDef, cfgPath string) error { + raw, err := s.read(cfgPath) + if err != nil { + return nil + } + var data map[string]interface{} + if client.Format == "toml" { + if _, derr := toml.Decode(string(raw), &data); derr != nil { + return nil + } + } else if derr := unmarshalLenientJSON(raw, &data); derr != nil { + return nil + } + _, _, malformed := resolveServersMapState(client, data) + if !malformed { + return nil + } + containerWord := "a JSON object" + if client.Format == "toml" { + containerWord = "a TOML table" + } + return fmt.Errorf("%s: %q was changed to a non-object value while MCPProxy was about to write it (expected %s); refusing to overwrite it — retry", cfgPath, client.ServerKey, containerWord) +} + // connectJSON writes the entry, adopting the entry `resolved` names when it // differs from serverName. The resolution is passed in rather than recomputed // so the write acts on exactly the entry the preview described and the @@ -639,9 +737,21 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, err } - // Get or create the servers section - serversMap, ok := getServersMap(client, data) - if !ok { + // Get or create the servers section. A key along the path (see + // serversMapPath — flat for most clients, nested for ZCode) that is + // PRESENT but not an object (a hand-edited string/number/array/bool, or a + // non-table intermediate level) must refuse, not silently fall through to + // "no entries yet": the code below would otherwise replace it with a + // brand-new empty map, discarding whatever was there without ever giving + // drift detection a chance to catch it. This is the first of THREE checks + // against this drift class — see the second, pre-backup one below and the + // third, inside atomicWriteFile's preRename hook, for why this one alone + // is not authoritative. + serversMap, found, malformed := resolveServersMapState(client, data) + if malformed { + return nil, fmt.Errorf("%s: %q is not a JSON object; refusing to overwrite it — fix the config manually and retry", cfgPath, client.ServerKey) + } + if !found { serversMap = make(map[string]interface{}) } @@ -681,6 +791,13 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for } } + // SECOND check for the same drift class (Spec 091 FR-005 gap; round-3 + // cross-model review finding): a fast-fail, before backupFile's own real + // I/O, for a change that landed since the top-of-function read. + if err := s.refuseIfServersSectionRaced(client, cfgPath); err != nil { + return nil, err + } + // Create backup before modifying backupPath, err := backupFile(cfgPath) if err != nil { @@ -699,7 +816,19 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("marshal config: %w", err) } - if err := atomicWriteFile(cfgPath, encoded, perm); err != nil { + // THIRD check (round-4/round-5 cross-model review findings): backupFile + // above just performed real Stat/Open/copy I/O — genuinely slow enough to + // race in practice — so the section could have been replaced with a + // non-object value DURING that backup, after the second check already + // passed. Passing this as atomicWriteFile's preRename hook (rather than + // calling it here, before atomicWriteFile) is what actually closes that + // window down to a few local syscalls: atomicWriteFile itself stages the + // temp file (MkdirAll/CreateTemp/Write/Close/Chmod — also real I/O) before + // this runs, so a check called from here would still leave THAT staging + // gap open, per round 5. + if err := atomicWriteFile(cfgPath, encoded, perm, func() error { + return s.refuseIfServersSectionRaced(client, cfgPath) + }); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -787,7 +916,7 @@ func (s *Service) disconnectJSON(client *ClientDef, cfgPath, serverName string) return nil, fmt.Errorf("marshal config: %w", err) } - if err := atomicWriteFile(cfgPath, encoded, perm); err != nil { + if err := atomicWriteFile(cfgPath, encoded, perm, nil); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -811,13 +940,17 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for return nil, err } - // Get or create mcp_servers section - serversRaw, ok := data["mcp_servers"] - var serversMap map[string]interface{} - if ok { - serversMap, _ = serversRaw.(map[string]interface{}) + // Get or create mcp_servers section. See the equivalent comment in + // connectJSON: present-but-wrong-type must refuse, not silently fall + // through to a fresh empty table that discards the value. This is the + // first of three checks against this drift class — see the second, + // pre-backup one below and the third, inside atomicWriteFile's preRename + // hook. + serversMap, found, malformed := resolveServersMapState(client, data) + if malformed { + return nil, fmt.Errorf("%s: %q is not a TOML table; refusing to overwrite it — fix the config manually and retry", cfgPath, client.ServerKey) } - if serversMap == nil { + if !found { serversMap = make(map[string]interface{}) } @@ -836,6 +969,13 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for action = "updated" } + // Second check — a fast-fail before backupFile's own real I/O. See + // refuseIfServersSectionRaced's doc comment for why this alone still + // leaves a window, and the third check below that closes it. + if err := s.refuseIfServersSectionRaced(client, cfgPath); err != nil { + return nil, err + } + // Backup backupPath, err := backupFile(cfgPath) if err != nil { @@ -846,7 +986,7 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for // exactly what preview renders (Spec 078 FR-002). entry := buildServerEntry(client.ID, s.entryParams(false)) serversMap[serverName] = entry - data["mcp_servers"] = serversMap + setServersMap(client, data, serversMap) // Encode TOML var buf bytes.Buffer @@ -855,7 +995,12 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("encode TOML: %w", err) } - if err := atomicWriteFile(cfgPath, buf.Bytes(), perm); err != nil { + // Third check (round-4/round-5 findings) — see connectJSON's equivalent + // comment for why this must be atomicWriteFile's preRename hook rather + // than a call from here. + if err := atomicWriteFile(cfgPath, buf.Bytes(), perm, func() error { + return s.refuseIfServersSectionRaced(client, cfgPath) + }); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -940,7 +1085,7 @@ func (s *Service) disconnectTOML(client *ClientDef, cfgPath, serverName string) return nil, fmt.Errorf("encode TOML: %w", err) } - if err := atomicWriteFile(cfgPath, buf.Bytes(), perm); err != nil { + if err := atomicWriteFile(cfgPath, buf.Bytes(), perm, nil); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -975,6 +1120,9 @@ func (s *Service) readOrCreateJSON(path string) (map[string]interface{}, os.File perm = info.Mode() } + // unmarshalLenientJSON normalizes a top-level JSON `null` (which decodes + // successfully but would otherwise leave a nil map) back to a non-nil + // empty map on success, so no additional nil check is needed here. var data map[string]interface{} if err := unmarshalLenientJSON(raw, &data); err != nil { return nil, perm, fmt.Errorf("parse JSON in %s: %w", path, err) @@ -1004,6 +1152,12 @@ func (s *Service) readOrCreateTOML(path string) (map[string]interface{}, os.File if _, err := toml.Decode(string(raw), &data); err != nil { return nil, perm, fmt.Errorf("parse TOML in %s: %w", path, err) } + if data == nil { + // Defensive: BurntSushi/toml initializes the map even for empty input + // today, but a nil top-level map here would panic the same way the JSON + // path's null-document case did. + data = make(map[string]interface{}) + } return data, perm, nil } @@ -1109,8 +1263,14 @@ func (s *Service) findEntryJSONBytes(client ClientDef, raw []byte) (loc entryLoc return entryLocation{}, false, false } - serversMap, ok := getServersMap(&client, data) - if !ok { + serversMap, keyFound, malformed := resolveServersMapState(&client, data) + if malformed { + // Present but not an object — same malformed classification as + // resolveExistingEntry/preWriteState, so GetStatus does not report + // "not connected" for a config a connect/preview call would refuse. + return entryLocation{}, false, false + } + if !keyFound { return entryLocation{}, false, true } @@ -1325,7 +1485,10 @@ func (s *Service) findEntryTOMLBytes(raw []byte) (loc entryLocation, found, pars serversMap, ok := serversRaw.(map[string]interface{}) if !ok { - return entryLocation{}, false, true + // Present but not a table — same malformed classification as + // resolveExistingEntry/preWriteState, so GetStatus does not report + // "not connected" for a config a connect/preview call would refuse. + return entryLocation{}, false, false } baseURL := s.baseURL() diff --git a/internal/connect/connect_test.go b/internal/connect/connect_test.go index 036f32f3d..07d7b4557 100644 --- a/internal/connect/connect_test.go +++ b/internal/connect/connect_test.go @@ -1354,7 +1354,7 @@ func TestAtomicWriteFile(t *testing.T) { path := filepath.Join(dir, "subdir", "test.json") content := []byte(`{"atomic": true}`) - if err := atomicWriteFile(path, content, 0o644); err != nil { + if err := atomicWriteFile(path, content, 0o644, nil); err != nil { t.Fatalf("atomicWriteFile failed: %v", err) } diff --git a/internal/connect/preview.go b/internal/connect/preview.go index 2bba5a605..4cc249eea 100644 --- a/internal/connect/preview.go +++ b/internal/connect/preview.go @@ -229,8 +229,21 @@ func (s *Service) resolveExistingEntry(client ClientDef, raw []byte, serverName return nil, false } - serversMap, ok := getServersMap(&client, data) - if !ok { + serversMap, keyFound, malformed := resolveServersMapState(&client, data) + if malformed { + // A key along the path is present but its value is not an object — a + // string, number, array or bool from a hand-edited config (or, for a + // nested path like ZCode's mcp.servers, an intermediate level that + // isn't a table). This must NOT fall through to "no entries yet": the + // precondition token only ever hashes the RESOLVED ENTRY, never the + // section's own raw value, so two different non-object section values + // would mint identical tokens and the write would silently replace the + // value with a fresh map. Reporting malformed here makes preWriteState + // refuse the connect outright instead. + return nil, false + } + if !keyFound { + // No servers section yet: a legitimate create case, not malformed. return nil, true } if value, ok := serversMap[serverName]; ok { diff --git a/internal/connect/token_test.go b/internal/connect/token_test.go index 5cc606bef..93a841c4d 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -3,6 +3,7 @@ package connect import ( "encoding/hex" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -400,6 +401,565 @@ func TestConnectWithPrecondition_NonObjectEntryDriftRefuses(t *testing.T) { }) } +// TestPreview_NonObjectServersSection_IsMalformed closes a DIFFERENT drift +// class than TestPreview_PreconditionToken_NonObjectEntryDrift above: that one +// covers the individual ENTRY (data["mcpServers"]["mcpproxy"]) holding a +// non-object value. This one covers the SERVERS SECTION ITSELF +// (data["mcpServers"], or data["mcp_servers"] for Codex/TOML) holding a +// non-object value — a string, number, array or bool from a hand-edited +// config. +// +// resolveExistingEntry used to type-assert data[client.ServerKey].(map[string +// ]interface{}) and, on failure, report "no servers section" — indistinguishable +// from the key being absent entirely. That collapsed every non-object section +// value into the same "create" classification, so the precondition token (which +// only hashes the RESOLVED ENTRY, never the section's own raw value/type) could +// not detect the section's value changing between preview and write, and the +// write silently replaced it with a fresh map. The fix treats "key present but +// not an object" as malformed — a distinct, refusable state — rather than +// falling through to "create". +func TestPreview_NonObjectServersSection_IsMalformed(t *testing.T) { + t.Run("JSON client (claude-code)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + writeFileT(t, cfgPath, `{"mcpServers":"old"}`) + + preview, err := svc.Preview("claude-code", "mcpproxy") + if err != nil { + t.Fatalf("Preview should not hard-error on a non-object servers section: %v", err) + } + if preview.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q for a non-object servers section, got %q", accessMalformed, preview.AccessState) + } + }) + + t.Run("TOML client (codex)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("codex", home) + writeFileT(t, cfgPath, `mcp_servers = "old"`+"\n") + + preview, err := svc.Preview("codex", "mcpproxy") + if err != nil { + t.Fatalf("Preview should not hard-error on a non-object servers section: %v", err) + } + if preview.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q for a non-object servers section, got %q", accessMalformed, preview.AccessState) + } + }) + + // ZCode (added by #1339, merged into main after this fix was written) is + // the one client with a NESTED servers path (mcp.servers, via + // serversMapPath) rather than a flat top-level key — proving the fix + // generalizes via resolveServersMapState, not just the flat case. + t.Run("JSON client with nested servers path (zcode), leaf non-object", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("zcode", home) + writeFileT(t, cfgPath, `{"mcp":{"servers":"old"}}`) + + preview, err := svc.Preview("zcode", "mcpproxy") + if err != nil { + t.Fatalf("Preview should not hard-error on a non-object servers section: %v", err) + } + if preview.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q for a non-object servers section, got %q", accessMalformed, preview.AccessState) + } + }) + + t.Run("JSON client with nested servers path (zcode), intermediate non-object", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("zcode", home) + // The INTERMEDIATE level ("mcp") is non-object here, not the leaf + // ("servers") — resolveServersMapState must catch this too, since a + // hand-edited config could corrupt either level of a nested path. + writeFileT(t, cfgPath, `{"mcp":"old"}`) + + preview, err := svc.Preview("zcode", "mcpproxy") + if err != nil { + t.Fatalf("Preview should not hard-error on a non-object servers section: %v", err) + } + if preview.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q for a non-object intermediate level, got %q", accessMalformed, preview.AccessState) + } + }) +} + +// TestConnect_ZCode_NonObjectServersSection_RefusesWithoutToken is the ZCode +// counterpart to TestConnect_NonObjectServersSection_RefusesWithoutToken, +// added once ZCode (#1339) actually existed in this codebase — the original +// task asked for coverage on "at least one flat-key client and zcode, to +// prove the fix isn't client-specific." Both the leaf and the intermediate +// non-object cases must refuse the write, at either nesting level, without +// creating a backup or touching the file. +func TestConnect_ZCode_NonObjectServersSection_RefusesWithoutToken(t *testing.T) { + t.Run("leaf (mcp.servers) is non-object", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("zcode", home) + const original = `{"mcp":{"servers":42}}` + writeFileT(t, cfgPath, original) + + res, err := svc.Connect("zcode", "mcpproxy", true) + if err == nil { + t.Fatalf("expected a refusal error, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if got := readConfigT(t, cfgPath); got != original { + t.Fatalf("config must be untouched after a refusal:\n got: %s\n want: %s", got, original) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } + }) + + t.Run("intermediate (mcp) is non-object", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("zcode", home) + const original = `{"mcp":["not","an","object"]}` + writeFileT(t, cfgPath, original) + + res, err := svc.Connect("zcode", "mcpproxy", true) + if err == nil { + t.Fatalf("expected a refusal error, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if got := readConfigT(t, cfgPath); got != original { + t.Fatalf("config must be untouched after a refusal:\n got: %s\n want: %s", got, original) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } + }) +} + +// TestConnectWithPrecondition_NonObjectServersSection_RefusesDrift is the exact +// repro from the cross-model review of PR #1339: write a config whose servers +// section is a non-object value, preview it (which used to report "no existing +// entry, this will create one" and mint a token blind to the section's value), +// change the section's value externally, then submit connect with the stale +// token. Before the fix this SUCCEEDED and silently destroyed the "new" value; +// it must now refuse. +func TestConnectWithPrecondition_NonObjectServersSection_RefusesDrift(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + writeFileT(t, cfgPath, `{"mcpServers":"old"}`) + + preview, err := svc.Preview("claude-code", "mcpproxy") + if err != nil { + t.Fatalf("Preview: %v", err) + } + if preview.EntryExists { + t.Fatal("a malformed section must not report an entry that would be overwritten") + } + + const drifted = `{"mcpServers":"new"}` + writeFileT(t, cfgPath, drifted) + + res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, preview.PreconditionToken) + if err == nil { + t.Fatalf("expected a refusal error, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if !strings.Contains(err.Error(), "mcpServers") || !strings.Contains(err.Error(), "not a JSON object") { + t.Fatalf("expected the refusal to name the section and explain why, got: %v", err) + } + if got := readConfigT(t, cfgPath); got != drifted { + t.Fatalf("config must be untouched after a refusal:\n got: %s\n want: %s", got, drifted) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } +} + +// TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriterRead +// proves the guard is not a single upstream check that a concurrent external +// edit could slip past (the TOCTOU a naive "check preWriteState, then write" +// design would have): the file is OBJECT-shaped on the FIRST read (matching +// what preWriteState/the precondition resolution sees) and is mutated to a +// non-object section before the SECOND, independent read the writer itself +// performs. The write must still refuse and must not touch the file — the +// authoritative check has to live at the actual point of mutation, using +// whatever was read there, not a value resolved earlier in the call. +func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriterRead(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + const objectShaped = `{"mcpServers":{}}` + const racedNonObject = `{"mcpServers":"raced-in-between-reads"}` + writeFileT(t, cfgPath, objectShaped) + + reads := 0 + svc.setReadFile(func(path string) ([]byte, error) { + reads++ + if reads == 1 { + // The read inside ConnectWithPrecondition's own preWriteState call: + // still object-shaped, so no upstream check (if one existed) would fire. + return []byte(objectShaped), nil + } + // Every subsequent read — including the writer's own — observes the file + // AFTER the simulated concurrent edit. + return []byte(racedNonObject), nil + }) + + res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") + // Exactly 2 reads for this client/path: preWriteState's (no jsonc guard for + // claude-code, no precondition token to additionally resolve) and + // connectJSON's own readOrCreateJSON. A count outside this range would mean + // the test's own premise — "read #1 sees the pre-race state, read #2+ sees + // the raced-in one" — no longer matches what actually ran. + if reads != 2 { + t.Fatalf("expected exactly 2 reads (preWriteState + the writer's own), got %d", reads) + } + if err == nil { + t.Fatalf("expected the writer's own read to catch the raced-in non-object section, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if !strings.Contains(err.Error(), "mcpServers") || !strings.Contains(err.Error(), "not a JSON object") { + t.Fatalf("expected the refusal to name the section and explain why, got: %v", err) + } + if got := readConfigT(t, cfgPath); got != objectShaped { + t.Fatalf("the ON-DISK file (never touched by the mocked reads) must be untouched after a refusal:\n got: %s\n want: %s", got, objectShaped) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } +} + +// TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtThePreBackupCheck +// closes the must-fix round-3 cross-model review found: connectJSON/ +// connectTOML's OWN read (the "writer's own read" the previous test proves is +// authoritative) is STILL not adjacent to the actual write — backupFile +// performs real file I/O in between, widening the window in which an +// external process can replace the servers section with a non-object value +// AFTER the writer already decided it was safe to proceed. Repro: both +// preWriteState's read and connectJSON's readOrCreateJSON read see an +// OBJECT-shaped section (so the earlier, top-of-function check passes +// cleanly); only the pre-backup check's read observes the section having +// been replaced with a non-object value in between. The write must still +// refuse, before any backup is created. +func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtThePreBackupCheck(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + const objectShaped = `{"mcpServers":{}}` + const racedNonObject = `{"mcpServers":"raced-in-before-backup"}` + writeFileT(t, cfgPath, objectShaped) + + reads := 0 + svc.setReadFile(func(path string) ([]byte, error) { + reads++ + if reads <= 2 { + // preWriteState's read (#1) and connectJSON's own readOrCreateJSON + // read (#2): both still object-shaped, so the top-of-function check + // passes and the function proceeds toward the pre-backup check. + return []byte(objectShaped), nil + } + // The THIRD read — refuseIfServersSectionRaced, immediately before + // backupFile — observes the file AFTER the simulated concurrent edit. + return []byte(racedNonObject), nil + }) + + res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") + if reads != 3 { + t.Fatalf("expected exactly 3 reads (preWriteState + the writer's own + the pre-backup check), got %d", reads) + } + if err == nil { + t.Fatalf("expected the pre-backup check to catch the raced-in non-object section, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if !strings.Contains(err.Error(), "mcpServers") { + t.Fatalf("expected the refusal to name the section, got: %v", err) + } + if got := readConfigT(t, cfgPath); got != objectShaped { + t.Fatalf("the ON-DISK file (never touched by the mocked reads) must be untouched after a refusal:\n got: %s\n want: %s", got, objectShaped) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup — this check runs BEFORE backupFile, found %d", n) + } +} + +// TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAfterBackup +// closes the must-fix round-4 cross-model review found: backupFile performs +// real Stat/Open/copy I/O — genuinely slow enough to race in practice — so a +// change landing DURING that backup (i.e. AFTER the pre-backup check already +// passed) was still able to slip through to atomicWriteFile undetected. +// Repro: preWriteState's read, connectJSON's own read, AND the pre-backup +// check's read all see an OBJECT-shaped section (so backupFile actually runs +// and a backup file IS created — that's expected and consistent with how a +// later atomicWriteFile failure already behaves in this codebase); only the +// final check's read observes the section having been replaced with a +// non-object value. The write must still refuse, and the on-disk config must +// be untouched (the backup file's existence does not imply the config itself +// was mutated). +// +// Round 5 found the round-4 fix's placement — a call made BEFORE invoking +// atomicWriteFile — still left atomicWriteFile's own temp-file staging +// (MkdirAll/CreateTemp/Write/Close/Chmod) as a real, uninstrumented I/O +// window before the rename. The final check now runs as atomicWriteFile's +// preRename hook instead — structurally after that staging, immediately +// before os.Rename — which this test's black-box read-counting cannot +// directly distinguish from the round-4 placement (none of the temp-file +// staging steps touch the s.read seam this mock intercepts, so the read +// COUNT is identical either way); the placement itself is verified by +// reading atomicWriteFile's implementation (backup.go) and its call sites +// here, not by this test alone. What this test DOES still prove, unchanged: +// a value raced in after backupFile completes is caught before any bytes of +// the actual config file are replaced. +func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAfterBackup(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + const objectShaped = `{"mcpServers":{}}` + const racedNonObject = `{"mcpServers":"raced-in-during-backup-io"}` + writeFileT(t, cfgPath, objectShaped) + + reads := 0 + svc.setReadFile(func(path string) ([]byte, error) { + reads++ + if reads <= 3 { + // preWriteState (#1), connectJSON's own read (#2), and the + // pre-backup check (#3): all still object-shaped, so backupFile + // actually runs. + return []byte(objectShaped), nil + } + // The FOURTH read — refuseIfServersSectionRaced, immediately before + // atomicWriteFile — observes the file AFTER the simulated edit landing + // during backupFile's own I/O. + return []byte(racedNonObject), nil + }) + + res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") + if reads != 4 { + t.Fatalf("expected exactly 4 reads (preWriteState + the writer's own + the pre-backup check + the post-backup check), got %d", reads) + } + if err == nil { + t.Fatalf("expected the post-backup check to catch the raced-in non-object section, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if !strings.Contains(err.Error(), "mcpServers") { + t.Fatalf("expected the refusal to name the section, got: %v", err) + } + if got := readConfigT(t, cfgPath); got != objectShaped { + t.Fatalf("the ON-DISK config file (never touched by the mocked reads) must be untouched after a refusal:\n got: %s\n want: %s", got, objectShaped) + } + // backupFile runs BEFORE this refusal, so — unlike the pre-backup-check + // test above — a backup IS expected here; its existence must not be + // confused with the config itself having been mutated (asserted above). + if n := backupCount(t, cfgPath); n != 1 { + t.Fatalf("expected exactly 1 backup (created before the post-backup check refused), got %d", n) + } +} + +// TestConnect_NonObjectServersSection_RefusesWithoutToken proves the guard does +// not depend on the precondition-token flow: a tokenless Connect() call (the +// Web UI / CLI's existing behavior, Spec 091 contracts §2) must also refuse +// rather than silently discarding the section's value, for both JSON and TOML +// clients. +func TestConnect_NonObjectServersSection_RefusesWithoutToken(t *testing.T) { + t.Run("JSON client (vscode)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("vscode", home) + const original = `{"servers":["not","an","object"]}` + writeFileT(t, cfgPath, original) + + res, err := svc.Connect("vscode", "mcpproxy", true) + if err == nil { + t.Fatalf("expected a refusal error, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if got := readConfigT(t, cfgPath); got != original { + t.Fatalf("config must be untouched after a refusal:\n got: %s\n want: %s", got, original) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } + }) + + t.Run("TOML client (codex)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("codex", home) + const original = "mcp_servers = 42\n" + writeFileT(t, cfgPath, original) + + res, err := svc.Connect("codex", "mcpproxy", true) + if err == nil { + t.Fatalf("expected a refusal error, got res=%+v err=nil", res) + } + if res != nil { + t.Fatalf("expected a nil result alongside the refusal error, got %+v", res) + } + if got := readConfigT(t, cfgPath); got != original { + t.Fatalf("config must be untouched after a refusal:\n got: %s\n want: %s", got, original) + } + if n := backupCount(t, cfgPath); n != 0 { + t.Fatalf("a refused write must not create a backup, found %d", n) + } + }) +} + +// TestConnect_NullTopLevelDocument_DoesNotPanic pins a crash the cross-model +// review of this fix surfaced: a config file containing exactly the JSON +// literal `null` (or a TOML document that otherwise decodes to a nil map) +// decodes successfully with a NIL top-level map — encoding/json leaves the +// unmarshal target untouched for a JSON null, it is not an error. Before the +// fix, readOrCreateJSON/readOrCreateTOML returned that nil map unchanged, and +// connectJSON/connectTOML's final `data[serversKey] = serversMap` assignment +// panicked with "assignment to entry in nil map" — a DoS reachable through the +// plain tokenless Connect() path with no precondition token involved at all. +func TestConnect_NullTopLevelDocument_DoesNotPanic(t *testing.T) { + t.Run("JSON client (claude-code)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + writeFileT(t, cfgPath, "null") + + res, err := svc.Connect("claude-code", "mcpproxy", false) + if err != nil { + t.Fatalf("Connect must not error on a null top-level document, got: %v", err) + } + if !res.Success { + t.Fatalf("expected a null document to be treated as an empty config, got %+v", res) + } + }) + + t.Run("TOML client (codex)", func(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("codex", home) + // TOML has no top-level null literal; an empty file is the closest + // equivalent and already exercised by TestConnect_Codex_NewFile, but + // pin it here too as a defense-in-depth regression guard alongside the + // JSON case above. + writeFileT(t, cfgPath, "") + + res, err := svc.Connect("codex", "mcpproxy", false) + if err != nil { + t.Fatalf("Connect must not error on an empty TOML document, got: %v", err) + } + if !res.Success { + t.Fatalf("expected an empty document to be treated as an empty config, got %+v", res) + } + }) +} + +// TestUndo_NullBackup_DoesNotPanic pins a SIBLING crash of +// TestConnect_NullTopLevelDocument_DoesNotPanic that round-2 cross-model +// review found: replayConnectWrite (internal/connect/undo.go) has its own +// independent JSON parse, and a backup file containing exactly `null` +// resets its pre-initialized `data` map back to nil the same way — but this +// path panicked at `data[client.ServerKey] = serversMap` instead. Repro: +// connect against a null-content config (which backs up the null bytes +// verbatim), then Undo with the returned backup name. +func TestUndo_NullBackup_DoesNotPanic(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + writeFileT(t, cfgPath, "null") + + connectRes, err := svc.Connect("claude-code", "mcpproxy", false) + if err != nil { + t.Fatalf("Connect: %v", err) + } + + undoRes, err := svc.Undo("claude-code", "mcpproxy", filepath.Base(connectRes.BackupPath)) + if err != nil { + t.Fatalf("Undo must not error on a null-content backup: %v", err) + } + if !undoRes.Success { + t.Fatalf("expected Undo to succeed, got %+v", undoRes) + } +} + +// TestGetStatus_NonObjectServersSection_IsMalformed closes the should-fix the +// cross-model review flagged: GetStatus used to disagree with Preview/Connect +// for the exact same config — findEntryJSONBytes/findEntryTOMLBytes collapsed +// "servers key present but not an object" into the same parsedOK=true, +// found=false outcome as a genuinely absent section, so the status API +// reported a plain "not connected" for a config Preview/Connect now refuse to +// touch. Both must report the same malformed classification. +func TestGetStatus_NonObjectServersSection_IsMalformed(t *testing.T) { + t.Run("JSON client (claude-code)", func(t *testing.T) { + svc, home := testService(t) + writeFileT(t, ConfigPath("claude-code", home), `{"mcpServers":"old"}`) + + status, err := svc.GetStatus("claude-code") + if err != nil { + t.Fatalf("GetStatus: %v", err) + } + if status.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q, got %q (status=%+v)", accessMalformed, status.AccessState, status) + } + if status.Connected { + t.Fatalf("a malformed section must not report Connected=true, got %+v", status) + } + }) + + t.Run("TOML client (codex)", func(t *testing.T) { + svc, home := testService(t) + writeFileT(t, ConfigPath("codex", home), `mcp_servers = "old"`+"\n") + + status, err := svc.GetStatus("codex") + if err != nil { + t.Fatalf("GetStatus: %v", err) + } + if status.AccessState != accessMalformed { + t.Fatalf("expected access_state=%q, got %q (status=%+v)", accessMalformed, status.AccessState, status) + } + if status.Connected { + t.Fatalf("a malformed section must not report Connected=true, got %+v", status) + } + }) +} + +// TestConnectWithPrecondition_GenuineIOErrorIsNotMaskedAsNonObjectSection +// closes the should-fix the cross-model review flagged against an earlier +// version of this fix: an upstream check that fires on the general +// accessMalformed classification cannot distinguish "section present but +// wrong type" from other malformed causes (a stat/read I/O error, e.g. +// syscall.EIO), so a blanket refusal message there would hide the real cause. +// The fix instead lets a genuine read error propagate through its own +// existing path unchanged (connectJSON/connectTOML's readOrCreateJSON/TOML +// error wrapping), while the NEW, section-specific check only ever fires +// after a successful parse. Prove the injected I/O error's own message +// survives, rather than being replaced by the generic "not an object" text. +func TestConnectWithPrecondition_GenuineIOErrorIsNotMaskedAsNonObjectSection(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + writeFileT(t, cfgPath, `{"mcpServers":{}}`) + + injected := errors.New("injected-io-failure: input/output error") + reads := 0 + svc.setReadFile(func(path string) ([]byte, error) { + reads++ + return nil, injected + }) + + _, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") + // preWriteState's read fails first (classified accessMalformed, not + // propagated as an error there by design), so the writer's own read is what + // actually surfaces this error — pinning that this test exercises + // readOrCreateJSON's wrapped-error path, not a check that short-circuits + // before ever reaching it. + if reads != 2 { + t.Fatalf("expected exactly 2 reads (preWriteState + the writer's own), got %d", reads) + } + if err == nil { + t.Fatal("expected an error for an unreadable config") + } + if !errors.Is(err, injected) { + t.Fatalf("expected the genuine I/O error to survive via %%w-wrapping, not be masked as a non-object section: %v", err) + } + if strings.Contains(err.Error(), "is not a JSON object") { + t.Fatalf("a genuine I/O error must not be reported as a non-object servers section: %v", err) + } +} + // The token binds a preview to the entry the write would produce, so the // REQUESTED name is part of that binding. It used to be absent from the // preimage: only the RESOLVED name was hashed, and that is the empty string diff --git a/internal/connect/undo.go b/internal/connect/undo.go index f5e3701f5..7e4d0ba26 100644 --- a/internal/connect/undo.go +++ b/internal/connect/undo.go @@ -183,7 +183,7 @@ func (s *Service) undo(client *ClientDef, cfgPath, serverName, backupPath string if info, statErr := os.Stat(cfgPath); statErr == nil { perm = info.Mode() } - if err := atomicWriteFile(cfgPath, backupRaw, perm); err != nil { + if err := atomicWriteFile(cfgPath, backupRaw, perm, nil); err != nil { return nil, fmt.Errorf("restore from backup: %w", err) } @@ -227,6 +227,11 @@ func (s *Service) replayConnectWrite(client *ClientDef, serverName string, backu } if len(backupRaw) > 0 { + // unmarshalLenientJSON normalizes a top-level JSON `null` back to a + // non-nil map on success (a backup that is exactly "null" would + // otherwise reset the pre-initialized `data` above to nil and panic on + // the setServersMap write below), so no additional nil check is needed + // here. if err := unmarshalLenientJSON(backupRaw, &data); err != nil { return nil, fmt.Errorf("parse backup JSON: %w", err) }