Skip to content

Commit 16c0b27

Browse files
Dumbrisclaude
andauthored
feat(connect): one-click undo that restores the pre-connect client config (Spec 078 US3) (#804)
* feat(connect): one-click undo that restores the pre-connect config (Spec 078 US3) The connect step's change is now fully reversible from the surface that made it: the wizard row and the standalone Connect modal gain a one-click Undo next to the backup path they surface. Undo restores the client config byte-for-byte from the backup the connect took — the only revert that can bring back a pre-existing same-named entry a force-connect overwrote (surgical disconnect cannot) — or removes the file when the connect created it. Before reverting, the change to be undone is shown again (FR-009), and the restore refuses with a clear 409 when the file changed since the connect, never clobbering later edits. ## Changes - internal/connect: Service.Undo — byte-identical restore from the connect backup; drift check replays the connect write from the backup and compares bytes (refuses on any mismatch); no-prior-file case deletes the created file; own safety backup before every mutation; backup paths validated to <config>.bak.* of that client only; TCC denials map to *AccessError - internal/connect: backup names are now collision-proof — same-second operations get a -1/-2 numeric suffix (O_EXCL, never overwrite), fixing the case where connect+disconnect within one second destroyed the very backup a later undo needs; existing <config>.bak.<ts> shape kept as prefix - 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, Undo/Keep); same session-scoped affordance on the ConnectModal result area - 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) - Docs: rest-api.md (preview + undo endpoints, backup naming/retention), setup.md wizard callout; OpenAPI regenerated ## Testing - Go: byte-identical restore (JSON + TOML), drift refusal (with and without a prior file), no-prior-file delete, missing/foreign backup, deterministic same-second backup collision; httptest for the endpoint incl. 409/404/403 - vitest: wizard + modal undo flows (revert panel first, honest refusal, session scoping) — 256 tests green - Live smoke on :18096 (scratch HOME): force-connect over a user-owned mcpproxy entry, undo via curl, diff byte-for-byte; drift edit → HTTP 409, file untouched * docs(api): drop stale 'sole endpoint' App-Data prompt claims now that preview/undo also read configs The per-client status doc claimed GET /connect/{client} is the only Connect endpoint that opens a client config file; the preview and undo routes (documented in the same section) also read it on demand, so the 'sole/only place' wording contradicted the surrounding text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(connect): reject traversal backup paths in undo (defense-in-depth) The undo backup-path guard was prefix-only: a path like "<config>.bak.x/../../secret.json" kept the "<config>.bak." prefix yet escaped the config directory, letting undo read (and, in a crafted case, restore) an arbitrary file — defeating the documented "must not become an arbitrary-file-restore primitive" invariant. Anchor validation on the cleaned path's parent directory and basename instead, both traversal-proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(connect): undo takes a validated backup basename, path resolved server-side CodeQL flagged path injection at the connect read() seam: the undo request's user-supplied backup_path flowed into os.ReadFile. Rather than sanitize a client-controlled path, stop trusting one entirely. The request now carries only backup_name — the bare filename of the backup the connect returned. Undo resolves the full path itself by joining it with THIS client's own config directory (derived from the client registry, never from the request), after rejecting anything whose filepath.Base differs from the input and requiring the strict "<config-basename>.bak." prefix. The user input can no longer contribute a directory component, so traversal is impossible by construction and the taint path breaks at the Base()==input guard + constant-dir join. - API: UndoConnectRequest.BackupPath -> BackupName (json backup_name); server resolves + validates; unknown basename still 404, path-shaped name -> 400. - Frontend: api.undoConnectClient sends filepath.Base (strips / and \) so both wizard and ConnectModal call sites emit a bare name; new connect-undo-api spec asserts the wire payload. - Docs + OpenAPI regenerated for backup_name. - Tests: absolute path rejected, separators-in-name rejected, unknown name 404, path-shaped name -> 400 at the HTTP boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent ed42d46 commit 16c0b27

16 files changed

Lines changed: 1982 additions & 33 deletions

File tree

docs/api/rest-api.md

Lines changed: 65 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -661,17 +661,18 @@ additive-compatible and gains two fields:
661661
A client that is installed but not yet content-checked reads as
662662
`exists=true, connected=false, access_state="unknown"`. Resolving `connected`
663663
requires an explicit per-client read (the per-client status route below,
664-
connect/disconnect, or the CLI `mcpproxy connect` command), which is the only
665-
place a privacy prompt may legitimately appear.
664+
connect/disconnect, or the CLI `mcpproxy connect` command), which is where a
665+
privacy prompt may legitimately appear.
666666

