fix: normalize Windows paths in MCP variable injection (RooCodeInc#4739) (RooCodeInc#4741)

Author: daniel-lxs

src/utils/__tests__/config.spec.ts

@@ -147,5 +147,109 @@ describe("injectVariables", () => {
 		expect(result).toEqual("Hello ")
 	})
 
+	it("should normalize Windows paths with backslashes to use forward slashes in JSON objects", async () => {
+		const config = {
+			command: "mcp-server",
+			args: ["${workspaceFolder}"],
+		}
+		const result = await injectVariables(config, { workspaceFolder: "C:\\Users\\project" })
+		expect(result).toEqual({
+			command: "mcp-server",
+			args: ["C:/Users/project"],
+		})
+	})
+
+	it("should handle complex Windows paths in nested objects", async () => {
+		const config = {
+			servers: {
+				git: {
+					command: "node",
+					args: ["${workspaceFolder}\\scripts\\mcp.js", "${workspaceFolder}\\data"],
+				},
+			},
+		}
+		const result = await injectVariables(config, { workspaceFolder: "C:\\Program Files\\My Project" })
+		expect(result).toEqual({
+			servers: {
+				git: {
+					command: "node",
+					args: ["C:/Program Files/My Project\\scripts\\mcp.js", "C:/Program Files/My Project\\data"],
+				},
+			},
+		})
+	})
+
+	it("should handle Windows paths when entire path is a variable", async () => {
+		const config = {
+			servers: {
+				git: {
+					command: "node",
+					args: ["${scriptPath}", "${dataPath}"],
+				},
+			},
+		}
+		const result = await injectVariables(config, {
+			scriptPath: "C:\\Program Files\\My Project\\scripts\\mcp.js",
+			dataPath: "C:\\Program Files\\My Project\\data",
+		})
+		expect(result).toEqual({
+			servers: {
+				git: {
+					command: "node",
+					args: ["C:/Program Files/My Project/scripts/mcp.js", "C:/Program Files/My Project/data"],
+				},
+			},
+		})
+	})
+
+	it("should normalize backslashes in plain string replacements", async () => {
+		const result = await injectVariables("Path: ${path}", { path: "C:\\Users\\test" })
+		expect(result).toEqual("Path: C:/Users/test")
+	})
+
+	it("should handle paths with mixed slashes", async () => {
+		const config = {
+			path: "${testPath}",
+		}
+		const result = await injectVariables(config, { testPath: "C:\\Users/test/mixed\\path" })
+		expect(result).toEqual({
+			path: "C:/Users/test/mixed/path",
+		})
+	})
+
+	it("should not affect non-path strings", async () => {
+		const config = {
+			message: "This is a string with a backslash \\ and a value: ${myValue}",
+		}
+		const result = await injectVariables(config, { myValue: "test" })
+		expect(result).toEqual({
+			message: "This is a string with a backslash \\ and a value: test",
+		})
+	})
+
+	it("should handle various non-path variables correctly", async () => {
+		const config = {
+			apiKey: "${key}",
+			url: "${endpoint}",
+			port: "${port}",
+			enabled: "${enabled}",
+			description: "${desc}",
+		}
+		const result = await injectVariables(config, {
+			key: "sk-1234567890abcdef",
+			endpoint: "https://api.example.com",
+			port: "8080",
+			enabled: "true",
+			desc: "This is a description with special chars: @#$%^&*()",
+		})
+		expect(result).toEqual({
+			apiKey: "sk-1234567890abcdef",
+			url: "https://api.example.com",
+			port: "8080",
+			enabled: "true",
+			description: "This is a description with special chars: @#$%^&*()",
+		})
+	})
+
 	// Variable maps are already tested by `injectEnv` tests above.
 })

src/utils/config.ts

@@ -37,23 +37,30 @@ export async function injectVariables<C extends InjectableConfigType>(
 	variables: Record<string, undefined | null | string | Record<string, undefined | null | string>>,
 	propNotFoundValue?: any,
 ) {
-	// Use simple regex replace for now, will see if object traversal and recursion is needed here (e.g: for non-serializable objects)
 	const isObject = typeof config === "object"
-	let _config: string = isObject ? JSON.stringify(config) : config
+	let configString: string = isObject ? JSON.stringify(config) : config
 
-	// Intentionally using `== null` to match null | undefined
 	for (const [key, value] of Object.entries(variables)) {
 		if (value == null) continue
 
-		if (typeof value === "string") _config = _config.replace(new RegExp(`\\$\\{${key}\\}`, "g"), value)
-		else
-			_config = _config.replace(new RegExp(`\\$\\{${key}:([\\w]+)\\}`, "g"), (match, name) => {
-				if (value[name] == null)
+		if (typeof value === "string") {
+			// Normalize paths to forward slashes for cross-platform compatibility
+			configString = configString.replace(new RegExp(`\\$\\{${key}\\}`, "g"), value.toPosix())
+		} else {
+			// Handle nested variables (e.g., ${env:VAR_NAME})
+			configString = configString.replace(new RegExp(`\\$\\{${key}:([\\w]+)\\}`, "g"), (match, name) => {
+				const nestedValue = value[name]
+
+				if (nestedValue == null) {
 					console.warn(`[injectVariables] variable "${name}" referenced but not found in "${key}"`)
+					return propNotFoundValue ?? match
+				}
 
-				return value[name] ?? propNotFoundValue ?? match
+				// Normalize paths for string values
+				return typeof nestedValue === "string" ? nestedValue.toPosix() : nestedValue
 			})
+		}
 	}
 
-	return (isObject ? JSON.parse(_config) : _config) as C extends string ? string : C
+	return (isObject ? JSON.parse(configString) : configString) as C extends string ? string : C
 }