feat(connect): one-click undo that restores the pre-connect client config (Spec 078 US3)

[Dumbris]

Motivation

Spec 078 US3 (specs/078-connect-trust-preview) — roadmap task connect-trust-undo, the last piece of the connect-trust epic ("Connect step trust: preview, visible backup, one-click undo"). Telemetry shows 72.4% of wizard-engaged users skip the connect step; completers retain ~50% at two weeks vs 6% for non-engaged. US1 (#802) previews the exact change and US2 (#799) surfaces the backup path — this PR closes the loop by making the connect fully reversible from the same surface that made it.

Implementation

• internal/connect/undo.go — new Service.Undo(clientID, serverName, backupPath):
  • backupPath != "": restores the client config byte-for-byte from the backup the preceding connect took — the only revert that can bring back a pre-existing same-named entry a force-connect overwrote (surgical disconnect cannot).
  • backupPath == "": the connect created the file; undo deletes it, restoring the pre-connect "no file" state.
  • Drift check (refuse, never clobber): replayConnectWrite reconstructs, from the backup bytes, the exact bytes the connect wrote (same buildServerEntry + same JSON/TOML marshal, incl. OpenCode adopted-name normalization) and requires the CURRENT file to be byte-identical; any mismatch → Success=false, Action="conflict" (HTTP 409), file untouched. This intentionally also refuses when listen address / API key / require_mcp_auth changed since the connect.
  • Takes its own safety backup before every mutation (returned as the new backup_path); validates backupPath is a <config>.bak.* sibling of that client's config (undo is not an arbitrary-file-restore primitive); TCC denials map through the existing asAccessError → 403 + remediation.
• internal/connect/backup.go — backup names are now collision-proof: same-second operations get a -1/-2 numeric suffix via an O_EXCL loop (never overwrite). Fixes the case where connect+disconnect within one second destroyed the very backup a later undo needs. Existing <config>.bak.<ts> shape kept as prefix.
• internal/httpapi — POST /api/v1/connect/{client}/undo: 200 restored|deleted, 409 conflict on drift, 404 missing backup / unknown client, 403 + remediation on macOS App-Data denial (mirrors the other per-client connect routes).
• Web UI — wizard ClientRow Undo affordance on the session backup line + revert-preview panel (shows the entry that will be reverted before touching anything, FR-009: Undo/Keep); same session-scoped affordance on the ConnectModal result area. The affordance is deliberately session-scoped (in-memory last-connect), not persisted across reloads.
• Telemetry — connect_step completion is deliberately NOT retracted on undo: the funnel event records that the step was completed; undo reverts the file change, not the funnel transition (FR-016).

Codex review outcome

One full-diff Codex round (gpt-5.5, read-only, origin/main..HEAD; focus: path-validation bypass, JSON/TOML replay fidelity, error mapping, hot-reload races, spec violations, test gaps). Verdict: REQUEST_CHANGES with two High findings — both rebutted (rationale below); no code changes resulted. Flagging them here explicitly so the merger can weigh the rebuttals:

1. "Backup-path prefix check is bypassable via ../ traversal or a symlink named <config>.bak.<ts>" — Rebutted. The restore destination is always this client's own config path, never caller-controlled. The restore source must additionally pass the byte-exact drift replay (replayConnectWrite(source) == current file), so the only bytes undo can ever write are the semantically-determined pre-connect state of that config — a foreign file that satisfies the replay is content-equivalent to the legitimate backup. And the caller is already an authenticated localhost API client holding the key that can call connect/disconnect (i.e., mutate the same file) directly. Lexical filepath.Clean + Lstat regular-file hardening noted as a follow-up (out of scope below).
2. "Drift check is check-then-write with no per-config lock; a concurrent connect/undo could interleave" — Rebutted / accepted risk. Codex itself confirmed there is no hot-reload watcher on client config files, so the only writer in the window is a second concurrent REST call from the same single local user; every mutation path takes its own safety backup first, so nothing is unrecoverable. A per-client mutex is a reasonable hardening follow-up (out of scope below).
3. Test-gap notes: the frontend conflict tests mock undoConnectClient resolving {success:false} while the real wrapper throws on 409 — verified the components' catch path surfaces the server's conflict message identically, so the UI flow is covered end-to-end by the live smoke; traversal/symlink and concurrency unit tests deferred with the hardening follow-up.

LIVE verification

All 9 mandated live checks passed against a real built binary (isolated --config/--data-dir, scratch HOME=/tmp/vf078u, port :18096):

• make build exit 0 (v0.47.0-rc.2, commit 8b5a7f50 in ldflags); /readyz = 200.
• Force-connect over a user-owned mcpproxy entry in a seeded .claude.json, then undo via curl POST /api/v1/connect/claude-code/undo with the connect's backup_path → Action="restored", restored file byte-for-byte identical (diff clean) to the pre-connect original, including the overwritten same-named entry.
• Drift case: hand-edit the config after connect, then undo → HTTP 409 Action="conflict", file untouched.
• No-prior-file case: connect creates the config, undo with empty backup_path → Action="deleted", file removed, safety copy left behind.
• Go: byte-identical restore (JSON + TOML), drift refusal (with and without a prior file), missing/foreign backup rejection, deterministic same-second backup-name collision; httptest for the endpoint incl. 409/404/403.
• vitest: wizard + modal undo flows (revert panel first, honest refusal, session scoping) — 256 tests green.

Docs updated

• docs/api/rest-api.md — preview + undo endpoints, backup naming/retention; also removed the now-stale claim that GET /connect/{client} is the sole Connect endpoint that opens a client config (preview and undo also read it on demand — the wording contradicted the surrounding section).
• docs/setup.md — wizard callout.
• oas/swagger.yaml / oas/docs.go — regenerated for the new endpoint.

Out of scope

• Per-client mutex serializing connect/disconnect/undo (closes the check-then-write window from Codex finding 2).
• Stricter lexical/symlink backup-path validation (filepath.Clean, same-dir + Lstat regular-file check) from Codex finding 1.
• CLI subcommand for undo — this ships REST + Web UI only.
• Backup retention/GC policy changes (naming documented; pruning unchanged).
• Persisting the undo affordance across page reloads (deliberately session-scoped).

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying mcpproxy-docs with    Cloudflare Pages

Latest commit:	a56dcf6
Status:	✅  Deploy successful!
Preview URL:	https://286c138c.mcpproxy-docs.pages.dev
Branch Preview URL:	https://feat-078-undo.mcpproxy-docs.pages.dev

View logs

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 73.17073% with 44 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/connect/undo.go	71.79%	17 Missing and 16 partials ⚠️
internal/httpapi/connect.go	75.00%	6 Missing and 3 partials ⚠️
internal/connect/backup.go	80.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

📦 Build Artifacts

Workflow Run: View Run
Branch: feat/078-undo

Available Artifacts

• archive-darwin-amd64 (28 MB)
• archive-darwin-arm64 (25 MB)
• archive-linux-amd64 (16 MB)
• archive-linux-arm64 (15 MB)
• archive-windows-amd64 (28 MB)
• archive-windows-arm64 (25 MB)
• frontend-dist-pr (0 MB)
• installer-dmg-darwin-amd64 (22 MB)
• installer-dmg-darwin-arm64 (19 MB)

How to Download

Option 1: GitHub Web UI (easiest)

1. Go to the workflow run page linked above
2. Scroll to the bottom "Artifacts" section
3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 28642517636 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

[Dumbris]

Final Codex review (post-merge state @ 50e1c08)

Ran codex exec --sandbox read-only over the full git diff origin/main...HEAD, then verified each finding against the code. Verdict: CHANGES_MADE — 1 real issue fixed, 2 rebutted.

Applied

• major — backup-path traversal in undo (internal/connect/undo.go:55): the guard was prefix-only, so <config>.bak.x/../../secret.json kept the <config>.bak. prefix yet escaped the config dir — undo would read (and in a crafted case restore) an arbitrary file, defeating the documented "not an arbitrary-file-restore primitive" invariant. Confirmed exploitable with a probe test (traversal accepted, foreign file read). Fixed by anchoring validation on the cleaned path's parent dir + basename. TDD: TestUndo_RejectsBackupPathTraversal (red→green). Commit 50e1c08.

Rebutted

• "blocker" — TOCTOU between drift-check and restore (undo.go:108): not a blocker. The safety backup (backupFile(cfgPath), line 121) copies the current on-disk file immediately before the restore, so any concurrent edit is preserved in safetyPath and returned as backup_path — no permanent data loss. For a local single-user config, a check-then-act window backstopped by a safety copy is acceptable; a file lock would be over-engineering.
• "major" — non-atomic safety backup (backup.go:51): rebutted. A partial/corrupt backup fails safe — replayConnectWrite on truncated content either errors or yields a byte mismatch → 409 conflict, never a silent corrupt restore. And connect returns backup_path only after backupFile succeeds, so no legitimate reference to a partial file exists.

Clean areas (codex-confirmed + independently verified)

• OpenAPI regen: parsed-YAML diff vs origin/main shows removed_ops [], removed_schemas [], only POST /connect/{client}/undo + UndoConnectRequest added. make swagger-verify passes (spec in sync with code). No annotations lost from either merge side.
• Frontend: undo state reset on connect/disconnect/close/reopen/after-undo in both ConnectModal.vue and OnboardingWizard.vue; telemetry completion intentionally preserved on undo (FR-016).
• Tests: byte-identical restore (JSON+TOML), drift refusal, no-prior-file delete, missing-backup 404, foreign-path reject, same-second collision, HTTP status mapping, 10 vitest specs — all pass.

Verification on 50e1c08: go test -race ./internal/connect/... ./internal/httpapi/... ✅ · golangci-lint v2 0 issues ✅ · vitest 10/10 ✅

[Dumbris]

CodeQL-driven hardening — undo now takes a validated basename (77aa802)

Follow-up to the traversal fix: CodeQL flagged 1 HIGH (path injection at the connect.read() seam — the undo request's backup_path flowed into os.ReadFile). Rather than sanitize a client-supplied path, the request no longer carries one.

What changed

• The undo request now carries backup_name (renamed from backup_path) — the bare filename of the backup the connect returned, never a path. The server resolves the full path itself by joining it with this client's own config directory (from the client registry, not the request), after rejecting anything whose filepath.Base differs from the input and requiring the strict <config-basename>.bak. prefix. User input can no longer contribute a directory component — traversal is impossible by construction, and the taint path breaks at the Base()==input guard + constant-dir join.
• Frontend: api.undoConnectClient sends filepath.Base (strips both / and \), so both the wizard and ConnectModal call sites emit a bare name. New connect-undo-api.spec.ts asserts the wire payload (backup_name, Windows-path stripping, null→empty).
• Docs + OpenAPI regenerated for backup_name.

Test coverage (per the new model): absolute path rejected, separators-in-name rejected, unknown basename → 404, path-shaped name → 400 at the HTTP boundary; existing byte-identical restore / drift-refusal / no-prior-file-delete retained.

Verified on 77aa802: go test -race ./internal/connect/... ./internal/httpapi/... ✅ · golangci-lint v2 0 issues ✅ · vue-tsc --noEmit ✅ · full vitest 260/260 ✅ · make swagger in sync ✅. CodeQL re-runs on this push.