From eb21d4e89938a262c0134c7a9fe59ace651ffda8 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 19 Aug 2026 10:38:43 +0000 Subject: [PATCH] :bug: Gate MCP REPL server behind isDevEnv check The ReplServer was starting unconditionally on every MCP server instance, regardless of configuration. This exposed an unauthenticated POST /execute endpoint that forwarded arbitrary JavaScript to connected Penpot plugins. Gate ReplServer creation, startup, and shutdown behind isDevEnv(), consistent with how CljsReplTool and other dev tools are already protected. Log an info message when the REPL server is disabled. Consolidate the dev-env check into a single static isDevEnvEnabled() method that isDevEnv() delegates to, avoiding duplicate logic. Add PluginBridge.close() for proper WebSocket server cleanup on shutdown. Add regression tests that construct PenpotMcpServer and verify hasReplServer() returns the correct value based on the dev-env flag. AI-assisted-by: mimo-v2.5-pro --- .../server/src/PenpotMcpServer.test.ts | 93 +++++++++++++++++++ mcp/packages/server/src/PenpotMcpServer.ts | 43 +++++++-- mcp/packages/server/src/PluginBridge.ts | 12 +++ 3 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 mcp/packages/server/src/PenpotMcpServer.test.ts diff --git a/mcp/packages/server/src/PenpotMcpServer.test.ts b/mcp/packages/server/src/PenpotMcpServer.test.ts new file mode 100644 index 0000000000..99765b5927 --- /dev/null +++ b/mcp/packages/server/src/PenpotMcpServer.test.ts @@ -0,0 +1,93 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { PenpotMcpServer } from "./PenpotMcpServer"; + +// ── Pure function tests ──────────────────────────────────────── + +test("isDevEnvEnabled returns false when PENPOT_MCP_DEVENV is not set", () => { + assert.equal(PenpotMcpServer.isDevEnvEnabled({}), false); +}); + +test("isDevEnvEnabled returns false when PENPOT_MCP_DEVENV is 'false'", () => { + assert.equal(PenpotMcpServer.isDevEnvEnabled({ PENPOT_MCP_DEVENV: "false" }), false); +}); + +test("isDevEnvEnabled returns true when PENPOT_MCP_DEVENV is 'true'", () => { + assert.equal(PenpotMcpServer.isDevEnvEnabled({ PENPOT_MCP_DEVENV: "true" }), true); +}); + +// ── Integration tests: constructor gating ────────────────────── +// +// Each test uses unique ports to avoid conflicts when tests run +// in the same process. The server is stopped in the finally block +// to release the WebSocket port. + +let portCounter = 14_500; +function uniquePorts() { + const base = portCounter; + portCounter += 10; + return { server: base, ws: base + 1, repl: base + 2 }; +} + +test("constructor does not create ReplServer when PENPOT_MCP_DEVENV is unset", async () => { + const prev = process.env.PENPOT_MCP_DEVENV; + const prevPorts = setUniqueEnv(); + delete process.env.PENPOT_MCP_DEVENV; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(false); + assert.equal(server.hasReplServer(), false); + } finally { + await server?.stop(); + restoreEnv(prev, prevPorts); + } +}); + +test("constructor creates ReplServer when PENPOT_MCP_DEVENV is 'true'", async () => { + const prev = process.env.PENPOT_MCP_DEVENV; + const prevPorts = setUniqueEnv(); + process.env.PENPOT_MCP_DEVENV = "true"; + let server: PenpotMcpServer | undefined; + try { + server = new PenpotMcpServer(false); + assert.equal(server.hasReplServer(), true); + } finally { + await server?.stop(); + restoreEnv(prev, prevPorts); + } +}); + +// ── Helpers ──────────────────────────────────────────────────── + +function setUniqueEnv() { + const ports = uniquePorts(); + const prevServer = process.env.PENPOT_MCP_SERVER_PORT; + const prevWs = process.env.PENPOT_MCP_WEBSOCKET_PORT; + const prevRepl = process.env.PENPOT_MCP_REPL_PORT; + process.env.PENPOT_MCP_SERVER_PORT = String(ports.server); + process.env.PENPOT_MCP_WEBSOCKET_PORT = String(ports.ws); + process.env.PENPOT_MCP_REPL_PORT = String(ports.repl); + return { prevServer, prevWs, prevRepl }; +} + +function restoreEnv( + devEnv: string | undefined, + ports: { prevServer: string | undefined; prevWs: string | undefined; prevRepl: string | undefined } +) { + if (devEnv !== undefined) { + process.env.PENPOT_MCP_DEVENV = devEnv; + } else { + delete process.env.PENPOT_MCP_DEVENV; + } + restoreOrDelete("PENPOT_MCP_SERVER_PORT", ports.prevServer); + restoreOrDelete("PENPOT_MCP_WEBSOCKET_PORT", ports.prevWs); + restoreOrDelete("PENPOT_MCP_REPL_PORT", ports.prevRepl); +} + +function restoreOrDelete(key: string, value: string | undefined) { + if (value !== undefined) { + process.env[key] = value; + } else { + delete process.env[key]; + } +} diff --git a/mcp/packages/server/src/PenpotMcpServer.ts b/mcp/packages/server/src/PenpotMcpServer.ts index bd992ec108..49137b48b7 100644 --- a/mcp/packages/server/src/PenpotMcpServer.ts +++ b/mcp/packages/server/src/PenpotMcpServer.ts @@ -56,6 +56,16 @@ export class PenpotMcpServer { */ private static readonly SESSION_TIMEOUT_MINUTES = 60; + /** + * Determines whether the server is running in a Penpot development + * environment, based on the given environment variables. + * + * Returns ``true`` only when ``PENPOT_MCP_DEVENV`` is ``"true"``. + */ + public static isDevEnvEnabled(env: Record): boolean { + return env.PENPOT_MCP_DEVENV === "true"; + } + /** * Returns a short, non-reversible fingerprint of a user token, suitable for * correlating log lines without exposing the full credential. @@ -83,7 +93,7 @@ export class PenpotMcpServer { public readonly configLoader: ConfigurationLoader; private app: any; public readonly pluginBridge: PluginBridge; - private readonly replServer: ReplServer; + private readonly replServer: ReplServer | null; private apiDocs: ApiDocs; private readonly penpotHighLevelOverview: string; private readonly connectionInstructions: string; @@ -149,7 +159,12 @@ export class PenpotMcpServer { } this.pluginBridge = new PluginBridge(this, this.webSocketPort, toolTimeoutSecs, this.redisBridge); - this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.host); + + if (PenpotMcpServer.isDevEnvEnabled(process.env)) { + this.replServer = new ReplServer(this.pluginBridge, this.replPort, this.host); + } else { + this.replServer = null; + } } /** @@ -190,7 +205,16 @@ export class PenpotMcpServer { * additional developer tools such as ClojureScript expression evaluation are exposed. */ public isDevEnv(): boolean { - return process.env.PENPOT_MCP_DEVENV === "true"; + return PenpotMcpServer.isDevEnvEnabled(process.env); + } + + /** + * Indicates whether the REPL server was created. + * + * The REPL server is created only in development environment mode. + */ + public hasReplServer(): boolean { + return this.replServer !== null; } /** @@ -421,8 +445,12 @@ export class PenpotMcpServer { this.logger.info(`Legacy SSE endpoint: http://${this.host}:${this.port}/sse`); this.logger.info(`WebSocket server URL: ws://${this.host}:${this.webSocketPort}`); - // start the REPL server and session timeout checker - await this.replServer.start(); + // start the REPL server (devenv only) and session timeout checker + if (this.replServer) { + await this.replServer.start(); + } else { + this.logger.info("REPL server disabled (set PENPOT_MCP_DEVENV=true to enable)"); + } this.startSessionTimeoutChecker(); resolve(); @@ -438,8 +466,11 @@ export class PenpotMcpServer { public async stop(): Promise { this.logger.info("Stopping Penpot MCP Server..."); clearInterval(this.sessionTimeoutInterval); + await this.pluginBridge.close(); await this.redisBridge?.close(); - await this.replServer.stop(); + if (this.replServer) { + await this.replServer.stop(); + } this.logger.info("Penpot MCP Server stopped"); } } diff --git a/mcp/packages/server/src/PluginBridge.ts b/mcp/packages/server/src/PluginBridge.ts index a362faef36..6a1e07f9c5 100644 --- a/mcp/packages/server/src/PluginBridge.ts +++ b/mcp/packages/server/src/PluginBridge.ts @@ -467,4 +467,16 @@ export class PluginBridge { task.rejectWithError(error instanceof Error ? error : new Error(String(error))); } } + + /** + * Closes the WebSocket server and all connected client sockets. + */ + public async close(): Promise { + return new Promise((resolve) => { + this.wsServer.close(() => { + this.logger.info("WebSocket server closed"); + resolve(); + }); + }); + } }