667667
#### GET /api/v1/connect/{client}
668668

669669
On-demand single-client status. Reads the one client's config **at request
670670
time** and returns a full `ClientStatus` with `access_state` resolved to
671671
`accessible | absent | malformed | denied` and `connected` set accordingly.
672-
This is the sole Connect endpoint that opens a client config file, so on macOS
673-
it is the only place an App-Data privacy prompt may legitimately appear (scoped
674-
to this user action). Unknown client → `404`. A denial is reported **in-band**
672+
This — like the other per-client routes below (preview, connect/disconnect,
673+
undo) — opens the client's config file at request time, so on macOS an App-Data
674+
privacy prompt may legitimately appear here (scoped to this user action), never
675+
from the overall listing. Unknown client → `404`. A denial is reported **in-band**
675676
(`200` with `access_state="denied"` + `remediation`), not as an HTTP error.
676677

677678
```bash
@@ -684,6 +685,63 @@ Connect/disconnect are unchanged except that a permission-denied config access
684685
now returns **`403 Forbidden`** whose error body carries the remediation text
685686
(distinct from a generic `400` or a `404` not-found).
686687

688+
Every connect/disconnect that modifies an **existing** config file first writes
689+
a timestamped backup next to it (`<config>.bak.<YYYYMMDD-HHMMSS>`, same
690+
directory and file mode) and returns its path as `backup_path` in the result.
691+
When two operations land in the same second, a numeric suffix keeps every
692+
backup distinct (`<config>.bak.<YYYYMMDD-HHMMSS>-1`, `-2`, …) — a backup is
693+
never overwritten. Backups accumulate one per operation and are **never
694+
deleted automatically**; there is no retention bound, so an undo (below) can
695+
always find its backup.
696+
697+
#### GET /api/v1/connect/{client}/preview
698+
699+
Returns the exact change a subsequent connect would make — target config path,
700+
format (`json`/`toml`), server key, entry name, and the exact entry contents —
701+
**without** modifying the file or creating a backup (Spec 078 US1). An embedded
702+
API key is masked in the payload (`contains_api_key` flags that a credential is
703+
written); `entry_exists` distinguishes a create from an overwrite of a
704+
same-named entry. Reads the config on demand to classify create-vs-overwrite,
705+
so on macOS this may raise an App-Data prompt; a denial returns `403` +
706+
remediation. Optional `?server_name=` mirrors the name a subsequent connect
707+
would use.
708+
709+
#### POST /api/v1/connect/{client}/undo
710+
711+
One-click undo of the immediately-preceding connect (Spec 078 US3). Body:
712+
713+
```json
714+
{ "server_name": "mcpproxy", "backup_name": "<basename of backup_path from the connect result>" }
715+
```
716+
717+
`backup_name` is the **bare filename** of the backup the connect returned in
718+
`backup_path` — a name, never a path. The server resolves the full path itself
719+
inside that client's own config directory (derived from the client registry, not
720+
the request), so a caller-supplied value can never contribute a directory
721+
component and cannot escape the config dir (defense against path injection).
722+
723+
- **`backup_name` set** — restores the config **byte-for-byte** from that
724+
backup. This is the only revert that can bring back a pre-existing
725+
same-named entry that a `force=true` connect overwrote (surgical
726+
`DELETE /connect/{client}` cannot).
727+
- **`backup_name` empty** — the connect created the file (its result carried no
728+
`backup_path`); undo deletes the created file, restoring the "no file" state.
729+
730+
Safety semantics:
731+
732+
- Undo **refuses with `409 Conflict`** when the config changed since the
733+
connect (it verifies the current file is byte-identical to what that connect
734+
wrote) — it never clobbers later edits. Fall back to
735+
`DELETE /connect/{client}` for a surgical entry removal.
736+
- A vanished backup returns `404`; a `backup_name` that is a path (contains a
737+
directory separator) or does not match `<config>.bak.*` for that client
738+
returns `400`.
739+
- Undo takes its **own safety backup** of the current file before restoring or
740+
deleting, returned as `backup_path` in the result
741+
(`action` = `restored` or `deleted`).
742+
- A macOS App-Data denial returns `403` + remediation, like the other
743+
per-client routes.
744+
687745
##### macOS App Data privacy & Connect
688746

689747
On macOS, client configs (Claude Desktop, Cursor, VS Code, …) live under another
@@ -698,7 +756,8 @@ tccutil reset SystemPolicyAppData com.smartmcpproxy.mcpproxy
698756
```
699757

