Skip to content

Commit 8684877

Browse files
mrubensmrubens
authored
Update list of default allowed commands (#7404)
1 parent 296edfc commit 8684877

File tree

3 files changed

+353
-4
lines changed

3 files changed

+353
-4
lines changed

‎src/package.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -321,9 +321,6 @@
321321
"type": "string"
322322
},
323323
"default": [
324-
"npm test",
325-
"npm install",
326-
"tsc",
327324
"git log",
328325
"git diff",
329326
"git show"

‎src/utils/__tests__/migrateSettings.spec.ts‎

Lines changed: 296 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,296 @@
1+
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
2+
import * as vscode from "vscode"
3+
import * as fs from "fs/promises"
4+
import * as path from "path"
5+
import { migrateSettings } from "../migrateSettings"
6+
7+
// Mock vscode module
8+
vi.mock("vscode", () => ({
9+
window: {
10+
showInformationMessage: vi.fn(),
11+
createOutputChannel: vi.fn(() => ({
12+
appendLine: vi.fn(),
13+
})),
14+
},
15+
commands: {
16+
executeCommand: vi.fn(),
17+
},
18+
}))
19+
20+
// Mock fs module
21+
vi.mock("fs/promises")
22+
23+
// Mock fs utils
24+
vi.mock("../fs", () => ({
25+
fileExistsAtPath: vi.fn(),
26+
}))
27+
28+
// Mock yaml module
29+
vi.mock("yaml", () => ({
30+
parse: vi.fn((content) => JSON.parse(content)),
31+
stringify: vi.fn((obj) => JSON.stringify(obj, null, 2)),
32+
}))
33+
34+
describe("migrateSettings", () => {
35+
let mockContext: any
36+
let mockOutputChannel: any
37+
let mockGlobalState: Map<string, any>
38+
39+
beforeEach(() => {
40+
// Reset all mocks
41+
vi.clearAllMocks()
42+
43+
// Create a mock global state
44+
mockGlobalState = new Map()
45+
46+
// Create mock output channel
47+
mockOutputChannel = {
48+
appendLine: vi.fn(),
49+
}
50+
51+
// Create mock context
52+
mockContext = {
53+
globalState: {
54+
get: vi.fn((key: string) => mockGlobalState.get(key)),
55+
update: vi.fn(async (key: string, value: any) => {
56+
mockGlobalState.set(key, value)
57+
}),
58+
},
59+
globalStorageUri: {
60+
fsPath: "/mock/storage/path",
61+
},
62+
}
63+
})
64+
65+
afterEach(() => {
66+
vi.restoreAllMocks()
67+
})
68+
69+
describe("default commands migration", () => {
70+
it("should only run migration once", async () => {
71+
// Set up initial state with old default commands
72+
const initialCommands = ["npm install", "npm test", "tsc", "git log"]
73+
mockGlobalState.set("allowedCommands", initialCommands)
74+
75+
// Mock file system
76+
const { fileExistsAtPath } = await import("../fs")
77+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
78+
79+
// Run migration first time
80+
await migrateSettings(mockContext, mockOutputChannel)
81+
82+
// Check that old default commands were removed
83+
expect(mockGlobalState.get("allowedCommands")).toEqual(["git log"])
84+
85+
// Check that migration was marked as complete
86+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
87+
88+
// Reset mocks but keep the migration flag
89+
mockGlobalState.set("defaultCommandsMigrationCompleted", true)
90+
mockGlobalState.set("allowedCommands", ["npm install", "npm test"])
91+
vi.mocked(mockContext.globalState.update).mockClear()
92+
93+
// Run migration again
94+
await migrateSettings(mockContext, mockOutputChannel)
95+
96+
// Verify commands were NOT modified the second time
97+
expect(mockGlobalState.get("allowedCommands")).toEqual(["npm install", "npm test"])
98+
expect(mockContext.globalState.update).not.toHaveBeenCalled()
99+
})
100+
101+
it("should remove npm install, npm test, and tsc from allowed commands", async () => {
102+
// Set up initial state with old default commands
103+
const initialCommands = ["git log", "npm install", "npm test", "tsc", "git diff", "echo hello"]
104+
mockGlobalState.set("allowedCommands", initialCommands)
105+
106+
// Mock file system to indicate no settings directory exists
107+
const { fileExistsAtPath } = await import("../fs")
108+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
109+
110+
// Run migration
111+
await migrateSettings(mockContext, mockOutputChannel)
112+
113+
// Check that old default commands were removed
114+
const updatedCommands = mockGlobalState.get("allowedCommands")
115+
expect(updatedCommands).toEqual(["git log", "git diff", "echo hello"])
116+
117+
// Verify the update was called
118+
expect(mockContext.globalState.update).toHaveBeenCalledWith("allowedCommands", [
119+
"git log",
120+
"git diff",
121+
"echo hello",
122+
])
123+
124+
// Verify migration was marked as complete
125+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
126+
127+
// No notification should be shown
128+
expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
129+
})
130+
131+
it("should not remove commands with arguments (only exact matches)", async () => {
132+
// Set up initial state with commands that have arguments
133+
const initialCommands = [
134+
"npm install express",
135+
"npm test --coverage",
136+
"tsc --watch",
137+
"npm list",
138+
"npm view",
139+
"yarn list",
140+
"git status",
141+
]
142+
mockGlobalState.set("allowedCommands", initialCommands)
143+
144+
// Mock file system
145+
const { fileExistsAtPath } = await import("../fs")
146+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
147+
148+
// Run migration
149+
await migrateSettings(mockContext, mockOutputChannel)
150+
151+
// Check that commands with arguments were NOT removed (only exact matches are removed)
152+
const updatedCommands = mockGlobalState.get("allowedCommands")
153+
expect(updatedCommands).toEqual([
154+
"npm install express",
155+
"npm test --coverage",
156+
"tsc --watch",
157+
"npm list",
158+
"npm view",
159+
"yarn list",
160+
"git status",
161+
])
162+
163+
// Migration should still be marked as complete
164+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
165+
166+
// No notification should be shown
167+
expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
168+
})
169+
170+
it("should handle case-insensitive matching", async () => {
171+
// Set up initial state with mixed case commands
172+
const initialCommands = ["NPM INSTALL", "Npm Test", "TSC", "git log"]
173+
mockGlobalState.set("allowedCommands", initialCommands)
174+
175+
// Mock file system
176+
const { fileExistsAtPath } = await import("../fs")
177+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
178+
179+
// Run migration
180+
await migrateSettings(mockContext, mockOutputChannel)
181+
182+
// Check that unsafe commands were removed regardless of case
183+
const updatedCommands = mockGlobalState.get("allowedCommands")
184+
expect(updatedCommands).toEqual(["git log"])
185+
})
186+
187+
it("should not modify commands if no old defaults are present", async () => {
188+
// Set up initial state with only safe commands
189+
const initialCommands = ["git log", "git diff", "ls -la", "echo hello"]
190+
mockGlobalState.set("allowedCommands", initialCommands)
191+
192+
// Mock file system
193+
const { fileExistsAtPath } = await import("../fs")
194+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
195+
196+
// Run migration
197+
await migrateSettings(mockContext, mockOutputChannel)
198+
199+
// Check that commands remain unchanged
200+
const updatedCommands = mockGlobalState.get("allowedCommands")
201+
expect(updatedCommands).toEqual(initialCommands)
202+
203+
// Verify no notification was shown
204+
expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
205+
206+
// Verify migration was still marked as complete
207+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
208+
})
209+
210+
it("should handle missing or invalid allowedCommands gracefully", async () => {
211+
// Test with no allowedCommands set
212+
const { fileExistsAtPath } = await import("../fs")
213+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
214+
215+
await migrateSettings(mockContext, mockOutputChannel)
216+
// Should still mark migration as complete
217+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
218+
219+
// Verify appropriate log messages
220+
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
221+
expect.stringContaining("marking migration as complete"),
222+
)
223+
})
224+
225+
it("should handle non-array allowedCommands gracefully", async () => {
226+
// Test with non-array value
227+
mockGlobalState.set("allowedCommands", "not an array")
228+
229+
const { fileExistsAtPath } = await import("../fs")
230+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
231+
232+
await migrateSettings(mockContext, mockOutputChannel)
233+
234+
// Should still mark migration as complete
235+
expect(mockContext.globalState.update).toHaveBeenCalledWith("defaultCommandsMigrationCompleted", true)
236+
237+
// Verify appropriate log messages
238+
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
239+
expect.stringContaining("marking migration as complete"),
240+
)
241+
})
242+
243+
it("should handle errors gracefully", async () => {
244+
// Set up state
245+
mockGlobalState.set("allowedCommands", ["npm install"])
246+
247+
// Make update throw an error
248+
mockContext.globalState.update = vi.fn().mockRejectedValue(new Error("Update failed"))
249+
250+
// Mock file system
251+
const { fileExistsAtPath } = await import("../fs")
252+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
253+
254+
// Run migration - should not throw
255+
await expect(migrateSettings(mockContext, mockOutputChannel)).resolves.toBeUndefined()
256+
257+
// Verify error was logged
258+
expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
259+
expect.stringContaining("[Default Commands Migration] Error"),
260+
)
261+
})
262+
263+
it("should only remove exact matches, not commands with arguments", async () => {
264+
// Set up initial state with exact matches and commands with arguments
265+
const initialCommands = [
266+
"npm install", // exact match - should be removed
267+
"npm install --save-dev typescript", // has arguments - should NOT be removed
268+
"npm test", // exact match - should be removed
269+
"npm test --coverage", // has arguments - should NOT be removed
270+
"tsc", // exact match - should be removed
271+
"tsc --noEmit", // has arguments - should NOT be removed
272+
"git log --oneline",
273+
]
274+
mockGlobalState.set("allowedCommands", initialCommands)
275+
276+
// Mock file system
277+
const { fileExistsAtPath } = await import("../fs")
278+
vi.mocked(fileExistsAtPath).mockResolvedValue(false)
279+
280+
// Run migration
281+
await migrateSettings(mockContext, mockOutputChannel)
282+
283+
// Check that only exact matches were removed
284+
const updatedCommands = mockGlobalState.get("allowedCommands")
285+
expect(updatedCommands).toEqual([
286+
"npm install --save-dev typescript",
287+
"npm test --coverage",
288+
"tsc --noEmit",
289+
"git log --oneline",
290+
])
291+
292+
// No notification should be shown
293+
expect(vscode.window.showInformationMessage).not.toHaveBeenCalled()
294+
})
295+
})
296+
})

