Update list of default allowed commands (#7404)

src/package.json

@@ -321,9 +321,6 @@
 						"type": "string"
 					},
 					"default": [
-						"npm test",
-						"npm install",
-						"tsc",
 						"git log",
 						"git diff",
 						"git show"

src/utils/__tests__/migrateSettings.spec.ts

@@ -0,0 +1,296 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
+import * as vscode from "vscode"
+import * as fs from "fs/promises"
+import * as path from "path"
+import { migrateSettings } from "../migrateSettings"
+
+// Mock vscode module
+vi.mock("vscode", () => ({
+	window: {
+		showInformationMessage: vi.fn(),
+		createOutputChannel: vi.fn(() => ({
+			appendLine: vi.fn(),
+		})),
+	},
+	commands: {
+		executeCommand: vi.fn(),
+	},
+}))
+
+// Mock fs module
+vi.mock("fs/promises")
+
+// Mock fs utils
+vi.mock("../fs", () => ({
+	fileExistsAtPath: vi.fn(),
+}))
+
+// Mock yaml module
+vi.mock("yaml", () => ({
+	parse: vi.fn((content) => JSON.parse(content)),
+	stringify: vi.fn((obj) => JSON.stringify(obj, null, 2)),
+}))
+
+describe("migrateSettings", () => {
+	let mockContext: any
+	let mockOutputChannel: any
+	let mockGlobalState: Map<string, any>
+
+	beforeEach(() => {
+		// Reset all mocks
+		vi.clearAllMocks()
+
+		// Create a mock global state
+		mockGlobalState = new Map()
+
+		// Create mock output channel
+		mockOutputChannel = {
+			appendLine: vi.fn(),
+		}
+
+		// Create mock context
+		mockContext = {
+			globalState: {
+				get: vi.fn((key: string) => mockGlobalState.get(key)),
+				update: vi.fn(async (key: string, value: any) => {
+					mockGlobalState.set(key, value)
+				}),
+			},
+			globalStorageUri: {
+				fsPath: "/mock/storage/path",
+			},
+		}
+	})
+
+	afterEach(() => {
+		vi.restoreAllMocks()
+	})
+
+	describe("default commands migration", () => {
+		it("should only run migration once", async () => {
+			// Set up initial state with old default commands
+			const initialCommands = ["npm install", "npm test", "tsc", "git log"]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration first time
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that old default commands were removed
+			expect(mockGlobalState.get("allowedCommands")).toEqual(["git log"])
+
+			// Check that migration was marked as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+
+			// Reset mocks but keep the migration flag
+			mockGlobalState.set("defaultCommandsMigrationCompleted", true)
+			mockGlobalState.set("allowedCommands", ["npm install", "npm test"])
+			vi.mocked(mockContext.globalState.update).mockClear()
+
+			// Run migration again
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Verify commands were NOT modified the second time
+			expect(mockGlobalState.get("allowedCommands")).toEqual(["npm install", "npm test"])
+			expect(mockContext.globalState.update).not.toHaveBeenCalled()
+		})
+
+		it("should remove npm install, npm test, and tsc from allowed commands", async () => {
+			// Set up initial state with old default commands
+			const initialCommands = ["git log", "npm install", "npm test", "tsc", "git diff", "echo hello"]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system to indicate no settings directory exists
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that old default commands were removed
+			const updatedCommands = mockGlobalState.get("allowedCommands")
+			expect(updatedCommands).toEqual(["git log", "git diff", "echo hello"])
+
+			// Verify the update was called
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("allowedCommands", [
+				"git log",
+				"git diff",
+				"echo hello",
+			])
+
+			// Verify migration was marked as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+
+			// No notification should be shown
+			expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
+		})
+
+		it("should not remove commands with arguments (only exact matches)", async () => {
+			// Set up initial state with commands that have arguments
+			const initialCommands = [
+				"npm install express",
+				"npm test --coverage",
+				"tsc --watch",
+				"npm list",
+				"npm view",
+				"yarn list",
+				"git status",
+			]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that commands with arguments were NOT removed (only exact matches are removed)
+			const updatedCommands = mockGlobalState.get("allowedCommands")
+			expect(updatedCommands).toEqual([
+				"npm install express",
+				"npm test --coverage",
+				"tsc --watch",
+				"npm list",
+				"npm view",
+				"yarn list",
+				"git status",
+			])
+
+			// Migration should still be marked as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+
+			// No notification should be shown
+			expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
+		})
+
+		it("should handle case-insensitive matching", async () => {
+			// Set up initial state with mixed case commands
+			const initialCommands = ["NPM INSTALL", "Npm Test", "TSC", "git log"]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that unsafe commands were removed regardless of case
+			const updatedCommands = mockGlobalState.get("allowedCommands")
+			expect(updatedCommands).toEqual(["git log"])
+		})
+
+		it("should not modify commands if no old defaults are present", async () => {
+			// Set up initial state with only safe commands
+			const initialCommands = ["git log", "git diff", "ls -la", "echo hello"]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that commands remain unchanged
+			const updatedCommands = mockGlobalState.get("allowedCommands")
+			expect(updatedCommands).toEqual(initialCommands)
+
+			// Verify no notification was shown
+			expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
+
+			// Verify migration was still marked as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+		})
+
+		it("should handle missing or invalid allowedCommands gracefully", async () => {
+			// Test with no allowedCommands set
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			await migrateSettings(mockContext, mockOutputChannel)
+			// Should still mark migration as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+
+			// Verify appropriate log messages
+			expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
+				expect.stringContaining("marking migration as complete"),
+			)
+		})
+
+		it("should handle non-array allowedCommands gracefully", async () => {
+			// Test with non-array value
+			mockGlobalState.set("allowedCommands", "not an array")
+
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Should still mark migration as complete
+			expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
+
+			// Verify appropriate log messages
+			expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
+				expect.stringContaining("marking migration as complete"),
+			)
+		})
+
+		it("should handle errors gracefully", async () => {
+			// Set up state
+			mockGlobalState.set("allowedCommands", ["npm install"])
+
+			// Make update throw an error
+			mockContext.globalState.update = vi.fn().mockRejectedValue(new Error("Update failed"))
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration - should not throw
+			await expect(migrateSettings(mockContext, mockOutputChannel)).resolves.toBeUndefined()
+
+			// Verify error was logged
+			expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
+				expect.stringContaining("[Default Commands Migration] Error"),
+			)
+		})
+
+		it("should only remove exact matches, not commands with arguments", async () => {
+			// Set up initial state with exact matches and commands with arguments
+			const initialCommands = [
+				"npm install", // exact match - should be removed
+				"npm install --save-dev typescript", // has arguments - should NOT be removed
+				"npm test", // exact match - should be removed
+				"npm test --coverage", // has arguments - should NOT be removed
+				"tsc", // exact match - should be removed
+				"tsc --noEmit", // has arguments - should NOT be removed
+				"git log --oneline",
+			]
+			mockGlobalState.set("allowedCommands", initialCommands)
+
+			// Mock file system
+			const { fileExistsAtPath } = await import("../fs")
+			vi.mocked(fileExistsAtPath).mockResolvedValue(false)
+
+			// Run migration
+			await migrateSettings(mockContext, mockOutputChannel)
+
+			// Check that only exact matches were removed
+			const updatedCommands = mockGlobalState.get("allowedCommands")
+			expect(updatedCommands).toEqual([
+				"npm install --save-dev typescript",
+				"npm test --coverage",
+				"tsc --noEmit",
+				"git log --oneline",
+			])
+
+			// No notification should be shown
+			expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
+		})
+	})
+})