700758
The overall `GET /api/v1/connect` listing never triggers this prompt (it is
701-
content-read-free); only the per-client read above can.
759+
content-read-free); only the per-client routes above (status, preview,
760+
connect/disconnect, undo) can.
702761

703762
### Real-time Updates
704763

docs/setup.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ Add mcpproxy as a remote MCP server via Settings → Connectors → Add Custom C
254254

255255
#### Option A: Free Plan — JSON Configuration
256256

257-
> **💡 Built-in wizard:** mcpproxy's **Connect** wizard (Web UI / tray) can write this bridge configuration for you — pick **Claude Desktop**, click **Review & connect** to see the exact entry that will be written (a timestamped backup is created first), then confirm with **Connect**. It registers the `npx -y mcp-remote` bridge shown below (Node.js required). The manual steps remain available if you prefer to edit the file yourself.
257+
> **💡 Built-in wizard:** mcpproxy's **Connect** wizard (Web UI / tray) can write this bridge configuration for you — pick **Claude Desktop**, click **Review & connect** to see the exact entry that will be written (a timestamped backup is created first), then confirm with **Connect**. It registers the `npx -y mcp-remote` bridge shown below (Node.js required). The manual steps remain available if you prefer to edit the file yourself. Changed your mind? The wizard offers a one-click **Undo** right next to the backup path it just showed: it reverts the connect by restoring the config byte-for-byte from that backup (or removing the file if the connect created it), and it refuses — rather than clobbering your edits — if the file changed in the meantime. Backups are never overwritten: two operations in the same second get distinct names (`.bak.<timestamp>-1`, `-2`, …), and none are deleted automatically.
258258
259259
1. Create the config file if it doesn't exist:
260260

frontend/src/components/ConnectModal.vue

