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>

Author: Dumbris

docs/api/rest-api.md

@@ -711,14 +711,20 @@ would use.
 One-click undo of the immediately-preceding connect (Spec 078 US3). Body:
 
 ```json
-{ "server_name": "mcpproxy", "backup_path": "<backup_path from the connect result>" }
+{ "server_name": "mcpproxy", "backup_name": "<basename of backup_path from the connect result>" }
 ```
 
-- **`backup_path` set** — restores the config **byte-for-byte** from that
+`backup_name` is the **bare filename** of the backup the connect returned in
+`backup_path` — a name, never a path. The server resolves the full path itself
+inside that client's own config directory (derived from the client registry, not
+the request), so a caller-supplied value can never contribute a directory
+component and cannot escape the config dir (defense against path injection).
+
+- **`backup_name` set** — restores the config **byte-for-byte** from that
   backup. This is the only revert that can bring back a pre-existing
   same-named entry that a `force=true` connect overwrote (surgical
   `DELETE /connect/{client}` cannot).
-- **`backup_path` empty** — the connect created the file (its result carried no
+- **`backup_name` empty** — the connect created the file (its result carried no
   `backup_path`); undo deletes the created file, restoring the "no file" state.
 
 Safety semantics:
@@ -727,8 +733,9 @@ Safety semantics:
   connect (it verifies the current file is byte-identical to what that connect
   wrote) — it never clobbers later edits. Fall back to
   `DELETE /connect/{client}` for a surgical entry removal.
-- A vanished backup returns `404`; a backup path that is not a
-  `<config>.bak.*` sibling of that client's config returns `400`.
+- A vanished backup returns `404`; a `backup_name` that is a path (contains a
+  directory separator) or does not match `<config>.bak.*` for that client
+  returns `400`.
 - Undo takes its **own safety backup** of the current file before restoring or
   deleting, returned as `backup_path` in the result
   (`action` = `restored` or `deleted`).

frontend/src/services/api.ts

@@ -1192,14 +1192,21 @@ class APIService {
   // backupPath is the backup_path that connect returned (null = no prior file
   // existed, so undo removes the file connect created). The backend refuses
   // with 409 when the config changed since the connect (never clobbers edits).
+  //
+  // The wire payload carries only the backup's bare FILENAME (backup_name), not
+  // a path: the server resolves the full path inside the client's own config
+  // directory and never trusts a client-supplied path (defense against path
+  // injection). We strip any directory component here on both / and \ so a
+  // Windows path resolves the same way.
   async undoConnectClient(
     clientId: string,
     serverName = 'mcpproxy',
     backupPath: string | null = null
   ): Promise<APIResponse<ConnectResult>> {
+    const backupName = backupPath ? (backupPath.split(/[/\\]/).pop() ?? '') : ''
     return this.request<ConnectResult>(`/api/v1/connect/${encodeURIComponent(clientId)}/undo`, {
       method: 'POST',
-      body: JSON.stringify({ server_name: serverName, backup_path: backupPath ?? '' }),
+      body: JSON.stringify({ server_name: serverName, backup_name: backupName }),
     })
   }
 

frontend/tests/unit/connect-undo-api.spec.ts

@@ -0,0 +1,68 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
+
+// Spec 078 US3 / CodeQL hardening: the undo request must carry only the backup's
+// bare FILENAME (backup_name), never a path. The server resolves the full path
+// inside the client's own config dir and never trusts a client-supplied path, so
+// api.undoConnectClient strips any directory component before sending.
+
+describe('api.undoConnectClient — sends a bare backup_name (not a path)', () => {
+  let fetchMock: ReturnType<typeof vi.fn>
+
+  beforeEach(() => {
+    fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      status: 200,
+      json: async () => ({ success: true, data: { success: true, action: 'restored' } }),
+    })
+    vi.stubGlobal('fetch', fetchMock)
+  })
+
+  afterEach(() => {
+    vi.unstubAllGlobals()
+    vi.resetModules()
+  })
+
+  async function freshApi() {
+    vi.resetModules()
+    const mod = await import('@/services/api')
+    return mod.default
+  }
+
+  it('POSTs the basename of a POSIX backup path', async () => {
+    const api = await freshApi()
+    await api.undoConnectClient('cursor', 'mcpproxy', '/Users/test/.cursor/mcp.json.bak.20260703-101530')
+
+    const [url, opts] = fetchMock.mock.calls[0]
+    expect(url).toBe('/api/v1/connect/cursor/undo')
+    expect(opts.method).toBe('POST')
+    expect(JSON.parse(opts.body)).toEqual({
+      server_name: 'mcpproxy',
+      backup_name: 'mcp.json.bak.20260703-101530',
+    })
+  })
+
+  it('strips a Windows backslash directory component too', async () => {
+    const api = await freshApi()
+    await api.undoConnectClient(
+      'claude-desktop',
+      'mcpproxy',
+      'C:\\Users\\test\\AppData\\claude_desktop_config.json.bak.20260703-101530'
+    )
+    const [, opts] = fetchMock.mock.calls[0]
+    expect(JSON.parse(opts.body).backup_name).toBe('claude_desktop_config.json.bak.20260703-101530')
+  })
+
+  it('sends an empty backup_name for the no-prior-file (null) case', async () => {
+    const api = await freshApi()
+    await api.undoConnectClient('cursor', 'mcpproxy', null)
+    const [, opts] = fetchMock.mock.calls[0]
+    expect(JSON.parse(opts.body)).toEqual({ server_name: 'mcpproxy', backup_name: '' })
+  })
+
+  it('percent-encodes a "/"-containing client id', async () => {
+    const api = await freshApi()
+    await api.undoConnectClient('weird/client', 'mcpproxy', null)
+    const [url] = fetchMock.mock.calls[0]
+    expect(url).toBe('/api/v1/connect/weird%2Fclient/undo')
+  })
+})

