Skip to content

Commit 2490f49

Browse files
committed
fix: preserve quoted args with spaces when launching via CLI
1 parent 94d98ec commit 2490f49

File tree

8 files changed

+218
-5
lines changed

8 files changed

+218
-5
lines changed

cli/__tests__/cli.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -868,4 +868,31 @@ describe("CLI Tests", () => {
868868
expectCliFailure(result);
869869
});
870870
});
871+
872+
describe("Args With Spaces", () => {
873+
it("passes an arg containing spaces to the child process as a single argv element", async () => {
874+
const { command, args } = getTestMcpServerCommand();
875+
const result = await runCli([
876+
command,
877+
...args,
878+
// Extra arg passed to the test MCP server — contains a space
879+
"--description",
880+
"get todays date",
881+
"--cli",
882+
"--method",
883+
"resources/read",
884+
"--uri",
885+
"test://argv",
886+
]);
887+
888+
expectCliSuccess(result);
889+
const json = expectValidJson(result);
890+
const argv: string[] = JSON.parse(json.contents[0].text);
891+
892+
// "get todays date" must appear as one element, not split into three
893+
expect(argv).toContain("get todays date");
894+
expect(argv).not.toContain("get");
895+
expect(argv).not.toContain("todays");
896+
});
897+
});
871898
});

client/bin/start.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ async function startProdServer(serverOptions) {
106106
inspectorServerPath,
107107
...(command ? [`--command=${command}`] : []),
108108
...(mcpServerArgs && mcpServerArgs.length > 0
109-
? [`--args=${mcpServerArgs.join(" ")}`]
109+
? [`--args=${JSON.stringify(mcpServerArgs)}`]
110110
: []),
111111
...(transport ? [`--transport=${transport}`] : []),
112112
...(serverUrl ? [`--server-url=${serverUrl}`] : []),

package-lock.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/**
2+
* Integration tests for the /config endpoint's args handling.
3+
*
4+
* These tests spawn the real proxy server process and make actual HTTP requests
5+
* to verify that args containing spaces survive the serialisation round-trip
6+
* introduced by the start.js → server path.
7+
*/
8+
9+
import { spawn, type ChildProcess } from "child_process";
10+
import { resolve } from "path";
11+
12+
const SERVER_SRC = resolve(__dirname, "../src/index.ts");
13+
14+
// Fixed credentials — the server is bound to localhost only and killed after each test.
15+
const AUTH_TOKEN = "test-token";
16+
const CLIENT_PORT = "17274";
17+
const ORIGIN = `http://localhost:${CLIENT_PORT}`;
18+
19+
// Use a different port per test to avoid EADDRINUSE across sequential runs.
20+
let portSeed = 17280;
21+
22+
interface ServerHandle {
23+
port: number;
24+
process: ChildProcess;
25+
}
26+
27+
async function startServer(extraArgs: string[] = []): Promise<ServerHandle> {
28+
const port = portSeed++;
29+
30+
const proc = spawn("tsx", [SERVER_SRC, ...extraArgs], {
31+
env: {
32+
...process.env,
33+
SERVER_PORT: String(port),
34+
CLIENT_PORT,
35+
MCP_PROXY_AUTH_TOKEN: AUTH_TOKEN,
36+
ALLOWED_ORIGINS: ORIGIN,
37+
},
38+
stdio: "pipe",
39+
});
40+
41+
await waitForHealth(port);
42+
return { port, process: proc };
43+
}
44+
45+
async function waitForHealth(port: number, timeoutMs = 5000): Promise<void> {
46+
const deadline = Date.now() + timeoutMs;
47+
while (Date.now() < deadline) {
48+
try {
49+
const res = await fetch(`http://localhost:${port}/health`);
50+
if (res.ok) return;
51+
} catch {
52+
// server not up yet
53+
}
54+
await new Promise((r) => setTimeout(r, 100));
55+
}
56+
throw new Error(
57+
`Proxy server on port ${port} did not become healthy in ${timeoutMs}ms`,
58+
);
59+
}
60+
61+
async function fetchConfig(handle: ServerHandle): Promise<{
62+
defaultArgs: string;
63+
defaultCommand: string;
64+
}> {
65+
const res = await fetch(`http://localhost:${handle.port}/config`, {
66+
headers: {
67+
Origin: ORIGIN,
68+
"x-mcp-proxy-auth": `Bearer ${AUTH_TOKEN}`,
69+
},
70+
});
71+
if (!res.ok) throw new Error(`/config returned ${res.status}`);
72+
return res.json() as Promise<{ defaultArgs: string; defaultCommand: string }>;
73+
}
74+
75+
let currentHandle: ServerHandle | null = null;
76+
77+
afterEach(() => {
78+
currentHandle?.process.kill();
79+
currentHandle = null;
80+
});
81+
82+
describe("proxy server /config: args passed from start.js", () => {
83+
it("converts JSON-array args (new start.js format) to a shell-quoted string", async () => {
84+
// start.js now does: `--args=${JSON.stringify(mcpServerArgs)}`
85+
const args = ["--description", "get todays date", "--command", "date"];
86+
currentHandle = await startServer([`--args=${JSON.stringify(args)}`]);
87+
88+
const config = await fetchConfig(currentHandle);
89+
90+
// The /config endpoint must shell-quote the array so the client UI can
91+
// display it and shellParseArgs can round-trip it correctly.
92+
expect(config.defaultArgs).toBe(
93+
"--description 'get todays date' --command date",
94+
);
95+
});
96+
97+
it("passes a legacy plain shell string through unchanged (backward compat)", async () => {
98+
// Direct invocations of the server binary that pass --args as a plain
99+
// shell string must continue to work.
100+
currentHandle = await startServer([
101+
"--args=--description 'get todays date' --command date",
102+
]);
103+
104+
const config = await fetchConfig(currentHandle);
105+
106+
expect(config.defaultArgs).toBe(
107+
"--description 'get todays date' --command date",
108+
);
109+
});
110+
111+
it("returns an empty string when no --args flag is given", async () => {
112+
currentHandle = await startServer([]);
113+
const config = await fetchConfig(currentHandle);
114+
expect(config.defaultArgs).toBe("");
115+
});
116+
117+
it("handles args with backslashes", async () => {
118+
const args = ["--path", "C:\\Users\\foo"];
119+
currentHandle = await startServer([`--args=${JSON.stringify(args)}`]);
120+
const config = await fetchConfig(currentHandle);
121+
122+
// Verify the round-trip: parse the shell-quoted string back into an array
123+
const { parse } = await import("shell-quote");
124+
const parsed = parse(config.defaultArgs) as string[];
125+
expect(parsed).toEqual(args);
126+
});
127+
128+
it("handles args that look like JSON themselves", async () => {
129+
const args = ["--config", '{"key":"val"}'];
130+
currentHandle = await startServer([`--args=${JSON.stringify(args)}`]);
131+
const config = await fetchConfig(currentHandle);
132+
133+
const { parse } = await import("shell-quote");
134+
const parsed = parse(config.defaultArgs) as string[];
135+
expect(parsed).toEqual(args);
136+
});
137+
});

server/jest.config.cjs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
module.exports = {
2+
preset: "ts-jest",
3+
testEnvironment: "node",
4+
transform: {
5+
"^.+\\.tsx?$": [
6+
"ts-jest",
7+
{
8+
tsconfig: "tsconfig.test.json",
9+
},
10+
],
11+
},
12+
transformIgnorePatterns: ["node_modules/(?!(@modelcontextprotocol)/)"],
13+
testRegex: "(/__tests__/.*|(\\.|/)(test|spec))\\.(jsx?|tsx?)$",
14+
};

server/package.json

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,17 @@
1919
"build": "tsc && shx cp -R static build",
2020
"start": "node build/index.js",
2121
"dev": "tsx watch --clear-screen=false src/index.ts",
22-
"dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL"
22+
"dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL",
23+
"test": "jest --config jest.config.cjs"
2324
},
2425
"devDependencies": {
2526
"@types/cors": "^2.8.19",
2627
"@types/express": "^5.0.0",
2728
"@types/shell-quote": "^1.7.5",
2829
"@types/ws": "^8.5.12",
30+
"@types/jest": "^29.5.14",
31+
"jest": "^29.7.0",
32+
"ts-jest": "^29.4.0",
2933
"tsx": "^4.19.0",
3034
"typescript": "^5.6.2"
3135
},

