Browse Source

Use an allowlist to keep the prompt default shell sane (#7681)

John Richmond 4 months ago
parent
commit
467c9f597d
2 changed files with 262 additions and 31 deletions
  1. 108 6
      src/utils/__tests__/shell.spec.ts
  2. 154 25
      src/utils/shell.ts

+ 108 - 6
src/utils/__tests__/shell.spec.ts

@@ -7,6 +7,15 @@ vi.mock("os", () => ({
 	userInfo: vi.fn(() => ({ shell: null })),
 }))
 
+// Mock path module for testing
+vi.mock("path", async () => {
+	const actual = await vi.importActual("path")
+	return {
+		...actual,
+		normalize: vi.fn((p: string) => p),
+	}
+})
+
 describe("Shell Detection Tests", () => {
 	let originalPlatform: string
 	let originalEnv: NodeJS.ProcessEnv
@@ -106,18 +115,25 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
 		})
 
-		it("respects userInfo() if no VS Code config is available", () => {
+		it("respects userInfo() if no VS Code config is available and shell is allowed", () => {
+			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
+			vi.mocked(userInfo).mockReturnValue({ shell: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" } as any)
+
+			expect(getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
+		})
+
+		it("falls back to safe shell when userInfo() returns non-allowlisted shell", () => {
 			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
 			vi.mocked(userInfo).mockReturnValue({ shell: "C:\\Custom\\PowerShell.exe" } as any)
 
-			expect(getShell()).toBe("C:\\Custom\\PowerShell.exe")
+			expect(getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
 		})
 
-		it("respects an odd COMSPEC if no userInfo shell is available", () => {
+		it("falls back to safe shell when COMSPEC is non-allowlisted", () => {
 			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
 			process.env.COMSPEC = "D:\\CustomCmd\\cmd.exe"
 
-			expect(getShell()).toBe("D:\\CustomCmd\\cmd.exe")
+			expect(getShell()).toBe("C:\\Windows\\System32\\cmd.exe")
 		})
 	})
 
@@ -191,10 +207,10 @@ describe("Shell Detection Tests", () => {
 	// Unknown Platform & Error Handling
 	// --------------------------------------------------------------------------
 	describe("Unknown Platform / Error Handling", () => {
-		it("falls back to /bin/sh for unknown platforms", () => {
+		it("falls back to /bin/bash for unknown platforms", () => {
 			Object.defineProperty(process, "platform", { value: "sunos" })
 			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
-			expect(getShell()).toBe("/bin/sh")
+			expect(getShell()).toBe("/bin/bash")
 		})
 
 		it("handles VS Code config errors gracefully, falling back to userInfo shell if present", () => {
@@ -228,4 +244,90 @@ describe("Shell Detection Tests", () => {
 			expect(getShell()).toBe("/bin/bash")
 		})
 	})
+
+	// --------------------------------------------------------------------------
+	// Shell Validation Tests
+	// --------------------------------------------------------------------------
+	describe("Shell Validation", () => {
+		it("should allow common Windows shells", () => {
+			Object.defineProperty(process, "platform", { value: "win32" })
+			mockVsCodeConfig("windows", "PowerShell", {
+				PowerShell: { path: "C:\\Program Files\\PowerShell\\7\\pwsh.exe" },
+			})
+			expect(getShell()).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe")
+		})
+
+		it("should allow common Unix shells", () => {
+			Object.defineProperty(process, "platform", { value: "linux" })
+			mockVsCodeConfig("linux", "CustomProfile", {
+				CustomProfile: { path: "/usr/bin/fish" },
+			})
+			expect(getShell()).toBe("/usr/bin/fish")
+		})
+
+		it("should handle case-insensitive matching on Windows", () => {
+			Object.defineProperty(process, "platform", { value: "win32" })
+			mockVsCodeConfig("windows", "PowerShell", {
+				PowerShell: { path: "c:\\windows\\system32\\cmd.exe" },
+			})
+			expect(getShell()).toBe("c:\\windows\\system32\\cmd.exe")
+		})
+
+		it("should reject unknown shells and use fallback", () => {
+			Object.defineProperty(process, "platform", { value: "linux" })
+			mockVsCodeConfig("linux", "CustomProfile", {
+				CustomProfile: { path: "/usr/bin/malicious-shell" },
+			})
+			expect(getShell()).toBe("/bin/bash")
+		})
+
+		it("should validate shells from VS Code config", () => {
+			Object.defineProperty(process, "platform", { value: "darwin" })
+			mockVsCodeConfig("osx", "MyCustomShell", {
+				MyCustomShell: { path: "/usr/local/bin/custom-shell" },
+			})
+
+			const result = getShell()
+			expect(result).toBe("/bin/zsh") // macOS fallback
+		})
+
+		it("should validate shells from userInfo", () => {
+			Object.defineProperty(process, "platform", { value: "linux" })
+			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
+			vi.mocked(userInfo).mockReturnValue({ shell: "/usr/bin/evil-shell" } as any)
+
+			const result = getShell()
+			expect(result).toBe("/bin/bash") // Linux fallback
+		})
+
+		it("should validate shells from environment variables", () => {
+			Object.defineProperty(process, "platform", { value: "linux" })
+			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
+			vi.mocked(userInfo).mockReturnValue({ shell: null } as any)
+			process.env.SHELL = "/opt/custom/shell"
+
+			const result = getShell()
+			expect(result).toBe("/bin/bash") // Linux fallback
+		})
+
+		it("should handle WSL bash correctly", () => {
+			Object.defineProperty(process, "platform", { value: "win32" })
+			mockVsCodeConfig("windows", "WSL", {
+				WSL: { source: "WSL" },
+			})
+
+			const result = getShell()
+			expect(result).toBe("/bin/bash") // Should be allowed
+		})
+
+		it("should handle empty or null shell paths", () => {
+			Object.defineProperty(process, "platform", { value: "linux" })
+			vscode.workspace.getConfiguration = () => ({ get: () => undefined }) as any
+			vi.mocked(userInfo).mockReturnValue({ shell: "" } as any)
+			delete process.env.SHELL
+
+			const result = getShell()
+			expect(result).toBe("/bin/bash") // Should fall back to safe default
+		})
+	})
 })

+ 154 - 25
src/utils/shell.ts

@@ -1,5 +1,97 @@
 import * as vscode from "vscode"
 import { userInfo } from "os"
+import * as path from "path"
+
+// Security: Allowlist of approved shell executables to prevent arbitrary command execution
+const SHELL_ALLOWLIST = new Set<string>([
+	// Windows PowerShell variants
+	"C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
+	"C:\\Program Files\\PowerShell\\7\\pwsh.exe",
+	"C:\\Program Files\\PowerShell\\6\\pwsh.exe",
+	"C:\\Program Files\\PowerShell\\5\\pwsh.exe",
+
+	// Windows Command Prompt
+	"C:\\Windows\\System32\\cmd.exe",
+
+	// Windows WSL
+	"C:\\Windows\\System32\\wsl.exe",
+
+	// Git Bash on Windows
+	"C:\\Program Files\\Git\\bin\\bash.exe",
+	"C:\\Program Files\\Git\\usr\\bin\\bash.exe",
+	"C:\\Program Files (x86)\\Git\\bin\\bash.exe",
+	"C:\\Program Files (x86)\\Git\\usr\\bin\\bash.exe",
+
+	// MSYS2/MinGW/Cygwin on Windows
+	"C:\\msys64\\usr\\bin\\bash.exe",
+	"C:\\msys32\\usr\\bin\\bash.exe",
+	"C:\\MinGW\\msys\\1.0\\bin\\bash.exe",
+	"C:\\cygwin64\\bin\\bash.exe",
+	"C:\\cygwin\\bin\\bash.exe",
+
+	// Unix/Linux/macOS - Bourne-compatible shells
+	"/bin/sh",
+	"/usr/bin/sh",
+	"/bin/bash",
+	"/usr/bin/bash",
+	"/usr/local/bin/bash",
+	"/opt/homebrew/bin/bash",
+	"/opt/local/bin/bash",
+
+	// Z Shell
+	"/bin/zsh",
+	"/usr/bin/zsh",
+	"/usr/local/bin/zsh",
+	"/opt/homebrew/bin/zsh",
+	"/opt/local/bin/zsh",
+
+	// Dash
+	"/bin/dash",
+	"/usr/bin/dash",
+
+	// Ash
+	"/bin/ash",
+	"/usr/bin/ash",
+
+	// C Shells
+	"/bin/csh",
+	"/usr/bin/csh",
+	"/bin/tcsh",
+	"/usr/bin/tcsh",
+	"/usr/local/bin/tcsh",
+
+	// Korn Shells
+	"/bin/ksh",
+	"/usr/bin/ksh",
+	"/bin/ksh93",
+	"/usr/bin/ksh93",
+	"/bin/mksh",
+	"/usr/bin/mksh",
+	"/bin/pdksh",
+	"/usr/bin/pdksh",
+
+	// Fish Shell
+	"/usr/bin/fish",
+	"/usr/local/bin/fish",
+	"/opt/homebrew/bin/fish",
+	"/opt/local/bin/fish",
+
+	// Modern shells
+	"/usr/bin/elvish",
+	"/usr/local/bin/elvish",
+	"/usr/bin/xonsh",
+	"/usr/local/bin/xonsh",
+	"/usr/bin/nu",
+	"/usr/local/bin/nu",
+	"/usr/bin/nushell",
+	"/usr/local/bin/nushell",
+	"/usr/bin/ion",
+	"/usr/local/bin/ion",
+
+	// BusyBox
+	"/bin/busybox",
+	"/usr/bin/busybox",
+])
 
 const SHELL_PATHS = {
 	// Windows paths
@@ -179,49 +271,86 @@ function getShellFromEnv(): string | null {
 }
 
 // -----------------------------------------------------
-// 4) Publicly Exposed Shell Getter
+// 4) Shell Validation Functions
+// -----------------------------------------------------
+
+/**
+ * Validates if a shell path is in the allowlist to prevent arbitrary command execution
+ */
+function isShellAllowed(shellPath: string): boolean {
+	if (!shellPath) return false
+
+	const normalizedPath = path.normalize(shellPath)
+
+	// Direct lookup first
+	if (SHELL_ALLOWLIST.has(normalizedPath)) {
+		return true
+	}
+
+	// On Windows, try case-insensitive comparison
+	if (process.platform === "win32") {
+		const lowerPath = normalizedPath.toLowerCase()
+		for (const allowedPath of SHELL_ALLOWLIST) {
+			if (allowedPath.toLowerCase() === lowerPath) {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
+/**
+ * Returns a safe fallback shell based on the platform
+ */
+function getSafeFallbackShell(): string {
+	if (process.platform === "win32") {
+		return SHELL_PATHS.CMD
+	} else if (process.platform === "darwin") {
+		return SHELL_PATHS.MAC_DEFAULT
+	} else {
+		return SHELL_PATHS.LINUX_DEFAULT
+	}
+}
+
+// -----------------------------------------------------
+// 5) Publicly Exposed Shell Getter
 // -----------------------------------------------------
 
 export function getShell(): string {
+	let shell: string | null = null
+
 	// 1. Check VS Code config first.
 	if (process.platform === "win32") {
 		// Special logic for Windows
-		const windowsShell = getWindowsShellFromVSCode()
-		if (windowsShell) {
-			return windowsShell
-		}
+		shell = getWindowsShellFromVSCode()
 	} else if (process.platform === "darwin") {
 		// macOS from VS Code
-		const macShell = getMacShellFromVSCode()
-		if (macShell) {
-			return macShell
-		}
+		shell = getMacShellFromVSCode()
 	} else if (process.platform === "linux") {
 		// Linux from VS Code
-		const linuxShell = getLinuxShellFromVSCode()
-		if (linuxShell) {
-			return linuxShell
-		}
+		shell = getLinuxShellFromVSCode()
 	}
 
 	// 2. If no shell from VS Code, try userInfo()
-	const userInfoShell = getShellFromUserInfo()
-	if (userInfoShell) {
-		return userInfoShell
+	if (!shell) {
+		shell = getShellFromUserInfo()
 	}
 
 	// 3. If still nothing, try environment variable
-	const envShell = getShellFromEnv()
-	if (envShell) {
-		return envShell
+	if (!shell) {
+		shell = getShellFromEnv()
 	}
 
 	// 4. Finally, fall back to a default
-	if (process.platform === "win32") {
-		// On Windows, if we got here, we have no config, no COMSPEC, and one very messed up operating system.
-		// Use CMD as a last resort
-		return SHELL_PATHS.CMD
+	if (!shell) {
+		shell = getSafeFallbackShell()
+	}
+
+	// 5. Validate the shell against allowlist
+	if (!isShellAllowed(shell)) {
+		shell = getSafeFallbackShell()
 	}
-	// On macOS/Linux, fallback to a POSIX shell - This is the behavior of our old shell detection method.
-	return SHELL_PATHS.FALLBACK
+
+	return shell
 }