‎src/utils/migrateSettings.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,16 @@ import * as yaml from "yaml"
88
const deprecatedCustomModesJSONFilename = "custom_modes.json"
99

1010
/**
11-
* Migrates old settings files to new file names
11+
* Migrates old settings files to new file names and removes commands from old defaults
1212
*
1313
* TODO: Remove this migration code in September 2025 (6 months after implementation)
1414
*/
1515
export async function migrateSettings(
1616
context: vscode.ExtensionContext,
1717
outputChannel: vscode.OutputChannel,
1818
): Promise<void> {
19+
// First, migrate commands from old defaults (security fix)
20+
await migrateDefaultCommands(context, outputChannel)
1921
// Legacy file names that need to be migrated to the new names in GlobalFileNames
2022
const fileMigrations = [
2123
// custom_modes.json to custom_modes.yaml is handled separately below
@@ -113,3 +115,57 @@ async function migrateCustomModesToYaml(settingsDir: string, outputChannel: vsco
113115
outputChannel.appendLine(`Error reading custom_modes.json: ${fileError}. Skipping migration.`)
114116
}
115117
}
118+
119+
/**
120+
* Removes commands from old defaults that could execute arbitrary code
121+
* This addresses the security vulnerability where npm install/test can run malicious postinstall scripts
122+
*/
123+
async function migrateDefaultCommands(
124+
context: vscode.ExtensionContext,
125+
outputChannel: vscode.OutputChannel,
126+
): Promise<void> {
127+
try {
128+
// Check if this migration has already been run
129+
const migrationKey = "defaultCommandsMigrationCompleted"
130+
if (context.globalState.get(migrationKey)) {
131+
outputChannel.appendLine("[Default Commands Migration] Migration already completed, skipping")
132+
return
133+
}
134+
135+
const allowedCommands = context.globalState.get<string[]>("allowedCommands")
136+
137+
if (!allowedCommands || !Array.isArray(allowedCommands)) {
138+
// Mark migration as complete even if no commands to migrate
139+
await context.globalState.update(migrationKey, true)
140+
outputChannel.appendLine("No allowed commands found in global state, marking migration as complete")
141+
return
142+
}
143+
144+
// Only migrate the specific commands that were removed from the defaults
145+
const oldDefaultCommands = ["npm install", "npm test", "tsc"]
146+
147+
// Filter out old default commands (case-insensitive exact match only)
148+
const originalLength = allowedCommands.length
149+
const filteredCommands = allowedCommands.filter((cmd) => {
150+
const cmdLower = cmd.toLowerCase().trim()
151+
return !oldDefaultCommands.some((oldDefault) => cmdLower === oldDefault.toLowerCase())
152+
})
153+
154+
if (filteredCommands.length < originalLength) {
155+
const removedCount = originalLength - filteredCommands.length
156+
await context.globalState.update("allowedCommands", filteredCommands)
157+
158+
outputChannel.appendLine(
159+
`[Default Commands Migration] Removed ${removedCount} command(s) from old defaults to prevent arbitrary code execution vulnerability`,
160+
)
161+
} else {
162+
outputChannel.appendLine("[Default Commands Migration] No old default commands found in allowed list")
163+
}
164+
165+
// Mark migration as complete
166+
await context.globalState.update(migrationKey, true)
167+
outputChannel.appendLine("[Default Commands Migration] Migration marked as complete")
168+
} catch (error) {
169+
outputChannel.appendLine(`[Default Commands Migration] Error migrating default commands: ${error}`)
170+
}
171+
}

0 commit comments

Comments
 (0)