Update list of default allowed commands #7404
Conversation
dosubot
bot
added
the
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| outputChannel: vscode.OutputChannel, | ||
| ): Promise<void> { | ||
| // First, migrate commands from old defaults (security fix) | ||
| await migrateDefaultCommands(context, outputChannel) |
There was a problem hiding this comment.
Is the order intentional here? The function now calls migrateDefaultCommands() first, then handles file migrations. If the migration fails partway through, users might have their commands migrated but not their files. Would it make sense to wrap these in separate try-catch blocks or consider the order of operations?
| const originalLength = allowedCommands.length | ||
| const filteredCommands = allowedCommands.filter((cmd) => { | ||
| const cmdLower = cmd.toLowerCase().trim() | ||
| return !oldDefaultCommands.some((oldDefault) => cmdLower === oldDefault.toLowerCase()) |
There was a problem hiding this comment.
Great implementation detail! The migration correctly only removes exact matches (e.g., 'npm install') while preserving commands with arguments (e.g., 'npm install express'). This is a thoughtful approach that maintains user flexibility while addressing the security concern.
| ) | ||
| }) | ||
|
|
||
| it("should only remove exact matches, not commands with arguments", async () => { |
There was a problem hiding this comment.
Excellent test coverage! This test suite thoroughly covers edge cases including case-insensitive matching, commands with arguments, error handling, and the one-time migration flag. The test for ensuring exact matches only (lines 263-294) is particularly important for validating the security fix doesn't over-reach.
hannesrudolph
added
the
Issue/PR - Triage
Thanks @thelicato for flagging that some of these default commands can be unsafe.
Important
Removed unsafe commands from default allowed list and added migration to update existing settings.
npm test,npm install, andtscfrom default allowed commands inpackage.json.migrateDefaultCommands()inmigrateSettings.tsto remove these commands from existing settings.migrateSettings.spec.tsto testmigrateDefaultCommands()for various scenarios, including case-insensitive matching and handling of non-arrayallowedCommands.migrateSettings.tsto reflect new migration functionality.This description was created by
for f9c17b3. You can customize this summary. It will automatically update as commits are pushed.