From faaf577d44a6644dfda8481ecad2a70b86679ff1 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 17:48:44 +0300 Subject: [PATCH 1/7] fix(connect): refuse writes when a client's servers section is not an object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveExistingEntry type-asserted data[client.ServerKey] to a map and, on failure, reported "no servers section" — indistinguishable from the key being absent entirely. A hand-edited config whose servers section holds a string, number, array, or bool (e.g. {"mcpServers":"old"}) therefore classified as a clean "create" case in both preview and write. Since the precondition token only ever hashes the resolved ENTRY (never the section's own raw value/type), two different non-object section values minted identical tokens, so drift in that value between preview and write went undetected (Spec 091 FR-005 gap, confirmed by a codex gpt-5.6-sol cross-model review of #1339 and reproduced independently against main). On write, connectJSON/connectTOML then silently replaced the section with a fresh map, destroying whatever was there. Treat "key present but not an object" as a distinct, refusable accessMalformed state instead of falling through to "create": resolveExistingEntry now separates key-absent (still a legitimate create) from key-present-wrong-type, and ConnectWithPrecondition refuses the write outright whenever the resolved access state is malformed, independent of whether a precondition token was even supplied. This closes the gap without touching the token's hash inputs. Co-Authored-By: Claude Sonnet 5 --- internal/connect/connect.go | 12 +++- internal/connect/preview.go | 16 ++++- internal/connect/token_test.go | 125 +++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 3 deletions(-) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index 7042ded8e..71a32b966 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -490,11 +490,21 @@ func (s *Service) ConnectWithPrecondition(clientID, serverName string, force boo // and the write must cover the SAME entry — re-resolving per step is what // let a token hash one entry while the write replaced or deleted another // (Spec 091 FR-005). - fileExists, existing, _, err := s.preWriteState(client, cfgPath, serverName) + fileExists, existing, accessState, err := s.preWriteState(client, cfgPath, serverName) if err != nil { return nil, s.asAccessError(client, cfgPath, err) } + // A servers section that exists but is not an object never reaches the + // precondition token at all — the token only hashes the resolved ENTRY, so + // it cannot see the section's own raw value drifting. Refuse unconditionally + // (independent of whether a token was even echoed) rather than let connectJSON + // / connectTOML's own type assertion silently discard the value under a fresh + // map. This mirrors how a fully unparseable config already refuses. + if accessState == accessMalformed { + return nil, fmt.Errorf("%s could not be parsed as a valid config, or its %q section is not an object; refusing to write — fix the config manually and retry", cfgPath, client.ServerKey) + } + // Precondition check BEFORE any backup or write, so a refusal is completely // inert (Spec 091 FR-005). if preconditionToken != "" { diff --git a/internal/connect/preview.go b/internal/connect/preview.go index 1a3c31746..566d7955b 100644 --- a/internal/connect/preview.go +++ b/internal/connect/preview.go @@ -229,10 +229,22 @@ func (s *Service) resolveExistingEntry(client ClientDef, raw []byte, serverName return nil, false } - serversMap, ok := data[client.ServerKey].(map[string]interface{}) - if !ok { + rawSection, keyPresent := data[client.ServerKey] + if !keyPresent { + // No servers section yet: a legitimate create case, not malformed. return nil, true } + serversMap, ok := rawSection.(map[string]interface{}) + if !ok { + // The key is present but its value is not an object — a string, number, + // array or bool from a hand-edited config. 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 value, ok := serversMap[serverName]; ok { return newExistingEntry(serverName, value), true } diff --git a/internal/connect/token_test.go b/internal/connect/token_test.go index 5cc606bef..f50d15cba 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -400,6 +400,131 @@ 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) + } + }) +} + +// 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 && res != nil && res.Success { + t.Fatalf("connect must refuse when the servers section is not an object, got success: %+v", res) + } + 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) + } +} + +// 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 && res != nil && res.Success { + t.Fatalf("connect must refuse when the servers section is not an object, got success: %+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 && res != nil && res.Success { + t.Fatalf("connect must refuse when the servers section is not an object, got success: %+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) + } + }) +} + // 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 From ca842a9a133006093aea4ad136a1bd19910b1d49 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:00:53 +0300 Subject: [PATCH 2/7] fix(connect): close TOCTOU and nil-map panic found by review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review (codex gpt-5.6-sol, round 1 of the PR #1340 gate) found three real issues in the first cut of the non-object-servers-section fix: 1. must-fix: the upstream accessMalformed check in ConnectWithPrecondition read the file once via preWriteState, but connectJSON/connectTOML each perform their OWN independent read a few lines later. A file mutated between those two reads (object-shaped at check time, non-object at write time) bypassed the guard entirely and reached the original destructive fallthrough. Fix: drop the upstream check and make the type-assertion sites inside connectJSON/connectTOML themselves the authoritative, last-read guard — they now distinguish "key absent" from "key present, wrong type" and refuse before backup/mutation, immune to the race because there is no later read to race against. 2. should-fix: the removed upstream check's blanket error message covered every accessMalformed cause (stat/read I/O errors like EIO, not just a non-object section), masking genuine I/O failures behind a misleading "not an object" message. Moot now that the section-shape check only lives at the writer's own type assertion, which only runs after a successful parse — a real read/stat error still propagates through its original, accurate wrapped-error path unchanged. 3. should-fix: GetStatus (via findEntryJSONBytes/findEntryTOMLBytes) had the same "key present, wrong type" blind spot as the write path, reporting a plain "not connected" for a config Preview/Connect now refuse to touch. Both now classify consistently as accessMalformed. Also fixed a genuine, PRE-EXISTING crash the review surfaced while checking this diff: a config file containing exactly the JSON literal `null` decodes successfully with a nil top-level map (encoding/json leaves the target untouched for a JSON null), and connectJSON's final `data[serversKey] = serversMap` assignment panicked with "assignment to entry in nil map" — reachable through the plain tokenless Connect() path, no precondition token involved. readOrCreateJSON/readOrCreateTOML now normalize a nil top-level document to an empty map. New tests: a TOCTOU race test using the readFile seam to prove the fix isn't just a single upstream check, a GetStatus consistency test, a null-document regression test, and an I/O-error-not-masked test. Tightened the round-1 tests to assert the exact (nil result, non-nil error) contract instead of loosely checking res.Success. Co-Authored-By: Claude Sonnet 5 --- internal/connect/connect.go | 83 +++++++++++---- internal/connect/token_test.go | 183 +++++++++++++++++++++++++++++++-- 2 files changed, 238 insertions(+), 28 deletions(-) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index 71a32b966..e9ae61ed2 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -490,20 +490,19 @@ func (s *Service) ConnectWithPrecondition(clientID, serverName string, force boo // and the write must cover the SAME entry — re-resolving per step is what // let a token hash one entry while the write replaced or deleted another // (Spec 091 FR-005). - fileExists, existing, accessState, err := s.preWriteState(client, cfgPath, serverName) + fileExists, existing, _, err := s.preWriteState(client, cfgPath, serverName) if err != nil { return nil, s.asAccessError(client, cfgPath, err) } - // A servers section that exists but is not an object never reaches the - // precondition token at all — the token only hashes the resolved ENTRY, so - // it cannot see the section's own raw value drifting. Refuse unconditionally - // (independent of whether a token was even echoed) rather than let connectJSON - // / connectTOML's own type assertion silently discard the value under a fresh - // map. This mirrors how a fully unparseable config already refuses. - if accessState == accessMalformed { - return nil, fmt.Errorf("%s could not be parsed as a valid config, or its %q section is not an object; refusing to write — fix the config manually and retry", cfgPath, client.ServerKey) - } + // 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 carry the authoritative, + // last-read check instead (see the comment at their servers-section type + // assertion), so this state is fully protected without racing. // Precondition check BEFORE any backup or write, so a refusal is completely // inert (Spec 091 FR-005). @@ -649,11 +648,23 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, err } - // Get or create the servers section + // Get or create the servers section. This is the AUTHORITATIVE check for a + // non-object section (Spec 091 FR-005 gap): data was just read fresh above, + // so — unlike a check earlier in the call chain — there is no window for the + // file to change between this read and the mutation below. A key that is + // PRESENT but not an object (a hand-edited string/number/array/bool) 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. serversKey := client.ServerKey - serversMap, ok := data[serversKey].(map[string]interface{}) - if !ok { + rawSection, keyPresent := data[serversKey] + var serversMap map[string]interface{} + if !keyPresent { serversMap = make(map[string]interface{}) + } else if m, ok := rawSection.(map[string]interface{}); ok { + serversMap = m + } else { + return nil, fmt.Errorf("%s: %q is not a JSON object; refusing to overwrite it — fix the config manually and retry", cfgPath, serversKey) } action := "created" @@ -823,14 +834,18 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for return nil, err } - // Get or create mcp_servers section - serversRaw, ok := data["mcp_servers"] + // Get or create mcp_servers section. See the equivalent comment in + // connectJSON: this is the authoritative, last-read check for a non-object + // section (Spec 091 FR-005 gap) — present-but-wrong-type must refuse, not + // silently fall through to a fresh empty table that discards the value. + rawSection, keyPresent := data["mcp_servers"] var serversMap map[string]interface{} - if ok { - serversMap, _ = serversRaw.(map[string]interface{}) - } - if serversMap == nil { + if !keyPresent { serversMap = make(map[string]interface{}) + } else if m, ok := rawSection.(map[string]interface{}); ok { + serversMap = m + } else { + return nil, fmt.Errorf("%s: %q is not a TOML table; refusing to overwrite it — fix the config manually and retry", cfgPath, "mcp_servers") } action := "created" @@ -991,6 +1006,14 @@ func (s *Service) readOrCreateJSON(path string) (map[string]interface{}, os.File if err := unmarshalLenientJSON(raw, &data); err != nil { return nil, perm, fmt.Errorf("parse JSON in %s: %w", path, err) } + if data == nil { + // A config file containing exactly the JSON literal `null` (or nested + // only in whitespace/comments that lenient-parse to null) decodes + // successfully with a nil top-level map — encoding/json leaves the target + // untouched for a JSON null. Treating that as "no top-level object yet" + // (same as a missing file) avoids a nil-map write panic below. + data = make(map[string]interface{}) + } return data, perm, nil } @@ -1016,6 +1039,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 } @@ -1121,10 +1150,17 @@ func (s *Service) findEntryJSONBytes(client ClientDef, raw []byte) (loc entryLoc return entryLocation{}, false, false } - serversMap, ok := data[client.ServerKey].(map[string]interface{}) - if !ok { + rawSection, keyPresent := data[client.ServerKey] + if !keyPresent { return entryLocation{}, false, true } + serversMap, ok := rawSection.(map[string]interface{}) + if !ok { + // 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 + } // Anchor on the credential-free base URL so both new clean entries and // legacy entries carrying a ?apikey= query are recognized (Spec 078). @@ -1318,7 +1354,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/token_test.go b/internal/connect/token_test.go index f50d15cba..2255a7ed4 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" @@ -471,8 +472,14 @@ func TestConnectWithPrecondition_NonObjectServersSection_RefusesDrift(t *testing writeFileT(t, cfgPath, drifted) res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, preview.PreconditionToken) - if err == nil && res != nil && res.Success { - t.Fatalf("connect must refuse when the servers section is not an object, got success: %+v", res) + 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) @@ -482,6 +489,47 @@ func TestConnectWithPrecondition_NonObjectServersSection_RefusesDrift(t *testing } } +// 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, "") + if reads < 2 { + t.Fatalf("expected the writer to perform its own independent read, only saw %d read(s)", 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) + } +} + // 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 @@ -495,8 +543,11 @@ func TestConnect_NonObjectServersSection_RefusesWithoutToken(t *testing.T) { writeFileT(t, cfgPath, original) res, err := svc.Connect("vscode", "mcpproxy", true) - if err == nil && res != nil && res.Success { - t.Fatalf("connect must refuse when the servers section is not an object, got success: %+v", res) + 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) @@ -513,8 +564,11 @@ func TestConnect_NonObjectServersSection_RefusesWithoutToken(t *testing.T) { writeFileT(t, cfgPath, original) res, err := svc.Connect("codex", "mcpproxy", true) - if err == nil && res != nil && res.Success { - t.Fatalf("connect must refuse when the servers section is not an object, got success: %+v", res) + 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) @@ -525,6 +579,123 @@ func TestConnect_NonObjectServersSection_RefusesWithoutToken(t *testing.T) { }) } +// 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) + } + }) +} + +// 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") + svc.setReadFile(func(path string) ([]byte, error) { + return nil, injected + }) + + _, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") + if err == nil { + t.Fatal("expected an error for an unreadable config") + } + if !errors.Is(err, injected) && !strings.Contains(err.Error(), injected.Error()) { + t.Fatalf("expected the genuine I/O error to propagate, 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 From 7aac1dca4eb9588723f89a9d06ee102685afc953 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:12:00 +0300 Subject: [PATCH 3/7] fix(connect): undo nil-map panic + test tightening from review round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the codex gpt-5.6-sol cross-model review of PR #1340 verified round 1's fixes and found one more genuine, in-scope bug plus two test nits: - must-fix: internal/connect/undo.go's replayConnectWrite has its own, independent JSON parse of the backup bytes (separate from readOrCreateJSON, which round 1 already fixed). A backup file containing exactly the JSON literal `null` hits the exact same nil-map reset encoding/json performs on a pre-initialized map target, but this path panicked at `data[client.ServerKey] = serversMap` instead — reachable via Connect (against a null-content config, which backs up the null bytes verbatim) followed by Undo. Same one-line guard as round 1's fix, applied here too. - nit: tightened the TOCTOU race test and the I/O-error test to assert the exact read count (2, not merely >=2) and require errors.Is() rather than an OR with a loose string match, pinning that they exercise the paths they claim to rather than passing for an unrelated reason. Two further must-fix findings from this round — a broader precondition-token TOCTOU (the token is checked against preWriteState's read but never re-validated against connectJSON/connectTOML's own later, independent read, so a same-shape entry that drifts in VALUE between the two reads can still be overwritten under force=true) and a sibling race in guardJsoncComments (OpenCode JSONC comments can be stripped by the same read/read gap) — were verified as genuine but PRE-EXISTING on main, unrelated to the non-object- section gap this PR set out to fix, and present since Spec 091 shipped. Given this PR's own scope was deliberately narrow specifically to avoid introducing a NEW drift-detection bug in this security-sensitive code, both are documented in place (see the comment block in ConnectWithPrecondition) and tracked as separate follow-up work rather than folded into this PR. Co-Authored-By: Claude Sonnet 5 --- internal/connect/connect.go | 23 ++++++++++++- internal/connect/token_test.go | 59 +++++++++++++++++++++++++++++++--- internal/connect/undo.go | 9 ++++++ 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index e9ae61ed2..a3f078f7e 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -502,7 +502,28 @@ func (s *Service) ConnectWithPrecondition(clientID, serverName string, force boo // non-object section before the write's own read runs, bypassing an // upstream-only guard. The write functions carry the authoritative, // last-read check instead (see the comment at their servers-section type - // assertion), so this state is fully protected without racing. + // assertion), so THAT specific drift class is fully protected without racing. + // + // 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). diff --git a/internal/connect/token_test.go b/internal/connect/token_test.go index 2255a7ed4..3b80b0d61 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -519,8 +519,13 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriter }) res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") - if reads < 2 { - t.Fatalf("expected the writer to perform its own independent read, only saw %d read(s)", reads) + // 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) @@ -528,6 +533,15 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriter 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) + } } // TestConnect_NonObjectServersSection_RefusesWithoutToken proves the guard does @@ -622,6 +636,33 @@ func TestConnect_NullTopLevelDocument_DoesNotPanic(t *testing.T) { }) } +// 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 @@ -680,16 +721,26 @@ func TestConnectWithPrecondition_GenuineIOErrorIsNotMaskedAsNonObjectSection(t * 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) && !strings.Contains(err.Error(), injected.Error()) { - t.Fatalf("expected the genuine I/O error to propagate, not be masked as a non-object section: %v", err) + 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) diff --git a/internal/connect/undo.go b/internal/connect/undo.go index 1127ea57e..8406d1be2 100644 --- a/internal/connect/undo.go +++ b/internal/connect/undo.go @@ -230,6 +230,15 @@ func (s *Service) replayConnectWrite(client *ClientDef, serverName string, backu if err := unmarshalLenientJSON(backupRaw, &data); err != nil { return nil, fmt.Errorf("parse backup JSON: %w", err) } + if data == nil { + // A backup containing exactly the JSON literal `null` unmarshals + // successfully but RESETS the pre-initialized `data` map above back to + // nil (encoding/json's null-into-pointer behavior applies even when the + // pointee already held a map) — the same nil-map class fixed in + // readOrCreateJSON, but here it panicked at the `data[client.ServerKey] + // = serversMap` assignment below instead. + data = make(map[string]interface{}) + } } serversMap, _ := data[client.ServerKey].(map[string]interface{}) if serversMap == nil { From 9aa37ec54ca088579a22bf9a6c5bc3c749507bee Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:22:37 +0300 Subject: [PATCH 4/7] fix(connect): close the remaining read-to-write race from review round 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 (codex gpt-5.6-sol) confirmed round 2's fixes (undo.go nil-map panic; tightened test assertions) and judged the two deferred pre-existing gaps (broader precondition-token TOCTOU; guardJsoncComments race) as genuinely orthogonal to this PR's guarantee, so deferring those was correct. But it found one more must-fix, and it's real: the "authoritative, last-read" comment on the servers-section type check in connectJSON/connectTOML overclaimed. That check runs right after readOrCreateJSON/readOrCreateTOML, but the actual disk write (atomicWriteFile) happens several lines later, after backupFile performs its own real file I/O. An external process could still replace the servers section with a non-object value in that gap and have it silently destroyed by the pending write — the exact harm this PR exists to prevent, just with a narrower window than before round 1. Fix: a second, minimal re-check (refuseIfServersSectionRaced) immediately before backupFile in both connectJSON and connectTOML, using a fresh read right at the point of committing. This shrinks the exploitable window to the residual gap between that check and atomicWriteFile's rename — eliminating it entirely would need an OS-level file lock held across the whole read-modify-write sequence, which is a larger change appropriately left to the same follow-up track as round 2's deferred findings, not bundled here. New test: TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalPreWriteCheck, using the read-file seam to make both the top-of-function check's reads observe an object-shaped section (so it passes) and only the new, later check observe the raced-in non-object value — proving the fix closes specifically the window round 3 identified, not just the one round 1 closed. Co-Authored-By: Claude Sonnet 5 --- internal/connect/connect.go | 88 +++++++++++++++++++++++++++++----- internal/connect/token_test.go | 54 +++++++++++++++++++++ 2 files changed, 131 insertions(+), 11 deletions(-) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index a3f078f7e..7c75c39a8 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -654,6 +654,50 @@ func (s *Service) guardJsoncComments(cfgPath string) error { return nil } +// refuseIfServersSectionRaced re-reads cfgPath and reports whether the +// servers section (serversKey, decoded per format — "json" or "toml") has +// become present-but-not-an-object since an earlier read. It is the second +// of two checks connectJSON/connectTOML run against the same drift class +// (Spec 091 FR-005 gap): the first, at the top of each function, uses the +// read those functions already need for their existence/force/adoption +// decisions, but that read is not adjacent to the actual write — backupFile +// performs its own real I/O afterward — so calling this again immediately +// before backup/write shrinks the exploitable window instead of leaving it +// at the width of the whole function body. +// +// 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(cfgPath, serversKey, format string) error { + raw, err := s.read(cfgPath) + if err != nil { + return nil + } + var data map[string]interface{} + if format == "toml" { + if _, derr := toml.Decode(string(raw), &data); derr != nil { + return nil + } + } else if derr := unmarshalLenientJSON(raw, &data); derr != nil { + return nil + } + rawSection, present := data[serversKey] + if !present { + return nil + } + if _, ok := rawSection.(map[string]interface{}); ok { + return nil + } + containerWord := "a JSON object" + if 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, serversKey, 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 @@ -669,14 +713,14 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, err } - // Get or create the servers section. This is the AUTHORITATIVE check for a - // non-object section (Spec 091 FR-005 gap): data was just read fresh above, - // so — unlike a check earlier in the call chain — there is no window for the - // file to change between this read and the mutation below. A key that is - // PRESENT but not an object (a hand-edited string/number/array/bool) 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. + // Get or create the servers section. A key that is PRESENT but not an + // object (a hand-edited string/number/array/bool) 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 TWO checks against this drift class — see the second one + // immediately before the backup/write below for why one alone is not + // authoritative. serversKey := client.ServerKey rawSection, keyPresent := data[serversKey] var serversMap map[string]interface{} @@ -724,6 +768,20 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for } } + // SECOND, closer-to-the-write check for the same drift class (Spec 091 + // FR-005 gap; round-3 cross-model review finding): the read at the top of + // this function is not actually adjacent to the write below — backupFile + // performs its own real file I/O in between, widening the window in which + // an external process could replace the servers section with a non-object + // value after this function already decided it was safe to proceed. A + // fresh, minimal re-check right here, immediately before backup/write, + // shrinks that window to the (unavoidable without an OS-level lock across + // the whole read-modify-write sequence) gap between THIS check and + // atomicWriteFile's rename. + if err := s.refuseIfServersSectionRaced(cfgPath, serversKey, "json"); err != nil { + return nil, err + } + // Create backup before modifying backupPath, err := backupFile(cfgPath) if err != nil { @@ -856,9 +914,10 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for } // Get or create mcp_servers section. See the equivalent comment in - // connectJSON: this is the authoritative, last-read check for a non-object - // section (Spec 091 FR-005 gap) — present-but-wrong-type must refuse, not - // silently fall through to a fresh empty table that discards the value. + // 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 two checks against this drift class — see the second, + // closer-to-the-write one below. rawSection, keyPresent := data["mcp_servers"] var serversMap map[string]interface{} if !keyPresent { @@ -884,6 +943,13 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for action = "updated" } + // Second, closer-to-the-write check — see the equivalent comment in + // connectJSON for why the check above alone leaves a window (backupFile's + // own I/O in between). + if err := s.refuseIfServersSectionRaced(cfgPath, "mcp_servers", "toml"); err != nil { + return nil, err + } + // Backup backupPath, err := backupFile(cfgPath) if err != nil { diff --git a/internal/connect/token_test.go b/internal/connect/token_test.go index 3b80b0d61..056162b5e 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -544,6 +544,60 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriter } } +// TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalPreWriteCheck +// closes the must-fix round-3 cross-model review found in the fix above: +// 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 SECOND, closer-to-backup/write 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_RaceIsClosedAtTheFinalPreWriteCheck(t *testing.T) { + svc, home := testService(t) + cfgPath := ConfigPath("claude-code", home) + const objectShaped = `{"mcpServers":{}}` + const racedNonObject = `{"mcpServers":"raced-in-during-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 backup/write. + 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 final pre-backup check), got %d", reads) + } + if err == nil { + t.Fatalf("expected the final pre-write 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 — the check must run BEFORE backupFile, found %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 From f48e7c534e8676e4af729b25d7b42a1edaf0f35e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:39:06 +0300 Subject: [PATCH 5/7] fix(connect): add post-backup pre-write check per review round 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 (codex gpt-5.6-sol) confirmed round 3's pre-backup check runs at the right point in both connectJSON/connectTOML with no intervening reads, and found no TOML-path bug — but flagged that the check alone still left a practically (not just theoretically) exploitable window: backupFile performs real Stat/Open/copy I/O, so a change landing DURING that backup — AFTER the pre-backup check already passed — could still slip through to atomicWriteFile undetected. Fix: a third refuseIfServersSectionRaced call, positioned after backupFile and marshaling, immediately before atomicWriteFile — as close to the actual write as this codebase's non-locking design allows. The residual gap between this final check and atomicWriteFile's own temp-write-then-rename is unavoidable without an OS-level file lock across the whole read-modify-write sequence (the same larger architectural change already correctly deferred for round 2's other findings), and is now the ONLY remaining, syscall-width window — no longer one wide enough to contain a real I/O operation. Also fixed an overclaiming comment in ConnectWithPrecondition ("fully protected without racing") that round 4 flagged as inconsistent with the acknowledged residual gap. Renamed the round-3 test to RaceIsClosedAtThePreBackupCheck for clarity against the new RaceIsClosedAfterBackup test, which simulates the race landing specifically during backupFile's I/O (reads #1-#3 see the object-shaped section, so a backup IS legitimately created; only the 4th, post-backup read sees the raced-in non-object value) and asserts the config file itself stays untouched despite the backup existing. Co-Authored-By: Claude Sonnet 5 --- internal/connect/connect.go | 78 ++++++++++++++++++++--------- internal/connect/token_test.go | 91 ++++++++++++++++++++++++++++------ 2 files changed, 129 insertions(+), 40 deletions(-) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index 7c75c39a8..c54619a60 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -500,9 +500,11 @@ func (s *Service) ConnectWithPrecondition(clientID, serverName string, force boo // 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 carry the authoritative, - // last-read check instead (see the comment at their servers-section type - // assertion), so THAT specific drift class is fully protected without racing. + // 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 @@ -656,14 +658,29 @@ func (s *Service) guardJsoncComments(cfgPath string) error { // refuseIfServersSectionRaced re-reads cfgPath and reports whether the // servers section (serversKey, decoded per format — "json" or "toml") has -// become present-but-not-an-object since an earlier read. It is the second -// of two checks connectJSON/connectTOML run against the same drift class -// (Spec 091 FR-005 gap): the first, at the top of each function, uses the -// read those functions already need for their existence/force/adoption -// decisions, but that read is not adjacent to the actual write — backupFile -// performs its own real I/O afterward — so calling this again immediately -// before backup/write shrinks the exploitable window instead of leaving it -// at the width of the whole function body. +// 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), because the function body's own read (used for their +// existence/force/adoption decisions) 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. +// - immediately before atomicWriteFile (after backupFile and marshaling): +// backupFile performs its own Stat/Open/copy — genuinely slow enough on +// a loaded filesystem to be practically raceable, per round-4 +// cross-model review — so a change landing DURING backup is caught by +// THIS second call rather than slipping through to the write. +// +// A residual gap remains between this second call and atomicWriteFile's own +// temp-file-write-then-rename — fully eliminating 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, @@ -768,16 +785,9 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for } } - // SECOND, closer-to-the-write check for the same drift class (Spec 091 - // FR-005 gap; round-3 cross-model review finding): the read at the top of - // this function is not actually adjacent to the write below — backupFile - // performs its own real file I/O in between, widening the window in which - // an external process could replace the servers section with a non-object - // value after this function already decided it was safe to proceed. A - // fresh, minimal re-check right here, immediately before backup/write, - // shrinks that window to the (unavoidable without an OS-level lock across - // the whole read-modify-write sequence) gap between THIS check and - // atomicWriteFile's rename. + // 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(cfgPath, serversKey, "json"); err != nil { return nil, err } @@ -800,6 +810,18 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("marshal config: %w", err) } + // THIRD check (round-4 cross-model review finding): 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. This + // re-check, as close as possible to the actual write, is what catches + // that window; see refuseIfServersSectionRaced's doc comment for the + // (unavoidable without OS-level locking) residual gap that remains + // between this point and atomicWriteFile's own rename. + if err := s.refuseIfServersSectionRaced(cfgPath, serversKey, "json"); err != nil { + return nil, err + } + if err := atomicWriteFile(cfgPath, encoded, perm); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -943,9 +965,9 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for action = "updated" } - // Second, closer-to-the-write check — see the equivalent comment in - // connectJSON for why the check above alone leaves a window (backupFile's - // own I/O in between). + // 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(cfgPath, "mcp_servers", "toml"); err != nil { return nil, err } @@ -969,6 +991,14 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("encode TOML: %w", err) } + // Third check (round-4 cross-model review finding): backupFile above just + // performed real I/O, so re-check as close as possible to the actual + // write — see connectJSON's equivalent comment for the residual gap that + // remains between this point and atomicWriteFile's own rename. + if err := s.refuseIfServersSectionRaced(cfgPath, "mcp_servers", "toml"); err != nil { + return nil, err + } + if err := atomicWriteFile(cfgPath, buf.Bytes(), perm); err != nil { return nil, fmt.Errorf("write config: %w", err) } diff --git a/internal/connect/token_test.go b/internal/connect/token_test.go index 056162b5e..41d375446 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -544,23 +544,23 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheWriter } } -// TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalPreWriteCheck -// closes the must-fix round-3 cross-model review found in the fix above: -// 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 +// 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 SECOND, closer-to-backup/write 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_RaceIsClosedAtTheFinalPreWriteCheck(t *testing.T) { +// 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-during-backup"}` + const racedNonObject = `{"mcpServers":"raced-in-before-backup"}` writeFileT(t, cfgPath, objectShaped) reads := 0 @@ -569,7 +569,7 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalP 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 backup/write. + // passes and the function proceeds toward the pre-backup check. return []byte(objectShaped), nil } // The THIRD read — refuseIfServersSectionRaced, immediately before @@ -579,10 +579,10 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalP res, err := svc.ConnectWithPrecondition("claude-code", "mcpproxy", true, "") if reads != 3 { - t.Fatalf("expected exactly 3 reads (preWriteState + the writer's own + the final pre-backup check), got %d", reads) + 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 final pre-write check to catch the raced-in non-object section, got res=%+v err=nil", res) + 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) @@ -594,7 +594,66 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtTheFinalP 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 — the check must run BEFORE backupFile, found %d", n) + 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 in the fix above: +// 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 THIRD, post-backup/pre-write 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). +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) } } From 4eb15b895b31e0ab9c1bef1195bd066267c1fcde Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:46:23 +0300 Subject: [PATCH 6/7] fix(connect): move final race check inside atomicWriteFile's rename step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 (codex gpt-5.6-sol) found round 4's "immediately before atomicWriteFile" placement for the third refuseIfServersSectionRaced call still left a real, I/O-bearing window: atomicWriteFile itself stages a temp file (MkdirAll, CreateTemp, Write, Close, Chmod — all real filesystem operations) before the rename that actually replaces the target file, and none of that staging was covered by the check. Fix: atomicWriteFile now accepts an optional preRename func() error, invoked after all staging completes and immediately before os.Rename — the true last point at which the write can still be aborted. connectJSON and connectTOML pass their section-race check as this hook instead of calling it themselves before atomicWriteFile. The other three call sites (disconnectJSON, disconnectTOML, undo.go's restore, and the direct unit test) pass nil, unaffected. This closes the residual down to a handful of fast local syscalls between the hook and the rename itself — the practical floor without an OS-level file lock across the whole read-modify-write sequence, which remains tracked as a separate, larger architectural change per the round-2 deferral. Also fixed two stale "first of TWO checks" comments round 5 flagged (now three: the top-of-function type assertion, the pre-backup fast-fail, and this new preRename hook), and updated refuseIfServersSectionRaced's and the round-4 test's doc comments to describe the corrected placement — the existing read-counting test's assertions were unaffected (none of atomicWriteFile's temp-file staging touches the s.read seam it mocks), but its comment previously implied a placement precision the black-box test can't itself prove. Co-Authored-By: Claude Sonnet 5 --- internal/connect/backup.go | 24 ++++++++- internal/connect/connect.go | 90 +++++++++++++++++--------------- internal/connect/connect_test.go | 2 +- internal/connect/token_test.go | 38 +++++++++----- internal/connect/undo.go | 2 +- 5 files changed, 98 insertions(+), 58 deletions(-) diff --git a/internal/connect/backup.go b/internal/connect/backup.go index 00d6ba55d..9728d5293 100644 --- a/internal/connect/backup.go +++ b/internal/connect/backup.go @@ -68,7 +68,23 @@ 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 — a few fast local syscalls, +// not a real I/O operation an external writer could plausibly land inside. +func atomicWriteFile(path string, data []byte, perm os.FileMode, preRename func() error) error { dir := filepath.Dir(path) // Ensure the directory exists @@ -102,6 +118,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/connect.go b/internal/connect/connect.go index c54619a60..f3ced2101 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -660,27 +660,33 @@ func (s *Service) guardJsoncComments(cfgPath string) error { // servers section (serversKey, decoded per format — "json" or "toml") 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), because the function body's own read (used for their -// existence/force/adoption decisions) is not adjacent to the actual write — -// several real I/O steps happen in between: +// 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. -// - immediately before atomicWriteFile (after backupFile and marshaling): -// backupFile performs its own Stat/Open/copy — genuinely slow enough on -// a loaded filesystem to be practically raceable, per round-4 -// cross-model review — so a change landing DURING backup is caught by -// THIS second call rather than slipping through to the write. +// - 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 and atomicWriteFile's own -// temp-file-write-then-rename — fully eliminating 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). +// A residual gap remains between this second call (inside atomicWriteFile, +// immediately before os.Rename) and the rename itself — a handful of fast +// local syscalls, not a real I/O operation. 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, @@ -735,9 +741,9 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for // 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 TWO checks against this drift class — see the second one - // immediately before the backup/write below for why one alone is not - // authoritative. + // 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. serversKey := client.ServerKey rawSection, keyPresent := data[serversKey] var serversMap map[string]interface{} @@ -810,19 +816,19 @@ func (s *Service) connectJSON(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("marshal config: %w", err) } - // THIRD check (round-4 cross-model review finding): 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. This - // re-check, as close as possible to the actual write, is what catches - // that window; see refuseIfServersSectionRaced's doc comment for the - // (unavoidable without OS-level locking) residual gap that remains - // between this point and atomicWriteFile's own rename. - if err := s.refuseIfServersSectionRaced(cfgPath, serversKey, "json"); err != nil { - return nil, 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(cfgPath, serversKey, "json") + }); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -911,7 +917,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) } @@ -938,8 +944,9 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for // 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 two checks against this drift class — see the second, - // closer-to-the-write one below. + // first of three checks against this drift class — see the second, + // pre-backup one below and the third, inside atomicWriteFile's preRename + // hook. rawSection, keyPresent := data["mcp_servers"] var serversMap map[string]interface{} if !keyPresent { @@ -991,15 +998,12 @@ func (s *Service) connectTOML(client *ClientDef, cfgPath, serverName string, for return nil, fmt.Errorf("encode TOML: %w", err) } - // Third check (round-4 cross-model review finding): backupFile above just - // performed real I/O, so re-check as close as possible to the actual - // write — see connectJSON's equivalent comment for the residual gap that - // remains between this point and atomicWriteFile's own rename. - if err := s.refuseIfServersSectionRaced(cfgPath, "mcp_servers", "toml"); err != nil { - return nil, 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(cfgPath, "mcp_servers", "toml") + }); err != nil { return nil, fmt.Errorf("write config: %w", err) } @@ -1084,7 +1088,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) } diff --git a/internal/connect/connect_test.go b/internal/connect/connect_test.go index 6edf13551..4e3802c83 100644 --- a/internal/connect/connect_test.go +++ b/internal/connect/connect_test.go @@ -1110,7 +1110,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/token_test.go b/internal/connect/token_test.go index 41d375446..e434250dc 100644 --- a/internal/connect/token_test.go +++ b/internal/connect/token_test.go @@ -599,18 +599,32 @@ func TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAtThePreBac } // TestConnectWithPrecondition_NonObjectServersSection_RaceIsClosedAfterBackup -// closes the must-fix round-4 cross-model review found in the fix above: -// 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 THIRD, post-backup/pre-write 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). +// 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) diff --git a/internal/connect/undo.go b/internal/connect/undo.go index 8406d1be2..a2e9ec4be 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) } From 65d12ecf87c19d94b0465c86769c3a1c392c7d0e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 19:04:21 +0300 Subject: [PATCH 7/7] docs(connect): soften overclaimed "not a real I/O operation" comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 review nit: os.Rename on Unix performs an Lstat before the actual rename/replace syscall, so the residual pre-rename window isn't literally zero I/O. Corrected the comment to say what's actually true — one fast local metadata lookup, not a copy or anything an external writer could practically race against — without changing any behavior. Round 6 (codex gpt-5.6-sol) verdict: CLEAN, merge-ready. Six rounds progressively closed a real TOCTOU from "no protection" down to this documented, irreducible platform-rename window, with two genuinely pre-existing/orthogonal architectural gaps deferred to follow-up tasks. Co-Authored-By: Claude Sonnet 5 --- internal/connect/backup.go | 7 +++++-- internal/connect/connect.go | 14 ++++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/connect/backup.go b/internal/connect/backup.go index 9728d5293..f006f5c6d 100644 --- a/internal/connect/backup.go +++ b/internal/connect/backup.go @@ -82,8 +82,11 @@ func backupFile(path string) (string, error) { // 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 — a few fast local syscalls, -// not a real I/O operation an external writer could plausibly land inside. +// 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) diff --git a/internal/connect/connect.go b/internal/connect/connect.go index f3ced2101..a8ee8b357 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -681,12 +681,14 @@ func (s *Service) guardJsoncComments(cfgPath string) error { // 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 — a handful of fast -// local syscalls, not a real I/O operation. 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). +// 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,