server/src/index.ts

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import cors from "cors";
44
import { parseArgs } from "node:util";
5-
import { parse as shellParseArgs } from "shell-quote";
5+
import { parse as shellParseArgs, quote as shellQuoteArgs } from "shell-quote";
66
import nodeFetch, { Headers as NodeHeaders } from "node-fetch";
77

88
// Type-compatible wrappers for node-fetch to work with browser-style types
@@ -375,7 +375,16 @@ const createTransport = async (
375375

376376
if (transportType === "stdio") {
377377
const command = (query.command as string).trim();
378-
const origArgs = shellParseArgs(query.args as string) as string[];
378+
const rawArgs = query.args as string;
379+
let origArgs: string[];
380+
try {
381+
const parsed = JSON.parse(rawArgs);
382+
origArgs = Array.isArray(parsed)
383+
? (parsed as string[])
384+
: (shellParseArgs(rawArgs) as string[]);
385+
} catch {
386+
origArgs = shellParseArgs(rawArgs) as string[];
387+
}
379388
const queryEnv = query.env ? JSON.parse(query.env as string) : {};
380389
const env = { ...defaultEnvironment, ...process.env, ...queryEnv };
381390

@@ -786,7 +795,18 @@ app.get("/config", originValidationMiddleware, authMiddleware, (req, res) => {
786795
res.json({
787796
defaultEnvironment,
788797
defaultCommand: values.command,
789-
defaultArgs: values.args,
798+
defaultArgs: (() => {
799+
if (!values.args) return values.args;
800+
try {
801+
const parsed = JSON.parse(values.args);
802+
if (Array.isArray(parsed)) {
803+
return shellQuoteArgs(parsed);
804+
}
805+
} catch {
806+
// Not JSON — legacy shell string, pass through unchanged
807+
}
808+
return values.args;
809+
})(),
790810
defaultTransport: values.transport,
791811
defaultServerUrl: values["server-url"],
792812
});

server/tsconfig.test.json

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"extends": "./tsconfig.json",
3+
"compilerOptions": {
4+
"module": "CommonJS",
5+
"moduleResolution": "Node"
6+
},
7+
"include": ["__tests__/**/*", "src/**/*"]
8+
}

0 commit comments

Comments
 (0)