From f9c17b340a7bea00fb9454dd20aaf4d3ee51a120 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Tue, 26 Aug 2025 00:32:41 -0400 Subject: [PATCH] Update list of default allowed commands --- src/package.json | 3 - src/utils/__tests__/migrateSettings.spec.ts | 296 ++++++++++++++++++++ src/utils/migrateSettings.ts | 58 +++- 3 files changed, 353 insertions(+), 4 deletions(-) create mode 100644 src/utils/__tests__/migrateSettings.spec.ts diff --git a/src/package.json b/src/package.json index b23d06137a..6099c88003 100644 --- a/src/package.json +++ b/src/package.json @@ -321,9 +321,6 @@ "type": "string" }, "default": [ - "npm test", - "npm install", - "tsc", "git log", "git diff", "git show" diff --git a/src/utils/__tests__/migrateSettings.spec.ts b/src/utils/__tests__/migrateSettings.spec.ts new file mode 100644 index 0000000000..5b8481949b --- /dev/null +++ b/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 + + 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() + }) + }) +}) diff --git a/src/utils/migrateSettings.ts b/src/utils/migrateSettings.ts index 43b1d7291f..0ddd553610 100644 --- a/src/utils/migrateSettings.ts +++ b/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 { + // 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 { + 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("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}`) + } +}