Skip to content

Commit 36dec5b

Browse files
davidjayanaclaude
authored andcommitted
fix(mcp): make MCP writes a separate opt-in and validate renderer input
Addresses the CodeRabbit review on #893. - MCP clients get their own "Project edits" switch (`allowEdits` in mcp-server.json), off by default and independent of the in-app agent's `allowAgentEdits`. Turning the server on now grants read access only. Before, a user with no provider configured had edits enabled by default with no way to turn them off. Shown in the MCP server settings, in all 15 locales, and documented in mcp-server.md and ai-editing.md. - The native bridge rejects a non-boolean `enabled` / `allowEdits` and an out-of-range or non-integer port as INVALID_REQUEST, instead of letting them fail later as INTERNAL_ERROR. - EditorDocumentHost.snapshot() returns null unless the editor's reply carries a finite numeric revision. A missing one would have reached applyAgentDocumentIfCurrent as undefined and skipped the stale-edit check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 194360c commit 36dec5b

32 files changed

Lines changed: 337 additions & 61 deletions

‎electron/ipc/handlers.ts‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4865,17 +4865,16 @@ export function registerIpcHandlers(
48654865
// (Claude Code, Codex…). Built here because this is where the agent's own
48664866
// dependencies live, but NOT started here: the headless CLI shares this
48674867
// function and must never bind the port a running app is listening on.
4868-
// `main.ts` starts it. Its tools obey the same "Project edits" switch as the
4869-
// in-app agent, read on every call.
4870-
const mcpController = new McpController(
4871-
new McpSettingsStore(app.getPath("userData"), safeStorage),
4872-
{
4873-
host: new EditorDocumentHost(ipcMain),
4874-
editsAllowed: () => getAiEditionLlmConfig().getConfig()?.allowAgentEdits !== false,
4875-
cursor: agentCursorTelemetryReader,
4876-
version: app.getVersion(),
4877-
},
4878-
);
4868+
// `main.ts` starts it. Its writes have their own switch, off by default and
4869+
// read on every call — separate from the in-app agent's "Project edits", so
4870+
// turning the server on grants a client read access and nothing more.
4871+
const mcpSettings = new McpSettingsStore(app.getPath("userData"), safeStorage);
4872+
const mcpController = new McpController(mcpSettings, {
4873+
host: new EditorDocumentHost(ipcMain),
4874+
editsAllowed: () => mcpSettings.getSettings().allowEdits,
4875+
cursor: agentCursorTelemetryReader,
4876+
version: app.getVersion(),
4877+
});
48794878

48804879
registerNativeBridgeHandlers({
48814880
getPlatform: () => process.platform,
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
// The MCP settings actions take renderer input, so the bridge checks its types
2+
// before anything reaches the settings store: a bad value is an INVALID_REQUEST,
3+
// not an INTERNAL_ERROR thrown further in.
4+
5+
import { beforeEach, describe, expect, it, vi } from "vitest";
6+
import type { NativeBridgeResponse } from "../../src/native/contracts";
7+
import { type NativeBridgeContext, registerNativeBridgeHandlers } from "./nativeBridge";
8+
9+
const electron = vi.hoisted(() => ({ handle: vi.fn() }));
10+
vi.mock("electron", () => ({
11+
app: { getAppPath: () => "", isPackaged: false },
12+
ipcMain: { handle: electron.handle, removeHandler: vi.fn() },
13+
shell: {},
14+
}));
15+
vi.mock("../native-bridge/services/compositorViewService", () => ({
16+
CompositorViewService: class {},
17+
}));
18+
19+
const service = vi.hoisted(() => ({
20+
mcpSetEnabled: vi.fn(async () => ({ ok: "enabled" })),
21+
mcpSetPort: vi.fn(async () => ({ ok: "port" })),
22+
mcpSetAllowEdits: vi.fn(async () => ({ ok: "allowEdits" })),
23+
}));
24+
vi.mock("../native-bridge/services/aiEditionService", () => ({
25+
AiEditionService: class {
26+
mcpSetEnabled = service.mcpSetEnabled;
27+
mcpSetPort = service.mcpSetPort;
28+
mcpSetAllowEdits = service.mcpSetAllowEdits;
29+
},
30+
}));
31+
32+
let invoke: (action: string, payload: unknown) => Promise<NativeBridgeResponse>;
33+
34+
beforeEach(() => {
35+
electron.handle.mockClear();
36+
for (const fn of Object.values(service)) fn.mockClear();
37+
registerNativeBridgeHandlers({
38+
getPlatform: () => "linux",
39+
getAiEditionDocuments: () => ({}),
40+
getAiEditionLlmConfig: () => ({}),
41+
} as unknown as NativeBridgeContext);
42+
const handler = electron.handle.mock.calls[0]?.[1] as (
43+
event: unknown,
44+
request: unknown,
45+
) => Promise<NativeBridgeResponse>;
46+
invoke = (action, payload) =>
47+
handler({ sender: {} }, { domain: "aiEdition", action, payload, requestId: "r" });
48+
});
49+
50+
function errorCode(response: NativeBridgeResponse): string | undefined {
51+
return response.ok ? undefined : response.error?.code;
52+
}
53+
54+
describe("native bridge MCP settings", () => {
55+
it.each([80, 70000, 4.5, "47821", null])("rejects port %s as a bad request", async (port) => {
56+
expect(errorCode(await invoke("mcp.setPort", { port }))).toBe("INVALID_REQUEST");
57+
expect(service.mcpSetPort).not.toHaveBeenCalled();
58+
});
59+
60+
it("passes a valid port on", async () => {
61+
const response = await invoke("mcp.setPort", { port: 47821 });
62+
expect(response.ok).toBe(true);
63+
expect(service.mcpSetPort).toHaveBeenCalledWith(47821);
64+
});
65+
66+
it.each(["true", 1, undefined])("rejects enabled=%s as a bad request", async (enabled) => {
67+
expect(errorCode(await invoke("mcp.setEnabled", { enabled }))).toBe("INVALID_REQUEST");
68+
expect(service.mcpSetEnabled).not.toHaveBeenCalled();
69+
});
70+
71+
it.each(["yes", 0, undefined])("rejects allowEdits=%s as a bad request", async (allowEdits) => {
72+
expect(errorCode(await invoke("mcp.setAllowEdits", { allowEdits }))).toBe("INVALID_REQUEST");
73+
expect(service.mcpSetAllowEdits).not.toHaveBeenCalled();
74+
});
75+
76+
it("passes valid flags on", async () => {
77+
await invoke("mcp.setEnabled", { enabled: true });
78+
await invoke("mcp.setAllowEdits", { allowEdits: false });
79+
expect(service.mcpSetEnabled).toHaveBeenCalledWith(true);
80+
expect(service.mcpSetAllowEdits).toHaveBeenCalledWith(false);
81+
});
82+
});

‎electron/ipc/nativeBridge.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
import type { ChatEventSink } from "../ai-edition/chat-service";
1414
import type { DocumentService } from "../ai-edition/document-service";
1515
import { StylePresetError, type StylePresetService } from "../ai-edition/style-preset-service";
16+
import { isValidMcpPort } from "../mcp/mcp-settings-store";
1617
import {
1718
type CursorTelemetryLoadResult,
1819
TelemetryCursorAdapter,
@@ -578,16 +579,44 @@ export function registerNativeBridgeHandlers(context: NativeBridgeContext) {
578579
);
579580
case "mcp.getStatus":
580581
return createSuccessResponse(requestId, await aiEditionService.mcpGetStatus());
582+
// Renderer input: check the types here, so a bad value is reported as
583+
// the bad request it is rather than failing later as an internal error.
581584
case "mcp.setEnabled":
585+
if (typeof request.payload?.enabled !== "boolean") {
586+
return createErrorResponse(
587+
requestId,
588+
"INVALID_REQUEST",
589+
"Invalid MCP enabled flag.",
590+
);
591+
}
582592
return createSuccessResponse(
583593
requestId,
584594
await aiEditionService.mcpSetEnabled(request.payload.enabled),
585595
);
586596
case "mcp.setPort":
597+
if (!isValidMcpPort(request.payload?.port)) {
598+
return createErrorResponse(
599+
requestId,
600+
"INVALID_REQUEST",
601+
"MCP port must be a whole number between 1024 and 65535.",
602+
);
603+
}
587604
return createSuccessResponse(
588605
requestId,
589606
await aiEditionService.mcpSetPort(request.payload.port),
590607
);
608+
case "mcp.setAllowEdits":
609+
if (typeof request.payload?.allowEdits !== "boolean") {
610+
return createErrorResponse(
611+
requestId,
612+
"INVALID_REQUEST",
613+
"Invalid MCP edit permission.",
614+
);
615+
}
616+
return createSuccessResponse(
617+
requestId,
618+
await aiEditionService.mcpSetAllowEdits(request.payload.allowEdits),
619+
);
591620
case "mcp.regenerateToken":
592621
return createSuccessResponse(requestId, await aiEditionService.mcpRegenerateToken());
593622
case "chat.run": {

‎electron/mcp/editor-document-host.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,18 @@ describe("EditorDocumentHost", () => {
6565
await expect(pending).resolves.toEqual({ document: { id: "d" }, revision: 3 });
6666
});
6767

68+
it("treats a snapshot without a numeric revision as no snapshot", async () => {
69+
const { host, fromRenderer } = setup();
70+
const editor = new FakeWebContents();
71+
fromRenderer(editor, AI_EDITION_MCP_HOST_CHANNEL, true);
72+
// A missing revision would reach the apply guard as `undefined` and skip it.
73+
for (const revision of [undefined, "3", Number.NaN]) {
74+
const pending = host.snapshot();
75+
await answer(editor, fromRenderer, { document: { id: "d" }, revision });
76+
await expect(pending).resolves.toBeNull();
77+
}
78+
});
79+
6880
it("passes the apply verdict through", async () => {
6981
const { host, fromRenderer } = setup();
7082
const editor = new FakeWebContents();

‎electron/mcp/editor-document-host.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,12 @@ export class EditorDocumentHost implements McpDocumentHost {
9090
async snapshot(): Promise<AiEditionMcpHostSnapshot | null> {
9191
const result = await this.request({ op: "snapshot" });
9292
if (!result || result === TIMED_OUT || typeof result !== "object") return null;
93-
return result as AiEditionMcpHostSnapshot;
93+
// The revision is the apply guard: one that is missing or not a number would
94+
// reach `applyAgentDocumentIfCurrent` as `undefined`, which skips the
95+
// stale-edit check entirely. Treat such a reply as no snapshot at all.
96+
const snapshot = result as Partial<AiEditionMcpHostSnapshot>;
97+
if (typeof snapshot.revision !== "number" || !Number.isFinite(snapshot.revision)) return null;
98+
return snapshot as AiEditionMcpHostSnapshot;
9499
}
95100

96101
async apply(document: AxcutDocument, expectedRevision: number): Promise<McpApplyResult> {

‎electron/mcp/mcp-controller.test.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,19 @@ async function initializeStatus(url: string, token: string) {
7070
describe("McpSettingsStore", () => {
7171
it("is off by default, on the default port", () => {
7272
const store = new McpSettingsStore(dir, fakeCrypto);
73-
expect(store.getSettings()).toEqual({ enabled: false, port: DEFAULT_MCP_PORT });
73+
expect(store.getSettings()).toEqual({
74+
enabled: false,
75+
port: DEFAULT_MCP_PORT,
76+
allowEdits: false,
77+
});
78+
});
79+
80+
it("keeps MCP edits off unless they were turned on explicitly", async () => {
81+
expect(new McpSettingsStore(dir, fakeCrypto).getSettings().allowEdits).toBe(false);
82+
await new McpSettingsStore(dir, fakeCrypto).setSettings({ enabled: true });
83+
expect(new McpSettingsStore(dir, fakeCrypto).getSettings().allowEdits).toBe(false);
84+
await new McpSettingsStore(dir, fakeCrypto).setSettings({ allowEdits: true });
85+
expect(new McpSettingsStore(dir, fakeCrypto).getSettings().allowEdits).toBe(true);
7486
});
7587

7688
it("keeps the token encrypted on disk and stable across instances", async () => {
@@ -126,6 +138,19 @@ describe("McpController", () => {
126138
expect(status.error).toContain(String(port));
127139
});
128140

141+
it("saves the edit permission without restarting the server", async () => {
142+
const store = new McpSettingsStore(dir, fakeCrypto);
143+
await store.setSettings({ port: await freePort() });
144+
controller = new McpController(store, deps);
145+
const on = await controller.setEnabled(true);
146+
expect(on.allowEdits).toBe(false);
147+
148+
const allowed = await controller.setAllowEdits(true);
149+
expect(allowed.allowEdits).toBe(true);
150+
expect(allowed.token).toBe(on.token);
151+
expect(await initializeStatus(allowed.url, allowed.token ?? "")).toBe(200);
152+
});
153+
129154
it("locks out the old token once it is regenerated", async () => {
130155
const store = new McpSettingsStore(dir, fakeCrypto);
131156
await store.setSettings({ port: await freePort() });

‎electron/mcp/mcp-controller.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ export class McpController {
6666

6767
async getStatus(): Promise<AiEditionMcpStatus> {
6868
await this.transition;
69-
const { enabled, port } = this.store.getSettings();
69+
const { enabled, port, allowEdits } = this.store.getSettings();
7070
let token: string | null = null;
7171
if (enabled) {
7272
try {
@@ -78,6 +78,7 @@ export class McpController {
7878
return {
7979
enabled,
8080
port,
81+
allowEdits,
8182
running: this.running !== null,
8283
url: `http://127.0.0.1:${port}${MCP_ENDPOINT_PATH}`,
8384
token,
@@ -101,6 +102,12 @@ export class McpController {
101102
return this.getStatus();
102103
}
103104

105+
/** No restart needed: the server reads it on every call. */
106+
async setAllowEdits(allowEdits: boolean): Promise<AiEditionMcpStatus> {
107+
await this.serially(() => this.store.setSettings({ allowEdits }));
108+
return this.getStatus();
109+
}
110+
104111
/** New token; the running server is restarted so the old one stops working at once. */
105112
async regenerateToken(): Promise<AiEditionMcpStatus> {
106113
await this.serially(async () => {

‎electron/mcp/mcp-settings-store.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ export const DEFAULT_MCP_PORT = 47821;
1616
export interface McpSettings {
1717
enabled: boolean;
1818
port: number;
19+
/**
20+
* Whether MCP clients may run the tools that change the project. Off by
21+
* default and separate from the in-app agent's "Project edits": turning the
22+
* server on gives a client read access, and writing is a second decision.
23+
*/
24+
allowEdits: boolean;
1925
}
2026

2127
export interface McpTokenCrypto {
@@ -49,9 +55,10 @@ export class McpSettingsStore {
4955
return {
5056
enabled: raw.enabled === true,
5157
port: isValidMcpPort(raw.port) ? raw.port : DEFAULT_MCP_PORT,
58+
allowEdits: raw.allowEdits === true,
5259
};
5360
} catch {
54-
return { enabled: false, port: DEFAULT_MCP_PORT };
61+
return { enabled: false, port: DEFAULT_MCP_PORT, allowEdits: false };
5562
}
5663
}
5764

‎electron/native-bridge/services/aiEditionService.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,10 @@ export class AiEditionService {
292292
return this.mcp.setPort(port);
293293
}
294294

295+
mcpSetAllowEdits(allowEdits: boolean): Promise<AiEditionMcpStatus> {
296+
return this.mcp.setAllowEdits(allowEdits);
297+
}
298+
295299
mcpRegenerateToken(): Promise<AiEditionMcpStatus> {
296300
return this.mcp.regenerateToken();
297301
}

‎src/components/ai-edition/McpServerSettings.test.tsx‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@ import type { AiEditionMcpStatus } from "@/native/contracts";
88
const TOKEN = "secret-token";
99
const URL = "http://127.0.0.1:47821/mcp";
1010

11-
function status(enabled: boolean): AiEditionMcpStatus {
11+
function status(enabled: boolean, allowEdits = false): AiEditionMcpStatus {
1212
return {
1313
enabled,
1414
port: 47821,
15+
allowEdits,
1516
running: enabled,
1617
url: URL,
1718
token: enabled ? TOKEN : null,
@@ -23,6 +24,7 @@ const bridge = vi.hoisted(() => ({
2324
mcpGetStatus: vi.fn(),
2425
mcpSetEnabled: vi.fn(),
2526
mcpSetPort: vi.fn(),
27+
mcpSetAllowEdits: vi.fn(),
2628
mcpRegenerateToken: vi.fn(),
2729
}));
2830

@@ -66,6 +68,19 @@ describe("McpServerSettings", () => {
6668
expect(screen.queryByText(new RegExp(TOKEN))).not.toBeInTheDocument();
6769
});
6870

71+
it("keeps MCP edits off until they are turned on", async () => {
72+
bridge.mcpGetStatus.mockResolvedValue(status(true));
73+
bridge.mcpSetAllowEdits.mockResolvedValue(status(true, true));
74+
renderSection();
75+
const toggle = await screen.findByTestId("mcp-edits-toggle");
76+
expect(toggle).toHaveAttribute("aria-pressed", "false");
77+
fireEvent.click(toggle);
78+
await waitFor(() => expect(bridge.mcpSetAllowEdits).toHaveBeenCalledWith(true));
79+
await waitFor(() =>
80+
expect(screen.getByTestId("mcp-edits-toggle")).toHaveAttribute("aria-pressed", "true"),
81+
);
82+
});
83+
6984
it("copies the real token into the Claude Code command", async () => {
7085
bridge.mcpGetStatus.mockResolvedValue(status(true));
7186
renderSection();

0 commit comments

Comments
 (0)