src/utils/migrateSettings.ts

@@ -8,7 +8,7 @@ import * as yaml from "yaml"
 const deprecatedCustomModesJSONFilename = "custom_modes.json"
 
 /**
- * Migrates old settings files to new file names
+ * Migrates old settings files to new file names and removes commands from old defaults
  *
  * TODO: Remove this migration code in September 2025 (6 months after implementation)
  */
@@ -16,6 +16,8 @@ export async function migrateSettings(
 	context: vscode.ExtensionContext,
 	outputChannel: vscode.OutputChannel,
 ): Promise<void> {
+	// First, migrate commands from old defaults (security fix)
+	await migrateDefaultCommands(context, outputChannel)
 	// Legacy file names that need to be migrated to the new names in GlobalFileNames
 	const fileMigrations = [
 		// custom_modes.json to custom_modes.yaml is handled separately below
@@ -113,3 +115,57 @@ async function migrateCustomModesToYaml(settingsDir: string, outputChannel: vsco
 		outputChannel.appendLine(`Error reading custom_modes.json: ${fileError}. Skipping migration.`)
 	}
 }
+
+/**
+ * Removes commands from old defaults that could execute arbitrary code
+ * This addresses the security vulnerability where npm install/test can run malicious postinstall scripts
+ */
+async function migrateDefaultCommands(
+	context: vscode.ExtensionContext,
+	outputChannel: vscode.OutputChannel,
+): Promise<void> {
+	try {
+		// Check if this migration has already been run
+		const migrationKey = "defaultCommandsMigrationCompleted"
+		if (context.globalState.get(migrationKey)) {
+			outputChannel.appendLine("[Default Commands Migration] Migration already completed, skipping")
+			return
+		}
+
+		const allowedCommands = context.globalState.get<string[]>("allowedCommands")
+
+		if (!allowedCommands || !Array.isArray(allowedCommands)) {
+			// Mark migration as complete even if no commands to migrate
+			await context.globalState.update(migrationKey, true)
+			outputChannel.appendLine("No allowed commands found in global state, marking migration as complete")
+			return
+		}
+
+		// Only migrate the specific commands that were removed from the defaults
+		const oldDefaultCommands = ["npm install", "npm test", "tsc"]
+
+		// Filter out old default commands (case-insensitive exact match only)
+		const originalLength = allowedCommands.length
+		const filteredCommands = allowedCommands.filter((cmd) => {
+			const cmdLower = cmd.toLowerCase().trim()
+			return !oldDefaultCommands.some((oldDefault) => cmdLower === oldDefault.toLowerCase())
+		})
+
+		if (filteredCommands.length < originalLength) {
+			const removedCount = originalLength - filteredCommands.length
+			await context.globalState.update("allowedCommands", filteredCommands)
+
+			outputChannel.appendLine(
+				`[Default Commands Migration] Removed ${removedCount} command(s) from old defaults to prevent arbitrary code execution vulnerability`,
+			)
+		} else {
+			outputChannel.appendLine("[Default Commands Migration] No old default commands found in allowed list")
+		}
+
+		// Mark migration as complete
+		await context.globalState.update(migrationKey, true)
+		outputChannel.appendLine("[Default Commands Migration] Migration marked as complete")
+	} catch (error) {
+		outputChannel.appendLine(`[Default Commands Migration] Error migrating default commands: ${error}`)
+	}
+}