Conversation
… object
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 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
bcbe41f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c85779c1.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-eager-galileo-780a52.mcpproxy-docs.pages.dev |
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ift fix main gained PR #1339 (ZCode client, nested mcp.servers schema via serversMapPath/getServersMap/setServersMap) after this branch's round-6 CLEAN review verdict, touching the same call sites this PR's fix does — exactly the coordination trap flagged in project memory when both PRs were open simultaneously. Reconciled by extracting resolveServersMapState (internal/connect/clients.go) alongside the existing getServersMap/setServersMap: it distinguishes "key absent" (legitimate create) from "key present but not an object" (malformed — must refuse) along serversMapPath, generalizing this PR's flat-key-only distinction to ZCode's nested path too. getServersMap is now defined in terms of it (behavior-preserving — it already collapsed both non-found cases into one `bool`). Updated to use resolveServersMapState: resolveExistingEntry (preview.go), connectJSON/connectTOML's top-of-function check, and findEntryJSONBytes (GetStatus consistency). refuseIfServersSectionRaced (the 3-checks-deep race guard from rounds 3-5) now takes *ClientDef instead of a flat (serversKey, format) pair, so all three of its checks are nested-path-aware too — ZCode gets the full defense-in-depth protection, not just the top-of-function one. Removed now-redundant nil-map guards in readOrCreateJSON and undo.go's replayConnectWrite: #1339 added a more centralized fix (normalizeNilConfigMap inside unmarshalLenientJSON itself) that covers the same cases more thoroughly than this PR's original per-call-site guards. Added zcode coverage to fulfill the original task's request for "at least one flat-key client and zcode" test coverage, which wasn't possible when this PR was first written (zcode didn't exist in this codebase yet): non-object-section detection at both the leaf (mcp.servers) and intermediate (mcp) nesting levels, in both Preview and the tokenless Connect() write path. All pre-merge tests (6 rounds of cross-model review) and all of #1339's own zcode tests pass unchanged. go build/vet/test -race/golangci-lint (both bare and --build-tags server) all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35752483604 --repo smart-mcp-proxy/mcpproxy-go
|
This was referenced Sep 23, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes a Spec 091 FR-005 precondition-token drift-detection gap flagged by a codex gpt-5.6-sol cross-model review of #1339 and reproduced independently against
main(pre-existing, not introduced by #1339; affects every flat-ServerKeyclient — claude-code, claude-desktop, cursor, windsurf, vscode, gemini, opencode, codex).The bug:
resolveExistingEntrytype-asserteddata[client.ServerKey]tomap[string]interface{}and, on failure, reported "no servers section present" — indistinguishable from the key being absent entirely. A hand-edited config whose servers section holds a non-object value (e.g.{"mcpServers":"old"}) therefore classified as a clean "this will create an entry" case in both preview and write:connectJSON/connectTOMLsilently replaced the non-object value with a fresh map containing just the new entry, destroying whatever was there — with the drift check never getting a chance to refuse.The fix (final shape, after 6 rounds of cross-model review — see the log below): treat "servers-section key present but not an object" as a distinct, refusable state, enforced with defense-in-depth at every I/O-bearing step of the write:
resolveExistingEntry(internal/connect/preview.go) separates key-absent (legitimate create) from key-present-with-wrong-type (accessMalformed), soPreview()/GetStatus()report the correct state.connectJSON/connectTOML(internal/connect/connect.go) check the servers-section type three times, each closing a progressively narrower window a prior round's placement still left open:backupFile(a fast-fail, and closes the gap between check 1 andbackupFile's own real I/O).atomicWriteFile's newpreRenamehook — invoked after all temp-file staging (MkdirAll/CreateTemp/Write/Close/Chmod) and immediately beforeos.Rename, the true last point the write can still be aborted.Lstatos.Renameperforms internally on Unix before the actual rename syscall — a documented, irreducible platform-level window, not a copy or anything an external writer could meaningfully race against. Fully eliminating even that needs an OS-level file lock across the whole read-modify-write sequence, which is a separate, larger architectural change (see deferred follow-ups below).nulldecodes with a nil top-level map, and the original write code panicked (assignment to entry in nil map). Fixed with a nil-normalization guard inreadOrCreateJSON/readOrCreateTOMLandundo.go'sreplayConnectWrite.Both JSON and TOML clients share these code paths, so the fix covers both formats from one change.
Cross-model review log (codex gpt-5.6-sol — opencode's Copilot quota was account-wide exhausted both times this PR needed a reviewer, confirmed by probing Sol and Terra before falling back per CLAUDE.md's ladder)
GetStatus/DisconnectinconsistencyconnectJSON/connectTOMLthemselves; fixedfindEntryJSONBytes/findEntryTOMLBytesclassificationundo.go; 2 test nitsreplayConnectWrite; tightened test assertions. Judged a broader precondition-token TOCTOU and aguardJsoncCommentsrace as genuinely pre-existing/orthogonal — deferred to follow-up tasks (confirmed correct scope call in round 3)backupFile's own I/O still left a window before the checkbackupFilebackupFile's I/O is practically (not just theoretically) raceableatomicWriteFileatomicWriteFile's own temp-file staging was still an unchecked windowatomicWriteFile'spreRenamehook, run after staging, immediately beforeos.RenameTwo out-of-scope, pre-existing gaps were deliberately deferred rather than folded in (per this PR's original framing that security-sensitive precondition-token changes deserve their own dedicated review, not scope creep) — follow-up tasks filed:
preWriteState's read but never re-validated against the writer's own later, independent read (present onmainsince Spec 091 shipped).guardJsoncComments/readOrCreateJSONindependent-reads race for OpenCode's JSONC comment preservation (connect opencode fails: OpenCode bootstraps opencode.jsonc, mcpproxy only recognizes opencode.json #922). (Update: a separate session independently fixed this on its own branch while this PR was in review — unrelated to this PR, noted for visibility.)Post-round-6 reconciliation with #1339 (ZCode)
Between round 6's CLEAN verdict and merge,
mainmerged #1339 (ZCode client support), which touches the exact same call sites this fix does — ZCode's config nests its servers map two levels deep ({"mcp":{"servers":{...}}}) instead of a flat top-level key, so #1339 introducedserversMapPath/getServersMap/setServersMapinclients.go, replacing every flatdata[client.ServerKey]access.Rebased and reconciled: added
resolveServersMapState(clients.go), generalizing this PR's "absent vs. present-but-wrong-type" distinction to walkserversMapPathlevel by level, so it classifies malformed correctly at either the leaf (mcp.serversitself) or an intermediate level (mcpitself) of a nested path.getServersMapis now defined in terms of it (behavior-preserving for every untouched caller). Every call site needing the distinction —resolveExistingEntry,connectJSON/connectTOML's top check,findEntryJSONBytes— and the 3-checks-deep race guardrefuseIfServersSectionRaced(now taking*ClientDefinstead of a flat key) were updated, so ZCode gets the full defense-in-depth protection from all 3 rounds' worth of checks, not just a partial one. Also removed this PR's now-redundant nil-map guards inreadOrCreateJSON/undo.go, since #1339 added a more centralized fix (normalizeNilConfigMapinsideunmarshalLenientJSON) covering the same cases.Added the ZCode test coverage the original task requested but couldn't get when this PR was first written (ZCode didn't exist yet): non-object detection at both the leaf and intermediate nesting levels, in Preview and the tokenless
Connect()write path.Round 7 re-reviewed this reconciliation specifically (full-file reads of
clients.go/connect.go/preview.go/undo.go, not just the noisy two-branch diff) — traced the nested-path walk by hand, verifiedgetServersMap's behavior-preservation for every untouched caller, confirmed all 3refuseIfServersSectionRacedcheckpoints pass the same*ClientDefso ZCode gets checked at every one, and confirmednormalizeNilConfigMapfully covers both removed guards. No findings — CLEAN.Test plan
GetStatusconsistency, I/O-error non-masking, and ZCode's nested path at both nesting levels)go test -race ./internal/connect/... ./internal/httpapi/...— pass, no existing test regressed (including all of feat(connect): add ZCode as a supported Connect client #1339's own ZCode tests, unchanged)go vet ./internal/connect/...golangci-lint run --config .github/.golangci.yml ./internal/connect/...and--build-tags servervariant — 0 issuesgo build ./...🤖 Generated with Claude Code