Fix: Enhance virtual environment removal validation

Author: karthiknadig

src/common/localize.ts

@@ -94,6 +94,10 @@ export namespace VenvManagerStrings {
 
     export const venvRemoving = l10n.t('Removing virtual environment');
     export const venvRemoveFailed = l10n.t('Failed to remove virtual environment');
+    export const venvRemoveInvalidPath = l10n.t(
+        'Cannot remove: path does not appear to be a valid virtual environment',
+    );
+    export const venvRemoveUnsafePath = l10n.t('Cannot remove: path appears to be a system or root directory');
 
     export const installEditable = l10n.t('Install project as editable');
     export const searchingDependencies = l10n.t('Searching for dependencies');

src/managers/builtin/venvUtils.ts

@@ -422,12 +422,87 @@ export async function createPythonVenv(
     return createStepBasedVenvFlow(nativeFinder, api, log, manager, basePythons, venvRoot, options);
 }
 
+/**
+ * Checks if a path is a drive root (e.g., "C:\" on Windows or "/" on Unix).
+ */
+function isDriveRoot(fsPath: string): boolean {
+    const normalized = path.normalize(fsPath);
+    if (os.platform() === 'win32') {
+        // Windows drive root: "C:\" or "C:"
+        return /^[a-zA-Z]:[\\/]?$/.test(normalized);
+    }
+    // Unix root
+    return normalized === '/';
+}
+
+/**
+ * Checks if a path has a minimum number of directory components to be considered safe.
+ * This helps prevent accidental deletion of high-level directories.
+ */
+function hasMinimumPathDepth(fsPath: string, minDepth: number = 2): boolean {
+    const normalized = path.normalize(fsPath);
+    const parts = normalized.split(path.sep).filter((p) => p.length > 0 && p !== '.');
+
+    if (os.platform() === 'win32') {
+        // On Windows, the drive letter counts as one component
+        // e.g., "C:\folder" has parts ["C:", "folder"] -> depth 2
+        return parts.length >= minDepth;
+    }
+    // On Unix, "/home/user" has parts ["home", "user"] -> depth 2
+    return parts.length >= minDepth;
+}
+
+/**
+ * Validates that a path appears to be a virtual environment by checking for pyvenv.cfg.
+ */
+async function isValidVenvPath(fsPath: string): Promise<boolean> {
+    try {
+        const pyvenvCfgPath = path.join(fsPath, 'pyvenv.cfg');
+        return await fsapi.pathExists(pyvenvCfgPath);
+    } catch {
+        return false;
+    }
+}
+
+/**
+ * Validates that a path is safe to remove as a virtual environment.
+ * Returns an error message if unsafe, or undefined if safe.
+ */
+async function validateVenvRemovalPath(envPath: string, log: LogOutputChannel): Promise<string | undefined> {
+    // Check if it's a drive root
+    if (isDriveRoot(envPath)) {
+        log.error(`Refusing to remove drive root: ${envPath}`);
+        return VenvManagerStrings.venvRemoveUnsafePath;
+    }
+
+    // Check minimum path depth (at least 2 components to be safe)
+    if (!hasMinimumPathDepth(envPath, 2)) {
+        log.error(`Refusing to remove path with insufficient depth: ${envPath}`);
+        return VenvManagerStrings.venvRemoveUnsafePath;
+    }
+
+    // Check if it's actually a venv (has pyvenv.cfg)
+    if (!(await isValidVenvPath(envPath))) {
+        log.error(`Path does not appear to be a valid venv (no pyvenv.cfg): ${envPath}`);
+        return VenvManagerStrings.venvRemoveInvalidPath;
+    }
+
+    return undefined;
+}
+
 export async function removeVenv(environment: PythonEnvironment, log: LogOutputChannel): Promise<boolean> {
     const pythonPath = os.platform() === 'win32' ? 'python.exe' : 'python';
 
-    const envPath = environment.environmentPath.fsPath.endsWith(pythonPath)
-        ? path.dirname(path.dirname(environment.environmentPath.fsPath))
-        : environment.environmentPath.fsPath;
+    // Normalize path separators before checking endsWith
+    const envFsPath = path.normalize(environment.environmentPath.fsPath);
+    const envPath = envFsPath.endsWith(pythonPath) ? path.dirname(path.dirname(envFsPath)) : envFsPath;
+
+    // Safety validation before proceeding
+    const validationError = await validateVenvRemovalPath(envPath, log);
+    if (validationError) {
+        showErrorMessage(validationError);
+        return false;
+    }
 
     // Normalize path for UI display - ensure forward slashes on Windows
     const displayPath = normalizePath(envPath);

src/test/managers/builtin/venvUtils.removeVenv.unit.test.ts

@@ -0,0 +1,155 @@
+import * as assert from 'assert';
+import * as os from 'os';
+import * as path from 'path';
+
+// We need to test the helper functions that are now part of venvUtils
+// Since they are not exported, we'll test the behavior through removeVenv
+// For unit testing the validation logic, we can re-implement the pure logic tests here
+
+suite('venvUtils Path Validation', () => {
+    suite('isDriveRoot behavior', () => {
+        test('should identify Windows drive roots correctly', function () {
+            if (os.platform() !== 'win32') {
+                this.skip();
+                return;
+            }
+
+            // Test cases that represent drive roots
+            const driveRoots = ['C:\\', 'D:\\', 'c:\\', 'C:/', 'C:'];
+
+            for (const root of driveRoots) {
+                const normalized = path.normalize(root);
+                const isDrive = /^[a-zA-Z]:[\\/]?$/.test(normalized);
+                assert.strictEqual(isDrive, true, `${root} should be identified as drive root`);
+            }
+        });
+
+        test('should not identify non-root Windows paths as drive roots', function () {
+            if (os.platform() !== 'win32') {
+                this.skip();
+                return;
+            }
+
+            const nonRoots = ['C:\\Users', 'C:\\Program Files', 'D:\\python\\venv', 'C:\\Users\\test\\.venv'];
+
+            for (const nonRoot of nonRoots) {
+                const normalized = path.normalize(nonRoot);
+                const isDrive = /^[a-zA-Z]:[\\/]?$/.test(normalized);
+                assert.strictEqual(isDrive, false, `${nonRoot} should not be identified as drive root`);
+            }
+        });
+
+        test('should identify Unix root correctly', function () {
+            if (os.platform() === 'win32') {
+                this.skip();
+                return;
+            }
+
+            const normalized = path.normalize('/');
+            assert.strictEqual(normalized, '/', 'Unix root should be /');
+        });
+    });
+
+    suite('hasMinimumPathDepth behavior', () => {
+        test('should correctly count path components on Windows', function () {
+            if (os.platform() !== 'win32') {
+                this.skip();
+                return;
+            }
+
+            const testCases: [string, number][] = [
+                ['C:\\', 1],
+                ['C:\\Users', 2],
+                ['C:\\Users\\test', 3],
+                ['C:\\Users\\test\\.venv', 4],
+            ];
+
+            for (const [testPath, expectedDepth] of testCases) {
+                const normalized = path.normalize(testPath);
+                const parts = normalized.split(path.sep).filter((p) => p.length > 0 && p !== '.');
+                assert.strictEqual(parts.length, expectedDepth, `${testPath} should have ${expectedDepth} components`);
+            }
+        });
+
+        test('should correctly count path components on Unix', function () {
+            if (os.platform() === 'win32') {
+                this.skip();
+                return;
+            }
+
+            const testCases: [string, number][] = [
+                ['/', 0],
+                ['/home', 1],
+                ['/home/user', 2],
+                ['/home/user/.venv', 3],
+            ];
+
+            for (const [testPath, expectedDepth] of testCases) {
+                const normalized = path.normalize(testPath);
+                const parts = normalized.split(path.sep).filter((p) => p.length > 0 && p !== '.');
+                assert.strictEqual(parts.length, expectedDepth, `${testPath} should have ${expectedDepth} components`);
+            }
+        });
+    });
+
+    suite('Path normalization in removeVenv', () => {
+        test('should normalize path separators before checking python.exe suffix', () => {
+            // This test verifies that paths with mixed separators are handled correctly
+            const pythonPath = os.platform() === 'win32' ? 'python.exe' : 'python';
+
+            // Example of a path that might have mixed separators on Windows
+            const mixedPath =
+                os.platform() === 'win32' ? 'C:/Users/test/.venv/Scripts/python.exe' : '/home/user/.venv/bin/python';
+
+            const normalizedPath = path.normalize(mixedPath);
+            const endsWithPython = normalizedPath.endsWith(pythonPath);
+
+            assert.strictEqual(endsWithPython, true, 'Normalized path should end with python executable');
+
+            // Verify the resulting env path after going up two directories
+            const envPath = path.dirname(path.dirname(normalizedPath));
+            const expectedEnvPath =
+                os.platform() === 'win32' ? path.normalize('C:/Users/test/.venv') : '/home/user/.venv';
+
+            assert.strictEqual(envPath, expectedEnvPath, 'Environment path should be the venv root');
+        });
+
+        test('should correctly derive venv path from python executable path', () => {
+            const pythonPath = os.platform() === 'win32' ? 'python.exe' : 'python';
+
+            // Simulate what removeVenv does
+            const testPaths =
+                os.platform() === 'win32'
+                    ? [
+                          { input: 'C:\\project\\.venv\\Scripts\\python.exe', expected: 'C:\\project\\.venv' },
+                          { input: 'D:\\envs\\myenv\\Scripts\\python.exe', expected: 'D:\\envs\\myenv' },
+                      ]
+                    : [
+                          { input: '/home/user/project/.venv/bin/python', expected: '/home/user/project/.venv' },
+                          { input: '/opt/envs/myenv/bin/python', expected: '/opt/envs/myenv' },
+                      ];
+
+            for (const { input, expected } of testPaths) {
+                const normalized = path.normalize(input);
+                const envPath = normalized.endsWith(pythonPath) ? path.dirname(path.dirname(normalized)) : normalized;
+
+                assert.strictEqual(envPath, expected, `${input} should derive to ${expected}`);
+            }
+        });
+    });
+});
+
+suite('venvUtils removeVenv validation integration', () => {
+    test('pyvenv.cfg detection should use correct path', async () => {
+        const testEnvPath = os.platform() === 'win32' ? 'C:\\Users\\test\\.venv' : '/home/user/.venv';
+
+        const expectedCfgPath = path.join(testEnvPath, 'pyvenv.cfg');
+
+        // Just verify the path is constructed correctly
+        assert.strictEqual(
+            expectedCfgPath,
+            os.platform() === 'win32' ? 'C:\\Users\\test\\.venv\\pyvenv.cfg' : '/home/user/.venv/pyvenv.cfg',
+            'Should check for pyvenv.cfg in the environment root',
+        );
+    });
+});