Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 108 additions & 6 deletions src/utils/__tests__/shell.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock returns the input unchanged, which might miss edge cases. Consider adding tests for actual path normalization scenarios like:

  • C:\Windows\..\Windows\System32\cmd.exe
  • Paths with multiple separators: C:\\Windows\\System32\\cmd.exe
  • Mixed separators: C:/Windows\System32/cmd.exe

These would help ensure the validation works correctly with various path formats.

}
})

describe("Shell Detection Tests", () => {
let originalPlatform: string
let originalEnv: NodeJS.ProcessEnv
Expand Down Expand Up @@ -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")
})
})

Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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
})
})
})
179 changes: 154 additions & 25 deletions src/utils/shell.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is path.normalize() sufficient here? It doesn't resolve symlinks, so could a malicious actor potentially create a symlink from an allowed path to an arbitrary executable?

Consider using fs.realpathSync() to resolve symlinks before validation:

Suggested change
const normalizedPath = path.normalize(shellPath)
const normalizedPath = path.normalize(shellPath)
try {
const resolvedPath = fs.realpathSync(normalizedPath)
// Then check resolvedPath against allowlist
} catch {
// Handle case where path doesn't exist
return false
}


// 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()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a shell is rejected and we fall back to the safe default, it might be helpful to log this event (perhaps at debug level) so users can understand why their configured shell isn't being used. This could help with troubleshooting:

Suggested change
shell = getSafeFallbackShell()
if (!isShellAllowed(shell)) {
console.debug(`Shell "${shell}" not in allowlist, falling back to safe default`)
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
}
Loading