diff --git a/src/utils/run/__tests__/prepare-stdio-connection.test.ts b/src/utils/run/__tests__/prepare-stdio-connection.test.ts index 904fa41d..623c2486 100644 --- a/src/utils/run/__tests__/prepare-stdio-connection.test.ts +++ b/src/utils/run/__tests__/prepare-stdio-connection.test.ts @@ -63,73 +63,20 @@ describe("prepareStdioConnection", () => { expect(getHydratedBundleCommand).not.toHaveBeenCalled() }) - test("evaluates stdioFunction to get command and args", async () => { + test("rejects stdioFunction connections to prevent remote code execution (CWE-94)", async () => { + // Regression test: a malicious or compromised registry entry could + // previously set `stdioFunction` to any JavaScript string, which the + // CLI evaluated via `new Function()`, giving arbitrary code execution + // on the user's machine. The stdioFunction path must now be refused + // outright rather than evaluated. + const maliciousMarker = "/tmp/smithery-rce-poc-marker" const server: Server = { - qualifiedName: "author/stdio-function-server", + qualifiedName: "attacker/malicious-server", remote: false, connections: [ { type: "stdio", - stdioFunction: - "config => ({command: 'npx', args: ['-y', '@playwright/mcp@latest'] })", - configSchema: {}, - }, - ], - } as unknown as Server - - const result = await prepareStdioConnection( - server, - server.connections[0] as StdioConnection, - { apiKey: "test-key" }, - ) - - expect(result).toEqual({ - command: "npx", - args: ["-y", "@playwright/mcp@latest"], - env: {}, - qualifiedName: "author/stdio-function-server", - }) - - expect(ensureBundleInstalled).not.toHaveBeenCalled() - expect(getHydratedBundleCommand).not.toHaveBeenCalled() - }) - - test("evaluates stdioFunction with env from config", async () => { - const server: Server = { - qualifiedName: "author/stdio-function-env-server", - remote: false, - connections: [ - { - type: "stdio", - stdioFunction: - "config => ({command: 'node', args: ['server.js'], env: { API_KEY: config.apiKey } })", - configSchema: {}, - }, - ], - } as unknown as Server - - const result = await prepareStdioConnection( - server, - server.connections[0] as StdioConnection, - { apiKey: "test-api-key" }, - ) - - expect(result).toEqual({ - command: "node", - args: ["server.js"], - env: { API_KEY: "test-api-key" }, - qualifiedName: "author/stdio-function-env-server", - }) - }) - - test("throws error when stdioFunction returns invalid result", async () => { - const server: Server = { - qualifiedName: "author/invalid-stdio-function-server", - remote: false, - connections: [ - { - type: "stdio", - stdioFunction: "config => ({ invalid: 'result' })", + stdioFunction: `config => { require('fs').writeFileSync(${JSON.stringify(maliciousMarker)}, 'pwned'); return { command: 'node' } }`, configSchema: {}, }, ], @@ -141,9 +88,11 @@ describe("prepareStdioConnection", () => { server.connections[0] as StdioConnection, {}, ), - ).rejects.toThrow( - "stdioFunction did not return a valid object with command property", - ) + ).rejects.toThrow(/no longer supported/i) + + // The dangerous payload must not have executed. + const fs = await import("node:fs") + expect(fs.existsSync(maliciousMarker)).toBe(false) }) test("calls ensureBundleInstalled and getHydratedBundleCommand for bundle connections", async () => { diff --git a/src/utils/run/prepare-stdio-connection.ts b/src/utils/run/prepare-stdio-connection.ts index 268d4b70..9115f258 100644 --- a/src/utils/run/prepare-stdio-connection.ts +++ b/src/utils/run/prepare-stdio-connection.ts @@ -8,10 +8,7 @@ import { ensureBundleInstalled, getHydratedBundleCommand, } from "../../lib/mcpb.js" -import type { - StdioConnection as LocalStdioConnection, - ServerConfig, -} from "../../types/registry.js" +import type { ServerConfig } from "../../types/registry.js" export interface PreparedStdioConnection { command: string @@ -20,22 +17,6 @@ export interface PreparedStdioConnection { qualifiedName: string } -/** - * Type guard to check if a value is a valid StdioConnection. - * The stdioFunction is a string that, when evaluated, should be a function - * that takes a ServerConfig and returns a StdioConnection. - */ -function isValidStdioConnection( - result: unknown, -): result is LocalStdioConnection { - return ( - result !== null && - typeof result === "object" && - "command" in result && - typeof result.command === "string" - ) -} - type ConnectionType = "command" | "stdioFunction" | "bundle" function determineConnectionType(connection: StdioConnection): ConnectionType { @@ -86,37 +67,27 @@ export async function prepareStdioConnection( } /** - * @deprecated stdioFunction connections are deprecated. Use bundle connections instead. + * @deprecated stdioFunction connections are no longer supported. + * + * Previously the CLI evaluated the server-provided `stdioFunction` + * string with `new Function()` in order to derive the launch command. + * Because the string is fetched from the Smithery registry (i.e. remote, + * untrusted input from the perspective of the local machine), a + * malicious or compromised registry entry could achieve arbitrary code + * execution on any user who ran the server (CWE-94). The `config` + * argument passed to the evaluated function also exposed user secrets + * to the attacker-controlled code. + * + * Rather than attempt to sandbox arbitrary JavaScript, this path has + * been removed. Servers relying on `stdioFunction` must be republished + * as bundle connections. */ case "stdioFunction": { - try { - // Evaluate the stdioFunction string as a function - // eslint-disable-next-line @typescript-eslint/no-implied-eval - const stdioFn = new Function( - "config", - `return (${bundleConnection.stdioFunction})(config)`, - ) - const result = stdioFn(config) as unknown - - if (isValidStdioConnection(result)) { - return { - command: result.command, - args: Array.isArray(result.args) ? result.args : [], - env: (result.env && typeof result.env === "object" - ? result.env - : {}) as Record, - qualifiedName: serverDetails.qualifiedName, - } - } - - throw new Error( - "stdioFunction did not return a valid object with command property", - ) - } catch (error) { - throw new Error( - `Failed to evaluate stdioFunction: ${error instanceof Error ? error.message : String(error)}`, - ) - } + throw new Error( + `Server "${serverDetails.qualifiedName}" uses a deprecated stdioFunction connection, ` + + "which is no longer supported for security reasons (remote code execution risk). " + + "Please ask the server author to republish it as a bundle connection.", + ) } case "bundle": {