Lines changed: 138 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,69 @@
240240
>
241241
No prior config file existed, so no backup was needed.
242242
</div>
243+
<!-- Spec 078 US3: one-click undo of the connect just performed (session-
244+
scoped). Clicking Undo first shows the change to be reverted
245+
(FR-009); the restore only runs on the panel's confirm. -->
246+
<div
247+
v-if="resultSuccess && lastConnect && !undoPanelOpen"
248+
data-test="connect-undo-offer"
249+
class="mt-2 flex items-center justify-between gap-2 rounded-lg bg-base-200 px-3 py-2"
250+
>
251+
<span class="text-xs opacity-70">Changed your mind? You can revert this connect.</span>
252+
<button
253+
data-test="connect-undo"
254+
@click="undoPanelOpen = true"
255+
class="btn btn-ghost btn-xs shrink-0 text-error"
256+
:disabled="undoBusy"
257+
title="Revert this connect: restore the config to its pre-connect state"
258+
>
259+
Undo
260+
</button>
261+
</div>
262+
<div
263+
v-if="resultSuccess && lastConnect && undoPanelOpen"
264+
data-test="connect-undo-panel"
265+
class="mt-2 rounded-lg bg-base-200/60 border border-error/40 px-3 py-2 space-y-2"
266+
>
267+
<p class="text-xs opacity-70 leading-relaxed">
268+
<template v-if="lastConnect.backupPath">
269+
Undo restores
270+
<code class="font-mono text-[11px] break-all">{{ lastConnect.configPath }}</code>
271+
to its exact pre-connect state from the backup (a safety copy of the current file is saved first).
272+
</template>
273+
<template v-else>
274+
Undo removes
275+
<code class="font-mono text-[11px] break-all">{{ lastConnect.configPath }}</code>
276+
— it did not exist before mcpproxy connected (a safety copy is saved first).
277+
</template>
278+
</p>
279+
<div v-if="lastConnect.preview">
280+
<div class="text-[11px] font-semibold uppercase tracking-wider text-error/80 mb-1">− will be reverted</div>
281+
<pre
282+
data-test="connect-undo-entry"
283+
class="text-[11px] font-mono whitespace-pre-wrap break-all rounded bg-base-300/60 border-l-2 border-error px-2 py-1.5 leading-relaxed"
284+
>{{ lastConnect.preview.entry_text }}</pre>
285+
</div>
286+
<div class="flex items-center gap-2 pt-0.5">
287+
<button
288+
data-test="connect-undo-confirm"
289+
@click="confirmUndo"
290+
class="btn btn-error btn-xs"
291+
:disabled="undoBusy"
292+
>
293+
<span v-if="undoBusy" class="loading loading-spinner loading-xs"></span>
294+
<span v-else>Undo connect</span>
295+
</button>
296+
<button
297+
data-test="connect-undo-cancel"
298+
@click="undoPanelOpen = false"
299+
class="btn btn-ghost btn-xs"
300+
:disabled="undoBusy"
301+
>
302+
Keep
303+
</button>
304+
</div>
305+
</div>
243306
<!-- Spec 078 US2 / SC-005: Connect All renders EVERY successful
244307
client's backup outcome — one line per client, each with its own
245308
copy affordance — instead of only the last connect's path. -->
@@ -341,6 +404,19 @@ const copiedClient = ref<string | null>(null)
341404
const previews = ref<Record<string, ConnectPreview>>({})
342405
const previewLoading = reactive<Record<string, boolean>>({})
343406
const previewError = ref<Record<string, string>>({})
407+
// Spec 078 US3: the connect performed last in THIS modal session, so a one-
408+
// click Undo can revert it. preview is the confirmed preview snapshot (what
409+
// was written), shown again in the undo panel before reverting (FR-009).
410+
// null = no undoable connect (none yet, undone, disconnected, or bulk run).
411+
const lastConnect = ref<{
412+
id: string
413+
serverName: string
414+
configPath: string
415+
backupPath: string | null
416+
preview?: ConnectPreview
417+
} | null>(null)
418+
const undoPanelOpen = ref(false)
419+
const undoBusy = ref(false)
344420
345421
// MCP-2952: `GET /api/v1/connect` is stat-only (#706/MCP-2829) and reports
346422
// connected=false for every client. Merge the content-resolved
@@ -457,20 +533,34 @@ function clearPreview(clientId: string) {
457533
// Confirm proceeds with the connect. If an entry already exists, confirming
458534
// implies force=true (the user saw the overwrite warning in the preview).
459535
async function confirmConnect(clientId: string) {
460-
const force = previews.value[clientId]?.entry_exists === true
536+
const preview = previews.value[clientId]
537+
const force = preview?.entry_exists === true
461538
bulkBackups.value = []
462539
copiedBulkClient.value = null
463-
await connect(clientId, force)
540+
const outcome = await connect(clientId, force)
541+
// Spec 078 US3: a successful single connect becomes undoable in this session.
542+
if (outcome.ok) {
543+
lastConnect.value = {
544+
id: clientId,
545+
serverName: 'mcpproxy',
546+
configPath: outcome.configPath,
547+
backupPath: outcome.backupPath,
548+
preview,
549+
}
550+
undoPanelOpen.value = false
551+
}
464552
clearPreview(clientId)
465553
}
466554
467555
// Returns the outcome so connectAll can accumulate per-client backup results
468556
// (ok=true with backupPath string = backup created; null = no prior file).
469-
async function connect(clientId: string, force = false): Promise<{ ok: boolean; backupPath: string | null }> {
557+
async function connect(clientId: string, force = false): Promise<{ ok: boolean; backupPath: string | null; configPath: string }> {
470558
loading.clients[clientId] = true
471559
resultMessage.value = ''
472560
resultBackupPath.value = undefined
473561
copiedBackup.value = false
562+
lastConnect.value = null
563+
undoPanelOpen.value = false
474564
475565
try {
476566
const response = await api.connectClient(clientId, 'mcpproxy', force)
@@ -486,7 +576,7 @@ async function connect(clientId: string, force = false): Promise<{ ok: boolean;
486576
title: 'Client Connected',
487577
message: `MCPProxy registered in ${clientId}`,
488578
})
489-
return { ok: true, backupPath }
579+
return { ok: true, backupPath, configPath: response.data.config_path }
490580
}
491581
resultMessage.value = response.error || 'Failed to connect'
492582
resultSuccess.value = false
@@ -500,7 +590,43 @@ async function connect(clientId: string, force = false): Promise<{ ok: boolean;
500590
} finally {
501591
loading.clients[clientId] = false
502592
}
503-
return { ok: false, backupPath: null }
593+
return { ok: false, backupPath: null, configPath: '' }
594+
}
595+
596+
// Spec 078 US3: revert the last connect performed in this modal session. The
597+
// backend refuses (409) when the config changed since the connect — surfaced
598+
// honestly as the error message; it never clobbers later edits.
599+
async function confirmUndo() {
600+
const target = lastConnect.value
601+
if (!target) return
602+
undoBusy.value = true
603+
try {
604+
const response = await api.undoConnectClient(target.id, target.serverName, target.backupPath)
605+
if (response.success && response.data) {
606+
resultMessage.value = response.data.message || `Reverted the ${target.id} connect`
607+
resultSuccess.value = true
608+
resultBackupPath.value = undefined
609+
lastConnect.value = null
610+
undoPanelOpen.value = false
611+
await fetchClients()
612+
void onboarding.fetchState()
613+
systemStore.addToast({
614+
type: 'info',
615+
title: 'Connect undone',
616+
message: `${target.id} restored to its pre-connect state`,
617+
})
618+
} else {
619+
resultMessage.value = response.error || 'Failed to undo the connect'
620+
resultSuccess.value = false
621+
undoPanelOpen.value = false
622+
}
623+
} catch (err) {
624+
resultMessage.value = err instanceof Error ? err.message : 'Unknown error'
625+
resultSuccess.value = false
626+
undoPanelOpen.value = false
627+
} finally {
628+
undoBusy.value = false
629+
}
504630
}
505631
506632
async function disconnect(clientId: string) {
@@ -510,6 +636,9 @@ async function disconnect(clientId: string) {
510636
copiedBackup.value = false
511637
bulkBackups.value = []
512638
copiedBulkClient.value = null
639+
// A disconnect supersedes any pending session undo (the entry is gone).
640+
lastConnect.value = null
641+
undoPanelOpen.value = false
513642
514643
try {
515644
const client = clients.value.find(c => c.id === clientId)
@@ -657,6 +786,8 @@ function close() {
657786
copiedBulkClient.value = null
658787
previews.value = {}
659788
previewError.value = {}
789+
lastConnect.value = null
790+
undoPanelOpen.value = false
660791
emit('close')
661792
}
662793
@@ -674,6 +805,8 @@ watch(() => props.show, (newVal) => {
674805
copiedBulkClient.value = null
675806
previews.value = {}
676807
previewError.value = {}
808+
lastConnect.value = null
809+
undoPanelOpen.value = false
677810
}
678811
})
679812
</script>

0 commit comments

Comments
 (0)