From c659190d77a75bf7bb8fc18aea1eb16fa7d7d3a4 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Fri, 5 Sep 2025 07:34:44 +0000 Subject: [PATCH 1/3] 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 --- src/utils/__tests__/shell.test.ts | 364 ++++++++++++++++++++++++++++++ src/utils/shell.ts | 44 ++-- 2 files changed, 395 insertions(+), 13 deletions(-) create mode 100644 src/utils/__tests__/shell.test.ts diff --git a/src/utils/__tests__/shell.test.ts b/src/utils/__tests__/shell.test.ts new file mode 100644 index 0000000000..54b5d508b5 --- /dev/null +++ b/src/utils/__tests__/shell.test.ts @@ -0,0 +1,364 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" +import * as vscode from "vscode" + +// Mock vscode module +vi.mock("vscode", () => ({ + workspace: { + getConfiguration: vi.fn(), + }, +})) + +// Mock os module +vi.mock("os", () => ({ + userInfo: vi.fn(), +})) + +describe("shell utilities", () => { + let originalPlatform: PropertyDescriptor | undefined + let originalEnv: NodeJS.ProcessEnv + + beforeEach(() => { + // Save original values + originalPlatform = Object.getOwnPropertyDescriptor(process, "platform") + originalEnv = process.env + + // Reset mocks + vi.clearAllMocks() + + // Reset module cache to ensure fresh imports + vi.resetModules() + }) + + afterEach(() => { + // Restore original values + if (originalPlatform) { + Object.defineProperty(process, "platform", originalPlatform) + } + process.env = originalEnv + }) + + describe("getShell", () => { + it("should handle string path from VSCode terminal profile on Windows", async () => { + // Set platform to Windows + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + // Mock VSCode configuration + 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", + }, + } + } + return undefined + }), + } + + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe") + }) + + it("should handle array path from VSCode terminal profile on Windows", async () => { + // Set platform to Windows + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + // 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) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + 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", async () => { + // Set platform to Windows + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + // 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 = { ...originalEnv, COMSPEC: "C:\\Windows\\System32\\cmd.exe" } + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + // Should fall back to cmd.exe + expect(result).toBe("C:\\Windows\\System32\\cmd.exe") + }) + + it("should handle string path on macOS", async () => { + // Set platform to macOS + Object.defineProperty(process, "platform", { + value: "darwin", + configurable: true, + }) + + // Mock VSCode configuration + const mockConfig = { + get: vi.fn((key: string) => { + if (key === "defaultProfile.osx") return "zsh" + if (key === "profiles.osx") { + return { + zsh: { + path: "/bin/zsh", + }, + } + } + return undefined + }), + } + + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + expect(result).toBe("/bin/zsh") + }) + + it("should handle array path on macOS", async () => { + // Set platform to macOS + Object.defineProperty(process, "platform", { + value: "darwin", + configurable: true, + }) + + // 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) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + // Should use the first element of the array + expect(result).toBe("/opt/homebrew/bin/zsh") + }) + + it("should handle string path on Linux", async () => { + // Set platform to Linux + Object.defineProperty(process, "platform", { + value: "linux", + configurable: true, + }) + + // Mock VSCode configuration + const mockConfig = { + get: vi.fn((key: string) => { + if (key === "defaultProfile.linux") return "bash" + if (key === "profiles.linux") { + return { + bash: { + path: "/bin/bash", + }, + } + } + return undefined + }), + } + + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + expect(result).toBe("/bin/bash") + }) + + it("should handle array path on Linux", async () => { + // Set platform to Linux + Object.defineProperty(process, "platform", { + value: "linux", + configurable: true, + }) + + // 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) + + // Import after mocks are set up + const { getShell } = await import("../shell") + + const result = getShell() + // Should use the first element of the array + expect(result).toBe("/usr/local/bin/bash") + }) + }) + + describe("isShellAllowed", () => { + it("should validate string shell paths", async () => { + // Import the module to get access to internal functions + const shellModule = await import("../shell") + + // Access the isShellAllowed function through module internals + // Since it's not exported, we need to test it indirectly through getShell + // or make it exported for testing + + // For now, we'll test the behavior through getShell + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + 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", + }, + } + } + return undefined + }), + } + + vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) + + const result = shellModule.getShell() + // Should return the allowed shell + expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe") + }) + + it("should validate array shell paths", async () => { + const shellModule = await import("../shell") + + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + 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 = shellModule.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", async () => { + const shellModule = await import("../shell") + + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + 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 = { ...originalEnv, COMSPEC: "C:\\Windows\\System32\\cmd.exe" } + + const result = shellModule.getShell() + // Should fall back to safe default (cmd.exe) + expect(result).toBe("C:\\Windows\\System32\\cmd.exe") + }) + }) +}) diff --git a/src/utils/shell.ts b/src/utils/shell.ts index 997c699fdb..fb6067edca 100644 --- a/src/utils/shell.ts +++ b/src/utils/shell.ts @@ -113,20 +113,20 @@ const SHELL_PATHS = { } as const interface MacTerminalProfile { - path?: string + path?: string | string[] } type MacTerminalProfiles = Record interface WindowsTerminalProfile { - path?: string + path?: string | string[] source?: "PowerShell" | "WSL" } type WindowsTerminalProfiles = Record interface LinuxTerminalProfile { - path?: string + path?: string | string[] } type LinuxTerminalProfiles = Record @@ -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) } // ----------------------------------------------------- @@ -275,12 +289,16 @@ function getShellFromEnv(): string | null { // ----------------------------------------------------- /** - * Validates if a shell path is in the allowlist to prevent arbitrary command execution + * Validates if a shell path is in the allowlist to prevent arbitrary command execution. + * Can handle both string and array inputs (arrays from VSCode API). */ -function isShellAllowed(shellPath: string): boolean { - if (!shellPath) return false +function isShellAllowed(shellPath: string | string[]): boolean { + // Handle array input by checking the first element + const pathToCheck = Array.isArray(shellPath) ? (shellPath.length > 0 ? shellPath[0] : null) : shellPath + + if (!pathToCheck) return false - const normalizedPath = path.normalize(shellPath) + const normalizedPath = path.normalize(pathToCheck) // Direct lookup first if (SHELL_ALLOWLIST.has(normalizedPath)) { From 74c7ea813447c1a0154e31a4b050ba17355e9be6 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Fri, 5 Sep 2025 07:40:01 +0000 Subject: [PATCH 2/3] 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 --- src/utils/__tests__/shell.test.ts | 128 ++++++++++++++++++++++++++++++ src/utils/shell.ts | 69 ++++++++++++++-- 2 files changed, 189 insertions(+), 8 deletions(-) diff --git a/src/utils/__tests__/shell.test.ts b/src/utils/__tests__/shell.test.ts index 54b5d508b5..6046f1480e 100644 --- a/src/utils/__tests__/shell.test.ts +++ b/src/utils/__tests__/shell.test.ts @@ -361,4 +361,132 @@ describe("shell utilities", () => { expect(result).toBe("C:\\Windows\\System32\\cmd.exe") }) }) + + describe("validateShellPath", () => { + it("should validate string shell paths", async () => { + const { validateShellPath } = await import("../shell") + + // Valid shells + expect(validateShellPath("/bin/bash")).toBe(true) + expect(validateShellPath("/bin/zsh")).toBe(true) + expect(validateShellPath("C:\\Windows\\System32\\cmd.exe")).toBe(true) + expect(validateShellPath("C:\\Program Files\\PowerShell\\7\\pwsh.exe")).toBe(true) + + // Invalid shells + expect(validateShellPath("/usr/bin/malicious")).toBe(false) + expect(validateShellPath("C:\\malicious\\shell.exe")).toBe(false) + }) + + it("should validate array shell paths using first element", async () => { + const { validateShellPath } = await import("../shell") + + // Valid array with allowed first element + expect(validateShellPath(["/bin/bash", "/bin/sh"])).toBe(true) + expect(validateShellPath(["C:\\Windows\\System32\\cmd.exe", "cmd"])).toBe(true) + + // Invalid array with disallowed first element + expect(validateShellPath(["/usr/bin/malicious", "/bin/bash"])).toBe(false) + expect(validateShellPath(["C:\\malicious\\shell.exe", "C:\\Windows\\System32\\cmd.exe"])).toBe(false) + }) + + it("should handle empty arrays", async () => { + const { validateShellPath } = await import("../shell") + + expect(validateShellPath([])).toBe(false) + }) + + it("should handle null and undefined", async () => { + const { validateShellPath } = await import("../shell") + + expect(validateShellPath(null)).toBe(false) + expect(validateShellPath(undefined)).toBe(false) + }) + + it("should handle nested arrays (edge case)", async () => { + const { validateShellPath } = await import("../shell") + + // Nested array - should recursively check first element + expect(validateShellPath([["/bin/bash"]] as any)).toBe(true) + expect(validateShellPath([["/usr/bin/malicious"]] as any)).toBe(false) + expect(validateShellPath([["C:\\Windows\\System32\\cmd.exe"]] as any)).toBe(true) + }) + + it("should handle empty strings", async () => { + const { validateShellPath } = await import("../shell") + + expect(validateShellPath("")).toBe(false) + expect(validateShellPath([""])).toBe(false) + }) + + it("should handle case-insensitive comparison on Windows", async () => { + // Set platform to Windows + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }) + + const { validateShellPath } = await import("../shell") + + // Different case variations should all be valid + expect(validateShellPath("c:\\windows\\system32\\cmd.exe")).toBe(true) + expect(validateShellPath("C:\\WINDOWS\\SYSTEM32\\CMD.EXE")).toBe(true) + expect(validateShellPath("C:\\Windows\\System32\\CMD.exe")).toBe(true) + }) + + it("should handle case-sensitive comparison on Unix", async () => { + // Set platform to Linux + Object.defineProperty(process, "platform", { + value: "linux", + configurable: true, + }) + + const { validateShellPath } = await import("../shell") + + // Exact case match required + expect(validateShellPath("/bin/bash")).toBe(true) + expect(validateShellPath("/BIN/BASH")).toBe(false) + expect(validateShellPath("/Bin/Bash")).toBe(false) + }) + + it("should normalize paths before validation", async () => { + const { validateShellPath } = await import("../shell") + + // Paths with extra slashes or dots should be normalized + expect(validateShellPath("/bin//bash")).toBe(true) + expect(validateShellPath("/bin/./bash")).toBe(true) + + // Windows paths with backslashes + if (process.platform === "win32") { + expect(validateShellPath("C:/Windows/System32/cmd.exe")).toBe(true) + } + }) + + it("should handle arrays with mixed valid and invalid paths", async () => { + const { validateShellPath } = await import("../shell") + + // Only the first element matters + expect(validateShellPath(["/bin/bash", "/usr/bin/malicious", "/bin/sh"])).toBe(true) + expect(validateShellPath(["/usr/bin/malicious", "/bin/bash", "/bin/sh"])).toBe(false) + }) + + it("should reject non-string, non-array types", async () => { + const { validateShellPath } = await import("../shell") + + // Numbers, objects, etc. should be rejected + expect(validateShellPath(123 as any)).toBe(false) + expect(validateShellPath({ path: "/bin/bash" } as any)).toBe(false) + expect(validateShellPath(true as any)).toBe(false) + }) + + it("should handle arrays containing non-string elements", async () => { + const { validateShellPath } = await import("../shell") + + // Array with null/undefined first element + expect(validateShellPath([null, "/bin/bash"] as any)).toBe(false) + expect(validateShellPath([undefined, "/bin/bash"] as any)).toBe(false) + + // Array with number first element + expect(validateShellPath([123, "/bin/bash"] as any)).toBe(false) + }) + }) }) diff --git a/src/utils/shell.ts b/src/utils/shell.ts index fb6067edca..f6017e296b 100644 --- a/src/utils/shell.ts +++ b/src/utils/shell.ts @@ -289,16 +289,13 @@ function getShellFromEnv(): string | null { // ----------------------------------------------------- /** - * Validates if a shell path is in the allowlist to prevent arbitrary command execution. - * Can handle both string and array inputs (arrays from VSCode API). + * Internal validation function that checks if a shell path is in the allowlist. + * This is the core validation logic that operates on normalized string paths. */ -function isShellAllowed(shellPath: string | string[]): boolean { - // Handle array input by checking the first element - const pathToCheck = Array.isArray(shellPath) ? (shellPath.length > 0 ? shellPath[0] : null) : shellPath - - if (!pathToCheck) return false +function isShellAllowedInternal(shellPath: string): boolean { + if (!shellPath) return false - const normalizedPath = path.normalize(pathToCheck) + const normalizedPath = path.normalize(shellPath) // Direct lookup first if (SHELL_ALLOWLIST.has(normalizedPath)) { @@ -318,6 +315,62 @@ function isShellAllowed(shellPath: string | string[]): boolean { return false } +/** + * Proxy function that validates shell paths, handling both string and array inputs. + * This function serves as a robust interface for shell path validation that can + * handle various input types from VSCode API and other sources. + * + * @param shellPath - The shell path to validate. Can be: + * - A string path to a shell executable + * - An array of string paths (VSCode may return this) + * - undefined or null + * @returns true if the shell path is allowed, false otherwise + * + * @example + * // String input + * validateShellPath("/bin/bash") // returns true + * + * @example + * // Array input (from VSCode API) + * validateShellPath(["/usr/local/bin/bash", "/bin/bash"]) // returns true if first is allowed + * + * @example + * // Empty or invalid input + * validateShellPath([]) // returns false + * validateShellPath(null) // returns false + */ +export function validateShellPath(shellPath: string | string[] | undefined | null): boolean { + // Handle null/undefined + if (!shellPath) { + return false + } + + // Handle array input - validate the first element + if (Array.isArray(shellPath)) { + if (shellPath.length === 0) { + return false + } + // Recursively validate the first element (in case of nested arrays) + return validateShellPath(shellPath[0]) + } + + // Handle string input + if (typeof shellPath === "string") { + return isShellAllowedInternal(shellPath) + } + + // Unknown type - reject for safety + return false +} + +/** + * Legacy function for backward compatibility. + * @deprecated Use validateShellPath instead + */ +function isShellAllowed(shellPath: string | string[]): boolean { + return validateShellPath(shellPath) +} + /** * Returns a safe fallback shell based on the platform */ From 332976b86e594a1967a095ef324394ca1883cf5c Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Fri, 5 Sep 2025 11:23:50 -0700 Subject: [PATCH 3/3] Simplify roomote's work a little --- src/utils/__tests__/shell.spec.ts | 155 ++++++++++ src/utils/__tests__/shell.test.ts | 492 ------------------------------ src/utils/shell.ts | 61 +--- 3 files changed, 157 insertions(+), 551 deletions(-) delete mode 100644 src/utils/__tests__/shell.test.ts diff --git a/src/utils/__tests__/shell.spec.ts b/src/utils/__tests__/shell.spec.ts index 352aedbbe5..8f370e4d7f 100644 --- a/src/utils/__tests__/shell.spec.ts +++ b/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", { diff --git a/src/utils/__tests__/shell.test.ts b/src/utils/__tests__/shell.test.ts deleted file mode 100644 index 6046f1480e..0000000000 --- a/src/utils/__tests__/shell.test.ts +++ /dev/null @@ -1,492 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" -import * as vscode from "vscode" - -// Mock vscode module -vi.mock("vscode", () => ({ - workspace: { - getConfiguration: vi.fn(), - }, -})) - -// Mock os module -vi.mock("os", () => ({ - userInfo: vi.fn(), -})) - -describe("shell utilities", () => { - let originalPlatform: PropertyDescriptor | undefined - let originalEnv: NodeJS.ProcessEnv - - beforeEach(() => { - // Save original values - originalPlatform = Object.getOwnPropertyDescriptor(process, "platform") - originalEnv = process.env - - // Reset mocks - vi.clearAllMocks() - - // Reset module cache to ensure fresh imports - vi.resetModules() - }) - - afterEach(() => { - // Restore original values - if (originalPlatform) { - Object.defineProperty(process, "platform", originalPlatform) - } - process.env = originalEnv - }) - - describe("getShell", () => { - it("should handle string path from VSCode terminal profile on Windows", async () => { - // Set platform to Windows - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - // Mock VSCode configuration - 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", - }, - } - } - return undefined - }), - } - - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe") - }) - - it("should handle array path from VSCode terminal profile on Windows", async () => { - // Set platform to Windows - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - // 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) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - 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", async () => { - // Set platform to Windows - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - // 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 = { ...originalEnv, COMSPEC: "C:\\Windows\\System32\\cmd.exe" } - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - // Should fall back to cmd.exe - expect(result).toBe("C:\\Windows\\System32\\cmd.exe") - }) - - it("should handle string path on macOS", async () => { - // Set platform to macOS - Object.defineProperty(process, "platform", { - value: "darwin", - configurable: true, - }) - - // Mock VSCode configuration - const mockConfig = { - get: vi.fn((key: string) => { - if (key === "defaultProfile.osx") return "zsh" - if (key === "profiles.osx") { - return { - zsh: { - path: "/bin/zsh", - }, - } - } - return undefined - }), - } - - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - expect(result).toBe("/bin/zsh") - }) - - it("should handle array path on macOS", async () => { - // Set platform to macOS - Object.defineProperty(process, "platform", { - value: "darwin", - configurable: true, - }) - - // 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) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - // Should use the first element of the array - expect(result).toBe("/opt/homebrew/bin/zsh") - }) - - it("should handle string path on Linux", async () => { - // Set platform to Linux - Object.defineProperty(process, "platform", { - value: "linux", - configurable: true, - }) - - // Mock VSCode configuration - const mockConfig = { - get: vi.fn((key: string) => { - if (key === "defaultProfile.linux") return "bash" - if (key === "profiles.linux") { - return { - bash: { - path: "/bin/bash", - }, - } - } - return undefined - }), - } - - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - expect(result).toBe("/bin/bash") - }) - - it("should handle array path on Linux", async () => { - // Set platform to Linux - Object.defineProperty(process, "platform", { - value: "linux", - configurable: true, - }) - - // 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) - - // Import after mocks are set up - const { getShell } = await import("../shell") - - const result = getShell() - // Should use the first element of the array - expect(result).toBe("/usr/local/bin/bash") - }) - }) - - describe("isShellAllowed", () => { - it("should validate string shell paths", async () => { - // Import the module to get access to internal functions - const shellModule = await import("../shell") - - // Access the isShellAllowed function through module internals - // Since it's not exported, we need to test it indirectly through getShell - // or make it exported for testing - - // For now, we'll test the behavior through getShell - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - 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", - }, - } - } - return undefined - }), - } - - vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig as any) - - const result = shellModule.getShell() - // Should return the allowed shell - expect(result).toBe("C:\\Program Files\\PowerShell\\7\\pwsh.exe") - }) - - it("should validate array shell paths", async () => { - const shellModule = await import("../shell") - - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - 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 = shellModule.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", async () => { - const shellModule = await import("../shell") - - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - 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 = { ...originalEnv, COMSPEC: "C:\\Windows\\System32\\cmd.exe" } - - const result = shellModule.getShell() - // Should fall back to safe default (cmd.exe) - expect(result).toBe("C:\\Windows\\System32\\cmd.exe") - }) - }) - - describe("validateShellPath", () => { - it("should validate string shell paths", async () => { - const { validateShellPath } = await import("../shell") - - // Valid shells - expect(validateShellPath("/bin/bash")).toBe(true) - expect(validateShellPath("/bin/zsh")).toBe(true) - expect(validateShellPath("C:\\Windows\\System32\\cmd.exe")).toBe(true) - expect(validateShellPath("C:\\Program Files\\PowerShell\\7\\pwsh.exe")).toBe(true) - - // Invalid shells - expect(validateShellPath("/usr/bin/malicious")).toBe(false) - expect(validateShellPath("C:\\malicious\\shell.exe")).toBe(false) - }) - - it("should validate array shell paths using first element", async () => { - const { validateShellPath } = await import("../shell") - - // Valid array with allowed first element - expect(validateShellPath(["/bin/bash", "/bin/sh"])).toBe(true) - expect(validateShellPath(["C:\\Windows\\System32\\cmd.exe", "cmd"])).toBe(true) - - // Invalid array with disallowed first element - expect(validateShellPath(["/usr/bin/malicious", "/bin/bash"])).toBe(false) - expect(validateShellPath(["C:\\malicious\\shell.exe", "C:\\Windows\\System32\\cmd.exe"])).toBe(false) - }) - - it("should handle empty arrays", async () => { - const { validateShellPath } = await import("../shell") - - expect(validateShellPath([])).toBe(false) - }) - - it("should handle null and undefined", async () => { - const { validateShellPath } = await import("../shell") - - expect(validateShellPath(null)).toBe(false) - expect(validateShellPath(undefined)).toBe(false) - }) - - it("should handle nested arrays (edge case)", async () => { - const { validateShellPath } = await import("../shell") - - // Nested array - should recursively check first element - expect(validateShellPath([["/bin/bash"]] as any)).toBe(true) - expect(validateShellPath([["/usr/bin/malicious"]] as any)).toBe(false) - expect(validateShellPath([["C:\\Windows\\System32\\cmd.exe"]] as any)).toBe(true) - }) - - it("should handle empty strings", async () => { - const { validateShellPath } = await import("../shell") - - expect(validateShellPath("")).toBe(false) - expect(validateShellPath([""])).toBe(false) - }) - - it("should handle case-insensitive comparison on Windows", async () => { - // Set platform to Windows - Object.defineProperty(process, "platform", { - value: "win32", - configurable: true, - }) - - const { validateShellPath } = await import("../shell") - - // Different case variations should all be valid - expect(validateShellPath("c:\\windows\\system32\\cmd.exe")).toBe(true) - expect(validateShellPath("C:\\WINDOWS\\SYSTEM32\\CMD.EXE")).toBe(true) - expect(validateShellPath("C:\\Windows\\System32\\CMD.exe")).toBe(true) - }) - - it("should handle case-sensitive comparison on Unix", async () => { - // Set platform to Linux - Object.defineProperty(process, "platform", { - value: "linux", - configurable: true, - }) - - const { validateShellPath } = await import("../shell") - - // Exact case match required - expect(validateShellPath("/bin/bash")).toBe(true) - expect(validateShellPath("/BIN/BASH")).toBe(false) - expect(validateShellPath("/Bin/Bash")).toBe(false) - }) - - it("should normalize paths before validation", async () => { - const { validateShellPath } = await import("../shell") - - // Paths with extra slashes or dots should be normalized - expect(validateShellPath("/bin//bash")).toBe(true) - expect(validateShellPath("/bin/./bash")).toBe(true) - - // Windows paths with backslashes - if (process.platform === "win32") { - expect(validateShellPath("C:/Windows/System32/cmd.exe")).toBe(true) - } - }) - - it("should handle arrays with mixed valid and invalid paths", async () => { - const { validateShellPath } = await import("../shell") - - // Only the first element matters - expect(validateShellPath(["/bin/bash", "/usr/bin/malicious", "/bin/sh"])).toBe(true) - expect(validateShellPath(["/usr/bin/malicious", "/bin/bash", "/bin/sh"])).toBe(false) - }) - - it("should reject non-string, non-array types", async () => { - const { validateShellPath } = await import("../shell") - - // Numbers, objects, etc. should be rejected - expect(validateShellPath(123 as any)).toBe(false) - expect(validateShellPath({ path: "/bin/bash" } as any)).toBe(false) - expect(validateShellPath(true as any)).toBe(false) - }) - - it("should handle arrays containing non-string elements", async () => { - const { validateShellPath } = await import("../shell") - - // Array with null/undefined first element - expect(validateShellPath([null, "/bin/bash"] as any)).toBe(false) - expect(validateShellPath([undefined, "/bin/bash"] as any)).toBe(false) - - // Array with number first element - expect(validateShellPath([123, "/bin/bash"] as any)).toBe(false) - }) - }) -}) diff --git a/src/utils/shell.ts b/src/utils/shell.ts index f6017e296b..45253c31b0 100644 --- a/src/utils/shell.ts +++ b/src/utils/shell.ts @@ -289,10 +289,9 @@ function getShellFromEnv(): string | null { // ----------------------------------------------------- /** - * Internal validation function that checks if a shell path is in the allowlist. - * This is the core validation logic that operates on normalized string paths. + * Validates if a shell path is in the allowlist to prevent arbitrary command execution */ -function isShellAllowedInternal(shellPath: string): boolean { +function isShellAllowed(shellPath: string): boolean { if (!shellPath) return false const normalizedPath = path.normalize(shellPath) @@ -315,62 +314,6 @@ function isShellAllowedInternal(shellPath: string): boolean { return false } -/** - * Proxy function that validates shell paths, handling both string and array inputs. - * This function serves as a robust interface for shell path validation that can - * handle various input types from VSCode API and other sources. - * - * @param shellPath - The shell path to validate. Can be: - * - A string path to a shell executable - * - An array of string paths (VSCode may return this) - * - undefined or null - * @returns true if the shell path is allowed, false otherwise - * - * @example - * // String input - * validateShellPath("/bin/bash") // returns true - * - * @example - * // Array input (from VSCode API) - * validateShellPath(["/usr/local/bin/bash", "/bin/bash"]) // returns true if first is allowed - * - * @example - * // Empty or invalid input - * validateShellPath([]) // returns false - * validateShellPath(null) // returns false - */ -export function validateShellPath(shellPath: string | string[] | undefined | null): boolean { - // Handle null/undefined - if (!shellPath) { - return false - } - - // Handle array input - validate the first element - if (Array.isArray(shellPath)) { - if (shellPath.length === 0) { - return false - } - // Recursively validate the first element (in case of nested arrays) - return validateShellPath(shellPath[0]) - } - - // Handle string input - if (typeof shellPath === "string") { - return isShellAllowedInternal(shellPath) - } - - // Unknown type - reject for safety - return false -} - -/** - * Legacy function for backward compatibility. - * @deprecated Use validateShellPath instead - */ -function isShellAllowed(shellPath: string | string[]): boolean { - return validateShellPath(shellPath) -} - /** * Returns a safe fallback shell based on the platform */