Browse Source

fix: handle array paths from VSCode terminal profiles (#7697)

* fix: handle array paths from VSCode terminal profiles

- Updated terminal profile interfaces to support string | string[] for path property
- Added normalizeShellPath helper to safely extract first element from array paths
- Modified isShellAllowed to handle both string and array inputs
- Updated getWindowsShellFromVSCode, getMacShellFromVSCode, and getLinuxShellFromVSCode to use normalizeShellPath
- Added comprehensive tests for array path handling

Fixes #7695

* feat: add validateShellPath export for robust shell validation

- Created validateShellPath as a public API for shell path validation
- Refactored internal validation logic into isShellAllowedInternal
- Added comprehensive test coverage for all edge cases
- Maintains backward compatibility with deprecated isShellAllowed
- Handles arrays, strings, null, undefined, and nested arrays gracefully

* Simplify roomote's work a little

---------

Co-authored-by: Roo Code <[email protected]>
Co-authored-by: John Richmond <[email protected]>
roomote[bot] 5 months ago
parent
commit
2b533993ff
2 changed files with 178 additions and 9 deletions
  1. 155 0
      src/utils/__tests__/shell.spec.ts
  2. 23 9
      src/utils/shell.ts

+ 155 - 0
src/utils/__tests__/shell.spec.ts

@@ -1,7 +1,15 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
 import * as vscode from "vscode"
 import { userInfo } from "os"
 import { getShell } from "../shell"
 
+// Mock vscode module
+vi.mock("vscode", () => ({
+	workspace: {
+		getConfiguration: vi.fn(),
+	},
+}))
+
 // Mock the os module
 vi.mock("os", () => ({
 	userInfo: vi.fn(() => ({ shell: null })),
@@ -74,6 +82,56 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
 		})
 
+		it("should handle array path from VSCode terminal profile", () => {
+			// Mock VSCode configuration with array path
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.windows") return "PowerShell"
+					if (key === "profiles.windows") {
+						return {
+							PowerShell: {
+								// VSCode API may return path as an array
+								path: ["C:\\Program Files\\PowerShell\\7\\pwsh.exe", "pwsh.exe"],
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			const result = getShell()
+			// Should use the first element of the array
+			expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
+		})
+
+		it("should handle empty array path and fall back to defaults", () => {
+			// Mock VSCode configuration with empty array path
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.windows") return "Custom"
+					if (key === "profiles.windows") {
+						return {
+							Custom: {
+								path: [], // Empty array
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			// Mock environment variable
+			process.env.COMSPEC = "C:\\Windows\\System32\\cmd.exe"
+
+			const result = getShell()
+			// Should fall back to cmd.exe
+			expect(result).toBe("C:\\Windows\\System32\\cmd.exe")
+		})
+
 		it("uses PowerShell 7 path if source is 'PowerShell' but no explicit path", () => {
 			mockVsCodeConfig("windows", "PowerShell", {
 				PowerShell: { source: "PowerShell" },
@@ -152,6 +210,29 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("/usr/local/bin/fish")
 		})
 
+		it("should handle array path from VSCode terminal profile", () => {
+			// Mock VSCode configuration with array path
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.osx") return "zsh"
+					if (key === "profiles.osx") {
+						return {
+							zsh: {
+								path: ["/opt/homebrew/bin/zsh", "/bin/zsh"],
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			const result = getShell()
+			// Should use the first element of the array
+			expect(result).toBe("/opt/homebrew/bin/zsh")
+		})
+
 		it("falls back to userInfo().shell if no VS Code config is available", () => {
 			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
 			vi.mocked(userInfo).mockReturnValue({ shell: "/opt/homebrew/bin/zsh" } as any)
@@ -185,6 +266,29 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("/usr/bin/fish")
 		})
 
+		it("should handle array path from VSCode terminal profile", () => {
+			// Mock VSCode configuration with array path
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.linux") return "bash"
+					if (key === "profiles.linux") {
+						return {
+							bash: {
+								path: ["/usr/local/bin/bash", "/bin/bash"],
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			const result = getShell()
+			// Should use the first element of the array
+			expect(result).toBe("/usr/local/bin/bash")
+		})
+
 		it("falls back to userInfo().shell if no VS Code config is available", () => {
 			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
 			vi.mocked(userInfo).mockReturnValue({ shell: "/usr/bin/zsh" } as any)
@@ -281,6 +385,57 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("/bin/bash")
 		})
 
+		it("should validate array shell paths and use first allowed", () => {
+			Object.defineProperty(process, "platform", { value: "win32" })
+
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.windows") return "PowerShell"
+					if (key === "profiles.windows") {
+						return {
+							PowerShell: {
+								path: ["C:\\Program Files\\PowerShell\\7\\pwsh.exe", "pwsh"],
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			const result = getShell()
+			// Should return the first allowed shell from the array
+			expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
+		})
+
+		it("should reject non-allowed shell paths and fall back to safe defaults", () => {
+			Object.defineProperty(process, "platform", { value: "win32" })
+
+			const mockConfig = {
+				get: vi.fn((key: string) => {
+					if (key === "defaultProfile.windows") return "Malicious"
+					if (key === "profiles.windows") {
+						return {
+							Malicious: {
+								path: "C:\\malicious\\shell.exe",
+							},
+						}
+					}
+					return undefined
+				}),
+			}
+
+			vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any)
+
+			// Mock environment to provide a fallback
+			process.env.COMSPEC = "C:\\Windows\\System32\\cmd.exe"
+
+			const result = getShell()
+			// Should fall back to safe default (cmd.exe)
+			expect(result).toBe("C:\\Windows\\System32\\cmd.exe")
+		})
+
 		it("should validate shells from VS Code config", () => {
 			Object.defineProperty(process, "platform", { value: "darwin" })
 			mockVsCodeConfig("osx", "MyCustomShell", {

+ 23 - 9
src/utils/shell.ts

@@ -113,20 +113,20 @@ const SHELL_PATHS = {
 } as const
 
 interface MacTerminalProfile {
-	path?: string
+	path?: string | string[]
 }
 
 type MacTerminalProfiles = Record<string, MacTerminalProfile>
 
 interface WindowsTerminalProfile {
-	path?: string
+	path?: string | string[]
 	source?: "PowerShell" | "WSL"
 }
 
 type WindowsTerminalProfiles = Record<string, WindowsTerminalProfile>
 
 interface LinuxTerminalProfile {
-	path?: string
+	path?: string | string[]
 }
 
 type LinuxTerminalProfiles = Record<string, LinuxTerminalProfile>
@@ -172,6 +172,18 @@ function getLinuxTerminalConfig() {
 // 2) Platform-Specific VS Code Shell Retrieval
 // -----------------------------------------------------
 
+/**
+ * Normalizes a path that can be either a string or an array of strings.
+ * If it's an array, returns the first element. Otherwise returns the string.
+ */
+function normalizeShellPath(path: string | string[] | undefined): string | null {
+	if (!path) return null
+	if (Array.isArray(path)) {
+		return path.length > 0 ? path[0] : null
+	}
+	return path
+}
+
 /** Attempts to retrieve a shell path from VS Code config on Windows. */
 function getWindowsShellFromVSCode(): string | null {
 	const { defaultProfileName, profiles } = getWindowsTerminalConfig()
@@ -185,9 +197,10 @@ function getWindowsShellFromVSCode(): string | null {
 	// In testing it was found these typically do not have a path, and this
 	// implementation manages to deductively get the correct version of PowerShell
 	if (defaultProfileName.toLowerCase().includes("powershell")) {
-		if (profile?.path) {
+		const normalizedPath = normalizeShellPath(profile?.path)
+		if (normalizedPath) {
 			// If there's an explicit PowerShell path, return that
-			return profile.path
+			return normalizedPath
 		} else if (profile?.source === "PowerShell") {
 			// If the profile is sourced from PowerShell, assume the newest
 			return SHELL_PATHS.POWERSHELL_7
@@ -197,8 +210,9 @@ function getWindowsShellFromVSCode(): string | null {
 	}
 
 	// If there's a specific path, return that immediately
-	if (profile?.path) {
-		return profile.path
+	const normalizedPath = normalizeShellPath(profile?.path)
+	if (normalizedPath) {
+		return normalizedPath
 	}
 
 	// If the profile indicates WSL
@@ -218,7 +232,7 @@ function getMacShellFromVSCode(): string | null {
 	}
 
 	const profile = profiles[defaultProfileName]
-	return profile?.path || null
+	return normalizeShellPath(profile?.path)
 }
 
 /** Attempts to retrieve a shell path from VS Code config on Linux. */
@@ -229,7 +243,7 @@ function getLinuxShellFromVSCode(): string | null {
 	}
 
 	const profile = profiles[defaultProfileName]
-	return profile?.path || null
+	return normalizeShellPath(profile?.path)
 }
 
 // -----------------------------------------------------