internal/connect/undo.go

@@ -10,13 +10,16 @@ import (
 	"github.com/BurntSushi/toml"
 )
 
-// Undo reverts the connect that produced backupPath, restoring the client
-// config to its exact pre-connect state (Spec 078 US3 / FR-008):
+// Undo reverts the connect that produced the named backup, restoring the client
+// config to its exact pre-connect state (Spec 078 US3 / FR-008). backupName is
+// the bare filename (filepath.Base) of the backup the connect returned, NOT a
+// path — undo resolves the full path itself against the client's own config
+// directory (see the resolution/validation below):
 //
-//   - backupPath != "": the config is restored byte-for-byte from that backup —
+//   - backupName != "": the config is restored byte-for-byte from that backup —
 //     this is the only revert that can bring back a pre-existing same-named
 //     entry a force-connect overwrote (surgical disconnect cannot).
-//   - backupPath == "": the preceding connect created the file (no prior file
+//   - backupName == "": the preceding connect created the file (no prior file
 //     existed, ConnectResult.backup_path was empty); undo deletes the file so
 //     the pre-connect "no file" state is restored.
 //
@@ -35,7 +38,7 @@ import (
 // address / API key / require_mcp_auth changed since the connect: the entry the
 // service would write today no longer matches the one on disk, so mcpproxy can
 // no longer prove the file is untouched.
-func (s *Service) Undo(clientID, serverName, backupPath string) (*ConnectResult, error) {
+func (s *Service) Undo(clientID, serverName, backupName string) (*ConnectResult, error) {
 	client := FindClient(clientID)
 	if client == nil {
 		return nil, fmt.Errorf("unknown client: %s", clientID)
@@ -51,18 +54,24 @@ func (s *Service) Undo(clientID, serverName, backupPath string) (*ConnectResult,
 		return nil, fmt.Errorf("cannot determine config path for %s", clientID)
 	}
 
-	// The backup must be a real backup OF THIS client's config, living beside it:
-	// undo must not become an arbitrary-file-restore primitive for API callers.
-	// A prefix-only check is not enough — a path like "<cfg>.bak.x/../../secret"
-	// keeps the prefix yet escapes the directory, so anchor on the cleaned path's
-	// parent directory and basename (both traversal-proof).
-	if backupPath != "" {
-		cleaned := filepath.Clean(backupPath)
-		if filepath.Dir(cleaned) != filepath.Dir(cfgPath) ||
-			!strings.HasPrefix(filepath.Base(cleaned), filepath.Base(cfgPath)+".bak.") {
-			return nil, fmt.Errorf("invalid backup path %q: not a backup of %s", backupPath, cfgPath)
+	// Resolve the backup path entirely server-side: the request carries only a
+	// bare FILENAME (backupName), never a path. Undo joins it with THIS client's
+	// own config directory — derived from the client registry, never from the
+	// request — so a client-supplied value can never contribute a directory
+	// component. That makes traversal impossible by construction (undo must not
+	// become an arbitrary-file-restore primitive): we reject anything whose
+	// filepath.Base is not identical to the input, and require the strict
+	// "<config-basename>.bak." prefix the connect write produced.
+	backupPath := ""
+	if backupName != "" {
+		base := filepath.Base(backupName)
+		if base != backupName {
+			return nil, fmt.Errorf("invalid backup name %q: must be a bare filename, not a path", backupName)
+		}
+		if !strings.HasPrefix(base, filepath.Base(cfgPath)+".bak.") {
+			return nil, fmt.Errorf("invalid backup name %q: not a backup of %s", backupName, filepath.Base(cfgPath))
 		}
-		backupPath = cleaned
+		backupPath = filepath.Join(filepath.Dir(cfgPath), base)
 	}
 
 	res, err := s.undo(client, cfgPath, serverName, backupPath)

internal/connect/undo_test.go

@@ -46,7 +46,7 @@ func TestUndo_RestoresByteIdenticalPreConnectFile(t *testing.T) {
 		t.Fatalf("connect result unexpected: %+v", res)
 	}
 
-	undo, err := svc.Undo("claude-code", "mcpproxy", res.BackupPath)
+	undo, err := svc.Undo("claude-code", "mcpproxy", filepath.Base(res.BackupPath))
 	if err != nil {
 		t.Fatalf("undo: %v", err)
 	}
@@ -89,7 +89,7 @@ func TestUndo_TOML_RestoresByteIdentical(t *testing.T) {
 		t.Fatalf("connect result unexpected: %+v", res)
 	}
 
-	undo, err := svc.Undo("codex", "mcpproxy", res.BackupPath)
+	undo, err := svc.Undo("codex", "mcpproxy", filepath.Base(res.BackupPath))
 	if err != nil {
 		t.Fatalf("undo: %v", err)
 	}
@@ -124,7 +124,7 @@ func TestUndo_RefusesWhenFileDriftedSinceConnect(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	undo, err := svc.Undo("claude-code", "mcpproxy", res.BackupPath)
+	undo, err := svc.Undo("claude-code", "mcpproxy", filepath.Base(res.BackupPath))
 	if err != nil {
 		t.Fatalf("undo returned hard error, want refusal result: %v", err)
 	}
@@ -214,7 +214,9 @@ func TestUndo_BackupMissingReturnsNotFound(t *testing.T) {
 		t.Fatalf("connect: %v", err)
 	}
 	cfgPath := res.ConfigPath
-	missing := cfgPath + ".bak.19990101-000000"
+	// A well-formed but non-existent backup NAME (bare filename, correct prefix)
+	// must resolve to not_found, not a hard error.
+	missing := filepath.Base(cfgPath) + ".bak.19990101-000000"
 
 	undo, err := svc.Undo("claude-code", "mcpproxy", missing)
 	if err != nil {
@@ -225,41 +227,44 @@ func TestUndo_BackupMissingReturnsNotFound(t *testing.T) {
 	}
 }
 
-func TestUndo_RejectsForeignBackupPath(t *testing.T) {
+// The request carries a bare backup FILENAME; an absolute path must be rejected
+// outright — undo must not become an arbitrary-file-restore primitive, and the
+// directory is always the client's own config dir, never the caller's.
+func TestUndo_RejectsAbsoluteBackupPath(t *testing.T) {
 	home := t.TempDir()
 	svc := NewServiceWithHome("127.0.0.1:8080", "", home)
 	if _, err := svc.Connect("claude-code", "mcpproxy", false); err != nil {
 		t.Fatalf("connect: %v", err)
 	}
 
-	// A path that is not a backup of THIS client's config must be rejected —
-	// the endpoint must not become an arbitrary-file-restore primitive.
+	// An absolute path is not a bare filename (filepath.Base != input).
 	foreign := filepath.Join(home, "evil.json")
 	if err := os.WriteFile(foreign, []byte(`{}`), 0o644); err != nil {
 		t.Fatal(err)
 	}
 	_, err := svc.Undo("claude-code", "mcpproxy", foreign)
 	if err == nil {
-		t.Fatal("undo must reject a backup path outside <config>.bak.*")
+		t.Fatal("undo must reject an absolute backup path")
 	}
 	if !strings.Contains(err.Error(), "backup") {
-		t.Fatalf("error should mention the backup path problem: %v", err)
+		t.Fatalf("error should mention the backup name problem: %v", err)
 	}
 }
 
-// A path that keeps the "<config>.bak." prefix but traverses out of the config
-// directory (e.g. "<config>.bak.x/../../secret.json") must be rejected before
-// any read — a prefix-only check would let undo read/restore an arbitrary file.
-func TestUndo_RejectsBackupPathTraversal(t *testing.T) {
+// A backup name that carries any directory separator or traversal component
+// (e.g. "<config>.bak.x/../../secret.json") must be rejected before any read:
+// resolution joins the name with the client's own config dir, so only a bare
+// filename is ever admitted (traversal impossible by construction).
+func TestUndo_RejectsBackupNameWithSeparators(t *testing.T) {
 	home := t.TempDir()
 	svc := NewServiceWithHome("127.0.0.1:8080", "", home)
 	res, err := svc.Connect("claude-code", "mcpproxy", false)
 	if err != nil {
 		t.Fatalf("connect: %v", err)
 	}
 
-	// Craft a traversal path that still starts with "<config>.bak." so a
-	// prefix-only guard would admit it, then climbs back out to a foreign file.
+	// Keep the "<config>.bak." prefix but climb out via separators — a prefix-
+	// only guard would admit it; the bare-filename guard must not.
 	secret := filepath.Join(home, "secret.json")
 	if err := os.WriteFile(secret, []byte(`{"secret":true}`), 0o644); err != nil {
 		t.Fatal(err)
@@ -268,14 +273,14 @@ func TestUndo_RejectsBackupPathTraversal(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	traversal := res.ConfigPath + ".bak.x/../" + rel
+	traversal := filepath.Base(res.ConfigPath) + ".bak.x/../" + rel
 
 	_, err = svc.Undo("claude-code", "mcpproxy", traversal)
 	if err == nil {
-		t.Fatal("undo must reject a traversal backup path that escapes the config directory")
+		t.Fatal("undo must reject a backup name that contains path separators")
 	}
 	if !strings.Contains(err.Error(), "backup") {
-		t.Fatalf("error should mention the backup path problem: %v", err)
+		t.Fatalf("error should mention the backup name problem: %v", err)
 	}
 }
 
@@ -290,7 +295,8 @@ func TestUndo_MissingConfigFileRefuses(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	undo, err := svc.Undo("claude-code", "mcpproxy", res.BackupPath)
+	// No prior file existed, so the connect returned an empty backup name.
+	undo, err := svc.Undo("claude-code", "mcpproxy", "")
 	if err != nil {
 		t.Fatalf("undo: %v", err)
 	}

internal/httpapi/connect.go

@@ -258,30 +258,36 @@ func (s *Server) handleDisconnectClient(w http.ResponseWriter, r *http.Request)
 // UndoConnectRequest is the JSON body for POST /api/v1/connect/{client}/undo.
 type UndoConnectRequest struct {
 	ServerName string `json:"server_name,omitempty"` // Defaults to "mcpproxy"
-	// BackupPath is the backup_path returned by the preceding connect for this
-	// client. Empty means the connect created the file (no prior file existed),
-	// so undo removes the created file.
-	BackupPath string `json:"backup_path,omitempty"`
+	// BackupName is the bare filename (filepath.Base) of the backup returned as
+	// backup_path by the preceding connect — a name, never a path. Undo resolves
+	// the full path server-side by joining it with the client's own config
+	// directory, so a client-supplied value can never contribute a directory
+	// component (traversal is impossible by construction). Empty means the
+	// connect created the file (no prior file existed), so undo removes it.
+	BackupName string `json:"backup_name,omitempty"`
 }
 
 // handleUndoConnectClient godoc
 // @Summary     Undo a connect, restoring the pre-connect config
-// @Description Reverts the connect that produced the given backup_path (Spec 078 US3):
+// @Description Reverts the connect that produced the named backup (Spec 078 US3):
 // @Description restores the client config byte-for-byte from that backup, or — when
-// @Description backup_path is empty because the connect created the file — deletes the
-// @Description created file. Refuses with 409 when the config changed since the connect
-// @Description (undo never clobbers later edits; use DELETE /connect/{client} for a
-// @Description surgical entry removal instead). Takes its own safety backup first;
-// @Description its path is returned as backup_path in the result.
+// @Description backup_name is empty because the connect created the file — deletes the
+// @Description created file. backup_name is the bare filename of the backup the connect
+// @Description returned (never a path); undo resolves the full path server-side inside
+// @Description the client's own config directory, so a client value cannot escape it.
+// @Description Refuses with 409 when the config changed since the connect (undo never
+// @Description clobbers later edits; use DELETE /connect/{client} for a surgical entry
+// @Description removal instead). Takes its own safety backup first; its path is returned
+// @Description as backup_path in the result.
 // @Tags        connect
 // @Accept      json
 // @Produce     json
 // @Security    ApiKeyAuth
 // @Security    ApiKeyQuery
 // @Param       client path   string             true  "Client ID (claude-code, claude-desktop, cursor, windsurf, vscode, codex, gemini, opencode)"
-// @Param       body   body   UndoConnectRequest false "Undo parameters (server_name, backup_path from the preceding connect)"
+// @Param       body   body   UndoConnectRequest false "Undo parameters (server_name, backup_name = the bare filename of the backup the preceding connect returned)"
 // @Success     200    {object} contracts.APIResponse "ConnectResult (action restored|deleted)"
-// @Failure     400    {object} contracts.ErrorResponse "Bad request (e.g. backup path not a backup of this client's config)"
+// @Failure     400    {object} contracts.ErrorResponse "Bad request (e.g. backup_name is a path, or not a backup of this client's config)"
 // @Failure     403    {object} contracts.ErrorResponse "Permission denied (macOS App-Data block)"
 // @Failure     404    {object} contracts.ErrorResponse "Unknown client or backup no longer exists"
 // @Failure     409    {object} contracts.ErrorResponse "Config changed since connect; undo refused"
@@ -308,7 +314,7 @@ func (s *Server) handleUndoConnectClient(w http.ResponseWriter, r *http.Request)
 		}
 	}
 
-	result, err := svc.Undo(clientID, req.ServerName, req.BackupPath)
+	result, err := svc.Undo(clientID, req.ServerName, req.BackupName)
 	if err != nil {
 		if s.writeIfAccessDenied(w, r, err) {
 			return