fix: snap env variables leaking into terminal env (#185834)

* fix: snap env variables leaking into terminal env * chore: add spec
microsoft · Jun 22, 2023 · 42457bd · 42457bd
1 parent 982869f
commit 42457bd
Show file tree

Hide file tree

Showing 5 changed files with 200 additions and 39 deletions.
diff --git a/resources/linux/snap/electron-launch b/resources/linux/snap/electron-launch
@@ -47,6 +47,15 @@ function append_dir() {
   fi
 }
 
+function copy_env_variable() {
+  local -n var="$1"
+  if [[ "+$var" ]]; then
+    export "${!var}_VSCODE_SNAP_ORIG=${var}"
+  else
+    export "${!var}_VSCODE_SNAP_ORIG=''"
+  fi
+}
+
 # shellcheck source=/dev/null
 source "$SNAP_USER_DATA/.last_revision" 2>/dev/null || true
 if [ "$SNAP_DESKTOP_LAST_REVISION" = "$SNAP_VERSION" ]; then
@@ -84,6 +93,19 @@ function can_open_file() {
   [ -f "$1" ] && [ -r "$1" ]
 }
 
+# Preserve system variables that get modified below
+copy_env_variable XDG_CONFIG_DIRS
+copy_env_variable XDG_DATA_DIRS
+copy_env_variable LOCPATH
+copy_env_variable GIO_MODULE_DIR
+copy_env_variable GSETTINGS_SCHEMA_DIR
+copy_env_variable GDK_PIXBUF_MODULE_FILE
+copy_env_variable GDK_PIXBUF_MODULEDIR
+copy_env_variable GDK_BACKEND
+copy_env_variable GTK_PATH
+copy_env_variable GTK_EXE_PREFIX
+copy_env_variable GTK_IM_MODULE_FILE
+
 # XDG Config
 prepend_dir XDG_CONFIG_DIRS "$SNAP/etc/xdg"
 

diff --git a/src/vs/platform/environment/electron-main/environmentMainService.ts b/src/vs/platform/environment/electron-main/environmentMainService.ts
@@ -5,6 +5,7 @@
 
 import { memoize } from 'vs/base/common/decorators';
 import { join } from 'vs/base/common/path';
+import { isLinux } from 'vs/base/common/platform';
 import { createStaticIPCHandle } from 'vs/base/parts/ipc/node/ipc.net';
 import { IEnvironmentService, INativeEnvironmentService } from 'vs/platform/environment/common/environment';
 import { NativeEnvironmentService } from 'vs/platform/environment/node/environmentService';
@@ -34,10 +35,15 @@ export interface IEnvironmentMainService extends INativeEnvironmentService {
 
 	// --- config
 	readonly disableUpdates: boolean;
+
+	unsetSnapExportedVariables(): void;
+	restoreSnapExportedVariables(): void;
 }
 
 export class EnvironmentMainService extends NativeEnvironmentService implements IEnvironmentMainService {
 
+	private _snapEnv: Record<string, string> = {};
+
 	@memoize
 	get cachedLanguagesPath(): string { return join(this.userDataPath, 'clp'); }
 
@@ -64,4 +70,39 @@ export class EnvironmentMainService extends NativeEnvironmentService implements
 
 	@memoize
 	get useCodeCache(): boolean { return !!this.codeCachePath; }
+
+	unsetSnapExportedVariables() {
+		if (!isLinux) {
+			return;
+		}
+		for (const key in process.env) {
+			if (key.endsWith('_VSCODE_SNAP_ORIG')) {
+				const originalKey = key.slice(0, -17); // Remove the _VSCODE_SNAP_ORIG suffix
+				if (this._snapEnv[originalKey]) {
+					continue;
+				}
+				// Preserve the original value in case the snap env is re-entered
+				if (process.env[originalKey]) {
+					this._snapEnv[originalKey] = process.env[originalKey]!;
+				}
+				// Copy the original value from before entering the snap env if available,
+				// if not delete the env variable.
+				if (process.env[key]) {
+					process.env[originalKey] = process.env[key];
+				} else {
+					delete process.env[originalKey];
+				}
+			}
+		}
+	}
+
+	restoreSnapExportedVariables() {
+		if (!isLinux) {
+			return;
+		}
+		for (const key in this._snapEnv) {
+			process.env[key] = this._snapEnv[key];
+			delete this._snapEnv[key];
+		}
+	}
 }
diff --git a/src/vs/platform/environment/test/electron-main/environmentMainService.test.ts b/src/vs/platform/environment/test/electron-main/environmentMainService.test.ts
@@ -0,0 +1,128 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as assert from 'assert';
+import { EnvironmentMainService } from 'vs/platform/environment/electron-main/environmentMainService';
+import product from 'vs/platform/product/common/product';
+import { isLinux } from 'vs/base/common/platform';
+
+suite('EnvironmentMainService', () => {
+
+	test('can unset and restore snap env variables', () => {
+		const service = new EnvironmentMainService({ '_': [] }, { '_serviceBrand': undefined, ...product });
+
+		process.env['TEST_ARG1_VSCODE_SNAP_ORIG'] = 'original';
+		process.env['TEST_ARG1'] = 'modified';
+		process.env['TEST_ARG2_SNAP'] = 'test_arg2';
+		process.env['TEST_ARG3_VSCODE_SNAP_ORIG'] = '';
+		process.env['TEST_ARG3'] = 'test_arg3_non_empty';
+
+		// Unset snap env variables
+		service.unsetSnapExportedVariables();
+		if (isLinux) {
+			assert.strictEqual(process.env['TEST_ARG1'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], undefined);
+		} else {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		}
+
+		// Restore snap env variables
+		service.restoreSnapExportedVariables();
+		if (isLinux) {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		} else {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		}
+	});
+
+	test('can invoke unsetSnapExportedVariables and restoreSnapExportedVariables multiple times', () => {
+		const service = new EnvironmentMainService({ '_': [] }, { '_serviceBrand': undefined, ...product });
+		// Mock snap environment
+		process.env['SNAP'] = '1';
+		process.env['SNAP_REVISION'] = 'test_revision';
+
+		process.env['TEST_ARG1_VSCODE_SNAP_ORIG'] = 'original';
+		process.env['TEST_ARG1'] = 'modified';
+		process.env['TEST_ARG2_SNAP'] = 'test_arg2';
+		process.env['TEST_ARG3_VSCODE_SNAP_ORIG'] = '';
+		process.env['TEST_ARG3'] = 'test_arg3_non_empty';
+
+		// Unset snap env variables
+		service.unsetSnapExportedVariables();
+		service.unsetSnapExportedVariables();
+		service.unsetSnapExportedVariables();
+		if (isLinux) {
+			assert.strictEqual(process.env['TEST_ARG1'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], undefined);
+		} else {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		}
+
+		// Restore snap env variables
+		service.restoreSnapExportedVariables();
+		service.restoreSnapExportedVariables();
+		if (isLinux) {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		} else {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		}
+
+		// Unset snap env variables
+		service.unsetSnapExportedVariables();
+		if (isLinux) {
+			assert.strictEqual(process.env['TEST_ARG1'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], undefined);
+		} else {
+			assert.strictEqual(process.env['TEST_ARG1'], 'modified');
+			assert.strictEqual(process.env['TEST_ARG2'], undefined);
+			assert.strictEqual(process.env['TEST_ARG1_VSCODE_SNAP_ORIG'], 'original');
+			assert.strictEqual(process.env['TEST_ARG2_SNAP'], 'test_arg2');
+			assert.strictEqual(process.env['TEST_ARG3_VSCODE_SNAP_ORIG'], '');
+			assert.strictEqual(process.env['TEST_ARG3'], 'test_arg3_non_empty');
+		}
+	});
+});
diff --git a/src/vs/platform/native/electron-main/nativeHostMainService.ts b/src/vs/platform/native/electron-main/nativeHostMainService.ts
@@ -12,7 +12,7 @@ import { Emitter, Event } from 'vs/base/common/event';
 import { Disposable } from 'vs/base/common/lifecycle';
 import { Schemas } from 'vs/base/common/network';
 import { dirname, join, resolve } from 'vs/base/common/path';
-import { isLinux, isLinuxSnap, isMacintosh, isWindows } from 'vs/base/common/platform';
+import { isLinux, isMacintosh, isWindows } from 'vs/base/common/platform';
 import { AddFirstParameterToFunctions } from 'vs/base/common/types';
 import { URI } from 'vs/base/common/uri';
 import { realpath } from 'vs/base/node/extpath';
@@ -433,43 +433,11 @@ export class NativeHostMainService extends Disposable implements INativeHostMain
 	}
 
 	async openExternal(windowId: number | undefined, url: string): Promise<boolean> {
-		if (isLinuxSnap) {
-			this.safeSnapOpenExternal(url);
-		} else {
-			shell.openExternal(url);
-		}
-
-		return true;
-	}
-
-	private safeSnapOpenExternal(url: string): void {
-
-		// Remove some environment variables before opening to avoid issues...
-		const gdkPixbufModuleFile = process.env['GDK_PIXBUF_MODULE_FILE'];
-		const gdkPixbufModuleDir = process.env['GDK_PIXBUF_MODULEDIR'];
-		const gtkIMModuleFile = process.env['GTK_IM_MODULE_FILE'];
-		const gdkBackend = process.env['GDK_BACKEND'];
-		const gioModuleDir = process.env['GIO_MODULE_DIR'];
-		const gtkExePrefix = process.env['GTK_EXE_PREFIX'];
-		const gsettingsSchemaDir = process.env['GSETTINGS_SCHEMA_DIR'];
-		delete process.env['GDK_PIXBUF_MODULE_FILE'];
-		delete process.env['GDK_PIXBUF_MODULEDIR'];
-		delete process.env['GTK_IM_MODULE_FILE'];
-		delete process.env['GDK_BACKEND'];
-		delete process.env['GIO_MODULE_DIR'];
-		delete process.env['GTK_EXE_PREFIX'];
-		delete process.env['GSETTINGS_SCHEMA_DIR'];
-
+		this.environmentMainService.unsetSnapExportedVariables();
 		shell.openExternal(url);
+		this.environmentMainService.restoreSnapExportedVariables();
 
-		// ...but restore them after
-		process.env['GDK_PIXBUF_MODULE_FILE'] = gdkPixbufModuleFile;
-		process.env['GDK_PIXBUF_MODULEDIR'] = gdkPixbufModuleDir;
-		process.env['GTK_IM_MODULE_FILE'] = gtkIMModuleFile;
-		process.env['GDK_BACKEND'] = gdkBackend;
-		process.env['GIO_MODULE_DIR'] = gioModuleDir;
-		process.env['GTK_EXE_PREFIX'] = gtkExePrefix;
-		process.env['GSETTINGS_SCHEMA_DIR'] = gsettingsSchemaDir;
+		return true;
 	}
 
 	moveItemToTrash(windowId: number | undefined, fullPath: string): Promise<void> {

diff --git a/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts b/src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts
@@ -3,7 +3,7 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { IEnvironmentService, INativeEnvironmentService } from 'vs/platform/environment/common/environment';
+import { IEnvironmentMainService } from 'vs/platform/environment/electron-main/environmentMainService';
 import { parsePtyHostDebugPort } from 'vs/platform/environment/node/environmentService';
 import { ILifecycleMainService } from 'vs/platform/lifecycle/electron-main/lifecycleMainService';
 import { ILogService } from 'vs/platform/log/common/log';
@@ -31,7 +31,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter {
 	constructor(
 		private readonly _reconnectConstants: IReconnectConstants,
 		@IConfigurationService private readonly _configurationService: IConfigurationService,
-		@IEnvironmentService private readonly _environmentService: INativeEnvironmentService,
+		@IEnvironmentMainService private readonly _environmentMainService: IEnvironmentMainService,
 		@ILifecycleMainService private readonly _lifecycleMainService: ILifecycleMainService,
 		@ILogService private readonly _logService: ILogService
 	) {
@@ -43,7 +43,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter {
 	start(lastPtyId: number): IPtyHostConnection {
 		this.utilityProcess = new UtilityProcess(this._logService, NullTelemetryService, this._lifecycleMainService);
 
-		const inspectParams = parsePtyHostDebugPort(this._environmentService.args, this._environmentService.isBuilt);
+		const inspectParams = parsePtyHostDebugPort(this._environmentMainService.args, this._environmentMainService.isBuilt);
 		const execArgv = inspectParams.port ? [
 			'--nolazy',
 			`--inspect${inspectParams.break ? '-brk' : ''}=${inspectParams.port}`
@@ -75,6 +75,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter {
 	}
 
 	private _createPtyHostConfiguration(lastPtyId: number) {
+		this._environmentMainService.unsetSnapExportedVariables();
 		const config: { [key: string]: string } = {
 			...deepClone(process.env),
 			VSCODE_LAST_PTY_ID: String(lastPtyId),
@@ -93,6 +94,7 @@ export class ElectronPtyHostStarter implements IPtyHostStarter {
 		if (startupDelay && typeof startupDelay === 'number') {
 			config.VSCODE_STARTUP_DELAY = String(startupDelay);
 		}
+		this._environmentMainService.restoreSnapExportedVariables();
 		return config;
 	}